classification
Title: Misleading error from unittest.mock's assert_has_calls
Type: behavior Stage: commit review
Components: Library (Lib) Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: cjw296, gregory.p.smith, lisroach, mariocj89, michael.foord, miss-islington, sfreilich, xtreak
Priority: normal Keywords: patch

Created on 2019-05-10 01:20 by gregory.p.smith, last changed 2019-09-25 05:39 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
mock_call_test.py gregory.p.smith, 2019-05-10 01:20
Pull Requests
URL Status Linked Edit
PR 13261 merged xtreak, 2019-05-12 07:30
PR 15577 merged miss-islington, 2019-08-29 06:09
PR 15578 merged miss-islington, 2019-08-29 06:09
PR 16005 merged sfreilich, 2019-09-11 19:23
PR 16361 merged sfreilich, 2019-09-24 20:36
PR 16364 merged miss-islington, 2019-09-24 22:05
PR 16371 closed miss-islington, 2019-09-25 04:07
PR 16372 closed miss-islington, 2019-09-25 04:07
PR 16374 merged gregory.p.smith, 2019-09-25 04:29
Messages (20)
msg342028 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-05-10 01:20
Thing: <class '__main__.Thing'>
mock_thing.method sig=(a=None, b=0)
F
======================================================================
FAIL: test_has_calls_on_thing (__main__.MockCallTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/home/gps/mock_call_test.py", line 25, in test_has_calls_on_thing
    mock_thing.assert_has_calls([
  File "/usr/local/google/home/gps/oss/cpython/gpshead/Lib/unittest/mock.py", line 843, in assert_has_calls
    raise AssertionError(
AssertionError: Calls not found.
Expected: [call.method(0.5, b=3000), call.method(0.6, b=6000), call.method(0.7, b=9000)]
Actual: [call.method(0.5, b=3000), call.method(0.6, b=6000), call.method(0.7, b=9000)].

See the attached mock_call_test.py.
msg342030 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-05-10 01:24
If you use a debugger on this, you'll discover that what is happening inside of mock's assert_has_calls is that it is catching and swallowing an exception around the inspect.Signature instances bind() call complaining about 'b' being an unexpected keyword argument.  Diving within the signature object being _used_ at the time, you find it checking against the signature of the class *constructor* rather than the methods. (!)

the real error here is that the mock.create_autospec(Thing) has created a mock of the class.  but the mock class (mock_thing) was never constructed.  Adding a call to the constructor makes this code work:

 mock_thing = mock.create_autospec(Thing)(constructor_param="spam")

But debugging this is insane, the error message is entirely misleading and shows two equal lists yet claiming they are different.
msg342241 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-05-12 05:28
This is little tricky to fix since the call-matcher needs the signature and also the information over whether to add self or not for some cases. Similar open issues.

https://bugs.python.org/issue27715
https://bugs.python.org/issue26752

An approach suggested by Mario : https://bugs.python.org/issue26752#msg337023
msg342245 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-05-12 08:16
I have created a PR for this. My approach is that when mock_thing.assert_has_calls(mock.call.method1(1, 2)) is made the assert_has_calls is made against mock_thing whose spec_signature (constructor signature) is always used. But there is _mock_children, a dictionary which has signature for the methods. Thus method1 can be used as key for _mock_children to get correct signature. 

Where it gets tricky is that if there is a nested class whose method is called like Foo.Bar.meth1 as below then the dictionary has only 'Bar' from which the children has to be obtained to get meth1 signature like {'Foo': {'bar1': signature}} and it's not stored with the key 'Foo.Bar.meth1' resulting in iteration. There could be a better way or some edge case not covered so I have opened up PR for review but if someone else has better approach then that would be great too since this is a long standing issue resulting autospec needing to be turned off.

class Foo:
    class Bar:
        def meth1(self, a): pass


This PR also solves the case at https://bugs.python.org/issue26752#msg287728. There is a test failure caught by doctest for nested calls without spec and not by unittest :) I have converted the doctest as a unittest.
msg350725 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2019-08-29 06:09
New changeset c96127821ebda50760e788b1213975a0d5bea37f by Chris Withers (Xtreak) in branch 'master':
bpo-36871: Ensure method signature is used when asserting mock calls to a method (GH13261)
https://github.com/python/cpython/commit/c96127821ebda50760e788b1213975a0d5bea37f
msg350737 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2019-08-29 06:57
New changeset be310e03d0b84ef56e9d35b0b1b21d685b7ea371 by Chris Withers (Miss Islington (bot)) in branch '3.7':
bpo-36871: Ensure method signature is used when asserting mock calls to a method (GH15577)
https://github.com/python/cpython/commit/be310e03d0b84ef56e9d35b0b1b21d685b7ea371
msg350738 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2019-08-29 06:58
New changeset 38d311d79e57479f7a684c2cd298293033dc4990 by Chris Withers (Miss Islington (bot)) in branch '3.8':
bpo-36871: Ensure method signature is used when asserting mock calls to a method (GH15578)
https://github.com/python/cpython/commit/38d311d79e57479f7a684c2cd298293033dc4990
msg350739 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-08-29 07:03
Closing this as fixed since all PRs are merged. Thank you all :)
msg352016 - (view) Author: Samuel Freilich (sfreilich) * Date: 2019-09-11 19:07
This is still not totally fixed. This solves the underlying error with method specs, but not the bug that was causing the error-swallowing in assert_has_calls:

https://github.com/python/cpython/blob/ee536b2020b1f0baad1286dbd4345e13870324af/Lib/unittest/mock.py#L2216

expected = [self._call_matcher(c) for c in calls]
cause = expected if isinstance(expected, Exception) else None

isinstance(expected, Exception) is never true, because expected is always a list. It should instead be:

cause = next((e for e in expected if isinstance(e, Exception)), None)
msg352020 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-09-11 20:02
It was a conscious decision over not to do the proposed change in the issue. There are other issues over signature difference during construction of mock and in the call_matcher over presence/absence of self. It needs a more careful fix.
msg352022 - (view) Author: Samuel Freilich (sfreilich) * Date: 2019-09-11 20:41
Sure, but the bug in error-handling should still be fixed. Currently, if _call_matcher returns an exception, that's ignored by assert_has_calls, and the error message generated as a result is extremely misleading!
msg352079 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-09-12 09:39
i reopened this without diving into the code to better understand based on Samuels comment.

We could really do with a testcase that demonstrates the misleading error message problem for some test driven development here.
msg352106 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2019-09-12 11:04
The code around whether or not to swallow self is hairy. Even if the original spec object is a class we may still be mocking an instance of the class (we don't want users to have to create an instance just to be able to use it as a spec). So we have to carry metadata about whether or not we're mocking an instance. But we also have to support the use case of when users are mocking an actual class object. 

This gets potentially recursive in the case of autospec and has to apply to the class object itself. If we're mocking an instance that is callable then the signature on the top level mock should be taken from __call__. If we're mocking the constructor the signature comes from __init__.

So it's all complicated. And when I originally wrote the code it was worse as it predated inspect.Signature (and was one of the driving use cases for it) and created functions with the right signature by exec'ing code.

So it's better code than it used to be, but I'm still scared of it and that particular bug came in the switch to use sig.bind which I didn't write.
msg352108 - (view) Author: Samuel Freilich (sfreilich) * Date: 2019-09-12 11:09
Check out my PR, which solves a much smaller issue: It fixes a bug in the exception raising logic in assert_has_calls (and _awaits) which makes complicated problems like the one you mention much harder to debug.

Also it has tests!
msg352119 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2019-09-12 11:23
(The code that generated functions was originally borrowed from the decorator module by Michele Simionato. When I first started in Python, around 2002, I was impressed by the Python community as it had two very prominent women amongst the part of the community I inhabited. Nicola Larosa and Michele Simionato. It turned out they were both Italian men.)
msg353113 - (view) Author: miss-islington (miss-islington) Date: 2019-09-24 19:08
New changeset b5a7a4f0c20717a4c92c371583b5521b83f40f32 by Miss Islington (bot) (Samuel Freilich) in branch 'master':
bpo-36871: Handle spec errors in assert_has_calls (GH-16005)
https://github.com/python/cpython/commit/b5a7a4f0c20717a4c92c371583b5521b83f40f32
msg353120 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-09-24 22:04
New changeset 2180f6b058effbf49ec819f7cedbe76ddd4b700c by Gregory P. Smith (Samuel Freilich) in branch 'master':
bpo-36871: Avoid duplicated 'Actual:' in assertion message (GH-16361)
https://github.com/python/cpython/commit/2180f6b058effbf49ec819f7cedbe76ddd4b700c
msg353126 - (view) Author: miss-islington (miss-islington) Date: 2019-09-25 00:29
New changeset 1a17a054f6314ce29cd2632c28aeed317a615360 by Miss Islington (bot) in branch '3.8':
[3.8] bpo-36871: Handle spec errors in assert_has_calls (GH-16005) (GH-16364)
https://github.com/python/cpython/commit/1a17a054f6314ce29cd2632c28aeed317a615360
msg353141 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-09-25 05:29
New changeset 4042e1afd252858de53e2b79d946a51a0182d1ba by Gregory P. Smith in branch '3.7':
[3.7] bpo-36871: Handle spec errors in assert_has_calls (GH-16364) (GH-16374)
https://github.com/python/cpython/commit/4042e1afd252858de53e2b79d946a51a0182d1ba
msg353142 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-09-25 05:39
i believe the issues surfaces in this are fixed at this point.

of note, my mock_call_test.py example now passes.

i'm not entirely sure that it _should_ pass though, but that depends on what we want create_autospec of a class to do.  should that autospec'd class require instantiation before methods are called?  _that_ is outside the scope of this issue.  As is, the current behavior is likely what people expect to find convenient.
History
Date User Action Args
2019-09-25 05:39:30gregory.p.smithsetstatus: open -> closed
messages: + msg353142

assignee: michael.foord -> gregory.p.smith
resolution: fixed
stage: patch review -> commit review
2019-09-25 05:29:23gregory.p.smithsetmessages: + msg353141
2019-09-25 04:29:59gregory.p.smithsetpull_requests: + pull_request15954
2019-09-25 04:07:09miss-islingtonsetpull_requests: + pull_request15952
2019-09-25 04:07:03miss-islingtonsetpull_requests: + pull_request15951
2019-09-25 00:29:20miss-islingtonsetmessages: + msg353126
2019-09-24 22:05:53miss-islingtonsetpull_requests: + pull_request15944
2019-09-24 22:04:32gregory.p.smithsetmessages: + msg353120
2019-09-24 20:36:47sfreilichsetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request15942
2019-09-24 19:08:44miss-islingtonsetnosy: + miss-islington
messages: + msg353113
2019-09-12 11:23:24michael.foordsetmessages: + msg352119
2019-09-12 11:09:55sfreilichsetmessages: + msg352108
2019-09-12 11:04:18michael.foordsetmessages: + msg352106
2019-09-12 09:39:26gregory.p.smithsetkeywords: - patch

messages: + msg352079
stage: needs patch -> test needed
2019-09-12 09:37:07gregory.p.smithsetassignee: michael.foord
2019-09-12 09:36:56gregory.p.smithsetstatus: closed -> open
resolution: fixed -> (no value)
stage: resolved -> needs patch
2019-09-11 20:41:35sfreilichsetmessages: + msg352022
2019-09-11 20:02:36xtreaksetmessages: + msg352020
2019-09-11 19:23:59sfreilichsetpull_requests: + pull_request15630
2019-09-11 19:07:16sfreilichsetnosy: + sfreilich
messages: + msg352016
2019-08-29 07:31:42ned.deilysetversions: + Python 3.9, - Python 3.6
2019-08-29 07:03:28xtreaksetstatus: open -> closed
resolution: fixed
messages: + msg350739

stage: patch review -> resolved
2019-08-29 06:58:29cjw296setmessages: + msg350738
2019-08-29 06:57:41cjw296setmessages: + msg350737
2019-08-29 06:09:40miss-islingtonsetpull_requests: + pull_request15254
2019-08-29 06:09:31miss-islingtonsetpull_requests: + pull_request15253
2019-08-29 06:09:05cjw296setmessages: + msg350725
2019-05-12 09:25:10cheryl.sabellasetnosy: + lisroach
2019-05-12 08:16:24xtreaksetnosy: + cjw296, michael.foord, mariocj89
messages: + msg342245
2019-05-12 07:30:59xtreaksetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request13171
2019-05-12 05:28:55xtreaksetnosy: + xtreak
messages: + msg342241
2019-05-10 01:24:07gregory.p.smithsetmessages: + msg342030
2019-05-10 01:20:28gregory.p.smithcreate