bpo-37555: Update _CallList.__contains__ to respect ANY - #14700
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
Which fails, where my_mock.call_args_list looks like
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
_CallListis 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 usingANY.__eq__, it's calling the Django model's__eq__withANYas an argument. Django first checks if the thing it's comparing to is another Django model, and returns False if not. So,<DjangoModel> == ANYis False, butANY == <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 honoringANYover 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_signatureattribute gets assigned, and when_spec_signatureis not none,_call_matcher, which is used to prep the calls for comparison, returns atupleof a string and aBoundArgumentsobject instead of a_Call(link here). When_call_matcherreturns_Callobjects, they use_Call's__eq__method, which flips the compared calls around, puttingANYon the left. When_call_matcherreturnstuples, it doesn't use this logic, andANYremains on the right, not being used for comparison. The simplest fix seemed to be to ensure_call_matcheralways returned a_Callobject.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