_CallList.__contains__ doesn't always respect ANY.

@cjw296

BPO	37555
Nosy	@cjw296, @voidspace, @serhiy-storchaka, @pganssle, @mariocj89, @tirkarthi, @ElizabethU, @iritkatriel
PRs	bpo-37555: Update _CallList.__contains__ to respect ANY #14700

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-05-26.13:26:55.129>
created_at = <Date 2019-07-11.05:12:04.785>
labels = ['3.7', '3.8', 'type-bug', 'library', '3.9']
title = "_CallList.__contains__ doesn't always respect ANY."
updated_at = <Date 2021-05-26.13:26:55.129>
user = 'https://github.com/ElizabethU'

bugs.python.org fields:

activity = <Date 2021-05-26.13:26:55.129>
actor = 'iritkatriel'
assignee = 'none'
closed = True
closed_date = <Date 2021-05-26.13:26:55.129>
closer = 'iritkatriel'
components = ['Library (Lib)']
creation = <Date 2019-07-11.05:12:04.785>
creator = 'ElizabethU'
dependencies = []
files = []
hgrepos = []
issue_num = 37555
keywords = ['patch']
message_count = 26.0
messages = ['347652', '347654', '347698', '347722', '347729', '347730', '347731', '347733', '347737', '347739', '347753', '347818', '347941', '347949', '347961', '348068', '348105', '348121', '348214', '348278', '348453', '352351', '378782', '378800', '378837', '393497']
nosy_count = 8.0
nosy_names = ['cjw296', 'michael.foord', 'serhiy.storchaka', 'p-ganssle', 'mariocj89', 'xtreak', 'ElizabethU', 'iritkatriel']
pr_nums = ['14700']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue37555'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

I have a test 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

[(, Decimal('20')), (, Decimal('10'))]


This seems like wrong behavior. ANY should be happy to be compared to anything, even a Django object. Doing some digging, I found that on line 340 of cpython/Lib/unittest/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.

Thanks for the report. Can you please add an example without Django or other dependencies so that I can try reproducing it? I am trying out the below program from the report where Foo has overridden __eq__ to return False if the other object being compared is not a Foo object. In the next statements ANY == Foo() returns True since the left side's object.__eq__ is used which in this case is ANY.__eq__ as you have noted in the original report.

When list of call objects are compared there is also a code comment about this in the tuple comparison of args and kwargs such that ANY is placed on the left side [0] so that ANY.__eq__ is used. A pure python example and traceback if any would help here.

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)])

print(ANY == Foo()) # ANY.__eq__ is called
print(Foo() == ANY) # Foo().__eq__ is called

Is the report more about the below case where position of call objects returns different values?

print(call(ANY, 1) == call(obj, 1)) # False
print(call(obj, 1) == call(ANY, 1)) # True

[0]

cpython/Lib/unittest/mock.py

Line 2407 in 2a3d4d9

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

Karthikeyan, I think there is a flaw in your example. The good __eq__ should return NotImplemented instead of False for other types. This wil allow the right operand's __eq__ to play.

Karthikeyan, I think there is a flaw in your example. The good __eq__ should return NotImplemented instead of False for other types. This wil allow the right operand's __eq__ to play.

Serhiy, my example was to try reproducing the original report where Django model tries to check if the other's type and returns False if the type is not a Django model. There could be cases where __eq__ implementation of a class could be just returning False instead of raising a NotImplemented error due to difference in type that affects comparison of ANY.

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.

Then I think the problem is with DjangoModel.

I suggest to close this issue as "third party".

Though some __eq__ implementations in the stdlib should be fixed too.

https://docs.python.org/3/library/unittest.mock.html#any

mock.ANY doesn't make any guarantees that this is a generic implementation that can be used to test equality against any object irrespective of the order to return True. It's documented only to be used for mock calls but there are many users using it for other objects too outside of mock assertions. mock.ANY also depends on the order so that mock.ANY.__eq__ is used but I am not sure about documenting the order here.

Would be helpful if Elizabeth presents a simplified example to evaluate if there are any bugs in the call comparison with ANY or to close it as third party as suggested. As noted in my PR comment there should be a test case for this change proposed to make sure someone doesn't change the order by accident in future : #14700 (comment)

Though some __eq__ implementations in the stdlib should be fixed too.

Curious about what problems that this example surface in the stdlib definition of __eq__ . Are you saying that there are Django model like __eq__ definitions where they return False when other's type is different?

Yes, see timedelta.__eq__ for example. datetime.__eq__ looks correct.

Maybe I am missing something, but while it is true that DjangoModel is doing the wrong thing by returning False instead of NotImplemented, the ANY sentinel is supposed to match anything, not just things that compare equal to it, right? I would expect this to work, for example:

  class EqualsNothing:
    def __eq__(self, other):
        return False

  m = Mock(spec_set=EqualsNothing)
  obj = EqualsNothing()
  m(obj)
  m.assert_has_calls([call(ANY)])

In that example, it is deliberate that EqualsNothing returns False for any type, but ANY should still match it. I think maybe the solution here is to special-case the matching with ANY so the criterion for a call matching would be y is ANY or x == y.

I feel like I agree with Paul here, unsurprising behavior from ANY is it matching anything, with no expectation that third party objects have to correctly have a return path for NotImplemented on their __eq__ method. ANY doesn't currently do that.

I've added a commit with couple other failing tests showing how using assertEqual is currently order sensitive when used with _Call or _CallList objects. I've got a work in progress fixing all three tests that just needs a little clean up, but think I might be able to lessen the complexity of it by adding Paul's idea of just special casing ANY directly, which may be smaller than the fix I have.

Thank you all for providing such great feedback so far, this is really interesting stuff to me, and a great first time submitting to Python experience so far.

Just added a fix for the tests I added in the last commit showing the order sensitivity in assertEqual when dealing with ANY and _Call or _CallList objects. _Call and _CallList currently depend on ordering to correctly process that an object being compared to ANY with __eq__ should return True. I put more notes in the commit message.

A note on the two newer tests: I want to be sensitive to the idea that I might be pushing back against desired behavior here. I think there is a case to be made that one should expect mock.assertEqual(a, b) to implement a == b, and not care about b == a, in which case maybe order sensitivity in assertEqual makes sense. That said, today I showed a few of my most Python experience heavy colleagues the tests showing that assertEqual was order sensitive when passed _Call or _CallList objects, and based on anecdotal evidence the existing behavior definitely violates the principle of least astonishment. Additionally, the test Karthikeyan kindly linked me https://github.com/python/cpython/blob/master/Lib/unittest/test/testmock/testhelpers.py#L46 seems to imply that an order agnostic assertEqual is the desired behavior.

A note on the fix: This fix feels like it's not as small as it could be. I am still musing over whether it'd be better if instead of the or statements and swapping the comparison order in _CallList's __contains__ method, I made a wrapper that would swap that comparison order in the case that one of the arguments is a BoundArgument, since that was the original problem. That fix would probably not affect the order sensitivity of assertEqual, so it's partially about what we want there. I'll play around with it and see what I can discover. I'll also see if I can do anything to more explicitly to check for ANY, as Paul suggested.

I'm very curious what you all think about whether the assertEqual behavior is a feature or a bug, and if the other fix idea seems better than this one. If neither of these ideas seem like a good fix, I'd love some guidance on what concerns are and what might be a good approach to try next.

Giving this a reread with fresh eyes this morning, I realized I wasn't explicit enough in saying that I see that this fix is about the same as the one you mentioned, Karthikeyan, but said you were concerned might cause more subtle bugs. I hear that, and will try some more specific fixes, I was just trying to gather whether the tests showing

self.assertEqual(call(ANY).call_list(), mock.mock_calls)
self.assertEqual(mock.mock_calls, call(ANY).call_list())

results in one passing and the other not (and the same for calls) changed the math on how brittle the current logic is, so just leaving it seemed like less of an option. I'll keep working on some other ideas though, whether we can check for ANY directly, or just wrap the BoundArguments that are causing problems when the mock has spec set.

This issue is a part of more general issue. I have opened a thread on Python-Dev for discussing it: https://mail.python.org/archives/list/[email protected]/thread/VSV4K4AOKM4CBQMOELPFV5VMYALPH464/.

Serhiy, thanks for pointing that out. I generally agree with everything in that thread, and learned some new things (I had no idea count(), index() and remove() used needle on the left side!) However, I'm not trying to spearhead a rewrite of everything. I think what I'm advocating for on the two assertEqual order tests is that as Guido mentioned "people should fix their __eq__ implementation to properly return NotImplemented". _Call's __eq__ currently has no path to NotImplemented, and if it did, those tests would pass. Perhaps I should change it from the or statement to make it more clear that's what's effectively happening.

I'd like to talk more about what a good solution would be for the _Call __eq__
issue, since people (including myself!) have concerns, but I think it would be best to open another bug for that, and not scope creep myself. I've reverted all the commits having to do with that part so hopefully the issue with _call_matcher can get sorted separately, and then we can talk about this as it's own issue.

See bpo-37685.

New changeset d6a9d17 by Paul Ganssle (Elizabeth Uselton) in branch 'master':
bpo-37555: Update _CallList.__contains__ to respect ANY (bpo-14700)
d6a9d17

Does this need a backport? It's in 3.9 but not 3.8.

I would prefer not backporting this change given the discussion in #14700 (comment)

I believe it should work in both 3.8 and 3.9, the difference is that someone (Serhiy if I'm remembering correctly?) did the work in 3.9 of standardizing which side of the = the needle was on. So, this change works on 3.9 without the _AnyComparer class I added, but I believe unless _AnyComparer has been removed, this change would work in 3.8. Xtreak, it seems like most of your concerns about backporting it are about how complicated this part of the code is, and wanting it to get more testing via the mock backport? Is there any way I can help mitigate those concerns, or keep up with how much use it's gotten from the mock backport so we can determine when (if ever) it would be a good time to backport this?

3.8 is in security fix mode now, so I think this issue can be closed.

Navigation Menu

Repository navigation

_CallList.contains doesn't always respect ANY. #81736

9 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

_CallList.__contains__ doesn't always respect ANY. #81736

Description

Activity

ElizabethU commented on Jul 11, 2019

tirkarthi commented on Jul 11, 2019

serhiy-storchaka commented on Jul 11, 2019

tirkarthi commented on Jul 12, 2019

serhiy-storchaka commented on Jul 12, 2019

serhiy-storchaka commented on Jul 12, 2019

tirkarthi commented on Jul 12, 2019

serhiy-storchaka commented on Jul 12, 2019

9 remaining items

pganssle commented on Jul 15, 2019

ElizabethU commented on Jul 17, 2019

ElizabethU commented on Jul 18, 2019

ElizabethU commented on Jul 18, 2019

serhiy-storchaka commented on Jul 20, 2019

ElizabethU commented on Jul 22, 2019

serhiy-storchaka commented on Jul 25, 2019

pganssle commented on Sep 13, 2019

iritkatriel commented on Oct 16, 2020

tirkarthi commented on Oct 17, 2020

ElizabethU commented on Oct 17, 2020

iritkatriel commented on May 12, 2021

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

_CallList.contains doesn't always respect ANY. #81736