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

docs: Add references to AsyncMock in unittest.mock.patch #13681

Merged
merged 1 commit into from
Sep 9, 2019

Conversation

mariocj89
Copy link
Contributor

@mariocj89 mariocj89 commented May 30, 2019

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

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
Copy link
Contributor

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

Copy link
Contributor Author

@mariocj89 mariocj89 May 30, 2019

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@mariocj89 mariocj89 May 30, 2019

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?

Copy link
Contributor

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.

@tirkarthi
Copy link
Member

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 :

if asyncio.iscoroutinefunction(original):

@cjw296
Copy link
Contributor

cjw296 commented May 30, 2019

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.

@mariocj89
Copy link
Contributor Author

ensure the documentation is also updated

Good point, I'll add a "versionadded" tag for the patch function.

I believe create_autospec has similar behavior :

Good point, I'll add a "versionadded" to mark the change.

I guess my reticence on this comes party from the fact that the AsyncMock stuff is something I'm still quite unsure about.

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.
@cjw296
Copy link
Contributor

cjw296 commented May 30, 2019

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

@asvetlov
Copy link
Contributor

asvetlov commented Jun 2, 2019

I'll review the PR just after the feature freeze.
Sorry, I'm pretty busy before this date (tomorrow, actually).

@mariocj89
Copy link
Contributor Author

mariocj89 commented Aug 27, 2019

Gentle ping @asvetlov

This is a change in 3.8 that will go undocumented otherwise.

Copy link
Contributor

@lisroach lisroach left a 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
Copy link
Contributor

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.

Copy link
Contributor

@asvetlov asvetlov left a 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 lisroach merged commit f5e7f39 into python:master Sep 9, 2019
@bedevere-bot
Copy link

@lisroach: Please replace # with GH- in the commit message next time. Thanks!

@lisroach
Copy link
Contributor

lisroach commented Sep 9, 2019

I thought I did @bedevere-bot :(

@mariocj89 mariocj89 deleted the pu/docs-patch-async branch September 9, 2019 14:22
@cjw296
Copy link
Contributor

cjw296 commented Sep 9, 2019

This catches me just about every time too :'(

@lisroach lisroach added the needs backport to 3.8 only security fixes label Sep 10, 2019
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 10, 2019
)

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 <[email protected]>
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.8 only security fixes label Sep 10, 2019
miss-islington added a commit that referenced this pull request Sep 10, 2019
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 <[email protected]>
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
Update the docs as patch can now return an AsyncMock if the patched
object is an async function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants