-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
docs: Add references to AsyncMock in unittest.mock.patch #13681
Conversation
f11000f
to
fc631e8
Compare
If *new* is omitted, then the target is replaced with a | ||
:class:`MagicMock`. If :func:`patch` is used as a decorator and *new* is | ||
If *new* is omitted, then the target is replaced with an | ||
:class:`AsyncMock` if the patched object is an async function or |
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.
Is it just async functions? What about async methods? Is there such a thing as an async __init__
or __new__
?
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 is what it is controlling it: https://github.com/python/cpython/blob/master/Lib/unittest/mock.py#L49 It is checking for __code__
so it seems to be only for functions at the moment.
I was wondering about that for callable class. My main concern was what if I change a function by a callable instance but it seems there is no good way to check that at the moment. asyncio.iscoroutinefunction
works only for functions.
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.
Hmm, doesn't seem sensible to make changes to the docs until the rest of this is bottomed 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.
Note this is landed already and docs are updated in some other places. The documentation for "patch" does not do what it says it does for async functions right now. Also, I am not sure at all this is going to change, why do you think so?
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 looks like correct wording to me, perhaps we will find a way to make the callable check a little more granular but there is no reason to block this PR on what might be a future change in implementation.
Thank you @mariocj89 for this. I had the same concern while fixing warnings caused due to this change in behavior in the testsuite : https://bugs.python.org/issue37015#msg343352 . I am okay with this being an AsyncMock because it means that there was a coroutine created and it was not awaited where raising a warning makes sense and the user can resort to using MagicMock if they are want it to be not awaited. I would prefer thoughts from @asvetlov @1st1 to discuss this behavior change from 3.7 to ensure the documentation is also updated and if needed an issue for this do discuss. I believe create_autospec has similar behavior : Line 2612 in 98ef920
|
I guess my reticence on this comes party from the fact that the AsyncMock stuff is something I'm still quite unsure about. If I'm honest, I was surprised to see the PR landed when it did and I'm a little concerned about unintended consequences of the stuff that has been added, along with the chance that this now means the backport is pretty much stuck until it can ditch support for all python versions that don't have the required async syntax. |
Good point, I'll add a "versionadded" tag for the patch function.
Good point, I'll add a "versionadded" to mark the change.
If you are saying that you are looking at reverting this behaviour soon, then sure, but otherwise we have out of date documentation, as beta 1 of 3.8 is getting released tomorrow. Not saying that we have to rush things, but people (from tomorrow) won't be seeing this change in the docs. Even if anything was reverted, we can revert the docs and implementation together at the same time. |
Update the docs as patch can now return an AsyncMock if the patched object is an async function.
fc631e8
to
c9cbe98
Compare
@mariocj89 - reverting would be quite an extreme move, and one I'm not comfortable taking, particularly with my very limited knowledge of async stuff. @asvetlo merged the original PR, IIRC, and it sounded like he's part of a team that had been using this already. I'm not sure what the best way to bottom out the remaining issues might be, including the questions I asked above. |
I'll review the PR just after the feature freeze. |
Gentle ping @asvetlov This is a change in 3.8 that will go undocumented otherwise. |
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.
I think this documentation looks good, @asvetlov I'll wait to merge so you have a chance to take a look as well.
If *new* is omitted, then the target is replaced with a | ||
:class:`MagicMock`. If :func:`patch` is used as a decorator and *new* is | ||
If *new* is omitted, then the target is replaced with an | ||
:class:`AsyncMock` if the patched object is an async function or |
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 looks like correct wording to me, perhaps we will find a way to make the callable check a little more granular but there is no reason to block this PR on what might be a future change in implementation.
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, please merge.
:class:`AsyncMock`
doesn't exist in the module documentation though.
@lisroach: Please replace |
I thought I did @bedevere-bot :( |
This catches me just about every time too :'( |
Thanks @mariocj89 for the PR, and @lisroach for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
) Update the docs as patch can now return an AsyncMock if the patched object is an async function. (cherry picked from commit f5e7f39) Co-authored-by: Mario Corchero <mcorcherojim@bloomberg.net>
GH-15842 is a backport of this pull request to the 3.8 branch. |
Update the docs as patch can now return an AsyncMock if the patched object is an async function. (cherry picked from commit f5e7f39) Co-authored-by: Mario Corchero <mcorcherojim@bloomberg.net>
Update the docs as patch can now return an AsyncMock if the patched object is an async function.
Update the docs as patch can now return an AsyncMock if the patched object is an async function.
Just noticed this, happy to create a bug report & news entry but not sure if it is worth it.
CC: @lisroach @tirkarthi @cjw296
Relates-to: #9296