bpo-35226: Fix equality for nested unittest.mock.call objects. #10555

cjw296 · 2018-11-15T06:57:18Z

https://bugs.python.org/issue35226

the-knights-who-say-ni · 2018-11-15T06:57:20Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

mariocj89 · 2018-11-15T14:10:19Z

Lib/unittest/mock.py

@@ -2054,6 +2054,10 @@ def __eq__(self, other):
        else:
            self_name, self_args, self_kwargs = self

+        if (getattr(self, 'parent', None) and getattr(other, 'parent', None)


self should always have a parent attribute, no need to use getattr, isn't it?

You are using gettattr to ignore the value and then compare it. What if it has a parent but it evaluates Falsy? Might be better to just use hasattr.

if hasattr(other, 'parent') and self.parent != other.parent: return False

I think it reads easier

The implementation was suggested in the bug discussion, @voidspace can make a call here.

I think @mariocj89 has a point. On call() the parent is None.

Is hasattr safe to use here or does it still risk swallowing exceptions other than AttributeError on Python 3.6+? The grumpy old python dev in me still prefers getattr(other, 'parent', None) out of paranoia...

hasattr was fixed quite some time ago I believe :-)

Surprising indeed!

Look at this:

>>> from unittest.mock import Mock >>> m = Mock() >>> m.abc.cde() <Mock name='mock.abc.cde()' id='139944355306408'> >>> m.mock_calls [call.abc.cde()] >>> print(m.mock_calls[0].parent) None

Seems parent is not set at all, which is causing a lot of tests to fail with my proposal :/

Wow. That seems like a bug too, let me add some cases and see about a fix.
@voidspace - open question: shall we keep getattr(other, 'parent', None) to cover the porting-to-testing-cabal version issue?

(just as a note, we can follow this up indeed in another issue as I am not sure about the fix, seems we are not passing the parent here): https://github.com/python/cpython/blob/master/Lib/unittest/mock.py#L986)

I'm afraid it's intrinsically linked to this issue, call children are designed to be compared to items in Mock().method_calls and Mock().mock_calls and I don't think that currently works safely... They'll generally always compare as equal, which isn't what we want with nested/generative calls...

Where there's a single call there's no parent call. So having parent not set in that example looks correct.

mariocj89 · 2018-11-15T14:19:19Z

Lib/unittest/test/testmock/testhelpers.py

@@ -269,6 +269,14 @@ 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


I know it works due to the implementation, but I'd also validate this works recursively for future changes to the module with

a = call(x=1).foo().bar b = call(x=2).foo().bar

cjw296 · 2018-11-28T07:06:24Z

@mariocj89 - okay, so I finally got a chance to come back to this.
So, the reason that calls found in mock_calls don't have parents set is that the process that builds them is fundamentally different. I've added some tests in c704bca to verify that they don't have the issue I was worried they might have.

@voidspace - I'm happy with this PR now, can we go for a merge? Then I'll get the backporting shenanigans done.

cjw296 · 2018-11-28T08:07:48Z

Gah, finally found the case that was the problem :-(

cjw296 · 2018-11-30T08:39:57Z

@mariocj89 / @voidspace - okay, after a good few hours of tinkering and thinkering, I think this PR is good to go.

State of play is as follows:

This PR does fix the issue with unittest.mock.call.
It does not fix the issue with mock_calls, but does add warnings about the problem to the docs.

I don't believe the mock_calls issue can be solved in a backwards compatible way, as a return_value that is a mock is not computed per call, but only once. This means that when we come to record mock_calls on a nested child, we have no way of knowing what parameters were passed to the ancestor call that created it, and we can't start tracking that without fundamentally changing how return_value works in a backwards-incompatible way.

So, if you could review this PR on that basis, and add backport tags to all maintained versions since this is now just a bugfix change, I'd be very grateful.

My plan is to start building a CallTrackingMock based on MagicMock over in https://github.com/Simplistix/testfixtures. Once I'm comfortable that's a good idea, I'll likely do a feature request PR into cpython. If you have thoughts, I'd welcome comments over on simplistix/testfixtures#90.

cjw296 · 2018-12-02T14:32:01Z

@mariocj89 - if you're okay with this, can you approve it so it gets merged? I can then backport to https://github.com/testing-cabal/mock where I'm now a maintainer.

mariocj89

A bunch of comments. I'd personally prefer @vstinner or @voidspace to review this PR as I don't agree with it (now, good news, I am just a random reviewer).

When validating calls in mocks, AFAIK people are expected to use mock_calls and call_list, that works file, see the example of the warnings you put:

>>> m = Mock()
>>> m.factory(important=True).deliver()
<Mock name='mock.factory().deliver()' id='...'>
>>> m.mock_calls[-1] == call.factory(important=False).deliver()
True

I think this is just a wrong usage of call, people should just always call call_list if that is what they want.
This works as expected if the API is used as documetned:

>>> m = Mock()
>>> m.factory(important=True).deliver()
<Mock name='mock.factory().deliver()' id='...'>
>>> m.mock_calls == call.factory(important=False).deliver().call_list()
False

The way call is done, allows you to also have the option of validating the call without caring about the parent argumetns if you wanted.

With that said about the API, I see three problems about this PR though:

It does not address the original concern about the usage (which I disagree with though), and with it the call __eq__ "feels" different. Right now __eq__ on a mock and a call should be done by using a method that returns you all the calls that happend (in case you want to validate all the arguments), this PR removes that option, and what is worse, people might be already using this API with that objective in their minds (which is what it seems to provde). I would not backport this to previous versions of Python.
Adds warnings about the a usage of call that is not the intended per the documentation.
Makes quite a refactor for a PR that plans to backport, I'd just "leave previous versions as they are" to minimize risks... :/ Maybe I am too paranoid.

TL;DR;

I would just change this whole PR for a warning in the call class saying:

When used with nested call, make sure to use call_list() if you want to compare recursively with the value of the argumetns.

Real apologies if you feel I am being "hard on the PR" but I just find things are fine. call(x=1).foo == call(x=2).foo because that is how the API is. If you want to check recursively you should do: call(x=1).foo.call_list() == call(x=2).foo.call_list(). Anyway, as said, just a random reviewer, there are 3 core devs in this PR, one of them the original designer of this API :)

mariocj89 · 2018-12-02T19:18:25Z

Misc/NEWS.d/next/Library/2018-11-15-07-14-32.bpo-35226.wJPEEe.rst

@@ -0,0 +1 @@
+Fix equality for nested unittest.mock.call objects.


I'd try to be more explicit here.

Recursively check arguments when testing for equality of :class:`unittest.mock.call` objects..
As the parent names are already being checked via the name attribute.

Happy to polish this once we get some consensus on this PR.

mariocj89 · 2018-12-02T19:19:42Z

Lib/unittest/mock.py

@@ -2054,6 +2054,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):


this needs one more level of indentation

I don't understand what you'd like here.

the indentation is at the same level as the inner block. PEP8 says it should have one more indentation (4 more spaces)

mariocj89 · 2018-12-02T19:20:39Z

Lib/unittest/test/testmock/testmock.py

+    def test_mock_call_not_equal_leaf(self):
+        m = Mock()
+        m.foo().bar()
+        self.assertNotEqual(m.mock_calls[1], call.foo().baz())


🤣 I've been scratching my head for 3 minutes until I noticed the baz rather than bar

cjw296 · 2018-12-02T21:37:09Z

Lib/unittest/mock.py

+                else:
+                    dot = '.'
+                is_a_call = _new_parent._mock_new_name == '()'
+                mock_call_name = _new_parent._mock_new_name + dot + mock_call_name


mariocj89 · 2018-12-02T19:27:22Z

Lib/unittest/mock.py

+                    dot = ''
+                else:
+                    dot = '.'
+                is_a_call = _new_parent._mock_new_name == '()'


Is this var used?

indeed, I quickly overview it and it is used within the loop.

mariocj89 · 2018-12-02T19:32:06Z

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 tracking
+nested calls where the parameters used to create ancestors are important is not possible:


Not sure what this means:

the parameters used to create ancestors are important is not possible

"tracking nested calls where the parameters used to create ancestors are important is not possible"

OK, I see. But this can be done with mock_calls or call_list isn't it? It just cannot be all extracted from a single call object within mock_calls.

No, it cannot, if it could, I wouldn't have reported the bug, put together a PR and spent several hours trying to figure out a way to do this that was backwards compatible ;-)

OK, I am clearly missing something here.

>>> m = Mock() >>> m.factory(important=True).deliver() <Mock name='mock.factory().deliver()' id='139810163105576'> >>> m.mock_calls == call.factory(important=False).deliver().call_list() False

Note the false at the end. This works AFAIK.

Setting the expectation right works:

>>> m.mock_calls == call.factory(important=True).deliver().call_list() True

A call to any individual mock always returns the same mock (unless you're using a side_effect to return a different one). So you can't track the parent parameters in general as soon as there's more than one call. As noted the parameters to the original call will still be in the full call chain. I think the behaviour here is working as designed it just doesn't solve every use case.

mariocj89 · 2018-12-02T19:35:07Z

Doc/library/unittest.mock-examples.rst

@@ -512,16 +521,6 @@ call::
    >>> something.backend = mock_backend
    >>> something.method()

-Using :attr:`~Mock.mock_calls` we can check the chained call with a single


Why do you remove this? This works fine.

...because it's not safe to use: because the ancestor parameters are not compared, you can easily be fooled into thinking something is being called correctly when it is not.

Maybe I am misunderstanding this whole thing, but what is not safe? How can you be fooled?

just try changing foobar here to foobaz, the assert will fail because this is using call_list

I don't mind a note being added, although perhaps just the note without the warning. call_list is still useful so I don't want to see this removed from the docs.

mariocj89 · 2018-12-02T19:36:10Z

Doc/library/unittest.mock-examples.rst

@@ -1144,6 +1143,15 @@ the ``mock_calls`` attribute on the manager mock:
    >>> manager.mock_calls == expected_calls
    True

+However, parameters to calls that return mocks are not recorded, which means tracking
+nested calls where the parameters used to create ancestors are important is not possible:


Note this is possible, you just need to use call_list(), as it was specified in the docs.

Not across mocks it's not.

What do you mean with across mocks?

Will cover this in a non-in-line response...

mariocj89 · 2018-12-02T19:39:04Z

Doc/library/unittest.mock.rst

+        .. warning::
+
+            The way :attr:`mock_calls` are recorded means that where nested
+            calls are made, the parameters of ancestor calls are not recorded


They are recorded, just as different calls.. :/ I think this is going to be really confusing for the reader.

The parameters are absolutely not recorded in mock_calls, this is the core problem that isn't solvable.

They are! just not in the last one (each call records only its own parameters), look:

>>> mock_backend.mock_calls [call.get_endpoint('foobar'), call.get_endpoint().create_call('spam', 'eggs'), call.get_endpoint().create_call().start_call()]

I'm not sure we're getting anywhere constructive with this, so let's wait for Michael to take a look.

...except when get_endpoint is called more than once.

cjw296 · 2018-12-02T21:37:09Z

Lib/unittest/mock.py

+                else:
+                    dot = '.'
+                is_a_call = _new_parent._mock_new_name == '()'
+                mock_call_name = _new_parent._mock_new_name + dot + mock_call_name


cjw296 · 2018-12-02T22:09:26Z

@vstinner / @voidspace - sorry, I thought @mariocj89 was an owner of unittest.mock now, would either of you be able to look at this? I'm trying trying to get my commit bit recovered that appears to have gotten list in the move to GitHub, but regardless, I'd prefer someone else to let me know I'm not missing anything on this ;-)

My recollection of mock_calls being added to Mock was that it was to be able to track calls across a hierarchy of mocks. I'm fairly sure I was one of the people who requested it, although I can't find the discussion in any list archives so maybe it was in-person at a conference.

I believe the intention was always for nested calls to not compare equal if their parameters were not the same, but since this current behaviour will silently have let this incorrectly pass for many years, I'm not sure anyone would have noticed. I only spotted it after 7 years as I was looking very closely in this area, and I've been a heavy user from this feature since it arrived!

I've now put a great deal of time into thoroughly understanding the problem, and I've summarised the position above. I believe this PR represents the best that can reasonably be achieved without breaking backwards compatibility.

cjw296 · 2018-12-02T22:33:07Z

Here's an example of why I think the current behaviour is dangerous, and needs fixing where possible (which turns out to be only on the _Call side) and warning about on the mock_call side:

>>> from unittest.mock import Mock, call
>>> from subprocess import Popen
>>> Popen = Mock(spec=Popen)
>>> p1 = Popen(['rm', '-rf', '/var'])
>>> p2 = Popen(['echo', 'hello'])
>>> p1.communicate()
<Mock name='mock().communicate()' id='4422077240'>
>>> p2.kill()
<Mock name='mock().kill()' id='4422077184'>
>>> Popen.mock_calls == [call(['rm', '-rf', '/var']),
...                      call(['echo', 'hello']),
...                      call(['echo', 'hello']).communicate(),
...                      call(['rm', '-rf', '/var']).kill()]
True

The intention here is to test that we're killing the rm -rf, hopefully before its too late, and then waiting for the echo to complete, but the bug that we're doing the opposite isn't picked up with this test which will pass either way. Real world issue relating to this came about from trying to fix simplistix/testfixtures#90.

vstinner · 2018-12-03T16:37:23Z

I have been asked to review this change, but I see a lot of comments which are not marked as "resolved" and I don't see any approval, so I prefer to not review this change yet.

voidspace

The doc changes are excessively dramatic. call is still useful, but because of the standard behaviour of mocks (repeated calls will always return the same mock) parent parameters aren't tracked. However the original call is still there in the chain.

Also refactor the call recording imolementation and add some notes about its limitations.

cjw296 · 2018-12-03T18:29:22Z

@voidspace - I believe that addresses all the feedback, let me know your thoughts.

voidspace

LGTM

bedevere-bot · 2018-12-03T21:31:39Z

@cjw296: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2018-12-03T21:31:56Z

Thanks @cjw296 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington · 2018-12-03T21:31:57Z

Thanks @cjw296 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-12-03T21:32:24Z

GH-10878 is a backport of this pull request to the 3.6 branch.

bedevere-bot · 2018-12-03T21:32:33Z

GH-10879 is a backport of this pull request to the 3.7 branch.

…nGH-10555) Also refactor the call recording imolementation and add some notes about its limitations. (cherry picked from commit 8ca0fa9) Co-authored-by: Chris Withers <chris@withers.org>

) Also refactor the call recording implementation and add some notes about its limitations. (cherry picked from commit 8ca0fa9) Co-authored-by: Chris Withers <chris@withers.org>

) Also refactor the call recording imolementation and add some notes about its limitations. (cherry picked from commit 8ca0fa9) Co-authored-by: Chris Withers <chris@withers.org>

the-knights-who-say-ni added the CLA not signed label Nov 15, 2018

bedevere-bot added the awaiting review label Nov 15, 2018

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Nov 15, 2018

cjw296 force-pushed the issue35226_mock_call_equality branch from 36fdd3e to 1732ed1 Compare November 15, 2018 07:15

cjw296 mentioned this pull request Nov 15, 2018

Parallel uses of mock Popen cross-contaminate simplistix/testfixtures#90

Closed

mariocj89 reviewed Nov 15, 2018

View reviewed changes

cjw296 force-pushed the issue35226_mock_call_equality branch from 91c1aed to d16ec07 Compare November 15, 2018 15:37

cjw296 force-pushed the issue35226_mock_call_equality branch from d16ec07 to c704bca Compare November 28, 2018 07:01

cjw296 mentioned this pull request Nov 28, 2018

port of fix for https://bugs.python.org/issue35226 testing-cabal/mock#437

Closed

cjw296 force-pushed the issue35226_mock_call_equality branch 2 times, most recently from 52fe7f5 to 74c23cd Compare November 30, 2018 08:30

cjw296 mentioned this pull request Nov 30, 2018

Implement CallTrackingMock. simplistix/testfixtures#97

Closed

cjw296 force-pushed the issue35226_mock_call_equality branch 4 times, most recently from 67d39fc to d807f86 Compare November 30, 2018 18:45

cjw296 mentioned this pull request Dec 2, 2018

Mock fixes simplistix/testfixtures#98

Merged

mariocj89 reviewed Dec 2, 2018

View reviewed changes

cjw296 commented Dec 2, 2018

View reviewed changes

pablogsal requested review from vstinner and pablogsal and removed request for vstinner December 3, 2018 14:08

voidspace suggested changes Dec 3, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Dec 3, 2018

bpo-35226: Fix equality for nested unittest.mock.call objects.

7413043

Also refactor the call recording imolementation and add some notes about its limitations.

cjw296 force-pushed the issue35226_mock_call_equality branch from d807f86 to 7413043 Compare December 3, 2018 18:28

voidspace approved these changes Dec 3, 2018

View reviewed changes

cjw296 merged commit 8ca0fa9 into python:master Dec 3, 2018

bedevere-bot removed the awaiting core review label Dec 3, 2018

cjw296 deleted the issue35226_mock_call_equality branch December 3, 2018 21:31

cjw296 added needs backport to 3.6 labels Dec 3, 2018

bedevere-bot removed the needs backport to 3.6 label Dec 3, 2018

bedevere-bot removed the needs backport to 3.7 label Dec 3, 2018

		@@ -0,0 +1 @@
		Fix equality for nested unittest.mock.call objects.

Uh oh!

bpo-35226: Fix equality for nested unittest.mock.call objects. #10555

bpo-35226: Fix equality for nested unittest.mock.call objects. #10555

Uh oh!

Conversation

cjw296 commented Nov 15, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Nov 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjw296 Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjw296 commented Nov 28, 2018

Uh oh!

cjw296 commented Nov 28, 2018

Uh oh!

cjw296 commented Nov 30, 2018

Uh oh!

cjw296 commented Dec 2, 2018

Uh oh!

mariocj89 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

TL;DR;

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocj89 Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariocj89 Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

cjw296 commented Nov 15, 2018 •

edited by bedevere-bot

Loading

cjw296 Nov 15, 2018 •

edited

Loading

mariocj89 left a comment •

edited

Loading

mariocj89 Dec 2, 2018 •

edited

Loading

mariocj89 Dec 2, 2018 •

edited

Loading

mariocj89 Dec 2, 2018 •

edited

Loading

cjw296 Dec 2, 2018 •

edited

Loading

mariocj89 Dec 2, 2018 •

edited

Loading