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

bpo-37047: Refactor AsyncMock setup logic for autospeccing #13574

Merged
merged 11 commits into from
May 27, 2019

Conversation

tirkarthi
Copy link
Member

@tirkarthi tirkarthi commented May 25, 2019

  • Initialize awaited attribute for AsyncMock during autospec.
  • Fix late binding issue for mock helpers API to ensure correct function is called during attribute access.
  • Document newly implemented async methods.
  • Fix assert_not_awaited error message.

https://bugs.python.org/issue37047

@tirkarthi
Copy link
Member Author

I am not sure if a NEWS entry is needed since the changes were not released but added one just in case it doesn't make it for 3.8 beta 1. Note to self and reviewer :

  • _is_async_obj and _is_async_func look very much same. Except there is isawaitable which also seems to accept coroutine. Since only coroutine function has to be accepted as spec I have kept them separate.
  • I have added test for much of the attribute access. So test might seem little verbose and can be removed since we can assuming one method call working properly would make sure late binding is handled.
  • autospec=True for patch uses create_autospec internally so a test has been added for the same.
  • I haven't added a test for error message change.

@pablogsal
Copy link
Member

CC: @mariocj89

@@ -51,6 +51,13 @@ def _is_async_obj(obj):
return False


def _is_async_func(func):
if getattr(func, '__code__', None):
Copy link
Contributor

@mariocj89 mariocj89 May 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have an async callable class? If so this won’t work.

Would a check for ‘callable’ work here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand "Asunción callable class" Do you mean something like async class Foo: pass which I think is not supported. Please add an example of the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no async version of callable() builtin like acallable().
asyncio.iscoroutinefunction() works for functions only

Copy link
Contributor

@mariocj89 mariocj89 May 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 note that the following will probably produce a standard mock rather than an async mock:

class A:
   async def __call__(self):
       await asyncio.sleep(1)

a = A()
# Creating a spec for a will not quite likely not create the right kind of mock, as a does not have __code__.

We can cover this in a different PR if possible I guess.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it produces a MagicMock but there could be a class with a mix of sync and async dunder methods too so not sure of the behavior and how to detect classes like this. I would like to cover it as part of different PR since this contain fixes for create_autospec. Thanks for catching this.

from unittest.mock import create_autospec

class A:

    async def __call__(self):
        await asyncio.sleep(1)

a = create_autospec(A)
b = a()
print(a)
print(b())
<MagicMock spec='A' id='4410501232'>
<MagicMock name='mock()()' id='4416843600'>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally fair indeed.

'assert_any_await',
'assert_has_awaits',
'assert_not_awaited'):
def f(attribute, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a different name than arte for this function. It makes it harder to read.
Or even better, take this function out of the loop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we do this over directly assigning the result of getattr here? Is it because the mock might still not be configured? Just confirming.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a different name than arte for this function. It makes it harder to read. Or even better, take this function out of the loop.

I moved it out of the loop and used a different variable name. I have added a comment. Feel free to suggest if it can be rephrased better.

Why do we do this over directly assigning the result of getattr here? Is it because the mock might still not be configured? Just confirming.

Yes, this is similar to a synchronous function being specced where a new function object is returned that needs to be configured like in _setup_func where synchronous mock helpers are attached to function object and when called delegate to mock's helper. The original async setup code was trying to do the same except defining a named function for every helper it uses a wrapper function and uses getattr to get the corresponding method as it is being called.

def assert_called_with(*args, **kwargs):
    return mock.assert_called_with(*args, **kwargs)
funcopy.assert_called_with = assert_called_with

# Could be rewritten as in this PR
def wrapper(attribute, *args, **kwargs):
    return getattr(mock, attr)(*args, **kwargs)
setattr(funcopy, 'assert_called_with', partial(wrapper, 'assert_called_with'))

@@ -0,0 +1,2 @@
Handle late binding and attribute access in AsyncMock setup for
autospeccing. Document newly implemented async methods in MagicMock.
Copy link
Contributor

@mariocj89 mariocj89 May 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: If you want to make this a link you can convert the AsyncMock and MagicMock to
:class:unittest.mock.AsyncMock and :class:unittest.mock.MagicMock

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks.

@1st1
Copy link
Member

1st1 commented May 27, 2019

@tirkarthi Do you want me or Andrew to merge this?

@tirkarthi
Copy link
Member Author

@1st1 Yes, this can be merged and all the items listed in the issue are complete. Thanks for the review everyone.

@1st1 1st1 merged commit ff6b2e6 into python:master May 27, 2019
@1st1
Copy link
Member

1st1 commented May 27, 2019

Thank you, @tirkarthi!

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
…13574)

Handle late binding and attribute access in unittest.mock.AsyncMock
setup for autospeccing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants