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-38093: Correctly returns AsyncMock for async subclasses. #15947

Merged
merged 15 commits into from Sep 20, 2019

Conversation

@lisroach
Copy link
Contributor

commented Sep 11, 2019

We noticed some funny behavior when mocking a async context manager and async iterator. Mainly that you needed to mock the CM or iterator as a MagicMock and additionally set __aenter__ and __aexit__ on the mock manually. Trying to use an AsyncMock would cause your code to fail. This is counterintuitive and makes the code hard to write.

Fixing the bad if statement in _def_get_child fixes AsyncMocks, now they work like you would think and don't crash.
You can now mock a async context manager or async iterator with either AsyncMock or MagicMock and both function the same.

https://bugs.python.org/issue38093

Lib/unittest/mock.py Outdated Show resolved Hide resolved
@lisroach

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Ah the doctest failure is what I added the #TODO about, I'll need to update the docs as well. I'll fix that TODO today or tomorrow so the failures aren't super opaque.

Looking into fixing the errors now.

lisroach added 6 commits Sep 11, 2019
@asvetlov
Copy link
Contributor

left a comment

please backport to 3.8 on merging

:ref:`async-context-managers` through ``__aenter__`` and ``__aexit__``. The
return value of ``__aenter__`` is an :class:`AsyncMock`.
return value of ``__aenter__`` is an async function.

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 12, 2019

Member

I think it would be better to say something like:

By default, ``__aenter__`` is set to an :class:`AsyncMock` that return an async function.

We want to make the distinction between what the methods are and what they return clear.

@tirkarthi
Copy link
Contributor

left a comment

Docs could be updated about return_value change for MagicMock with respect to async functions and also a test to make sure we don't regress in the future.

@@ -1867,7 +1870,7 @@ def _patch_stopall():
'__reduce__', '__reduce_ex__', '__getinitargs__', '__getnewargs__',
'__getstate__', '__setstate__', '__getformat__', '__setformat__',
'__repr__', '__dir__', '__subclasses__', '__format__',
'__getnewargs_ex__', '__aenter__', '__aexit__', '__anext__', '__aiter__',

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 12, 2019

Contributor

Can we have a test for the default values of these functions? There is a behavior change in 3.8b4 and now where __aexit__ for MagicMock returns False and now it returns a coroutine. The docs can also be updated in the "mock and default values" section since these don't have default values : https://docs.python.org/3.8/library/unittest.mock.html?highlight=asyncmock#unittest.mock.NonCallableMagicMock . I guess this also makes updating _return_values dictionary since the default values are not needed now for these and can be removed.

➜  cpython git:(pr_15947) ✗ python3.8
Python 3.8.0b4 (v3.8.0b4:d93605de72, Aug 29 2019, 21:47:47)
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from unittest.mock import MagicMock
>>> m = MagicMock()
>>> m.__aexit__()
False
➜  cpython git:(pr_15947) ✗ ./python.exe
Python 3.9.0a0 (heads/pr_15947:f476b5845a, Sep 12 2019, 10:52:24)
[Clang 7.0.2 (clang-700.1.81)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from unittest.mock import MagicMock
>>> m = MagicMock()
>>> m.__aexit__()
<coroutine object AsyncMockMixin._mock_call at 0x107005d40>

This comment has been minimized.

Copy link
@lisroach

lisroach Sep 12, 2019

Author Contributor

Hmm this is a good point about __aexit__, I think it should have a default return_value of False (which is in the _return_values dict, but maybe needs to be added to the _calculate_return_value instead.

This section specifically is the "non-default" values, so I don't think it make sense to update that section of the docs, and there is a unit test for the default values that are set in test_magicmock_defaults. I will update __aexit__ and add it there.

This comment has been minimized.

Copy link
@lisroach

lisroach Sep 12, 2019

Author Contributor

Ah well, this is actually another case of "called" vs "awaited". Once you await __aexit__ it returns False correctly, just calling prints the type of __aexit__ which is a coroutine object.

So I want to leave the __aexit__ in _return_value and I will update the test. Do you think that would resolve this?

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 12, 2019

Contributor

Ah okay so it's a case where once __aexit__ is awaited then it returns the calculated value of False. Sorry, I am not sure of async internals to see if __aexit__ can be used like await __aexit__() or makes sense but I can understand in terms of the PR context with mock that the coroutine needs to be always awaited.

This comment has been minimized.

Copy link
@asvetlov

asvetlov Sep 12, 2019

Contributor

await cm.__aexit__() is a legal call.

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 13, 2019

Member

For reference, this is how async cms work: https://www.python.org/dev/peps/pep-0492/#new-syntax
Both __aenter__ and __aexit__ are awaited.

pc.session = MagicMock(name='sessionmock')
cm = MagicMock(name='magic_cm')
response = AsyncMock(name='response')
response.json = AsyncMock(return_value={'json':123})

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 12, 2019

Contributor

PEP8 nit regarding space here and in below dict declarations.

Suggested change
response.json = AsyncMock(return_value={'json':123})
response.json = AsyncMock(return_value={'json': 123})

def test_mock_supports_async_context_manager(self):
called = False
instance = self.WithAsyncContextManager()
mock_instance = MagicMock(instance)
# TODO: Horrible error message if you use a MagicMock here

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 12, 2019

Contributor

Please open a bpo in case it will not be part of the PR.

This comment has been minimized.

Copy link
@lisroach

lisroach Sep 12, 2019

Author Contributor

Actually I think this is fixed, will remove comment and make it a subTest to test both classes.

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 12, 2019

Contributor

Thanks for the clarification. Mine was due to the comment over todo #15947 (comment). I would be happy if it was fixed then.

asyncio.iscoroutine(mock_instance.__anext__))

iterator = instance.__aiter__()
if asyncio.iscoroutine(iterator):

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 12, 2019

Contributor

The test is not parameterized or have subtests so would it be better to have an assertion here like self.assertTrue(asyncio.iscoroutine(iterator)) ?

iterator = asyncio.run(iterator)

mock_iterator = mock_instance.__aiter__()
if asyncio.iscoroutine(mock_iterator):

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 12, 2019

Contributor

The test is not parameterized or have subtests so would it be better to have an assertion here like self.assertTrue(asyncio.iscoroutine(iterator)) ?

with self.subTest("iterate through set return_value"):
with self.subTest("iterate through default value using asyncmock"):
mock_instance = AsyncMock(self.WithAsyncIterator())
self.assertEqual([], asyncio.run(iterate(mock_instance)))

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 12, 2019

Member

The usual convention is to have assertEqual(actual, expected), but here and in the rest of the function the values are inverted. The fix for this might belong to a different PR.

mock_instance = MagicMock(self.WithAsyncIterator())
self.assertEqual([], asyncio.run(iterate(mock_instance)))

with self.subTest("iterate through set return_value"):
with self.subTest("iterate through default value using asyncmock"):

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 12, 2019

Member

The only difference between this with and the one above is the use of AsyncMock instead of MagickMock. Can we use for MockClass in MagicMock, AsyncMock: ... and remove the duplication?

self.assertEqual(asyncio.iscoroutine(instance.__aiter__),
asyncio.iscoroutine(mock_instance.__aiter__))
self.assertEqual(asyncio.iscoroutine(instance.__anext__),
asyncio.iscoroutine(mock_instance.__anext__))

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 12, 2019

Member

I think it would be better to use assertTrue here, and maybe just add a comment to clarify that we are checking that the instance and the mock_instance behave the same.

cm.__aenter__.return_value = response
pc.session.post.return_value = cm
result = asyncio.run(pc.main())
self.assertEqual(result, {'json':123})

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 12, 2019

Member

I would turn this in a helper method that accepts a MockClass as argument and call it from both these methods passing MagicMock and AsyncMock.

@bedevere-bot

This comment has been minimized.

Copy link

commented Sep 12, 2019

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@lisroach

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

I have made the requested changes; please review again.

self.assertTrue(asyncio.iscoroutinefunction(cm.__aenter__))
self.assertTrue(asyncio.iscoroutinefunction(cm.__aexit__))
# These should pass but cause warnings to be raised
# self.assertTrue(inspect.isawaitable(cm.__aenter__()))

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 13, 2019

Contributor

Can we just await on the method for the tests? Would prefer not having commented code.

+        for meth in [cm.__aenter__, cm.__aexit__]:
+            awaitable = meth()
+            self.assertTrue(inspect.isawaitable(awaitable))
+            await awaitable

This comment has been minimized.

Copy link
@lisroach

lisroach Sep 13, 2019

Author Contributor

This will fail because we are trying to await outside of an async function :(

I haven't looked at IsolatedAsyncioTestCase yet! Would that allow us to have an async def test_blah that would solve this?

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 13, 2019

Contributor

I haven't looked at IsolatedAsyncioTestCase yet! Would that allow us to have an async def test_blah that would solve this?

I just documented it yesterday ;) yes, you can write async def test_blah and that would be collected by test runner. It's like a drop in replacement to TestCase and in addition also takes coroutines/async functions as test cases.

This comment has been minimized.

Copy link
@lisroach

lisroach Sep 13, 2019

Author Contributor

Perfect! That would make testing AsyncMock more robust, I've found a good number of places I would like to test with await but haven't been able to. Let's make it another issue, I can take these commented lines out and they will eventually get added back in with the new IsolatedAsyncioTestCase.

This comment has been minimized.

Copy link
@asvetlov

asvetlov Sep 13, 2019

Contributor

IsolatedAsyncioTestCase is not used by any CPython test yet (except self-testing) but I want to rewrite the most of asyncio tests with its usage eventually.

@tirkarthi

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

Slightly offtopic but there is unittest.IsolatedAsyncioTestCase (https://docs.python.org/3.8/library/unittest.html#unittest.IsolatedAsyncioTestCase) so it can remove a bunch of boilerplate to have async functions just to test await statements and using asyncio.run. I would try opening a separate issue for this for discussion.

mock_iter = AsyncMock(name="tester")
mock_iter.__aiter__.return_value = [1, 2, 3]
async def main():
async for i in mock_iter:

This comment has been minimized.

Copy link
@tirkarthi

tirkarthi Sep 13, 2019

Contributor

Slightly confused over if this is needed since we just return a list comprehension and assert only once.

This comment has been minimized.

Copy link
@lisroach

lisroach Sep 13, 2019

Author Contributor

Ah, you're right, didn't notice the double loop

@ezio-melotti
Copy link
Member

left a comment

More comments, mostly minor things, some unrelated things that we discussed briefly IRL -- you know what to do.

>>> async def main():
... async with mock_instance as result:
... pass
...
>>> asyncio.run(main())
>>> mock_instance.__aenter__.assert_called_once()
>>> mock_instance.__aexit__.assert_called_once()

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 13, 2019

Member

Eventually we probably want to replace these either with assert_aiwated_once() or by returning a result and checking for that.

This comment has been minimized.

Copy link
@lisroach

lisroach Sep 13, 2019

Author Contributor

Agreed, I will update them in this PR if that's okay.

if issubclass(_type, AsyncMockMixin):
elif _new_name in _sync_async_magics:
# Special case these ones b/c users will assume they are async,
# but they are actually sync

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 13, 2019

Member

An example of such method in the comment would probably help. Is this about e.g. __aiter__ that sounds async but is actually sync?
Edit: now I saw where _sync_async_magics is defined that __aiter__ is the only one.

@@ -1867,7 +1870,7 @@ def _patch_stopall():
'__reduce__', '__reduce_ex__', '__getinitargs__', '__getnewargs__',
'__getstate__', '__setstate__', '__getformat__', '__setformat__',
'__repr__', '__dir__', '__subclasses__', '__format__',
'__getnewargs_ex__', '__aenter__', '__aexit__', '__anext__', '__aiter__',

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 13, 2019

Member

For reference, this is how async cms work: https://www.python.org/dev/peps/pep-0492/#new-syntax
Both __aenter__ and __aexit__ are awaited.

Lib/unittest/test/testmock/testasync.py Show resolved Hide resolved
def inner_test(mock_type):
called = False
instance = self.WithAsyncContextManager()
mock_instance = mock_type(instance)

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 13, 2019

Member

What about calling these cm and mock_cm?

Lib/unittest/test/testmock/testasync.py Show resolved Hide resolved
result = asyncio.run(use_context_manager())
self.assertTrue(called)
self.assertTrue(mock_instance.__aenter__.called)
self.assertTrue(mock_instance.__aexit__.called)

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 13, 2019

Member

This should probably check if they have been awaited.

mock_iterator = asyncio.run(mock_iterator)
def test_aiter_set_return_value(self):
mock_iter = AsyncMock(name="tester")
mock_iter.__aiter__.return_value = [1, 2, 3]

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 13, 2019

Member

We talked about briefly this at the sprint:

  • check what happens with a genexp
  • check what happens without setting a return_value (should still be iterable)

This comment has been minimized.

Copy link
@lisroach

lisroach Sep 13, 2019

Author Contributor

This is not working quite right yet, but it should be fixed with issue38163


def test_mock_aiter_and_anext_asyncmock(self):
def inner_test(mock_type):
instance = self.WithAsyncIterator

This comment has been minimized.

Copy link
@ezio-melotti

ezio-melotti Sep 13, 2019

Member

Here you are missing the (), you have them at line 438 459.
Is this intentional?

This comment has been minimized.

Copy link
@lisroach

lisroach Sep 13, 2019

Author Contributor

Fixed, it makes no difference so tests don't fail but want to keep them consistent.

@bedevere-bot

This comment has been minimized.

Copy link

commented Sep 13, 2019

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@lisroach lisroach requested a review from ezio-melotti Sep 15, 2019

Update Misc/NEWS.d/next/Library/2019-09-11-14-45-30.bpo-38093.yQ6k7y.rst
Fixes spelling error.

Co-Authored-By: Ezio Melotti <ezio.melotti@gmail.com>

@lisroach lisroach merged commit 8b03f94 into python:master Sep 20, 2019

4 checks passed

Azure Pipelines PR #20190918.5 succeeded
Details
bedevere/issue-number Issue number 38093 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lisroach lisroach deleted the lisroach:asyncmock_magic_cm branch Sep 20, 2019

@miss-islington

This comment has been minimized.

Copy link

commented Sep 20, 2019

Thanks @lisroach for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒🤖

@miss-islington

This comment has been minimized.

Copy link

commented Sep 20, 2019

Sorry, @lisroach, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 8b03f943c37e07fb2394acdcfacd066647f9b1fd 3.8

lisroach added a commit to lisroach/cpython that referenced this pull request Sep 20, 2019
lisroach added a commit to lisroach/cpython that referenced this pull request Sep 20, 2019
[3.8] bpo-38093: Correctly returns AsyncMock for async subclasses. (p…
…ythonGH-15947).

(cherry picked from commit 8b03f94)

Co-authored-by: Lisa Roach <lisaroach14@gmail.com>
@bedevere-bot

This comment has been minimized.

Copy link

commented Sep 20, 2019

GH-16299 is a backport of this pull request to the 3.8 branch.

matrixise added a commit that referenced this pull request Sep 21, 2019
[3.8] bpo-38093: Correctly returns AsyncMock for async subclasses. (G…
…H-15947) (GH-16299)

(cherry picked from commit 8b03f94)

Co-authored-by: Lisa Roach <lisaroach14@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.