Mocking a MagicMock with a function spec results in an AsyncMock

[jeremycline]

BPO	37251
Nosy	@lisroach, @hroncok, @mariocj89, @miss-islington, @tirkarthi, @jeremycline, @msuozzo
PRs	bpo-37251: MagicMock with function spec used as a spec in mock.patch should return a MagicMock #14117 bpo-37251: Removes __code__ check from _is_async_obj. #15830 [3.8] bpo-37251: Removes __code__ check from _is_async_obj. (GH-15830) #15837 gh-81432: Make create_autospec generate AsyncMocks for awaitable classes #25347

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2019-06-12.14:00:18.556>
labels = ['3.8', 'type-bug', 'library', '3.9']
title = 'Mocking a MagicMock with a function spec results in an AsyncMock'
updated_at = <Date 2021-04-11.17:20:48.974>
user = 'https://github.com/jeremycline'

bugs.python.org fields:

activity = <Date 2021-04-11.17:20:48.974>
actor = 'matthew.suozzo'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2019-06-12.14:00:18.556>
creator = 'jcline'
dependencies = []
files = []
hgrepos = []
issue_num = 37251
keywords = ['patch']
message_count = 11.0
messages = ['345358', '345377', '345399', '345704', '346091', '346150', '346583', '351385', '351621', '351644', '390689']
nosy_count = 7.0
nosy_names = ['lisroach', 'hroncok', 'mariocj89', 'miss-islington', 'xtreak', 'jcline', 'matthew.suozzo']
pr_nums = ['14117', '15830', '15837', '25347']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue37251'
versions = ['Python 3.8', 'Python 3.9']

[jeremycline]

This is related to the new AsyncMock[0] class in Python 3.8b1. A simple reproducer is:

from unittest import mock

mock_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]#9296

[lisroach]

Following up from xtreak's proposal (#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.

[tirkarthi]

I will wait for a couple of days for suggestions and will raise a PR to check for __code__ to be a CodeType. Thanks.

[tirkarthi]

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.exe
Python 3.9.0a0 (heads/master:7a68f8c28b, Jun 15 2019, 21:00:05)
[Clang 7.0.2 (clang-700.1.81)] on darwin
Type "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]

cpython/Lib/unittest/mock.py

Line 55 in 3a1d50e

def _is_async_func(func):

[lisroach]

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)
False

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.

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 awaited
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
False
>>> _is_async_func(asyncio.sleep(1))
False

_is_async_obj evaluates to False when it should be True

After 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 awaited
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
True
>>> _is_async_func(asyncio.sleep(1))
False

It correctly evaluates to True

All tests pass as well. What do you think?

[tirkarthi]

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 Awaitable
from unittest.mock import patch

class NewCoroutine(Awaitable):
    def __await__():
        pass

obj = 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 awaited
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
True

[lisroach]

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)

[lisroach]

I made a new branch with the changes I am suggesting here to try to be more clear: https://github.com/lisroach/cpython/tree/issue37251

What do you think?

[lisroach]

New changeset f1a297a by Lisa Roach in branch 'master':
bpo-37251: Removes __code__ check from _is_async_obj. (GH-15830)
f1a297a

[miss-islington]

New changeset c3008dd by Miss Islington (bot) in branch '3.8':
bpo-37251: Removes __code__ check from _is_async_obj. (GH-15830)
c3008dd