openstack/nova
Code Issues Proposed changes

Add two new hacking rules

Browse Source 
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
This commit is contained in:
Balazs Gibizer 2021-08-23 19:23:31 +02:00 committed by Lee Yarwood
parent 7c1ca501ee
commit 9f8cc2f038
6 changed files with 135 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
HACKING.rst
View File

@ -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
-------------------

62
nova/hacking/checks.py
View File

@ -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."
)

2
nova/tests/unit/compute/test_compute.py
View File

@ -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

66
nova/tests/unit/test_hacking.py
View File

@ -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")

2
nova/tests/unit/virt/libvirt/test_driver.py
View File

@ -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()

2
tox.ini
View File

@ -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