Disallow Mock spec arguments from being Mocks

[msuozzo]

BPO	43478
Nosy	@gpshead, @vstinner, @cjw296, @voidspace, @lisroach, @mariocj89, @pablogsal, @tirkarthi, @msuozzo
PRs	bpo-43478: Restrict use of Mock objects as specs #25326 bpo-43478: Fix formatting of NEWS entry #25335 bpo-43753: Add Py_Is() and Py_IsNone() functions #25227 bpo-43478: Restrict use of Mock objects as specs #31090

^{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:

assignee = None
closed_at = None
created_at = <Date 2021-03-12.01:53:43.131>
labels = ['type-feature', 'tests', '3.10']
title = 'Disallow Mock spec arguments from being Mocks'
updated_at = <Date 2022-02-02.23:26:50.979>
user = 'https://bugs.python.org/msuozzo'

bugs.python.org fields:

activity = <Date 2022-02-02.23:26:50.979>
actor = 'matthew.suozzo'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Tests']
creation = <Date 2021-03-12.01:53:43.131>
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']

[msuozzo]

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:

• Re-patching an existing autospec.

  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.

• Deriving an autospec Mock from an already-mocked object

  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.

[gpshead]

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.

[tirkarthi]

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)

[cjw296]

I agree that this should raise an exception.
Can the two failing tests in mock's own suite be easily fixed?

[msuozzo]

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.

[tirkarthi]

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.

[msuozzo]

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.

[msuozzo]

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

[gpshead]

New changeset dccdc50 by Matthew Suozzo in branch 'master':
bpo-43478: Restrict use of Mock objects as specs (GH-25326)
dccdc50

[pablogsal]

New changeset 6e468cb by Pablo Galindo in branch 'master':
bpo-43478: Fix formatting of NEWS entry (GH-25335)
6e468cb