From 8ca0fa9d2f4de6e69f0902790432e0ab2f37ba68 Mon Sep 17 00:00:00 2001 From: Chris Withers Date: Mon, 3 Dec 2018 21:31:37 +0000 Subject: [PATCH] bpo-35226: Fix equality for nested unittest.mock.call objects. (#10555) Also refactor the call recording imolementation and add some notes about its limitations. --- Doc/library/unittest.mock-examples.rst | 9 +++ Doc/library/unittest.mock.rst | 13 +++++ Lib/unittest/mock.py | 57 +++++++++++-------- Lib/unittest/test/testmock/testhelpers.py | 16 ++++++ Lib/unittest/test/testmock/testmock.py | 51 +++++++++++++++++ .../2018-11-15-07-14-32.bpo-35226.wJPEEe.rst | 3 + 6 files changed, 125 insertions(+), 24 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-11-15-07-14-32.bpo-35226.wJPEEe.rst diff --git a/Doc/library/unittest.mock-examples.rst b/Doc/library/unittest.mock-examples.rst index 60db4c2ba4d..16690f34982 100644 --- a/Doc/library/unittest.mock-examples.rst +++ b/Doc/library/unittest.mock-examples.rst @@ -166,6 +166,15 @@ You use the :data:`call` object to construct lists for comparing with >>> mock.mock_calls == expected True +However, parameters to calls that return mocks are not recorded, which means it is not +possible to track nested calls where the parameters used to create ancestors are important: + + >>> m = Mock() + >>> m.factory(important=True).deliver() + + >>> m.mock_calls[-1] == call.factory(important=False).deliver() + True + Setting Return Values and Attributes ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/Doc/library/unittest.mock.rst b/Doc/library/unittest.mock.rst index 0ae29546586..bfab00eb751 100644 --- a/Doc/library/unittest.mock.rst +++ b/Doc/library/unittest.mock.rst @@ -702,6 +702,19 @@ the *new_callable* argument to :func:`patch`. unpacked as tuples to get at the individual arguments. See :ref:`calls as tuples `. + .. note:: + + The way :attr:`mock_calls` are recorded means that where nested + calls are made, the parameters of ancestor calls are not recorded + and so will always compare equal: + + >>> mock = MagicMock() + >>> mock.top(a=3).bottom() + + >>> mock.mock_calls + [call.top(a=3), call.top().bottom()] + >>> mock.mock_calls[-1] == call.top(a=-1).bottom() + True .. attribute:: __class__ diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index 9547b1a1fad..d13f7498208 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -977,46 +977,51 @@ def _mock_call(_mock_self, *args, **kwargs): self = _mock_self self.called = True self.call_count += 1 - _new_name = self._mock_new_name - _new_parent = self._mock_new_parent + # handle call_args _call = _Call((args, kwargs), two=True) self.call_args = _call self.call_args_list.append(_call) - self.mock_calls.append(_Call(('', args, kwargs))) seen = set() - skip_next_dot = _new_name == '()' + + # initial stuff for method_calls: do_method_calls = self._mock_parent is not None - name = self._mock_name + method_call_name = self._mock_name + + # initial stuff for mock_calls: + mock_call_name = self._mock_new_name + is_a_call = mock_call_name == '()' + self.mock_calls.append(_Call(('', args, kwargs))) + + # follow up the chain of mocks: + _new_parent = self._mock_new_parent while _new_parent is not None: - this_mock_call = _Call((_new_name, args, kwargs)) - if _new_parent._mock_new_name: - dot = '.' - if skip_next_dot: - dot = '' - - skip_next_dot = False - if _new_parent._mock_new_name == '()': - skip_next_dot = True - - _new_name = _new_parent._mock_new_name + dot + _new_name + # handle method_calls: if do_method_calls: - if _new_name == name: - this_method_call = this_mock_call - else: - this_method_call = _Call((name, args, kwargs)) - _new_parent.method_calls.append(this_method_call) - + _new_parent.method_calls.append(_Call((method_call_name, args, kwargs))) do_method_calls = _new_parent._mock_parent is not None if do_method_calls: - name = _new_parent._mock_name + '.' + name + method_call_name = _new_parent._mock_name + '.' + method_call_name + # handle mock_calls: + this_mock_call = _Call((mock_call_name, args, kwargs)) _new_parent.mock_calls.append(this_mock_call) + + if _new_parent._mock_new_name: + if is_a_call: + dot = '' + else: + dot = '.' + is_a_call = _new_parent._mock_new_name == '()' + mock_call_name = _new_parent._mock_new_name + dot + mock_call_name + + # follow the parental chain: _new_parent = _new_parent._mock_new_parent - # use ids here so as not to call __hash__ on the mocks + # check we're not in an infinite loop: + # ( use ids here so as not to call __hash__ on the mocks) _new_parent_id = id(_new_parent) if _new_parent_id in seen: break @@ -2054,6 +2059,10 @@ def __eq__(self, other): else: self_name, self_args, self_kwargs = self + if (getattr(self, 'parent', None) and getattr(other, 'parent', None) + and self.parent != other.parent): + return False + other_name = '' if len_other == 0: other_args, other_kwargs = (), {} diff --git a/Lib/unittest/test/testmock/testhelpers.py b/Lib/unittest/test/testmock/testhelpers.py index 9edebf55166..9388311e3e2 100644 --- a/Lib/unittest/test/testmock/testhelpers.py +++ b/Lib/unittest/test/testmock/testhelpers.py @@ -270,6 +270,22 @@ def test_extended_call(self): self.assertEqual(mock.mock_calls, last_call.call_list()) + def test_extended_not_equal(self): + a = call(x=1).foo + b = call(x=2).foo + self.assertEqual(a, a) + self.assertEqual(b, b) + self.assertNotEqual(a, b) + + + def test_nested_calls_not_equal(self): + a = call(x=1).foo().bar + b = call(x=2).foo().bar + self.assertEqual(a, a) + self.assertEqual(b, b) + self.assertNotEqual(a, b) + + def test_call_list(self): mock = MagicMock() mock(1) diff --git a/Lib/unittest/test/testmock/testmock.py b/Lib/unittest/test/testmock/testmock.py index ac6eea3720b..e46ef7bd5f0 100644 --- a/Lib/unittest/test/testmock/testmock.py +++ b/Lib/unittest/test/testmock/testmock.py @@ -925,6 +925,57 @@ def test_mock_calls(self): call().__int__().call_list()) + def test_child_mock_call_equal(self): + m = Mock() + result = m() + result.wibble() + # parent looks like this: + self.assertEqual(m.mock_calls, [call(), call().wibble()]) + # but child should look like this: + self.assertEqual(result.mock_calls, [call.wibble()]) + + + def test_mock_call_not_equal_leaf(self): + m = Mock() + m.foo().something() + self.assertNotEqual(m.mock_calls[1], call.foo().different()) + self.assertEqual(m.mock_calls[0], call.foo()) + + + def test_mock_call_not_equal_non_leaf(self): + m = Mock() + m.foo().bar() + self.assertNotEqual(m.mock_calls[1], call.baz().bar()) + self.assertNotEqual(m.mock_calls[0], call.baz()) + + + def test_mock_call_not_equal_non_leaf_params_different(self): + m = Mock() + m.foo(x=1).bar() + # This isn't ideal, but there's no way to fix it without breaking backwards compatibility: + self.assertEqual(m.mock_calls[1], call.foo(x=2).bar()) + + + def test_mock_call_not_equal_non_leaf_attr(self): + m = Mock() + m.foo.bar() + self.assertNotEqual(m.mock_calls[0], call.baz.bar()) + + + def test_mock_call_not_equal_non_leaf_call_versus_attr(self): + m = Mock() + m.foo.bar() + self.assertNotEqual(m.mock_calls[0], call.foo().bar()) + + + def test_mock_call_repr(self): + m = Mock() + m.foo().bar().baz.bob() + self.assertEqual(repr(m.mock_calls[0]), 'call.foo()') + self.assertEqual(repr(m.mock_calls[1]), 'call.foo().bar()') + self.assertEqual(repr(m.mock_calls[2]), 'call.foo().bar().baz.bob()') + + def test_subclassing(self): class Subclass(Mock): pass diff --git a/Misc/NEWS.d/next/Library/2018-11-15-07-14-32.bpo-35226.wJPEEe.rst b/Misc/NEWS.d/next/Library/2018-11-15-07-14-32.bpo-35226.wJPEEe.rst new file mode 100644 index 00000000000..b95cc979573 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-11-15-07-14-32.bpo-35226.wJPEEe.rst @@ -0,0 +1,3 @@ +Recursively check arguments when testing for equality of +:class:`unittest.mock.call` objects and add note that tracking of parameters +used to create ancestors of mocks in ``mock_calls`` is not possible.