classification
Title: Disallow Mock spec arguments from being Mocks
Type: enhancement Stage: patch review
Components: Tests Versions: Python 3.10
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: cjw296, gregory.p.smith, lisroach, mariocj89, matthew.suozzo, michael.foord, msuozzo, pablogsal, vstinner, xtreak
Priority: normal Keywords: patch

Created on 2021-03-12 01:53 by msuozzo, last changed 2021-04-10 22:14 by vstinner.

Pull Requests
URL Status Linked Edit
PR 25326 merged matthew.suozzo, 2021-04-10 01:57
PR 25335 merged pablogsal, 2021-04-10 21:13
PR 25227 vstinner, 2021-04-10 22:14
Messages (10)
msg388532 - (view) Author: Matthew Suozzo (msuozzo) Date: 2021-03-12 01:53
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.
msg388753 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-03-15 17:39
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.
msg388755 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2021-03-15 17:49
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)
msg388766 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2021-03-15 19:29
I agree that this should raise an exception.
Can the two failing tests in mock's own suite be easily fixed?
msg388767 - (view) Author: Matthew Suozzo (msuozzo) Date: 2021-03-15 19:34
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.
msg388769 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2021-03-15 19:38
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.
msg388878 - (view) Author: Matthew Suozzo (msuozzo) Date: 2021-03-16 20:44
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.
msg388880 - (view) Author: Matthew Suozzo (msuozzo) Date: 2021-03-16 20:49
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
msg390690 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-04-10 03:45
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
msg390743 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-10 21:19
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
History
Date User Action Args
2021-04-10 22:14:04vstinnersetnosy: + vstinner
pull_requests: + pull_request24071
2021-04-10 21:19:05pablogsalsetmessages: + msg390743
2021-04-10 21:13:36pablogsalsetnosy: + pablogsal
pull_requests: + pull_request24069
2021-04-10 10:29:35shreyanavigyansetnosy: - shreyanavigyan
2021-04-10 10:29:14shreyanavigyansetpull_requests: - pull_request24064
2021-04-10 10:26:49shreyanavigyansetnosy: + shreyanavigyan
pull_requests: + pull_request24064
2021-04-10 03:45:57gregory.p.smithsetmessages: + msg390690
2021-04-10 01:57:35matthew.suozzosetkeywords: + patch
nosy: + matthew.suozzo

pull_requests: + pull_request24061
stage: patch review
2021-03-16 20:49:38msuozzosetmessages: + msg388880
2021-03-16 20:44:24msuozzosetmessages: + msg388878
2021-03-15 19:38:57xtreaksetmessages: + msg388769
2021-03-15 19:34:36msuozzosetmessages: + msg388767
2021-03-15 19:29:56cjw296setmessages: + msg388766
2021-03-15 17:49:02xtreaksetmessages: + msg388755
2021-03-15 17:44:54xtreaksetnosy: + cjw296, michael.foord, lisroach, mariocj89
2021-03-15 17:39:05gregory.p.smithsetmessages: + msg388753
2021-03-15 17:37:14gregory.p.smithsetnosy: + gregory.p.smith
2021-03-12 02:14:47xtreaksetnosy: + xtreak
2021-03-12 01:53:43msuozzocreate