◐ Shell
reader mode source ↗
Skip to content

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

Merged
lisroach merged 1 commit into
python:masterfrom
mariocj89:pu/docs-patch-async
Sep 9, 2019
Merged

docs: Add references to AsyncMock in unittest.mock.patch#13681
lisroach merged 1 commit into
python:masterfrom
mariocj89:pu/docs-patch-async

Conversation

@mariocj89

@mariocj89 mariocj89 commented May 30, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@bedevere-bot bedevere-bot added the docs Documentation in the Doc dir label May 30, 2019
@mariocj89 mariocj89 force-pushed the pu/docs-patch-async branch from f11000f to fc631e8 Compare May 30, 2019 13:42
@tirkarthi

tirkarthi commented May 30, 2019

Copy link
Copy Markdown
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 :

cpython/Lib/unittest/mock.py

Line 2612 in 98ef920

if asyncio.iscoroutinefunction(original):

@cjw296

cjw296 commented May 30, 2019

Copy link
Copy Markdown
Contributor

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

mariocj89 commented May 30, 2019

Copy link
Copy Markdown
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.

@mariocj89
docs: Add references to AsyncMock in unittest.mock.patch …
c9cbe98 
Update the docs as patch can now return an AsyncMock if the patched
object is an async function.
@mariocj89 mariocj89 force-pushed the pu/docs-patch-async branch from fc631e8 to c9cbe98 Compare May 30, 2019 17:02
@cjw296

cjw296 commented May 30, 2019

Copy link
Copy Markdown
Contributor

@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

asvetlov commented Jun 2, 2019

Copy link
Copy Markdown
Contributor

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

@asvetlov asvetlov self-assigned this Jun 2, 2019
@tirkarthi tirkarthi mentioned this pull request Aug 26, 2019
Merged
@mariocj89

mariocj89 commented Aug 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Gentle ping @asvetlov

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

lisroach
lisroach approved these changes Sep 9, 2019
View reviewed changes

@lisroach lisroach left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

I think this documentation looks good, @asvetlov I'll wait to merge so you have a chance to take a look as well.

asvetlov
asvetlov approved these changes Sep 9, 2019
View reviewed changes

@asvetlov asvetlov left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

bedevere-bot commented Sep 9, 2019

Copy link
Copy Markdown

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

@lisroach

lisroach commented Sep 9, 2019

Copy link
Copy Markdown
Contributor

I thought I did @bedevere-bot :(

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

cjw296 commented Sep 9, 2019

Copy link
Copy Markdown
Contributor

This catches me just about every time too :'(

@miss-islington

miss-islington commented Sep 10, 2019

Copy link
Copy Markdown
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
@mariocj89 @miss-islington
docs: Add references to AsyncMock in unittest.mock.patch (pythonGH-13681 …
d5c5d8a 
)

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>
@bedevere-bot

bedevere-bot commented Sep 10, 2019

Copy link
Copy Markdown

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

miss-islington added a commit that referenced this pull request Sep 10, 2019
@miss-islington @mariocj89
docs: Add references to AsyncMock in unittest.mock.patch (GH-13681) …
eaa1b09 
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>
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@mariocj89 @websurfer5
docs: Add references to AsyncMock in unittest.mock.patch (python#13681) …
65a1e1c 
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

Reviewers

@cjw296 cjw296 cjw296 left review comments

@asvetlov asvetlov asvetlov approved these changes

@lisroach lisroach

+1 more reviewer

@auvipy auvipy auvipy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@asvetlov asvetlov

Labels

docs Documentation in the Doc dir skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@mariocj89 @tirkarthi @cjw296 @asvetlov @bedevere-bot @lisroach @miss-islington @auvipy @the-knights-who-say-ni