Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
bpo-26467: Adds AsyncMock for asyncio Mock library support#9296
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lisroach commentedSep 14, 2018
Talking with@asvetlov, it might be easier/more useful to implement this purely as an AwaitableMock, instead of mocking Coroutines themselves. This should also be more flexible in terms of inspect, because I could assert isawaitable() more easily than iscoroutine(). We also chatted about ideas around mocking AsyncIterators so we can continue to supportaiter andanext, but that will probably come in a separate commit. |
1st1 commentedSep 14, 2018
+1. |
cjw296 commentedMay 2, 2019
@lisroach - I'm afraid I don't any anywhere near enough experience with asyncio to sensibly review this, however one request: please can you split any tests that use the 3.7+ async syntax into their own file ( I'm not sure if this will be possible, but if you could avoid using any 3.7+ syntax inside mock.py itself, that would, again, make backporting significantly easier :-) |
lisroach commentedMay 6, 2019
@cjw296 Good point about the backporting. So separating the tests is no problem, easy-peasy. For the internals of mock.py itself it's a bit more challenging because of the What if I try to wrap all the AsyncMock related code in mock.py in some regexable comment block (like |
cjw296 commentedMay 7, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What might work better is if you could put the |
lisroach commentedMay 7, 2019
I don't think it's possible to separate it out into a different file, the code is too integrated with the mock code itself we can't avoid the circular dependencies. I've moved all of the 3.7+ syntax code to the bottom of mock.py so it's all in one spot, will it be possible for it to be dropped when backporting with it like this? |
asvetlov commentedMay 7, 2019
@cjw296 the PR not only adds Please elaborate how |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
cjw296 commentedMay 7, 2019
@asvetlov - yeah, I've commented on the "hacks already existing mock classes". This doesn't feel like a good idea to me, do we really need it? The elaboration requested, would be this in cpython: This would be backported as: ...so the patch can stay pretty much identical bar adding one line and changing another. It does require all Python 3.7+-only syntax going into mockasync.py. |
1st1 commentedMay 16, 2019
@tirkarthi Most of your comments are on point, but I think we can address them once this is merged. |
tirkarthi commentedMay 16, 2019
@1st1 I would request create_autospec to be fixed as it's one line change causing NameError before beta release and if possible as part of this PR. Fixing the ResourceWarning would also be great just to make sure there is no buildbot failure outside of primary CI since I don't know the configuration in which they are run. More tests and docs could be added after beta and I would be happy to help with follow up PRs for the same. Thanks for approving this. |
tirkarthi commentedMay 16, 2019
@lisroach Attached is a patch fixing docs and doctest that would make CI green. I tested it on my fork and seems to work fine. diff --git a/Doc/library/unittest.mock.rst b/Doc/library/unittest.mock.rstindex a8d05b216c..5d4737b60f 100644--- a/Doc/library/unittest.mock.rst+++ b/Doc/library/unittest.mock.rst@@ -201,9 +201,11 @@ The Mock Class .. testsetup::+ import asyncio+ import inspect import unittest from unittest.mock import sentinel, DEFAULT, ANY- from unittest.mock import patch, call, Mock, MagicMock, PropertyMock+ from unittest.mock import patch, call, Mock, MagicMock, PropertyMock, AsyncMock from unittest.mock import mock_open :class:`Mock` is a flexible mock object intended to replace the use of stubs and@@ -885,9 +887,9 @@ object:: ... >>> mock = MagicMock(async_func) >>> mock- <MagicMock spec='function' id='4568403696'>+ <MagicMock spec='function' id='...'> >>> mock()- <coroutine object AsyncMockMixin._mock_call at 0x1104cb440>+ <coroutine object AsyncMockMixin._mock_call at ...> .. method:: assert_awaited()@@ -976,11 +978,11 @@ object:: Assert the mock has been awaited with the specified calls. The :attr:`await_args_list` list is checked for the awaits.- If `any_order` is False (the default) then the awaits must be+ If *any_order* is False (the default) then the awaits must be sequential. There can be extra calls before or after the specified awaits.- If `any_order` is True then the awaits can be in any order, but+ If *any_order* is True then the awaits can be in any order, but they must all appear in :attr:`await_args_list`. >>> mock = AsyncMock() |
| deftest_create_autospec_instance(self): | ||
| withself.assertRaises(RuntimeError): | ||
| create_autospec(async_func,instance=True) |
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.
This test fails since theRuntimeError check is commented out.
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.
Ah yeah, sorry I was trying to remember the answer to your question on why it was needed. It might not be and I'm debating about taking it out, I think I don't fully understand the purpose of instance=True and what we would want the behavior to be if it is True with create_autospec on a async function.
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.
No problem, we can remove the restriction later if needed. I have added one more doctest suggestion that would make the CI green and this can be merged.
Doc/library/unittest.mock.rst Outdated
| >>>mock= AsyncMock() | ||
| >>>asyncdefmain(*args): | ||
| ...await mock() |
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.
| ... await mock() | |
| ... await mock(*args) |
Doc/library/unittest.mock.rst Outdated
| >>>mock= AsyncMock() | ||
| >>>asyncdefmain(*args): | ||
| ...await mock() |
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.
| ... await mock() | |
| ... await mock(*args) |
Doc/library/unittest.mock.rst Outdated
| >>>mock.await_args_list | ||
| [] | ||
| >>>asyncio.run(main('foo')) | ||
| >>>mock.await_args |
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.
| >>>mock.await_args | |
| >>>mock.await_args_list |
tirkarthi left a comment
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.
LGTM. Buildbots are green and it will be great to have this merged for beta. I will create a followup PR that can be reviewed separately without holding this up any further.
Thank you very much@lisroach :)
lisroach commentedMay 20, 2019
Thanks for all the help and reviews everyone! |
mariocj89 commentedMay 22, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I know I am late to the party. Real apologies about it, I had LASIK past week and could not check this on the weekend. I don't know how many times people have asked about My main comment here would be whether it is useful or confusing the fact that Example, not sure this is expected: >>>fromunittest.mockimportAsyncMock>>>m=AsyncMock()>>>m.a.return_value=1>>>print(m.a)<AsyncMockname='mock.a'id='140389785043824'>>>>print(m.a())<coroutineobjectAsyncMockMixin._mock_callat0x7faf0ab8d240><stdin>:1:RuntimeWarning:coroutine'AsyncMockMixin._mock_call'wasneverawaitedRuntimeWarning:Enabletracemalloctogettheobjectallocationtraceback I think I'd use the
In short, if we have a class that has async and normal methods, Should we use a normal mock and add If this was thought and 3 was chosen purposely because it makes more sense for async users, all good (as said, not an expert in async at all), just raising the question. |
mariocj89 commentedMay 22, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Example, how do we expect users to simulate the following class? classX:asyncdefa(self):return"a"defb(self):return"b" If the plan is to use AsyncMock for it, the following does not work: m=AsyncMock(**{"a.return_value":"mocka","b.return_value":"mockb"})asyncdefcheck(m):print(m.b())print(awaitm.a()) Output: If we say that users should specify the exact type of mock on each of the attribute, what is the purpose of automatically generating them for the attribute for As said, minor concern, there might be no better solution, but wanted to have a look at it. |
asvetlov commentedMay 22, 2019
Thanks for the comment. |
lisroach commentedMay 24, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for pointing that out@mariocj89! I made it recursive asyncmocks more as a way to copy the normal functionality of Mock, which creates an internal mock of the same kind when calling an attribute, for example: It is mentioned in the docstring "By default child mocks will be the same type as the parent", although this can be overridden. The current functionality is that if you want to mock a mixed class, you use a normal mock and it figures out for you which functions are async: But you are right if someone was attempting to build their own class via mock the would need to specify the mock type of the arguments. I don't usually use mock this way so I hadn't considered that use case. In your example it looks like there would be no way to tell it is supposed to be an awaitable mock until the actual call occurs, which might be tricky to implement. If you think it will be useful though I don't mind if you open up a new bug and we can figure it out there! |
jeremycline commentedJun 11, 2019
Hi folks, I've hit anissue I think is related to this PR in Python 3.8b1. The minimal reproducer I came up with is: What happens is that I thought about just submitting a fix, but it's not clear to me whether adding a check to |
lisroach commentedJun 12, 2019
Hey@jeremycline, would you mind making a bug on bugs.python.org? It will be best to centralize discussion there. Thanks for reporting this! It is definitely a bug. I understand the desire to change MagicMock to avoid this behavior, but I think it is the wrong approach. It is not evaluating to True because of the The issue comes in when checking:
in
It does not take into account the The only way I see us getting around this would be to set the It might be best to make a workaround in the |
njsmith commentedJun 12, 2019
Maybe |
tirkarthi commentedJun 12, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I feel this is a reasonable trade off that maybe we could check if def_is_async_obj(obj):code=getattr(obj,'__code__',None)ifcodeandisinstance(code,CodeType):returnasyncio.iscoroutinefunction(obj)orinspect.isawaitable(obj)else:returnFalse |
jeremycline commentedJun 12, 2019
Thanks for the quick response, I've filed an issue athttps://bugs.python.org/issue37251. |
| _spec_asyncs= [] | ||
| forattrindir(spec): | ||
| ifasyncio.iscoroutinefunction(getattr(spec,attr,None)): |
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.
Late note, not sure if anyone gets a notification for this: This broke a use case for me. Before this change, theMock constructor did not callgetattr on any of the attributes of the mocked object specification. I used it to mock an instance of a base class that does not have all its properties implemented, using the**kwargs in theMock constructor to ensure that unimplemented properties that were accessed by any tests got a sane value. I can solve this by using a subclass that does not have unimplemented properties. But maybe it would be wise to skip all attributes here that appear in**kwargs, because they will be subsequently overwritten?
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.
not sure if anyone gets a notification for this
For this reason it's worth creating a new issue and mentioning this PR there.
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.
@f0k - better yet, if you could work up a PR with a test that shows the problem you encountered, along with a fix for it, that would be better!
tirkarthiFeb 21, 2023 • edited by cjw296
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by cjw296
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
For this reason it's worth creating a new issue
I wasn't sure yet whether this warrants a fix, and was aiming low :)
some related issues
Good spot, yes, they're both the same problem, and there are already three PRs proposing a fix. I will continue the discussion in#85934, as it's the older one of the two issues.
Uh oh!
There was an error while loading.Please reload this page.
This is my initial pass at supporting coroutine mocking via a new Mock subclass, CoroutineMock.
It can be used to mock out coroutines and have them validate as coroutines:
Test that a coroutine was awaited:
Also awaited_with, awaited_once_with, etc.
Things that I could use advice on:
Some of the code has been borrowed by asynctest (https://pypi.org/project/asynctest/) which is licensed under Apache 2. I have reached out to the author and am waiting to hear back, but am not sure how we feel about having Apache 2 licensed stuff in CPython. I could rewrite those spots if needed.
inspect.iscoroutinewill return False for CoroutineMock objects. This is because the check isisinstance(object, types.CoroutineType), and CoroutineTypes are concrete. I've looked into using something like, register, but it doesn't work here, so I need someway to make a Mock look like a types.CoroutineType but I am unsure how.In CoroutineMockAssert tests, I have
asyncio.events._event_loop_policyunset because it causes the env to change when running tests if it is not unset. There is probably a better way to do this where the environment doesn't get polluted?I plan on working on getting more example test cases for the CoroutineArguments test section, would be happy for advice though.
And I will be writing up the documentation once the code has settled.
https://bugs.python.org/issue26467