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.

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
merged 6 commits into from
Aug 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 35 additions & 1 deletion Lib/unittest/mock.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
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. 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:
Copy link
Member

Choose a reason for hiding this comment

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

Q: Is being of tuple type actually important, or is any sequence okay?
My assumption: tuple is easy to express and being an internal API we may be able to guarantee the type so this could be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 call which inherits from tuple or tuple as a recommended one.

>>> from unittest.mock import Mock
>>> m = Mock()
>>> m(1)
<Mock name='mock()' id='4554182024'>
>>> m.assert_has_calls([[(1,), {}]])

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@tirkarthi - thoughts on my comment here ^^

Copy link
Member Author

Choose a reason for hiding this comment

The 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 _call_matcher that it could be a tuple. It's not specified in docs that this could be a tuple. I haven't seen a usecase of it in other projects too. There is a test for this usecase but it's not using autospec so it didn't fail with my PR I guess. Relevant test using tuples

((3, 4),), ((), {'a': 3}),

Given a call (or simply an (args, kwargs) tuple), return a
comparison key suitable for matching with other calls.

There could be a below test that passes with checking _call to be a tuple.

    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 _Call it will fail since it will go on to use the constructor signature.

./python.exe -m unittest Lib.unittest.test.testmock.testmock.MockTest.test_assert_has_calls_tuple
F
======================================================================
FAIL: test_assert_has_calls_tuple (Lib.unittest.test.testmock.testmock.MockTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/test/testmock/testmock.py", line 1387, in test_assert_has_calls_tuple
    m.assert_has_calls([('meth', (1, 2, 3, 1), {})])
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 876, in assert_has_calls
    raise AssertionError(
AssertionError: Calls not found.
Expected: [('meth', (1, 2, 3, 1), {})]
Actual: [call.meth(1, 2, 3, 1)].

----------------------------------------------------------------------
Ran 1 test in 0.063s

FAILED (failures=1)

I am leaning towards removing tuple to support only _Call since the error message is also not useful. We can perhaps change it in 3.9 to accept only _Call and then revert back if someone complains that it breaks their workflow given there is plenty of time. I am not sure about backporting it to 3.8 though since beta is out. Would also like to know thoughts from @voidspace since this was added in the original commit where mock was added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, if it being _Call-only causes problems then that's probably a good reason to leave it as it is..

sig = self._get_call_signature_from_name(_call[0])
else:
sig = self._spec_signature

if sig is not None:
if len(_call) == 2:
name = ''
Expand Down
48 changes: 48 additions & 0 deletions Lib/unittest/test/testmock/testmock.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
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...

Copy link
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 :

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)

Copy link
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?

Copy link
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

Copy link
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_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

Expand Down
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.