GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — docs: Add references to AsyncMock in unittest.mock.patch by mariocj89 · Pull Request #13681 · python/cpython

Conversation

@mariocj89

@mariocj89 mariocj89 commented May 30, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added the docs Documentation in the Doc dir label May 30, 2019
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 30, 2019
@bedevere-bot bedevere-bot added the awaiting review label May 30, 2019
@mariocj89
mariocj89 force-pushed the pu/docs-patch-async branch from f11000f to fc631e8 Compare May 30, 2019 13:42
cjw296
cjw296 reviewed May 30, 2019
View reviewed changes
Comment thread Doc/library/unittest.mock.rst Outdated

@cjw296 cjw296 May 30, 2019

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mariocj89 mariocj89 May 30, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjw296 cjw296 May 30, 2019

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mariocj89 mariocj89 May 30, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lisroach lisroach Sep 9, 2019

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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):

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
auvipy
auvipy approved these changes May 30, 2019
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels May 30, 2019
@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.
mariocj89 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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).
mariocj89 and tirkarthi reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@asvetlov asvetlov self-assigned this Jun 2, 2019
@tirkarthi tirkarthi mentioned this pull request Aug 26, 2019
bpo-37955: mock.patch incorrect reference to Mock #15521
Merged
@mariocj89

mariocj89 commented Aug 27, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Gentle ping @asvetlov

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lisroach lisroach added skip issue skip news labels Sep 9, 2019
lisroach
lisroach approved these changes Sep 9, 2019
View reviewed changes

@lisroach lisroach left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/unittest.mock.rst Outdated

@lisroach lisroach Sep 9, 2019

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Sep 9, 2019
asvetlov
asvetlov approved these changes Sep 9, 2019
View reviewed changes

@asvetlov asvetlov left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 9, 2019
@lisroach

lisroach commented Sep 9, 2019

Copy link
Copy Markdown
Contributor

I thought I did @bedevere-bot :(
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 :'(
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lisroach lisroach added the needs backport to 3.8 label Sep 10, 2019
@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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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 <[email protected]>
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.8 label Sep 10, 2019
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 <[email protected]>
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 lisroach approved these changes
+1 more reviewer
@auvipy auvipy auvipy approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@mariocj89 @tirkarthi @cjw296 @asvetlov @bedevere-bot @lisroach @miss-islington @auvipy @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.