mock.assert_called_once() is not a valid method

mock.assert_called_once() is a no-op that tests nothing.  Instead
with mock.assert_called_once_with() should be used (or use
assertEqual(1, mock_obj.call_count) if you don't want to check
parameters).

Add a new HACKING rule for nova to prevent assert_called_once()
usage from creeping in.

Closes-Bug: #1365751

Change-Id: I1055093a1c31792b6411a3cd46c80b8bcaaf78c1

HACKING.rst

@@ -39,6 +39,7 @@ Nova Specific Commandments
 - [N324] Ensure that jsonutils.%(fun)s must be used instead of json.%(fun)s
 - [N325] str() cannot be used on an exception.  Remove use or use six.text_type()
 - [N326] Translated messages cannot be concatenated.  String should be included in translated message.
+- [N327] assert_called_once() is not a valid method
 
 Creating Unit Tests
 -------------------

nova/hacking/checks.py

@@ -347,6 +347,17 @@ def use_jsonutils(logical_line, filename):
                 yield (pos, msg % {'fun': f[:-1]})
 
 
+def check_assert_called_once(logical_line, filename):
+    msg = ("N327: assert_called_once is a no-op. please use assert_called_"
+           "once_with to test with explicit parameters or an assertEqual with"
+           " call_count.")
+
+    if 'nova/tests/' in filename:
+        pos = logical_line.find('.assert_called_once(')
+        if pos != -1:
+            yield (pos, msg)
+
+
 class CheckForStrExc(BaseASTChecker):
     """Checks for the use of str() on an exception.
 
@@ -423,5 +434,6 @@ def factory(register):
     register(no_mutable_default_args)
     register(check_explicit_underscore_import)
     register(use_jsonutils)
+    register(check_assert_called_once)
     register(CheckForStrExc)
     register(CheckForTransAdd)

nova/tests/test_hacking.py

@@ -248,20 +248,34 @@ class HackingTestCase(test.NoDBTestCase):
     # installed.
     @mock.patch('pep8._checks',
                 {'physical_line': {}, 'logical_line': {}, 'tree': {}})
-    def _run_check(self, code, checker):
+    def _run_check(self, code, checker, filename=None):
         pep8.register_check(checker)
 
         lines = textwrap.dedent(code).strip().splitlines(True)
 
-        checker = pep8.Checker(lines=lines)
+        checker = pep8.Checker(filename=filename, lines=lines)
         checker.check_all()
         checker.report._deferred_print.sort()
         return checker.report._deferred_print
 
-    def _assert_has_errors(self, code, checker, expected_errors=None):
-        actual_errors = [e[:3] for e in self._run_check(code, checker)]
+    def _assert_has_errors(self, code, checker, expected_errors=None,
+                           filename=None):
+        actual_errors = [e[:3] for e in
+                         self._run_check(code, checker, filename)]
         self.assertEqual(expected_errors or [], actual_errors)
 
+    def test_assert_called_once(self):
+
+        checker = checks.check_assert_called_once
+        code = """
+               mock = Mock()
+               mock.method(1, 2, 3, test='wow')
+               mock.method.assert_called_once()
+               """
+        errors = [(3, 11, 'N327')]
+        self._assert_has_errors(code, checker, expected_errors=errors,
+                                filename='nova/tests/test_assert.py')
+
     def test_str_exception(self):
 
         checker = checks.CheckForStrExc

nova/tests/virt/hyperv/test_ioutils.py

@@ -50,7 +50,7 @@ class IOThreadTestCase(test.NoDBTestCase):
         self._iothread._copy(self._FAKE_SRC, self._FAKE_DEST)
 
         fake_dest.write.assert_called_once_with(fake_data)
-        fake_dest.close.assert_called_once()
+        fake_dest.close.assert_called_once_with()
         fake_rename.assert_called_once_with(
             self._iothread._dest, self._iothread._dest_archive)
         fake_remove.assert_called_once_with(

nova/tests/virt/ironic/test_driver.py

@@ -529,7 +529,7 @@ class IronicDriverTestCase(test.NoDBTestCase, test_driver.DriverAPITestHelper):
                                              instance)
         fake_looping_call.start.assert_called_once_with(
             interval=CONF.ironic.api_retry_interval)
-        fake_looping_call.wait.assert_called_once()
+        fake_looping_call.wait.assert_called_once_with()
 
     @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall')
     @mock.patch.object(FAKE_CLIENT, 'node')
@@ -1117,7 +1117,7 @@ class IronicDriverTestCase(test.NoDBTestCase, test_driver.DriverAPITestHelper):
                                              instance)
         fake_looping_call.start.assert_called_once_with(
             interval=CONF.ironic.api_retry_interval)
-        fake_looping_call.wait.assert_called_once()
+        fake_looping_call.wait.assert_called_once_with()
 
     def test_rebuild_preserve_ephemeral(self):
         self._test_rebuild(preserve=True)

nova/tests/virt/libvirt/test_driver.py

@@ -3968,7 +3968,10 @@ class LibvirtConnTestCase(test.TestCase,
 
         mock_lookupByName.assert_called_once_with("instance-00000001")
         mock_volume_info.assert_has_calls(mock_volume_info_calls)
-        mock_convert_image.assert_called_once()
+        mock_convert_image.assert_called_once_with('/dev/nova-vg/lv',
+                                                   mock.ANY,
+                                                   'raw',
+                                                   run_as_root=True)
         snapshot = image_service.show(context, recv_meta['id'])
         mock_update_task_state.assert_has_calls(update_task_state_calls)
         self.assertEqual('available', snapshot['properties']['image_state'])
@@ -4186,7 +4189,10 @@ class LibvirtConnTestCase(test.TestCase,
 
         mock_lookupByName.assert_called_once_with("instance-00000001")
         mock_volume_info.assert_has_calls(mock_volume_info_calls)
-        mock_convert_image.assert_called_once()
+        mock_convert_image.assert_called_once_with('/dev/nova-vg/lv',
+                                                   mock.ANY,
+                                                   'qcow2',
+                                                   run_as_root=True)
         snapshot = image_service.show(context, recv_meta['id'])
         mock_update_task_state.assert_has_calls(update_task_state_calls)
         self.assertEqual('available', snapshot['properties']['image_state'])
@@ -9870,7 +9876,7 @@ Active:          8381604 kB
                             "host1.example.com",
                             lambda x: x,
                             lambda x: x)
-        mock_spawn.assert_called_once()
+        self.assertEqual(1, mock_spawn.call_count)
 
     @mock.patch.object(greenthread, "spawn")
     @mock.patch.object(fake_libvirt_utils, "is_valid_hostname")