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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-37555: Update _CallList.__contains__ to respect ANY #14700
bpo-37555: Update _CallList.__contains__ to respect ANY #14700
Changes from all commits
94ddf54
b4c7d78
ad99a9d
49c5310
874fb69
d72d6f5
f0e8411
f295eac
18e964b
883841a
f4844c7
84489c8
344ef17
bdf430d
d3522b1
f47699d
38650c9
24973c0
001d708
25dec66
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withcpython/Lib/unittest/mock.py
Line 340 in 455122a
__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
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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, whetheractual
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 thevalue
, 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
self
iscall(<ANY>, 1)
andother
is an item from the list, meaning that when it hits the reversed comparison at the end of the methodcpython/Lib/unittest/mock.py
Line 2444 in 455122a
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't have any changes more to push too as I can see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a list and
list.__contains__
is used only when it's not a list. So it takes the other code pathsub_list == value
to wherevalue
is the tuple that contains ANY. ANY is on the left asother_args
andother_kwargs
. as I can see.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, anything that means things work on 3.6+ would be very much appreciated by me :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more case: Please do the check for cause to be exception
if cause or expected not in _AnyComparer(actual):
inassert_any_await
too to catch the exception like you did inassert_any_call
. This would cause below test to fail. I had a test in my patch for this case. Something like below test :There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @tirkarthi ! I also saw in your patch you added the
two=True
parameter toassert_called_with
which strikes me as good for consistency, so I added that back in too.