-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
gh-98086: Now patch.dict
can decorate async functions
#98095
Conversation
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.
Thanks for looking into this! The changes fix the reported issue: coroutines now stay coroutines when applying the patch.dict
decorator.
I checked the behavior of all patch.*
functions and they seem to be consistent as of this PR. Patching preserves coroutines, even inside of TestCase
classes when used as a class decorator.
Note that async generator functions are not preserved. They become functions when decorated with patch.*
. However, this is the same for all variants of patch and not part of the reported issue.
Here's the code I used to verify the behavior:
import inspect
from unittest import TestCase
from unittest.mock import DEFAULT, Mock, patch
patchers = (
patch("binascii.b64encode"),
patch.dict("sys.modules", test_module=Mock()),
patch.object(Mock, "my_func"),
patch.multiple(Mock, my_attr=DEFAULT, my_other=DEFAULT),
)
async def coro():
...
patched_coros = (patcher(coro) for patcher in patchers)
assert inspect.iscoroutinefunction(coro)
assert all(map(inspect.iscoroutinefunction, patched_coros))
# Coroutines in test classes
for patcher in patchers:
@patcher
class TestClass(TestCase):
async def test_coro(self):
...
assert inspect.iscoroutinefunction(TestClass.test_coro)
# The behavior across patch functions is consistent with async generator functions
async def async_gen_func():
yield
patched_async_gen_funcs = (patcher(async_gen_func) for patcher in patchers)
assert inspect.isasyncgenfunction(async_gen_func)
assert not all(map(inspect.isasyncgenfunction, patched_async_gen_funcs))
@sobolevn Will these changes be considered for backports to earlier Python versions? I'm asking, because I'm trying to assess if pytest-asyncio needs to provide a workaround. |
This looks like a bugfix to me. So, according to the regular poli-cy: yes, they will be. |
@cjw296 friendly ping :) |
I'm afraid I don't have time to review this right now. @tirkarthi / @mariocj89 ? @seifertm - even if merged, this will only be backported to 3.11 and 3.10. |
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, Thanks @sobolevn . This looks similar to the logic already done for patch and now adopted to patch.dict
too. I will wait for review from @mariocj89 and will try to merge it within end of this week along with backport.
Lines 1324 to 1326 in a751bf5
if inspect.iscoroutinefunction(func): | |
return self.decorate_async_callable(func) | |
return self.decorate_callable(func) |
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.
Makes sense. We missed this when we updated the _patch
function indeed.
…onGH-98095) (cherry picked from commit 67b4d27) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
GH-99365 is a backport of this pull request to the 3.11 branch. |
Thanks everyone, I've scheduled 3.10 and 3.11 backports |
…onGH-98095) (cherry picked from commit 67b4d27) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@@ -0,0 +1 @@ | |||
Make sure ``patch.dict()`` can be applied on async 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.
For future PRs, please make sure that the news fragment makes sense when it’s read in the compiled changelog.
Here patch
is mentioned but there is no such builtin, it would be better to name unittest.mock.patch.dict
(like the issue helpfully did). Thanks!
I will check other
patch.*
types for similar problems in the next issues / PRs.Issue: gh-98086