-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-36871: Ensure method signature is used when asserting mock calls to a method #13261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
acfe960
4788924
5e9e4e6
ec2daf5
01aac71
67c9299
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -737,14 +737,48 @@ def _format_mock_failure_message(self, args, kwargs): | |||
return message % (expected_string, actual_string) | ||||
|
||||
|
||||
def _get_call_signature_from_name(self, name): | ||||
""" | ||||
* If call objects are asserted against a method/function like obj.meth1 | ||||
then there could be no name for the call object to lookup. Hence just | ||||
return the spec_signature of the method/function being asserted against. | ||||
* If the name is not empty then remove () and split by '.' to get | ||||
list of names to iterate through the children until a potential | ||||
match is found. A child mock is created only during attribute access | ||||
so if we get a _SpecState then no attributes of the spec were accessed | ||||
and can be safely exited. | ||||
""" | ||||
if not name: | ||||
return self._spec_signature | ||||
|
||||
sig = None | ||||
names = name.replace('()', '').split('.') | ||||
children = self._mock_children | ||||
|
||||
for name in names: | ||||
child = children.get(name) | ||||
if child is None or isinstance(child, _SpecState): | ||||
break | ||||
else: | ||||
children = child._mock_children | ||||
sig = child._spec_signature | ||||
|
||||
return sig | ||||
|
||||
|
||||
def _call_matcher(self, _call): | ||||
""" | ||||
Given a call (or simply an (args, kwargs) tuple), return a | ||||
comparison key suitable for matching with other calls. | ||||
This is a best effort method which relies on the spec's signature, | ||||
if available, or falls back on the arguments themselves. | ||||
""" | ||||
sig = self._spec_signature | ||||
|
||||
if isinstance(_call, tuple) and len(_call) > 2: | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: Is being of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, any sequence can be used. It should be a two element sequence args and kwargs or three element sequence in case of name being present. But most examples and tests construct
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like it would be sensible to just state and ensure that these are only ever There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tirkarthi - thoughts on my comment here ^^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though this is an internal method there is a note on the docstring of cpython/Lib/unittest/test/testmock/testmock.py Line 1265 in 9fe42b4
There could be a below test that passes with checking def test_assert_has_calls_tuple(self):
class Something:
def __init__(self): pass
def meth(self, a, b, c, d=None): pass
m = create_autospec(Something)
m.meth(1, 2, 3, 1)
m.assert_has_calls([('meth', (1, 2, 3, 1), {})]) On having a strict check of only using
I am leaning towards removing tuple to support only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, if it being |
||||
sig = self._get_call_signature_from_name(_call[0]) | ||||
else: | ||||
sig = self._spec_signature | ||||
|
||||
if sig is not None: | ||||
if len(_call) == 2: | ||||
name = '' | ||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1336,6 +1336,54 @@ def test_assert_has_calls(self): | |||||||||||||||||||||||
) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
def test_assert_has_calls_nested_spec(self): | ||||||||||||||||||||||||
class Something: | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
def __init__(self): pass | ||||||||||||||||||||||||
def meth(self, a, b, c, d=None): pass | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
class Foo: | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
def __init__(self, a): pass | ||||||||||||||||||||||||
def meth1(self, a, b): pass | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
mock_class = create_autospec(Something) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
for m in [mock_class, mock_class()]: | ||||||||||||||||||||||||
m.meth(1, 2, 3, d=1) | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused: for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I too had the same confusion :) It seems for both class and instance the signature skips self as in the below example. There is some note in docs at https://docs.python.org/3/library/unittest.mock.html#create-autospec but I am also little surprised at this.
from unittest.mock import *
import inspect
class A:
def meth1(self, a): pass
a = create_autospec(A)
a.meth1(1)
print(inspect.signature(a.meth1))
print(a.mock_calls)
# Use as an object
obj = a()
obj.meth1(1)
print(inspect.signature(obj.meth1))
print(obj.mock_calls)
Similar test : cpython/Lib/unittest/test/testmock/testhelpers.py Lines 533 to 543 in 0f6f73f
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unrelated to this PR, but this just feels like a bug to me. If you've autospecced from a class, the mock shouldn't have things that behave like classmethods if those things aren't classmethods on the object the spec is from. The "similar test" you've included is just as confusing: why should the mock have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one, two and three are methods of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! That makes more sense :-) |
||||||||||||||||||||||||
m.assert_has_calls([call.meth(1, 2, 3, d=1)]) | ||||||||||||||||||||||||
m.assert_has_calls([call.meth(1, 2, 3, 1)]) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
mock_class.reset_mock() | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
for m in [mock_class, mock_class()]: | ||||||||||||||||||||||||
self.assertRaises(AssertionError, m.assert_has_calls, [call.Foo()]) | ||||||||||||||||||||||||
m.Foo(1).meth1(1, 2) | ||||||||||||||||||||||||
m.assert_has_calls([call.Foo(1), call.Foo(1).meth1(1, 2)]) | ||||||||||||||||||||||||
m.Foo.assert_has_calls([call(1), call().meth1(1, 2)]) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
mock_class.reset_mock() | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
invalid_calls = [call.meth(1), | ||||||||||||||||||||||||
call.non_existent(1), | ||||||||||||||||||||||||
call.Foo().non_existent(1), | ||||||||||||||||||||||||
call.Foo().meth(1, 2, 3, 4)] | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
for kall in invalid_calls: | ||||||||||||||||||||||||
self.assertRaises(AssertionError, | ||||||||||||||||||||||||
mock_class.assert_has_calls, | ||||||||||||||||||||||||
[kall] | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
def test_assert_has_calls_nested_without_spec(self): | ||||||||||||||||||||||||
m = MagicMock() | ||||||||||||||||||||||||
m().foo().bar().baz() | ||||||||||||||||||||||||
m.one().two().three() | ||||||||||||||||||||||||
calls = call.one().two().three().call_list() | ||||||||||||||||||||||||
m.assert_has_calls(calls) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
def test_assert_has_calls_with_function_spec(self): | ||||||||||||||||||||||||
def f(a, b, c, d=None): pass | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Ensure method signature is used instead of constructor signature of a class | ||
while asserting mock object against method calls. Patch by Karthikeyan | ||
Singaravelan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much clearer, thanks!