Skip to content
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

Mocking a MagicMock with a function spec results in an AsyncMock #81432

Open
jeremycline mannequin opened this issue Jun 12, 2019 · 11 comments
Open

Mocking a MagicMock with a function spec results in an AsyncMock #81432

jeremycline mannequin opened this issue Jun 12, 2019 · 11 comments
Labels
3.8 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jeremycline
Copy link
Mannequin

jeremycline mannequin commented Jun 12, 2019

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
    Copy link
    Mannequin Author

    jeremycline mannequin commented Jun 12, 2019

    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

    @jeremycline jeremycline mannequin added 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jun 12, 2019
    @lisroach
    Copy link
    Contributor

    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
    Copy link
    Member

    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
    Copy link
    Member

    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]

    def _is_async_func(func):

    @tirkarthi tirkarthi added the 3.9 only security fixes label Jun 15, 2019
    @lisroach
    Copy link
    Contributor

    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
    Copy link
    Member

    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
    Copy link
    Contributor

    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
    Copy link
    Contributor

    lisroach commented Sep 9, 2019

    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
    Copy link
    Contributor

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

    @miss-islington
    Copy link
    Contributor

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

    @msuozzo
    Copy link
    Mannequin

    msuozzo mannequin commented Apr 10, 2021

    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 implementing https://bugs.python.org/issue43478. (#87644)

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    Status: No status
    Development

    No branches or pull requests

    4 participants