[issue37555] _CallList.__contains__ doesn't always respect ANY.
Elizabeth Uselton added the 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? -- ___ Python tracker <https://bugs.python.org/issue37555> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37555] _CallList.__contains__ doesn't always respect ANY.
Elizabeth Uselton added the comment: 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. -- ___ Python tracker <https://bugs.python.org/issue37555> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37555] _CallList.__contains__ doesn't always respect ANY.
Elizabeth Uselton added the comment: 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. -- ___ Python tracker <https://bugs.python.org/issue37555> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37555] _CallList.__contains__ doesn't always respect ANY.
Elizabeth Uselton added the comment: 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. -- ___ Python tracker <https://bugs.python.org/issue37555> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37555] _CallList.__contains__ doesn't always respect ANY.
Elizabeth Uselton added the comment: 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. -- ___ Python tracker <https://bugs.python.org/issue37555> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37555] _CallList.__contains__ doesn't always respect ANY.
Elizabeth Uselton added the comment: Hi there, I completely missed that this had caused so much interesting discussion. I've added a regression test that shows the bug I was encountering, which seems to be related to spec_set bypassing _Call's overwritten __eq__ and so not respecting ANY https://github.com/python/cpython/pull/14700 I agree, this is definitely also a third party bug with Django, but I think it's a problem here too, and am working on a fix. -- components: +Tests -Library (Lib) nosy: -serhiy.storchaka, xtreak versions: +Python 3.5, Python 3.6 ___ Python tracker <https://bugs.python.org/issue37555> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue37555] _CallList.__contains__ doesn't always respect ANY.
New submission from Elizabeth Uselton :
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,
== ANY is False, but ANY == 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.
--
components: Tests
messages: 347652
nosy: Elizabeth Uselton
priority: normal
severity: normal
status: open
title: _CallList.__contains__ doesn't always respect ANY.
type: behavior
versions: Python 3.5, Python 3.6, Python 3.7, Python 3.8, Python 3.9
___
Python tracker
<https://bugs.python.org/issue37555>
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
