diff options
| -rw-r--r-- | HACKING.rst | 3 | ||||
| -rw-r--r-- | nova/hacking/checks.py | 62 | ||||
| -rw-r--r-- | nova/tests/unit/compute/test_compute.py | 2 | ||||
| -rw-r--r-- | nova/tests/unit/test_hacking.py | 66 | ||||
| -rw-r--r-- | nova/tests/unit/virt/libvirt/test_driver.py | 2 | ||||
| -rw-r--r-- | tox.ini | 2 |
6 files changed, 135 insertions, 2 deletions
diff --git a/HACKING.rst b/HACKING.rst index 76ff4b6c22..0f98901864 100644 --- a/HACKING.rst +++ b/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 ------------------- diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index b45f7254b5..28fbb47a90 100644 --- a/nova/hacking/checks.py +++ b/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." + ) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index dfbe245027..b24759b869 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/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 diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index 00daefc19e..944083a8f5 100644 --- a/nova/tests/unit/test_hacking.py +++ b/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") diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 0fa9c1c721..b0e0d04102 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -23300,7 +23300,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() @@ -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 |