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

Add mock_async_callable for asyncio support #60

Closed
wants to merge 3 commits into from

Conversation

jivid
Copy link

@jivid jivid commented Oct 27, 2019

Add a mock_async_callable DSL which works exactly like mock_callable but for async functions.

The decision to not patch mock_callable to support async functions was intentional and made to make it clear when reading the test what kind of code is being mocked. Just as you can't just stick async in front of a function and expect everything to work, you shouldn't rely on mock_callable just working either.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 27, 2019
"Mocking __new__ is not allowed with mock_callable(), please use "
"mock_constructor()."
)
return _MockAsyncCallableDSL(target, method)
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic for __new__ is duplicated, it could be moved to _MockCallableDSL.

@fornellas
Copy link
Contributor

Neat! This seems in a good direction! There are a few things that we need to look over as well:

  • Documentation needs to be updated to explain about async usage (including test framework integration) . Tip: do make docs to build and test locally.
  • mock_callable() supports a lot of different configurations. I don't expect them all to work with the current implementation, we need tests for all of them.

From the top of my mind, I do expect these to "just work":

  • .to_return_value(value)
  • .to_return_values(values_list)
  • .to_raise(exception)
  • .and_assert_called_exactly(times)
  • .and_assert_called_once()
  • .and_assert_called_twice()
  • .and_assert_called_at_least(times)
  • .and_assert_called_at_most(times)
  • .and_assert_called()
  • .and_assert_called_ordered()
  • .and_assert_not_called()

But I have my doubts about these:

  • .for_call(*args, **kwargs)
  • .to_yield_values(values_list)
  • .with_implementation(func)
  • .with_wrapper(func)
  • .to_call_original()

Unfortunately Python has shown to be full of quicks and exception cases on how things work. The current tests, while hard to maintain and confusing (sorry about that), were extremely helpful in getting all corner cases in the past, we'd loose a lot if we did not have the same coverage for async.

Ideally, we'd need to duplicate all sync wyth async equivalents of the tests, but in some cases (eg: iterator, calling original) we'd need more async specific stuff. I'm happy to assist you on that if needed.

@jivid
Copy link
Author

jivid commented Oct 28, 2019

Thanks for the comments @fornellas, I'll update this PR with the requested changes

@jivid
Copy link
Author

jivid commented Oct 28, 2019

@fornellas I'm still working on this, not ready for review yet. Just pushed my changes so far so I don't lose them.

@fornellas
Copy link
Contributor

FYI, #59 should help testing async stuff, I'm still working on it.

@jivid jivid mentioned this pull request Oct 28, 2019
@fornellas fornellas added this to the Async support milestone Oct 29, 2019
@fornellas
Copy link
Contributor

@jivid as we spoke, I'm gonna start working on this issue now so we can deliver it ASAP. I'm closing this PR so we don't clash, but feel free to ping me to join forces if you can spare bandwidth.

Thanks for what we have here already, I have something to grow on top :-D

@fornellas fornellas closed this Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants