
This issue trackerhas been migrated toGitHub, and is currentlyread-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.
Created on2019-06-12 14:00 byjcline, last changed2022-04-11 14:59 byadmin.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 14117 | closed | xtreak,2019-06-15 16:34 | |
| PR 15830 | merged | lisroach,2019-09-10 09:57 | |
| PR 15837 | merged | miss-islington,2019-09-10 11:18 | |
| PR 25347 | open | matthew.suozzo,2021-04-11 17:20 | |
| Messages (11) | |||
|---|---|---|---|
| msg345358 -(view) | Author: Jeremy Cline (jcline)* | Date: 2019-06-12 14:00 | |
This is related to the new AsyncMock[0] class in Python 3.8b1. A simple reproducer is:from unittest import mockmock_obj = mock.MagicMock()mock_obj.mock_func = mock.MagicMock(spec=lambda x: x)with mock.patch.object(mock_obj, "mock_func") as nested: print(type(nested))Instead of a MagicMock (the behavior in Python 3.7) in Python 3.8b1 this results in an AsyncMock.[0]https://github.com/python/cpython/pull/9296 | |||
| msg345377 -(view) | Author: Lisa Roach (lisroach)*![]() | Date: 2019-06-12 16:14 | |
Following up from xtreak's proposal (https://github.com/python/cpython/pull/9296) I think checking if __code__ is actually a CodeType is a good idea. It's simple and doesn't change any other functionality in an unwanted way. | |||
| msg345399 -(view) | Author: Karthikeyan Singaravelan (xtreak)*![]() | Date: 2019-06-12 18:16 | |
I will wait for a couple of days for suggestions and will raise a PR to check for __code__ to be a CodeType. Thanks. | |||
| msg345704 -(view) | Author: Karthikeyan Singaravelan (xtreak)*![]() | Date: 2019-06-15 16:53 | |
I created PR to ensure the __code__ object is checked to be a CodeType and converted the report to a unittest. I also found a similar function _is_async_func [0] which also seems to perform similar check but is used only in create_autospec. creating an autospec function out of MagicMock with a function spec is not possible so though the change could be made it's not testable. Also changing _is_async_func to _is_async_obj in create_autospec shows no test case failure. Can this be removed to use only _is_async_obj? Is there a difference in their usage due to isawaitable check present in _is_async_obj that needs a test?# create_autospec with MagicMock(spec=lambda x: x)$ cpython git:(bpo37251) ./python.exePython 3.9.0a0 (heads/master:7a68f8c28b, Jun 15 2019, 21:00:05)[Clang 7.0.2 (clang-700.1.81)] on darwinType "help", "copyright", "credits" or "license" for more information.>>> from unittest.mock import *>>> create_autospec(MagicMock())<MagicMock spec='MagicMock' id='4370353280'>>>> create_autospec(MagicMock(spec=lambda x: x))Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 2547, in create_autospec mock = Klass(parent=_parent, _new_parent=_parent, _new_name=_new_name, File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 2066, in __init__ super().__init__(*args, **kwargs) File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 1996, in __init__ _safe_super(AsyncMagicMixin, self).__init__(*args, **kw) File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 1007, in __init__ _safe_super(CallableMixin, self).__init__( File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 442, in __init__ self._mock_add_spec(spec, spec_set, _spec_as_instance, _eat_self) File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 503, in _mock_add_spec res = _get_signature_object(spec, File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 99, in _get_signature_object return func, inspect.signature(sig_func) File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 3093, in signature return Signature.from_callable(obj, follow_wrapped=follow_wrapped) File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 2842, in from_callable return _signature_from_callable(obj, sigcls=cls, File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 2292, in _signature_from_callable return _signature_from_function(sigcls, obj, File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 2175, in _signature_from_function parameters.append(Parameter(name, annotation=annotation, File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 2495, in __init__ raise TypeError(msg)TypeError: name must be a str, not a MagicMock[0]https://github.com/python/cpython/blob/3a1d50e7e573efb577714146bed5c03b9c95f466/Lib/unittest/mock.py#L55 | |||
| msg346091 -(view) | Author: Lisa Roach (lisroach)*![]() | Date: 2019-06-20 00:21 | |
Thanks for the patch!To answer your question, I do not think we can remove _is_async_func in favor of _is_async_obj, _is_async_obj will evaluate to True in cases where _is_async_func would not. For example:>>> class NewCoroutine(Awaitable):... def __await__():... pass...>>> c = NewCoroutine()>>> import inspect>>> inspect.isawaitable(c)True>>> inspect.iscoroutinefunction(c)FalseBUT I think removing the `if getattr(obj, '__code__', None)` from `_is_async_obj` actually makes this work correctly. It is possible for a coroutine object to not have a __code__, but I don't think it is possible for a coroutine function to be missing a __code__. Before removing the __code__ check:>>> from unittest.mock import _is_async_func, _is_async_obj>>> import asyncio>>> _is_async_obj(asyncio.sleep(1))<stdin>:1: RuntimeWarning: coroutine 'sleep' was never awaitedRuntimeWarning: Enable tracemalloc to get the object allocation tracebackFalse>>> _is_async_func(asyncio.sleep(1))False_is_async_obj evaluates to False when it should be TrueAfter removing it:>>> from unittest.mock import _is_async_func, _is_async_obj>>> import asyncio>>> _is_async_obj(asyncio.sleep(1))<stdin>:1: RuntimeWarning: coroutine 'sleep' was never awaitedRuntimeWarning: Enable tracemalloc to get the object allocation tracebackTrue>>> _is_async_func(asyncio.sleep(1))FalseIt correctly evaluates to TrueAll tests pass as well. What do you think? | |||
| msg346150 -(view) | Author: Karthikeyan Singaravelan (xtreak)*![]() | Date: 2019-06-20 18:11 | |
> BUT I think removing the `if getattr(obj, '__code__', None)` from `_is_async_obj` actually makes this work correctly. It is possible for a coroutine object to not have a __code__, but I don't think it is possible for a coroutine function to be missing a __code__. Sorry, I am little confused here. If the code attribute check is removed then my test case in PR fails since obj.__code__ that is passed through iscoroutinefunction returns True. Maybe something along the lines of below that if there is a __code__ attribute then always check it's of CodeType. So that my test passes with MagicMock.__code__ detected.If I understand the awaitable examples correctly, mocking the obj which is an Awaitable should be returning an AsyncMock. But obj doesn't contain __code__ and hence check for inspect.isawaitable is never done causing _is_async_obj(obj) to return False and subsequently it's patched with MagicMock.from collections.abc import Awaitablefrom unittest.mock import patchclass NewCoroutine(Awaitable): def __await__(): passobj = NewCoroutine()with patch(f"{__name__}.obj") as m: print(m)$ ./python.exe ../backups/bpo37251_awaitable.py<MagicMock name='obj' id='4552158896'>On removing the __code__ attribute check my test case of MagicMock with __code__ passes through iscoroutinefunction. Perhaps an acceptable tradeoff would be to check for __code__ and if present to be a CodeType or else to resume normal check like below. This way an AsyncMock is returned. Also there is no test failure. I have less understanding on asyncio terminologies over coroutine and awaitables so feel free to correct me if I am wrong. I guess it would be also helpful to have good number of tests for different asyncio object cases so that this could also be documented.$ ./python.exe ../backups/bpo37251_awaitable.py<AsyncMock name='obj' id='4363294672'># _is_async_obj to check for __code__ to be CodeType only if present.def _is_async_obj(obj): code = getattr(obj, '__code__', None) if code and not isinstance(code, CodeType): return False return asyncio.iscoroutinefunction(obj) or inspect.isawaitable(obj)# Also verified asyncio.sleep() to return True for _is_async_obj with above definition>>> from unittest.mock import _is_async_func, _is_async_obj>>> import asyncio>>> _is_async_obj(asyncio.sleep(1))<stdin>:1: RuntimeWarning: coroutine 'sleep' was never awaitedRuntimeWarning: Enable tracemalloc to get the object allocation tracebackTrue | |||
| msg346583 -(view) | Author: Lisa Roach (lisroach)*![]() | Date: 2019-06-26 04:14 | |
Yes, sorry I wasn't clear, I was thinking about the functions and testing without your PR. I think removing the __code__ object (or working around it) is the correct way to go, but just removing it wouldn't solve this particular problem."If I understand the awaitable examples correctly, mocking the obj which is an Awaitable should be returning an AsyncMock. But obj doesn't contain __code__ and hence check for inspect.isawaitable is never done causing _is_async_obj(obj) to return False and subsequently it's patched with MagicMock."Exactly! This is why I think technically removing the __code__ check is correct. Probably removing the __code__ attribute for any AsyncMock that is mocking an async object and not an async function is best, but I don't know how I would do that. I may also be misunderstanding some asyncio concepts, that is just what I observed :)What if instead of checking for the __code__ object at all we check if there it is a Mock object (excluding AsyncMock): def _is_async_obj(obj): sync_mocks = [MagicMock, Mock, PropertyMock, NonCallableMock, NonCallableMagicMock] if (any(isinstance(obj, sync_mock) for sync_mock in sync_mocks) and not isinstance(obj, AsyncMock)): return False return asyncio.iscoroutinefunction(obj) or inspect.isawaitable(obj) | |||
| msg351385 -(view) | Author: Lisa Roach (lisroach)*![]() | Date: 2019-09-09 09:44 | |
I made a new branch with the changes I am suggesting here to try to be more clear:https://github.com/lisroach/cpython/tree/issue37251What do you think? | |||
| msg351621 -(view) | Author: Lisa Roach (lisroach)*![]() | Date: 2019-09-10 11:18 | |
New changesetf1a297acb60b88917712450ebd3cfa707e6efd6b by Lisa Roach in branch 'master':bpo-37251: Removes __code__ check from _is_async_obj. (GH-15830)https://github.com/python/cpython/commit/f1a297acb60b88917712450ebd3cfa707e6efd6b | |||
| msg351644 -(view) | Author: miss-islington (miss-islington) | Date: 2019-09-10 13:16 | |
New changesetc3008dd480d645678409273eecbfd24bbc9669d7 by Miss Islington (bot) in branch '3.8':bpo-37251: Removes __code__ check from _is_async_obj. (GH-15830)https://github.com/python/cpython/commit/c3008dd480d645678409273eecbfd24bbc9669d7 | |||
| msg390689 -(view) | Author: Matthew Suozzo (matthew.suozzo)* | Date: 2021-04-10 03:22 | |
I don't think this was actually fixed for the create_autospec case. create_autospec still uses the only is_async_func check to enable use of AsyncMock and that still does a __code__ check.There was a test submitted to check this case but the test itself was bugged and discovered in the process of implementinghttps://bugs.python.org/issue43478. | |||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:59:16 | admin | set | github: 81432 |
| 2021-04-11 17:20:48 | matthew.suozzo | set | stage: needs patch -> patch review pull_requests: +pull_request24081 |
| 2021-04-10 17:46:38 | gregory.p.smith | set | status: closed -> open resolution: fixed -> stage: resolved -> needs patch |
| 2021-04-10 03:22:54 | matthew.suozzo | set | nosy: +matthew.suozzo messages: +msg390689 |
| 2019-09-10 13:16:03 | miss-islington | set | nosy: +miss-islington messages: +msg351644 |
| 2019-09-10 11:19:58 | lisroach | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
| 2019-09-10 11:18:50 | miss-islington | set | pull_requests: +pull_request15482 |
| 2019-09-10 11:18:48 | lisroach | set | messages: +msg351621 |
| 2019-09-10 09:57:08 | lisroach | set | pull_requests: +pull_request15476 |
| 2019-09-09 09:44:13 | lisroach | set | messages: +msg351385 |
| 2019-06-26 04:14:59 | lisroach | set | messages: +msg346583 |
| 2019-06-20 18:11:12 | xtreak | set | messages: +msg346150 |
| 2019-06-20 00:21:24 | lisroach | set | messages: +msg346091 |
| 2019-06-15 16:53:06 | xtreak | set | messages: +msg345704 versions: + Python 3.9 |
| 2019-06-15 16:34:36 | xtreak | set | keywords: +patch stage: patch review pull_requests: +pull_request13967 |
| 2019-06-12 18:16:29 | xtreak | set | messages: +msg345399 |
| 2019-06-12 16:14:41 | lisroach | set | messages: +msg345377 |
| 2019-06-12 15:05:33 | hroncok | set | nosy: +hroncok |
| 2019-06-12 14:11:44 | mariocj89 | set | nosy: +mariocj89 |
| 2019-06-12 14:06:24 | xtreak | set | nosy: +lisroach,xtreak |
| 2019-06-12 14:00:18 | jcline | create | |