Skip to content

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.

Sign up for GitHub

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

Merged
cjw296 merged 6 commits into from
Aug 29, 2019
Merged

bpo-36871: Ensure method signature is used when asserting mock calls to a method #13261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
acfe960
Fix call_matcher for mock when using methods
tirkarthi May 12, 2019
4788924
Add NEWS entry
tirkarthi May 12, 2019
5e9e4e6
Use None check and convert doctest to unittest
tirkarthi May 12, 2019
ec2daf5
Use better name for mock in tests. Handle _SpecState when the attribu…
tirkarthi May 13, 2019
01aac71
Use reset_mock instead of reinitialization. Change inner class constr…
tirkarthi Jun 24, 2019
67c9299
Reword comment regarding call object lookup logic
tirkarthi Jul 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Use better name for mock in tests. Handle _SpecState when the attribu… 
…te was not accessed and add tests.
  • Loading branch information
@tirkarthi
tirkarthi committed May 13, 2019
commit ec2daf55ed1237720740e33c7eccccc807af4115
10 changes: 5 additions & 5 deletions Lib/unittest/mock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,9 @@ def _get_call_signature_from_name(self, name):
with tuples then return the self's spec signature.

@cjw296 cjw296 Jul 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really follow what this sentence is saying, could it be reworded?

@tirkarthi tirkarthi Jul 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reworded the comment. Feel free to suggest if it can be rephrased better. An explanation using examples would be as below but I am not sure of using examples in the code comment.

  • If an assertion is made against a method like obj.meth1.assert_has_calls([call(1)] here call(1) object has no name to lookup and hence just return obj.meth1._spec_signature.
  • If an assertion is made against the obj like obj.assert_has_calls([call.meth1(1)]) here call.meth1(1) will have name 'meth1' that should be used to return signature of obj.meth1 unlike the bug where obj signature was returned.

Thanks

If the name is not empty then remove () and split by '.' to get
list of names to iterate through the children until a potential

@cjw296 cjw296 Jul 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much clearer, thanks!

match is found.
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
Expand All @@ -754,13 +756,11 @@ def _get_call_signature_from_name(self, name):

for name in names:
child = children.get(name)
if child is None:
if child is None or isinstance(child, _SpecState):
break
else:
children = child._mock_children.get(name)
children = child._mock_children
sig = child._spec_signature
if children is None:
break

return sig

Expand Down
23 changes: 14 additions & 9 deletions Lib/unittest/test/testmock/testmock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1339,31 +1339,36 @@ def test_assert_has_calls(self):
def test_assert_has_calls_nested_spec(self):
class Something:

def __init__(self, a): pass
def __init__(self): pass
def meth(self, a, b, c, d=None): pass

class Foo:

def __init__(self): pass
def meth1(self, a, b): pass

mock = create_autospec(Something)
mock.meth(1, 2, 3, d=1)
mock_class = create_autospec(Something)

mock.assert_has_calls([call.meth(1, 2, 3, d=1)])
mock.assert_has_calls([call.meth(1, 2, 3, 1)])
for m in [mock_class, mock_class()]:
m.meth(1, 2, 3, d=1)

@cjw296 cjw296 Jul 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused: for the mock_class case, why is this expected to work? meth isn't a classmethod...

@tirkarthi tirkarthi Jul 3, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

If a class is used as a spec then the return value of the mock (the instance of the class) will have the same spec. You can use a class as the spec for an instance object by passing instance=True. The returned mock will only be callable if instances of the mock are callable.

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)
./python.exe /tmp/bar.py
(a)
[call.meth1(1)]
(a)
[call.meth1(1)]

Similar test :

cpython/Lib/unittest/test/testmock/testhelpers.py

Lines 533 to 543 in 0f6f73f

def test_method_calls(self):
class Sub(SomeClass):
attr = SomeClass()
mock = create_autospec(Sub)
mock.one(1, 2)
mock.two()
mock.three(3)
expected = [call.one(1, 2), call.two(), call.three(3)]
self.assertEqual(mock.method_calls, expected)

@cjw296 cjw296 Jul 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 one, two or three attributes when the original does not? @voidspace?

@tirkarthi tirkarthi Jul 3, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one, two and three are methods of SomeClass that is inherited by Sub

@cjw296 cjw296 Jul 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 = create_autospec(Something)
mock.Foo().meth1(1, 2)
mock_class = create_autospec(Something)

mock.assert_has_calls([call.Foo(), call.Foo().meth1(1, 2)])
for m in [mock_class, mock_class()]:
self.assertRaises(AssertionError, m.assert_has_calls, [call.Foo()])
m.Foo().meth1(1, 2)
m.assert_has_calls([call.Foo(), call.Foo().meth1(1, 2)])

mock_class = create_autospec(Something)
invalid_calls = [call.meth(1),
call.non_existent(1),
call.Foo().non_existent(1)]

for kall in invalid_calls:
self.assertRaises(AssertionError,
mock.assert_has_calls,
mock_class.assert_has_calls,
[kall]
)

Expand Down