bpo-37555: Update _CallList.__contains__ to respect ANY

[ElizabethU]

Greetings! Long time fan of the language, first time submitting a pull request to it.

I have a test on another project that goes something like:

@patch('a.place.to.patch')
def test_a_thing_calls_what_it_should(self, my_mock):
    # Set up logic here
    my_mock.assert_has_calls([
        call(
            ANY,
            Decimal('20')
        ),
        call(
            ANY,
            Decimal('10')
        )
    ])

Which fails, where my_mock.call_args_list looks like

[(<A Django Model>, Decimal('20')), (<A Django Model>, Decimal('10'))]

This seems like wrong behavior. ANY should be happy to be compared to anything, even a random object. I've added a test showing the behavior that fails on master.

Doing some digging, I found that in mock.py _CallList is overriding __contains__ and comparing each item in the tuples with what I'd passed in to assert_has_calls on the right, which means that instead of using ANY.__eq__, it's calling the Django model's __eq__ with ANY as an argument. Django first checks if the thing it's comparing to is another Django model, and returns False if not. So, <DjangoModel> == ANY is False, but ANY == <DjangoModel> is True. I know that this could also be considered a bug with Django, and I plan to file one with them too, but I don't see any downside to improving the mock library to be more defensive in honoring ANY over any other custom class's overridden __eq__ method.

More digging and I realized the reason this was only a problem on a mock with a spec. Specifically, when a mock is instantiated with a spec, the _spec_signature attribute gets assigned, and when _spec_signature is not none, _call_matcher, which is used to prep the calls for comparison, returns a tuple of a string and a BoundArguments object instead of a _Call (link here). When _call_matcher returns _Call objects, they use _Call's __eq__ method, which flips the compared calls around, putting ANY on the left. When _call_matcher returns tuples, it doesn't use this logic, and ANY remains on the right, not being used for comparison. The simplest fix seemed to be to ensure _call_matcher always returned a _Call object.

I have verified that there are several tests covering the use of spec signature, which make sure that things continue to match regardless of if they are args or kwargs and break when that logic is broken, and none of them break with my change.

https://bugs.python.org/issue37555

[the-knights-who-say-ni]

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!

[tirkarthi]

This might break below program I posted on the tracker. I am slightly surprised there are no CI failures with this change.

from unittest.mock import call, ANY, Mock

class Foo:

    def __eq__(self, other):
        if not isinstance(other, Foo):
            return False
        return True

m = Mock()
obj = Foo()
m(obj, 1)
m.assert_has_calls([call(ANY, 1)])

Without patch

➜  cpython git:(master) ./python.exe ../backups/bpo37555_1.py

With patch

➜  cpython git:(master) git checkout pr_14700 Lib/unittest/mock.py
➜  cpython git:(master) ✗ ./python.exe ../backups/bpo37555_1.py
Traceback (most recent call last):
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/../backups/bpo37555_1.py", line 14, in <module>
    m.assert_has_calls([call(ANY, 1)])
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 900, in assert_has_calls
    raise AssertionError(
AssertionError: Calls not found.
Expected: [call(<ANY>, 1)]
Actual: [call(<__main__.Foo object at 0x10b4a5280>, 1)].

[tirkarthi]

Okay, there is a test at https://github.com/python/cpython/blob/master/Lib/unittest/test/testmock/testhelpers.py#L46 . It's just that it doesn't use assert_has_calls where this code path is executed. Adding mock.assert_has_calls(expected) to the test fails with the PR that passes on master.

[ElizabethU]

Thanks, that's really helpful. I'll see what I can do on this after work today.

[ElizabethU]

Okay, I've reverted the original submission and instead added a regression test that shows the specific issue I'm having: When the mock has a spec, assert_has_calls is not using ANY's __eq__.

I'm working on a fix.

[pganssle]

@ElizabethU I can't think of any obvious downsides to the current fix, but I do think it's worth noting that this still changes the semantics more broadly than just making it work with ANY, because it means that when (x == y) != (y == x), it always returns True.

In the BPO issue, I suggested that a more tightly scoped fix would be to special-case mock.ANY so that if one of the two objects is mock.ANY, it would be treated as equal, otherwise the old equality rules would apply. This is just food for thought, though (maybe after this is merged if someone complains about the changes to the semantics we can try the x is ANY mechanism).

Edit: I see that you did see my BPO comment and indeed responded to it, I guess I missed the e-mail about that.

[pganssle]

@ElizabethU Would you mind adding a NEWS entry?

Assuming you would like credit by name, please add "Patch by Elizabeth Uselton." at the end of the blurb. You may also add yourself to Misc/ACKS.

[ElizabethU]

Hi @pganssle sorry, I missed your comments over here, since most of the action seemed to be happening on the BPO issue.

1. I agree, special casing mock.ANY would be the way to go, however I'm not quite sure I see the path to doing so. ANY already has __eq__ overridden, so surely that's not what you mean. I thought maybe you meant wrapping the self_list and other_list args in a tuple subclass that would look at each object, and compare the first to the second unless the second was ANY, in which case it would return true. But then I realized the second arg could be {'data': ANY} or {'data': {'name': ANY} }, etc. And then I realized I was writing a recursive JSON parser, and that felt bad, so I stopped. What am I missing here? I'd really like to get to the more elegant solution.

2. I added another commit that I probably should have started with, wrapping the tuple that _call_matcher returns if there is a spec passed to the mock with call() so that function consistently returns a _Call object. This entirely fixes the problem demonstrated by the first test I added where assert_has_calls fails to match ANY on badly formed objects with a spec, without affecting the ordering of comparisons on _Call or _CallList. I still think the ordering fix is an improvement, and that making the other two tests pass is desirable, but I understand that may be more contentious. If you or any other Python caretaker would prefer, I'm happy to cherry-pick the commits for this smaller fix and the first test, and open a second issue for the ordering issue where it can be discussed more.

3. I changed the eq in the comparisons to == after a mentor pointed out to me that bool(NotImplemented) is truthy, so without that change a _Call and _CallList would return True instead of False if both sides had no way to compare to each other. 😱 I added his name to the acknowledgements too, because he's been a great sounding board for me through this process. I hope that's okay.

I'm happy to keep refining this, especially in regards to 1 or 2, but otherwise I feel good about this and would really like to ship my first (and hopefully not last) Python contribution.

[ElizabethU]

Hey, I've reverted all my commits related to the _Call.__eq__ part of this problem, since that seemed to have more uncertainty around it. The original bug that brought me here of assert_has_calls not working with ANY when specced on a class that's __eq__ doesn't return NotImplemented is still totally fixed by this patch.

I'm not quite ready to give up on the _Call.__eq__ part yet, but thought it would be better served by being a separate ticket we could all discuss more, and that hopefully there could be consensus around this smaller change first.

[ElizabethU]

@tirkarthi @pganssle Have you had a chance to see these changes? I updated the initial comment on this to explain the bug and fix more accurately, and think this de-scoped solution has less opportunity for consequences. Happy to do any further work on it if you have concerns or critiques.

[ElizabethU]

@tirkarthi I refactored the test you suggested I add so that it's four tests covering all the functions using _call_matcher both with and without spec, so every use case would be covered. By doing this I found out that assert_has_awaits has the same bug as assert_has_calls, which makes sense given how similar they are.

Your patch with the extra parameter felt like special casing to me too, so I spent some time trying to really understand the reasons the different cases were failing, and the more I looked at it, the more I felt like the root of the problem is that _call_matcher sometimes takes _Calls and sometimes doesn't, and sometimes returns _Calls and sometimes doesn't, sometimes returning a different type than what was passed in. The solution I've implemented makes _call_matcher consistently use and return _Call objects, which feels simpler to reason about or extend off of for future developers. This isn't the simplest change I could have made, so I'm open to doing something else. I'm not particularly happy about the _AnyComparer class, but as a list subclasses to deal with the flipped item == self at the end of _Call.__eq__, it's just following a precedent set by _CallList.

Here are some other options that I tried out and think would work, please let me know if you have a strong preference for one of them over what I ended up doing.

1. Convert anything coming back from _call_matcher to a _Call instance in assert_has_calls and assert_has_awaits. Has a smaller scope but leaves the problem with _call_matcher being used for inconsistent things.

2. Convert anything coming into _CallList.__eq__ to a _Call, since _CallList should always be a list of _Calls. Same pros and cons as option 1 above, really feels better to assure _CallList gets the right arg types than to try to force it.

3. Check the type of arguments to _call_matcher and return something of the same type. Seems like adding logic and less explicit.

4. Special case ANY in Call.__eq__ so it's respected whether it's on the left or right of a comparison, as Paul suggested earlier. I like this one the most out of all the other working ideas, and think it has some good side effects that might be a great case for it even independent of this bug, but this bug is caused by conflating types in _call_matcher, and it feels like leaving that unfixed is inviting more complexity to this code, even if we can find a way to get around it.

5. Bonus: My favorite idea that sadly doesn't work: If we could override a function ANY to say that it's a subclass of everything, similar to how we override eq on it to say it's equal to everything. The python == algorithm pulls the subclass on the left, so ANY.__eq__ would always get used in any comparison involving ANY. Unfortunately, this is not possible because __subclasscheck__ would only let me make everything a subclass of ANY, not the other way around, and also rich comparison uses PyType_IsSubtype which actually walks the inheritance, not PyObject_IsSubclass which uses __subclasscheck__. Just a fun idea I got excited about before I realized it wouldn't work.

Those are my thoughts, but I'm very happy to change to a different approach if you, @pganssle @mariocj89 or anyone else have opinions.

[ElizabethU]

@tirkarthi @pganssle Hey there, not trying to be impatient, just checking in because I haven't heard anything in a while. I'm sorry about the amount of iteration this has needed, is there anything I can do to make it easier to review?

[ElizabethU]

@tirkarthi @pganssle @mariocj89 Hi again, I know you mentioned there's quite a backlog to get through, so hopefully it's just that, but after this long of not hearing anything, I'm worried reviewing this has fallen off the to do list, or that I've done something egregiously bad in this latest draft. I'd really appreciate it if someone could let me know this isn't abandoned, even if you don't have time for it today.

[pganssle]

At this point I mostly want to defer to @tirkarthi, because he seems to understand the edge cases better than the rest of us. I am happy to do a final review and merge once he gives approval.

[ElizabethU]

@pganssle Totally understand, @tirkarthi 's input has been really helpful, and after the last bug he caught, I'd feel worried about merging this without his approval too. Unfortunately, that still leaves me in the same position of worrying it's fallen off the to do list. Is there any way I could ask you to help (earlier you were mentioning helping track down the reviewer I need), or is the best thing just for me to ping him again if I don't hear back in another week or so?

Also, I want to be open to the idea that I'm just not used to the speed at which open source moves, if I should just wait patiently and trust that it'll get reviewed with time?

[tirkarthi]

I will try to give a review of it in couple of weeks given that this is little complex area. Sorry, I will be having little time for open source given my day job workload :( I will get back to you by September mid and I haven't forgotten the issue. Friendly ping to @cjw296 who can also give some helpful input since they have experience with mock codebase.

Thanks

[ElizabethU]

Thank for the update @tirkarthi Sorry if I've been pushy about it, that wasn't my intention. I'll put this on the back burner and maybe dig up something else to work on in the mean time, since you said there are other issues in the tracker around this.

[voidspace]

This is a high priority issue and thanks for the work on it, especially the places that need to change.

I think explicitly checking for ANY rather than reversing the order of arguments will be clearer and less likely to have other, potentially subtle, effects.

[voidspace]

No, we can't explicitly check for ANY because we're comparing data structures. ANY could appear anywhere! @tirkarthi has a slight change to this PR that we're looking on getting committed during the core-dev sprints this week.

[ElizabethU]

Hi @voidspace I was reluctant to just check for ANY because while the documentation for ANY doesn't explicitly say ANY can be used in nested objects, it supports it now, and it would feel bad to take that away from people, which I think is what you're saying, too.

This is my first contribution, so I'm new to how this works. What does that mean that @tirkarthi has a slight change to this PR that you're looking to commit? Will that use some of my commits? Will I still be a contributor? I'm very excited and invested in this bug and would love to stay with it after all the work I've put in on it, if I can make any changes to get this merged.

[tirkarthi]

@ElizabethU I have made some changes to this in my branch few weeks back . My change is at https://github.com/tirkarthi/cpython/commits/pr_14700_re and commit change is tirkarthi@bc09989. We can use this PR. I will add in the suggestions tomorrow as PR comments and you can review and comment if my comments are okay. You will remain as the author and this will be the same like the review changes we made earlier. Thanks.

[tirkarthi]

I think as per my commit we can remove _AnyComparer. It takes the expected value which could contain ANY and make sure it stays on the left while comparing with actual. The calllist __contains__ already does the same check here with

cpython/Lib/unittest/mock.py

Line 340 in 455122a

return list.__contains__(self, value)

. The list __contains__ does an equal per item and hence it does the same check at _Call.__eq__

cpython/Lib/unittest/mock.py

Line 2394 in 455122a

def __eq__(self, other):

There are also no test failures and I see the behavior around comparison to be the same. We also can skip the check for cause as I go with the approach. @ElizabethU Thoughts?

[ElizabethU]

@tirkarthi I'm all for removing _AnyComparer as I think it's pretty clunky, but I'm seeing test failures with [tirkarthi/cpython@bc09989]. First I cherry-picked your commit onto my branch and saw test failures, then I tried on the branch on your fork, thinking that you might have had some other changes or even that someone else might have made some changes that you'd have if you'd synced your fork since I'd opened this PR. I'm still getting the same 4 failures either way:

unittest.test.testmock.testasync.AsyncMockAssert.test_awaits_asserts_with_any
unittest.test.testmock.testasync.AsyncMockAssert.test_awaits_asserts_with_spec_and_any
unittest.test.testmock.testhelpers.AnyTest.test_any_and_spec_set
unittest.test.testmock.testhelpers.AnyTest.test_any_no_spec

Are those four tests working for you? If so, I'm pretty confused since these failures really make sense to me. In assert_any_call above, whether actual is just a regular list or a _CallList, it goes into the default list class's __contains__ either way. And when it does, call(<ANY>, 1) is the value, and it gets a little confusing, but the way item by item comparison is done, when it enters _Call.__eq__

cpython/Lib/unittest/mock.py

Line 2394 in 455122a

def __eq__(self, other):

self is call(<ANY>, 1) and other is an item from the list, meaning that when it hits the reversed comparison at the end of the method

cpython/Lib/unittest/mock.py

Line 2444 in 455122a

return (other_args, other_kwargs) == (self_args, self_kwargs)

ANY is on the right, and it doesn't work.

At least that's what I'm seeing on my machine. I'd be really excited if you've found a way around _AnyComparer, but it seems like we still need it.

[tirkarthi]

I don't see any failures on checking out your branch and cherry picking my changes on to your branch. My terminal output as below assuming I am doing it correct.

➜  cpython git:(pr_14700_re) git pr 14700
From github.com:python/cpython
 * [new ref]               refs/pull/14700/head -> pr_14700
➜  cpython git:(pr_14700_re) git checkout pr_14700
Updating files: 100% (506/506), done.
Switched to branch 'pr_14700'
➜  cpython git:(pr_14700) git cherry-pick bc099891dd327e00bc0271da9ddb752ae9198ce6
[pr_14700 5b7c21b188] Remove AnyCompare and use call objects everywhere.
 Date: Mon Aug 19 15:11:01 2019 +0530
 2 files changed, 8 insertions(+), 21 deletions(-)
➜  cpython git:(pr_14700) ./python.exe -m unittest Lib.unittest.test.testmock
...........................................................................................................................................................................................................................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 443 tests in 1.470s

OK

[tirkarthi]

I also don't have any changes more to push too as I can see.

➜  cpython git:(pr_14700_re) git show --summary | cat
commit bc099891dd327e00bc0271da9ddb752ae9198ce6
Author: Xtreak <tir.karthi@gmail.com>
Date:   Mon Aug 19 15:11:01 2019 +0530

    Remove AnyCompare and use call objects everywhere.

➜  cpython git:(pr_14700_re) ./python.exe -m unittest Lib.unittest.test.testmock
.............................................................................................................................................................................................................................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 445 tests in 1.449s

OK
➜  cpython git:(pr_14700_re) git push origin pr_14700_re
Everything up-to-date

[tirkarthi]

self is call(, 1) and other is an item from the list, meaning that when it hits the reversed comparison at the end of the method
ANY is on the right, and it doesn't work.

It's a list and list.__contains__ is used only when it's not a list. So it takes the other code path sub_list == value to where value is the tuple that contains ANY. ANY is on the left as other_args and other_kwargs. as I can see.

[tirkarthi]

Confirmed that this change in the order of comparison to be crucial to my patch over change in the arguments. That means we can either

1. Have ANYCompare and apply this fix on 3.9 and 3.8 . I am not sure of backporting this to 3.7 at this point since it's in 3.7.5 but I will leave it upto the core dev.
2. Depend on the below order in list comparison and remove ANYCompare to use just contains and keep the fix only to 3.9 since the listobject patch is not on 3.8 and below.

Thoughts?

diff --git a/Objects/listobject.c b/Objects/listobject.c
index d012ab933a..cea9b24a3b 100644
--- a/Objects/listobject.c
+++ b/Objects/listobject.c
@@ -449,8 +449,7 @@ list_contains(PyListObject *a, PyObject *el)
     int cmp;

     for (i = 0, cmp = 0 ; cmp == 0 && i < Py_SIZE(a); ++i)
-        cmp = PyObject_RichCompareBool(el, PyList_GET_ITEM(a, i),
-                                           Py_EQ);
+        cmp = PyObject_RichCompareBool(PyList_GET_ITEM(a, i), el, Py_EQ);
     return cmp;
 }

[tirkarthi]

Thinking a bit further there is also mock backport which would need to run on 3.6+ so this would cause problems. Now I am more leaning towards to reverting my change to keep ANYCompare though the interpreter is fixed in 3.9+. At this point I think I depended on the patch in 3.9 during debugging and made some incorrect assumptions with respect to ordering, sorry about that. I would be okay with this landing on 3.8+ with ANYcompare and the backport can work on 3.6+ with the PR.

@ElizabethU Feel free to revert my patch.

[ElizabethU]

I am able to get it to work by rebasing 🎉 🎊 🎺 Thanks, I should have tried that earlier, but I thought since I was still having the problem on your branch and you had that passing, that it couldn't be a rebase. I should have tried anyway.

My input on the patch for 3.9 is that it's great and a big improvement over what I had alone and I would love to see it get reviewed whenever there's time. As for backporting, maybe we'd still need the _AnyComparer class for 3.8 then? Or maybe this just doesn't get backported? Not sure what's the norm, but I'm happy to learn and go along with it.

Thank you so, so much for sticking with me through this. I've learned so much, and your expertise has been a big part of that. I hope I can contribute again and take up a lot less of your time now that I know the routine and some of the codebase.

[ElizabethU]

@tirkarthi Ah, I didn't see your last comments. So reverting is the right thing to do? I do like the patch, and was hoping we could keep it for 3.9, but I'm unfamiliar with the mock backport situation, and why it means we couldn't use the patch in just 3.9?

I'm reverting the patch per your instructions.

[cjw296]

FWIW, anything that means things work on 3.6+ would be very much appreciated by me :-)

[tirkarthi]

One more case: Please do the check for cause to be exception if cause or expected not in _AnyComparer(actual): in assert_any_await too to catch the exception like you did in assert_any_call. This would cause below test to fail. I had a test in my patch for this case. Something like below test :

diff --git a/Lib/unittest/test/testmock/testasync.py b/Lib/unittest/test/testmock/testasync.py
index 1b1f9111d2..87f3cfc8fa 100644
--- a/Lib/unittest/test/testmock/testasync.py
+++ b/Lib/unittest/test/testmock/testasync.py
@@ -192,6 +192,10 @@ class AsyncAutospecTest(unittest.TestCase):
         spec.assert_awaited_with(1, 2, c=3)
         spec.assert_awaited()

+        with self.assertRaises(AssertionError):
+            spec.assert_any_await(e=1)
+
+
     def test_patch_with_autospec(self):

         async def test_async():

[ElizabethU]

Thank you @tirkarthi ! I also saw in your patch you added the two=True parameter to assert_called_with which strikes me as good for consistency, so I added that back in too.

[tirkarthi]

LGTM. Thank you very much Elizabeth for your patience and tenacity throughout the review process though the process unfortunately turned out to be long due to the complexity of the bug and internals of how __eq__ works in CPython :) Your bug report also helped in the 3.9 change discussion and fixing an issue in datetime module. Apologies for the confusion from my patch which worked correctly due to the change in master which I was thinking to be fine along in 3.8 too.

Looking forward to your contributions :)

[pganssle]

Great work everyone, thanks again!

[bedevere-bot]

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

[pganssle]

Ugh, wtf, I wrote a custom commit message and changed the commit title and github ate it and applied the default one 😦

[matrixise]

@pganssle question, this PR is not backported to 3.8, is there a reason? Thanks

[pganssle]

@matrixise My understanding is that we were unable to fix this without some very minor backwards compatibility breaking - the order of an == comparison is swapped, for example. I highly doubt anyone would notice the difference, honestly, but my instinct is to leave this for 3.9, particularly if it will be included in the backport.

@tirkarthi Can you comment as to whether or not the final version of this ended up with any backwards incompatibilities? Should we go ahead and backport to 3.8?

[tirkarthi]

@tirkarthi Can you comment as to whether or not the final version of this ended up with any backwards incompatibilities? Should we go ahead and backport to 3.8?

My version of the patch was backwards incompatible but the one on current master doesn't depend on the incompatible change and should work on mock's backport and 3.8 . I was initially okay with backporting to 3.8. But given the complexity of the issue and also that 3.8 RC1 is just in 2 weeks I don't want this to affect stability though I believe the fix is good. This will be on 3.9 and the mock backport which would give us more time and exposure to testing to address issues if any. My opinion is that as far as I know this issue wasn't noticed and reported for 3.6 and 3.7 I think we can keep it on master and mock's backport only due to complexity.

[ElizabethU]

Does that mean that this will eventually find it's way to 3.8, after it's had more time to be vetted as part of the mock backport to 3.6? It seems counterintuitive to me that it would be in 3.6 but not in 3.8, but I'm just learning about release schedules and don't have a great understanding of the mock backport.

[tirkarthi]

mock has a rolling backport on pypi that now supports 2.7 and 3.5+. Plan would be to drop Python 2 support and have it 3.6+ only so essentially the current mock.py will be copied over. Benefit is that users from 3.6 (3.6 is in security fixes only mode) can install the backport from pypi and use 3.8 features like AsyncMock. Mock backport has several million downloads per month and could get some testing. If there is a regression found then we can fix it in master and again merge it to rolling backport. Meanwhile with 3.8 branch getting a backport now it will have a release candidate in two weeks like a code freeze. So if there is a regression then there would be only few weeks to fix it and get another RC. Given the complexity I would prefer the patch can get testing from mock backport that users can install and 3.9 release. If there are requests over this bug being crucial then it can be considered for a 3.8.x release as a bug fix.