From 9f8cc2f03827c10f89ee652bc73790b3c58111d3 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 23 Aug 2021 19:23:31 +0200 Subject: [PATCH] Add two new hacking rules As the bug and fix If71620e808744736cb4fe3abda76d81a6335311b showed it is dangerous to forget instantiating the Mock class before it is used in the test as changes on the class directly leaks out from the test. In almost all the cases using Mock class directly is a bug and the author original intention is to use an instance instead, just forgot about the parents. So this patch adds two new hacking rules: N367: catches the case when Mock class is aliased in the test: self.mock_mystuff = mock.Mock N368: catches when mock.patch instructed to use the Mock class as replacement value during patching: mock.patch('Bar.foo', new=mock.Mock) For N367 the previous patch removed the last hit. For N368 this patch removes the two hits exists. Change-Id: Id42ca571b1569886ef47aa350369e9d2068e77bc Related-Bug: #1936849 --- HACKING.rst | 3 + nova/hacking/checks.py | 62 +++++++++++++++++++ nova/tests/unit/compute/test_compute.py | 2 +- nova/tests/unit/test_hacking.py | 66 +++++++++++++++++++++ nova/tests/unit/virt/libvirt/test_driver.py | 2 +- tox.ini | 2 + 6 files changed, 135 insertions(+), 2 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 76ff4b6c22d3..0f98901864d6 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 b45f7254b57b..28fbb47a90d3 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 dfbe245027f2..b24759b8696b 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 00daefc19ee3..944083a8f518 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 0fa9c1c72109..b0e0d0410212 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() diff --git a/tox.ini b/tox.ini index 6dfde9afbb64..65d145e40f90 100644 --- a/tox.ini +++ b/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