Open 1f9907dc-f261-461d-a62c-e5e74baf0104 opened 3 years ago
An unfortunately common pattern over large codebases of Python tests is for spec'd Mock instances to be provided with Mock objects as their specs. This gives the false sense that a spec constraint is being applied when, in fact, nothing will be disallowed.
The two most frequently observed occurrences of this anti-pattern are as follows:
def setUp(self):
mock.patch.object(mod, 'Klass', autospec=True).start()
self.addCleanup(mock.patch.stopall)
@mock.patch.object(mod, 'Klass', autospec=True) # :( def testFoo(self, mock_klass): # mod.Klass has no effective spec.
def setUp(self):
mock.patch.object(mod, 'Klass').start()
...
mock_klass = mock.create_autospec(mod.Klass) # :(
# mock_klass has no effective spec
This is fairly easy to detect using _is_instance_mock at patch time however it can break existing tests.
I have a patch ready and it seems like this error case is not frequent enough that it would be disruptive to address.
Another option would be add it as a warning for a version e.g. 3.10 and then potentially make it a breaking change in 3.11. However considering this is a test-only change with a fairly clear path to fix it, that might be overly cautious.
Lets go ahead and try making this a breaking change in 3.10. If users report it causes a bunch of problems during the beta -that they don't want to address so soon- (they are all likely bugs in test suites...) we can soften it to a warning for a cycle. My hope is that we won't need to do that.
For a simple experiment raising an exception I can see two tests failing in test suite that have the pattern of having an autospec which is a mock object.
diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py
index 720f682efb..d33c7899a1 100644
--- a/Lib/unittest/mock.py
+++ b/Lib/unittest/mock.py
@@ -2612,6 +2612,9 @@ def create_autospec(spec, spec_set=False, instance=False, _parent=None,
# interpreted as a list of strings
spec = type(spec)
+ if _is_instance_mock(spec):
+ 1 / 0
+
is_type = isinstance(spec, type)
is_async_func = _is_async_func(spec)
_kwargs = {'spec': spec}
./python -m unittest Lib.unittest.test.testmock ....................................................................................................................................................E..............................................................................................................................................................E........................................................................................................................................................................................... \====================================================================== ERROR: test_bool_not_called_when_passing_spec_arg (unittest.test.testmock.testmock.MockTest) ----------------------------------------------------------------------
Traceback (most recent call last):
File "/root/cpython/Lib/unittest/test/testmock/testmock.py", line 2180, in test_bool_not_called_when_passing_spec_arg
with unittest.mock.patch.object(obj, 'obj_with_bool_func', autospec=True): pass
File "/root/cpython/Lib/unittest/mock.py", line 1503, in __enter__
new = create_autospec(autospec, spec_set=spec_set,
File "/root/cpython/Lib/unittest/mock.py", line 2616, in create_autospec
1 / 0
ZeroDivisionError: division by zero
====================================================================== ERROR: test_create_autospec_awaitable_class (unittest.test.testmock.testasync.AsyncAutospecTest) ----------------------------------------------------------------------
Traceback (most recent call last):
File "/root/cpython/Lib/unittest/test/testmock/testasync.py", line 204, in test_create_autospec_awaitable_class
self.assertIsInstance(create_autospec(awaitable_mock), AsyncMock)
File "/root/cpython/Lib/unittest/mock.py", line 2616, in create_autospec
1 / 0
ZeroDivisionError: division by zero
Ran 495 tests in 2.039s
FAILED (errors=2)
I agree that this should raise an exception. Can the two failing tests in mock's own suite be easily fixed?
I've fixed a bunch of these in our internal repo so I'd be happy to add that to a patch implementing raising exceptions for these cases.
The tests can be fixed. The change to raise exception can be merged to gather feedback during alpha/beta and see if there are any valid usecases.
A few more things:
Assertions on Mock-autospec'ed Mocks will silently pass since e.g. assert_called_once_with will now be mocked out. This may justify a more stringent stance on the pattern since it risks hiding real test failures.
One complicating factor with the implementation is that autospec'd objects may have children that are already Mocked out. Failing eagerly, however, would break cases where the child is never used (and consequently risk causing more friction than may be necessary) but failing only upon access of that child makes it trickier for the user to trace back to where the parent was mocked.
And to give some context for the above autospec child bit, this is the relevant code that determines the spec to use for each child: https://github.com/python/cpython/blob/master/Lib/unittest/mock.py#L2671-L2696
New changeset dccdc500f9b5dab0a20407ae0178d393796a8828 by Matthew Suozzo in branch 'master': bpo-43478: Restrict use of Mock objects as specs (GH-25326) https://github.com/python/cpython/commit/dccdc500f9b5dab0a20407ae0178d393796a8828
New changeset 6e468cb16bde483ad73c1eb13b20a08d74e30846 by Pablo Galindo in branch 'master': bpo-43478: Fix formatting of NEWS entry (GH-25335) https://github.com/python/cpython/commit/6e468cb16bde483ad73c1eb13b20a08d74e30846
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields: ```python assignee = None closed_at = None created_at =
labels = ['type-feature', 'tests', '3.10']
title = 'Disallow Mock spec arguments from being Mocks'
updated_at =
user = 'https://bugs.python.org/msuozzo'
```
bugs.python.org fields:
```python
activity =
actor = 'matthew.suozzo'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Tests']
creation =
creator = 'msuozzo'
dependencies = []
files = []
hgrepos = []
issue_num = 43478
keywords = ['patch']
message_count = 10.0
messages = ['388532', '388753', '388755', '388766', '388767', '388769', '388878', '388880', '390690', '390743']
nosy_count = 10.0
nosy_names = ['gregory.p.smith', 'vstinner', 'cjw296', 'michael.foord', 'lisroach', 'mariocj89', 'pablogsal', 'xtreak', 'msuozzo', 'matthew.suozzo']
pr_nums = ['25326', '25335', '25227', '31090']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue43478'
versions = ['Python 3.10']
```