Merge "Add two new hacking rules"

HACKING.rst

@@ -68,6 +68,9 @@ Nova Specific Commandments
 - [N364] Check non-existent mock assertion methods and attributes.
 - [N365] Check misuse of assertTrue/assertIsNone.
 - [N366] The assert_has_calls is a method rather than a variable.
+- [N367] Disallow aliasing the mock.Mock and similar classes in tests.
+- [N368] Reject if the mock.Mock class is used as a replacement value instead of and
+  instance of a mock.Mock during patching in tests.
 
 Creating Unit Tests
 -------------------

nova/hacking/checks.py

@@ -130,6 +130,13 @@ useless_assertion_re = re.compile(
 # Regex for misuse of assert_has_calls
 mock_assert_has_calls_re = re.compile(r"\.assert_has_calls\s?=")
 
+# Regex for catching aliasing mock.Mock class in test
+mock_class_aliasing_re = re.compile(
+    r"^[A-Za-z0-9_.]+\s*=\s*mock\.(Magic|NonCallable)?Mock$")
+# Regex for catching aliasing mock.Mock class in test
+mock_class_as_new_value_in_patching_re = re.compile(
+    r"mock\.patch(\.object)?.* new=mock\.(Magic|NonCallable)?Mock[^(]")
+
 
 class BaseASTChecker(ast.NodeVisitor):
     """Provides a simple framework for writing AST-based checks.
@@ -939,3 +946,58 @@ def check_assert_has_calls(logical_line, filename):
     if ('nova/tests/' in filename and
             mock_assert_has_calls_re.search(logical_line)):
         yield (0, msg)
+
+
+@core.flake8ext
+def do_not_alias_mock_class(logical_line, filename):
+    """Check for aliasing Mock class
+
+    Aliasing Mock class almost always a bad idea. Consider the test code
+    trying to catch the instantiation of the Rados class but instead
+    introducing a global change on the Mock object:
+    https://github.com/openstack/nova/blob/10b1dc84f47a71061340f8e0ae0fe32dca44061a/nova/tests/unit/storage/test_rbd.py#L122-L125
+    After this code every test that assumes that mock.Mock().shutdown is a new
+    auto-generated mock.Mock() object will fail a shutdown is now defined in
+    the Mock class level and therefore surviving between test cases.
+
+    N367
+    """
+    if 'nova/tests/' in filename:
+        res = mock_class_aliasing_re.match(logical_line)
+        if res:
+            yield (
+                0,
+                "N367: Aliasing mock.Mock class is dangerous as it easy to "
+                "introduce class level changes in Mock that survives "
+                "between test cases. If you want to mock object creation "
+                "then mock the class under test with a mock _instance_ and "
+                "set the return_value of the mock to return mock instances. "
+                "See for example: "
+                "https://review.opendev.org/c/openstack/nova/+/805657"
+            )
+
+
+@core.flake8ext
+def do_not_use_mock_class_as_new_mock_value(logical_line, filename):
+    """Check if mock.Mock class is used during set up of a patcher as new
+    kwargs.
+
+    The mock.patch and mock.patch.object takes a `new` kwargs and use that
+    value as the replacement during the patching. Using new=mock.Mock
+    (instead of new=mock.Mock() or new_callable=mock.Mock) results in code
+    under test pointing to the Mock class. This is almost always a wrong thing
+    as any changes on that class will leak between test cases uncontrollably.
+
+    N368
+    """
+    if 'nova/tests/' in filename:
+        res = mock_class_as_new_value_in_patching_re.search(logical_line)
+        if res:
+            yield (
+                0,
+                "N368: Using mock.patch(..., new=mock.Mock) causes that the "
+                "patching will introduce the Mock class as replacement value "
+                "instead of a mock object. Any change on the Mock calls will "
+                "leak out from the test and can cause interference. "
+                "Use new=mock.Mock() or new_callable=mock.Mock instead."
+            )

nova/tests/unit/compute/test_compute.py

@@ -10936,7 +10936,7 @@ class ComputeAPITestCase(BaseTestCase):
             mock.patch(
                 'nova.compute.utils.'
                 'update_pci_request_spec_with_allocated_interface_name',
-                new=mock.NonCallableMock),
+                new=mock.NonCallableMock()),
         ) as (
             mock_get_nodename, mock_get_alloc_candidates, mock_add_res,
             mock_update_pci

nova/tests/unit/test_hacking.py

@@ -933,3 +933,69 @@ class HackingTestCase(test.NoDBTestCase):
         self._assert_has_no_errors(
             code, checks.check_assert_has_calls,
             filename="nova/tests/unit/test_context.py")
+
+    def test_do_not_alias_mock_class(self):
+        code = """
+                    my_var = mock.Mock
+                    self.mock_rados.Rados = mock.Mock
+                    self.mock_rados.Rados = mock.MagicMock
+                    self.mock_rados.Rados = mock.NonCallableMock
+               """
+        errors = [(x + 1, 0, 'N367') for x in range(4)]
+        # Check errors in 'nova/tests' directory.
+        self._assert_has_errors(
+            code, checks.do_not_alias_mock_class,
+            expected_errors=errors, filename="nova/tests/unit/test_context.py")
+        # Check no errors in other than 'nova/tests' directory.
+        self._assert_has_no_errors(
+            code, checks.do_not_alias_mock_class,
+            filename="nova/compute/api.py")
+        code = """
+                    my_var = mock.Mock()
+                    self.mock_rados.Rados = mock.Mock()
+                    self.mock_rados.Rados = mock.MagicMock()
+                    self.mock_rados.Rados = mock.NonCallableMock()
+               """
+        self._assert_has_no_errors(
+            code, checks.do_not_alias_mock_class,
+            filename="nova/tests/unit/test_context.py")
+
+    def test_do_not_use_mock_class_as_new_mock_value(self):
+        code = """
+                    patcher = mock.patch('Bar.foo', new=mock.Mock)
+                    patcher = mock.patch.object(Bar, 'foo', new=mock.Mock)
+                    patcher = mock.patch('Bar.foo', new=mock.MagicMock)
+                    patcher = mock.patch('Bar.foo', new=mock.NonCallableMock)
+                    @mock.patch('Bar.foo', new=mock.Mock)
+                    def a():
+                        pass
+
+                    with mock.patch('Bar.foo', new=mock.Mock) as m:
+                        pass
+               """
+        errors = [(x + 1, 0, 'N368') for x in range(5)] + [(9, 0, 'N368')]
+        # Check errors in 'nova/tests' directory.
+        self._assert_has_errors(
+            code, checks.do_not_use_mock_class_as_new_mock_value,
+            expected_errors=errors, filename="nova/tests/unit/test_context.py")
+        # Check no errors in other than 'nova/tests' directory.
+        self._assert_has_no_errors(
+            code, checks.do_not_use_mock_class_as_new_mock_value,
+            filename="nova/compute/api.py")
+        code = """
+                    patcher = mock.patch('Bar.foo', new=mock.Mock())
+                    patcher = mock.patch.object(Bar, 'foo', new=mock.Mock())
+                    patcher = mock.patch('Bar.foo', new=mock.MagicMock())
+                    patcher = mock.patch('Bar.foo', new=mock.NonCallableMock())
+                    @mock.patch('Bar.foo', new=mock.Mock())
+                    def a():
+                        pass
+
+                    with mock.patch('Bar.foo', new=mock.Mock()) as m:
+                        pass
+
+                    patcher = mock.patch('Bar.foo', new_callable=mock.Mock)
+               """
+        self._assert_has_no_errors(
+            code, checks.do_not_use_mock_class_as_new_mock_value,
+            filename="nova/tests/unit/test_context.py")

nova/tests/unit/virt/libvirt/test_driver.py

@@ -23228,7 +23228,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin):
         self._test_attach_interface(
             power_state.SHUTDOWN, fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG)
 
-    @mock.patch('threading.Event.wait', new=mock.Mock)
+    @mock.patch('threading.Event.wait', new=mock.Mock())
     def _test_detach_interface(self, state, device_not_found=False):
         # setup some mocks
         instance = self._create_instance()

tox.ini

@@ -317,6 +317,8 @@ extension =
   N364 = checks:nonexistent_assertion_methods_and_attributes
   N365 = checks:useless_assertion
   N366 = checks:check_assert_has_calls
+  N367 = checks:do_not_alias_mock_class
+  N368 = checks:do_not_use_mock_class_as_new_mock_value
 paths =
   ./nova/hacking