Skip to content
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

Merged
merged 20 commits into from
Sep 13, 2019
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
94ddf54
Flip equality to use mock calls' __eq__
ElizabethU Jul 11, 2019
b4c7d78
bpo-37555: Regression test demonstrating assert_has_calls not working…
ElizabethU Jul 14, 2019
ad99a9d
Revert "Flip equality to use mock calls' __eq__"
ElizabethU Jul 14, 2019
49c5310
bpo-37555: Add regression tests for mock ANY ordering issues
ElizabethU Jul 15, 2019
874fb69
bpo-37555: Fix _CallList and _Call order sensitivity
ElizabethU Jul 18, 2019
d72d6f5
bpo-37555: Ensure _call_matcher returns _Call object
ElizabethU Jul 20, 2019
f0e8411
Adding ACK and news entry
ElizabethU Jul 20, 2019
f295eac
bpo-37555: Replacing __eq__ with == to sidestep NotImplemented
ElizabethU Jul 20, 2019
18e964b
bpo-37555: cleaning up changes unnecessary to the final product
ElizabethU Jul 20, 2019
883841a
bpo-37555: Fixed call on bound arguments to respect args and kwargs
ElizabethU Jul 20, 2019
f4844c7
Revert "bpo-37555: Add regression tests for mock ANY ordering issues"
ElizabethU Jul 22, 2019
84489c8
Revert "bpo-37555: cleaning up changes unnecessary to the final product"
ElizabethU Jul 22, 2019
344ef17
Revert "bpo-37555: Replacing __eq__ with == to sidestep NotImplemented"
ElizabethU Jul 22, 2019
bdf430d
Revert "bpo-37555: Fix _CallList and _Call order sensitivity"
ElizabethU Jul 22, 2019
d3522b1
Updated NEWS.d
ElizabethU Jul 22, 2019
f47699d
bpo-37555: Add tests checking every function using _call_matcher both…
ElizabethU Aug 4, 2019
38650c9
bpo-37555: Ensure all assert methods using _call_matcher are actually…
ElizabethU Aug 5, 2019
24973c0
Remove AnyCompare and use call objects everywhere.
tirkarthi Aug 19, 2019
001d708
Revert "Remove AnyCompare and use call objects everywhere."
ElizabethU Sep 13, 2019
25dec66
Check for exception in assert_any_await
ElizabethU Sep 13, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions Lib/unittest/mock.py
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,8 @@ def _call_matcher(self, _call):
else:
name, args, kwargs = _call
try:
return name, sig.bind(*args, **kwargs)
bound_call = sig.bind(*args, **kwargs)
return call(name, bound_call.args, bound_call.kwargs)
except TypeError as e:
return e.with_traceback(None)
else:
Expand Down Expand Up @@ -863,9 +864,9 @@ def assert_called_with(self, /, *args, **kwargs):
def _error_message():
msg = self._format_mock_failure_message(args, kwargs)
return msg
expected = self._call_matcher((args, kwargs))
expected = self._call_matcher(_Call((args, kwargs)))
actual = self._call_matcher(self.call_args)
if expected != actual:
if actual != expected:
asvetlov marked this conversation as resolved.
Show resolved Hide resolved
cause = expected if isinstance(expected, Exception) else None
raise AssertionError(_error_message()) from cause

Expand Down Expand Up @@ -925,10 +926,10 @@ def assert_any_call(self, /, *args, **kwargs):
The assert passes if the mock has *ever* been called, unlike
`assert_called_with` and `assert_called_once_with` that only pass if
the call is the most recent one."""
expected = self._call_matcher((args, kwargs))
expected = self._call_matcher(_Call((args, kwargs), two=True))
cause = expected if isinstance(expected, Exception) else None
actual = [self._call_matcher(c) for c in self.call_args_list]
if expected not in actual:
cause = expected if isinstance(expected, Exception) else None
if cause or expected not in _AnyComparer(actual):
Copy link
Member

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 with

return list.__contains__(self, value)
. The list __contains__ does an equal per item and hence it does the same check at _Call.__eq__
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?

Copy link
Contributor Author

@ElizabethU ElizabethU Sep 12, 2019

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, 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__

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
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.

Copy link
Member

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.

➜  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

Copy link
Member

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.

➜  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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

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

  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;
 }

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Member

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): 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():

Copy link
Contributor Author

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 to assert_called_with which strikes me as good for consistency, so I added that back in too.

expected_string = self._format_mock_call_signature(args, kwargs)
raise AssertionError(
'%s call not found' % expected_string
Expand Down Expand Up @@ -981,6 +982,22 @@ def _calls_repr(self, prefix="Calls"):
return f"\n{prefix}: {safe_repr(self.mock_calls)}."


class _AnyComparer(list):
"""A list which checks if it contains a call which may have an
argument of ANY, flipping the components of item and self from
their traditional locations so that ANY is guaranteed to be on
the left."""
def __contains__(self, item):
for _call in self:
if len(item) != len(_call):
continue
if all([
expected == actual
for expected, actual in zip(item, _call)
]):
return True
return False


def _try_iter(obj):
if obj is None:
Expand Down Expand Up @@ -2132,9 +2149,9 @@ def _error_message():
msg = self._format_mock_failure_message(args, kwargs, action='await')
return msg

expected = self._call_matcher((args, kwargs))
expected = self._call_matcher(_Call((args, kwargs), two=True))
actual = self._call_matcher(self.await_args)
if expected != actual:
if actual != expected:
cause = expected if isinstance(expected, Exception) else None
raise AssertionError(_error_message()) from cause

Expand All @@ -2153,9 +2170,9 @@ def assert_any_await(self, /, *args, **kwargs):
"""
Assert the mock has ever been awaited with the specified arguments.
"""
expected = self._call_matcher((args, kwargs))
expected = self._call_matcher(_Call((args, kwargs), two=True))
actual = [self._call_matcher(c) for c in self.await_args_list]
if expected not in actual:
if expected not in _AnyComparer(actual):
cause = expected if isinstance(expected, Exception) else None
expected_string = self._format_mock_call_signature(args, kwargs)
raise AssertionError(
Expand Down
28 changes: 26 additions & 2 deletions Lib/unittest/test/testmock/testasync.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
import inspect
import unittest

from unittest.mock import (call, AsyncMock, patch, MagicMock, create_autospec,
_AwaitEvent)
from unittest.mock import (ANY, call, AsyncMock, patch, MagicMock,
create_autospec, _AwaitEvent)


def tearDownModule():
Expand Down Expand Up @@ -599,6 +599,30 @@ def test_assert_has_awaits_no_order(self):
asyncio.run(self._runnable_test('SomethingElse'))
self.mock.assert_has_awaits(calls)

def test_awaits_asserts_with_any(self):
class Foo:
def __eq__(self, other): pass

asyncio.run(self._runnable_test(Foo(), 1))

self.mock.assert_has_awaits([call(ANY, 1)])
self.mock.assert_awaited_with(ANY, 1)
self.mock.assert_any_await(ANY, 1)

def test_awaits_asserts_with_spec_and_any(self):
class Foo:
def __eq__(self, other): pass

mock_with_spec = AsyncMock(spec=Foo)

async def _custom_mock_runnable_test(*args):
await mock_with_spec(*args)

asyncio.run(_custom_mock_runnable_test(Foo(), 1))
mock_with_spec.assert_has_awaits([call(ANY, 1)])
mock_with_spec.assert_awaited_with(ANY, 1)
mock_with_spec.assert_any_await(ANY, 1)

def test_assert_has_awaits_ordered(self):
calls = [call('NormalFoo'), call('baz')]
with self.assertRaises(AssertionError):
Expand Down
21 changes: 21 additions & 0 deletions Lib/unittest/test/testmock/testhelpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,28 @@ def __ne__(self, other): pass
self.assertEqual(expected, mock.mock_calls)
self.assertEqual(mock.mock_calls, expected)

def test_any_no_spec(self):
# This is a regression test for bpo-37555
class Foo:
def __eq__(self, other): pass

mock = Mock()
mock(Foo(), 1)
mock.assert_has_calls([call(ANY, 1)])
mock.assert_called_with(ANY, 1)
mock.assert_any_call(ANY, 1)

def test_any_and_spec_set(self):
# This is a regression test for bpo-37555
class Foo:
def __eq__(self, other): pass

mock = Mock(spec=Foo)

mock(Foo(), 1)
mock.assert_has_calls([call(ANY, 1)])
mock.assert_called_with(ANY, 1)
mock.assert_any_call(ANY, 1)

class CallTest(unittest.TestCase):

Expand Down
2 changes: 2 additions & 0 deletions Misc/ACKS
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,7 @@ Tomer Filiba
Segev Finer
Jeffrey Finkelstein
Russell Finn
Neal Finne
Dan Finnie
Nils Fischbeck
Frederik Fix
Expand Down Expand Up @@ -1699,6 +1700,7 @@ Roger Upole
Daniel Urban
Michael Urman
Hector Urtubia
Elizabeth Uselton
Lukas Vacek
Ville Vainio
Andi Vajda
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix `NonCallableMock._call_matcher` returning tuple instead of `_Call` object
when `self._spec_signature` exists. Patch by Elizabeth Uselton