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
|
|
msg342028 - (view) |
Author: Gregory P. Smith (gregory.p.smith) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:15 | admin | set | github: 81052 |
2019-09-25 05:39:30 | gregory.p.smith | set | status: open -> closed messages:
+ msg353142
assignee: michael.foord -> gregory.p.smith resolution: fixed stage: patch review -> commit review |
2019-09-25 05:29:23 | gregory.p.smith | set | messages:
+ msg353141 |
2019-09-25 04:29:59 | gregory.p.smith | set | pull_requests:
+ pull_request15954 |
2019-09-25 04:07:09 | miss-islington | set | pull_requests:
+ pull_request15952 |
2019-09-25 04:07:03 | miss-islington | set | pull_requests:
+ pull_request15951 |
2019-09-25 00:29:20 | miss-islington | set | messages:
+ msg353126 |
2019-09-24 22:05:53 | miss-islington | set | pull_requests:
+ pull_request15944 |
2019-09-24 22:04:32 | gregory.p.smith | set | messages:
+ msg353120 |
2019-09-24 20:36:47 | sfreilich | set | keywords:
+ patch stage: test needed -> patch review pull_requests:
+ pull_request15942 |
2019-09-24 19:08:44 | miss-islington | set | nosy:
+ miss-islington messages:
+ msg353113
|
2019-09-12 11:23:24 | michael.foord | set | messages:
+ msg352119 |
2019-09-12 11:09:55 | sfreilich | set | messages:
+ msg352108 |
2019-09-12 11:04:18 | michael.foord | set | messages:
+ msg352106 |
2019-09-12 09:39:26 | gregory.p.smith | set | keywords:
- patch
messages:
+ msg352079 stage: needs patch -> test needed |
2019-09-12 09:37:07 | gregory.p.smith | set | assignee: michael.foord |
2019-09-12 09:36:56 | gregory.p.smith | set | status: closed -> open resolution: fixed -> (no value) stage: resolved -> needs patch |
2019-09-11 20:41:35 | sfreilich | set | messages:
+ msg352022 |
2019-09-11 20:02:36 | xtreak | set | messages:
+ msg352020 |
2019-09-11 19:23:59 | sfreilich | set | pull_requests:
+ pull_request15630 |
2019-09-11 19:07:16 | sfreilich | set | nosy:
+ sfreilich messages:
+ msg352016
|
2019-08-29 07:31:42 | ned.deily | set | versions:
+ Python 3.9, - Python 3.6 |
2019-08-29 07:03:28 | xtreak | set | status: open -> closed resolution: fixed messages:
+ msg350739
stage: patch review -> resolved |
2019-08-29 06:58:29 | cjw296 | set | messages:
+ msg350738 |
2019-08-29 06:57:41 | cjw296 | set | messages:
+ msg350737 |
2019-08-29 06:09:40 | miss-islington | set | pull_requests:
+ pull_request15254 |
2019-08-29 06:09:31 | miss-islington | set | pull_requests:
+ pull_request15253 |
2019-08-29 06:09:05 | cjw296 | set | messages:
+ msg350725 |
2019-05-12 09:25:10 | cheryl.sabella | set | nosy:
+ lisroach
|
2019-05-12 08:16:24 | xtreak | set | nosy:
+ cjw296, michael.foord, mariocj89 messages:
+ msg342245
|
2019-05-12 07:30:59 | xtreak | set | keywords:
+ patch stage: needs patch -> patch review pull_requests:
+ pull_request13171 |
2019-05-12 05:28:55 | xtreak | set | nosy:
+ xtreak messages:
+ msg342241
|
2019-05-10 01:24:07 | gregory.p.smith | set | messages:
+ msg342030 |
2019-05-10 01:20:28 | gregory.p.smith | create | |