bpo-38093: Correctly returns AsyncMock for async subclasses. #15947

lisroach · 2019-09-11T13:41:59Z

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

PR. This reverts commit 7fbd8fa.

Lib/unittest/test/testmock/testasync.py

Lib/unittest/mock.py

lisroach · 2019-09-11T14:18:34Z

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.

asvetlov

please backport to 3.8 on merging

ezio-melotti · 2019-09-12T10:15:24Z

Doc/library/unittest.mock-examples.rst

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


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

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.

tirkarthi · 2019-09-12T10:14:34Z

Lib/unittest/mock.py

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


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>

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.

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?

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.

await cm.__aexit__() is a legal call.

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

tirkarthi · 2019-09-12T10:17:02Z

Lib/unittest/test/testmock/testasync.py

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


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

tirkarthi · 2019-09-12T10:18:09Z

Lib/unittest/test/testmock/testasync.py


    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


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

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

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

tirkarthi · 2019-09-12T10:20:04Z

Lib/unittest/test/testmock/testasync.py

+                         asyncio.iscoroutine(mock_instance.__anext__))
+
+        iterator = instance.__aiter__()
+        if asyncio.iscoroutine(iterator):


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

tirkarthi · 2019-09-12T10:20:21Z

Lib/unittest/test/testmock/testasync.py

+            iterator = asyncio.run(iterator)
+
+        mock_iterator = mock_instance.__aiter__()
+        if asyncio.iscoroutine(mock_iterator):


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

ezio-melotti · 2019-09-12T10:49:33Z

Lib/unittest/test/testmock/testasync.py

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


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.

ezio-melotti · 2019-09-12T11:15:32Z

Lib/unittest/test/testmock/testasync.py

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


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?

ezio-melotti · 2019-09-12T11:18:40Z

Lib/unittest/test/testmock/testasync.py

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


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.

ezio-melotti · 2019-09-12T11:20:39Z

Lib/unittest/test/testmock/testasync.py

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


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 · 2019-09-12T11:20:48Z

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

lisroach · 2019-09-12T15:26:04Z

I have made the requested changes; please review again.

tirkarthi · 2019-09-13T10:04:05Z

Lib/unittest/test/testmock/testasync.py

+        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__()))


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

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.

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.

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 · 2019-09-13T10:06:37Z

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.

tirkarthi · 2019-09-13T10:41:10Z

Lib/unittest/test/testmock/testasync.py

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


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

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

ezio-melotti

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

ezio-melotti · 2019-09-13T12:32:04Z

Doc/library/unittest.mock-examples.rst

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


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

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

ezio-melotti · 2019-09-13T12:40:44Z

Lib/unittest/mock.py

-        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


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.

ezio-melotti · 2019-09-13T12:46:07Z

Lib/unittest/mock.py

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


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

ezio-melotti · 2019-09-13T13:17:51Z

Lib/unittest/test/testmock/testasync.py

+        def inner_test(mock_type):
+            called = False
+            instance = self.WithAsyncContextManager()
+            mock_instance = mock_type(instance)


What about calling these cm and mock_cm?

Lib/unittest/test/testmock/testasync.py

ezio-melotti · 2019-09-13T15:49:05Z

Lib/unittest/test/testmock/testasync.py

+            result = asyncio.run(use_context_manager())
+            self.assertTrue(called)
+            self.assertTrue(mock_instance.__aenter__.called)
+            self.assertTrue(mock_instance.__aexit__.called)


This should probably check if they have been awaited.

ezio-melotti · 2019-09-13T16:02:59Z

Lib/unittest/test/testmock/testasync.py

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


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 is not working quite right yet, but it should be fixed with issue38163

ezio-melotti · 2019-09-13T16:05:27Z

Lib/unittest/test/testmock/testasync.py

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


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

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

Lib/unittest/test/testmock/testmagicmethods.py

bedevere-bot · 2019-09-13T16:11:42Z

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

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>

miss-islington · 2019-09-20T04:04:21Z

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

miss-islington · 2019-09-20T04:04:42Z

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

…H-15947) (cherry picked from commit 8b03f94)

…ythonGH-15947). (cherry picked from commit 8b03f94) Co-authored-by: Lisa Roach <lisaroach14@gmail.com>

bedevere-bot · 2019-09-20T16:48:03Z

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

…H-15947) (GH-16299) (cherry picked from commit 8b03f94) Co-authored-by: Lisa Roach <lisaroach14@gmail.com>

lisroach added 3 commits September 10, 2019 15:33

Adds references to AsyncMock in more places.

7fbd8fa

bpo-38093: Correctly returns AsyncMock for async subclasses.

8ef74fe

Revert "Adds references to AsyncMock in more places." for a different

d3f2706

PR. This reverts commit 7fbd8fa.

lisroach requested review from voidspace and ezio-melotti September 11, 2019 13:41

the-knights-who-say-ni added the CLA signed label Sep 11, 2019

bedevere-bot added the awaiting core review label Sep 11, 2019

Adds news entry Blurb.

bd02a3a

tirkarthi reviewed Sep 11, 2019

View reviewed changes

Lib/unittest/test/testmock/testasync.py Show resolved Hide resolved

tirkarthi reviewed Sep 11, 2019

View reviewed changes

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

lisroach and others added 6 commits September 11, 2019 15:41

Removes async magics from _calculage_return_values.

d656592

Fixes aiter, warnings, and example docs.

b831e58

Merge branch 'master' into asyncmock_magic_cm

5b3f96a

Adds more tests for aiter using AsyncMock.

b4e164c

Updates news to be more concise.

14e684e

Imports AsyncMock into mock examples.

f476b58

asvetlov approved these changes Sep 12, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 12, 2019

ezio-melotti reviewed Sep 12, 2019

View reviewed changes

tirkarthi reviewed Sep 12, 2019

View reviewed changes

Updates docs, adds test.

0d3af26

ezio-melotti requested changes Sep 12, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 12, 2019

Subtests things and fixes with comments.

7d9dfa0

bedevere-bot removed the awaiting changes label Sep 12, 2019

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 12, 2019

tirkarthi reviewed Sep 13, 2019

View reviewed changes

Updates docs, takes out commented lines in test.

d671277

tirkarthi reviewed Sep 13, 2019

View reviewed changes

ezio-melotti requested changes Sep 13, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 13, 2019

Updates tests with comments.

bb57b0c

lisroach requested a review from ezio-melotti September 15, 2019 12:04

ezio-melotti approved these changes Sep 17, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 17, 2019

ezio-melotti reviewed Sep 17, 2019

View reviewed changes

Misc/NEWS.d/next/Library/2019-09-11-14-45-30.bpo-38093.yQ6k7y.rst Outdated Show resolved Hide resolved

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

8bc3e32

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

lisroach added the needs backport to 3.8 label Sep 20, 2019

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

lisroach deleted the asyncmock_magic_cm branch September 20, 2019 04:04

bedevere-bot removed the awaiting merge label Sep 20, 2019

miss-islington assigned lisroach Sep 20, 2019

lisroach added a commit to lisroach/cpython that referenced this pull request Sep 20, 2019

bpo-38093: Correctly returns AsyncMock for async subclasses. (pythonG…

7977a59

…H-15947) (cherry picked from commit 8b03f94)

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…

73f7577

…ythonGH-15947). (cherry picked from commit 8b03f94) Co-authored-by: Lisa Roach <lisaroach14@gmail.com>

bedevere-bot removed the needs backport to 3.8 label Sep 20, 2019

matrixise pushed a commit that referenced this pull request Sep 21, 2019

[3.8] bpo-38093: Correctly returns AsyncMock for async subclasses. (G…

865bb68

…H-15947) (GH-16299) (cherry picked from commit 8b03f94) Co-authored-by: Lisa Roach <lisaroach14@gmail.com>

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

bpo-38093: Correctly returns AsyncMock for async subclasses. #15947

bpo-38093: Correctly returns AsyncMock for async subclasses. #15947

Conversation

lisroach commented Sep 11, 2019 • edited Loading

lisroach commented Sep 11, 2019

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirkarthi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirkarthi Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 12, 2019

lisroach commented Sep 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirkarthi commented Sep 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezio-melotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 13, 2019

miss-islington commented Sep 20, 2019

miss-islington commented Sep 20, 2019

bedevere-bot commented Sep 20, 2019

lisroach commented Sep 11, 2019 •

edited

Loading

tirkarthi Sep 12, 2019 •

edited

Loading