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-17013: Extend Mock.called to allow waiting for calls #17133

Closed
wants to merge 1 commit into from

Conversation

Kentzo
Copy link
Contributor

@Kentzo Kentzo commented Nov 12, 2019

This implementation of bpo-17013 is alternative to #16094

Changes are based on my work for asynctest. Specifically on _AwaitEvent that was left out when related code was ported to CPython.

Key features:

  • Gives meaning to the existing Mock.called property, otherwise not much useful
  • Does not require end users to do anything: change is automatically available in every Mock
  • Utilizes existing semantics of python conditionals (both asyncio and threading)

Accepting this change will allow me to port _AwaitEvent therefore giving identical semantics to both wait-for-calls and wait-for-awaits.

I will provide necessary typing annotations for typeshed.

Considerations:

  1. This approach changes type of the Mock.called property from bool to private bool-like _CallEvent. However, the only practical problem I could think of is if someone was checking type of Mock.called (e.g. via isinstance). That does not sound like a plausible problem: after all the property itself is seldom used with exception of conditional expressions where _CallEvent.__bool__ and _CallEvent.__eq__ is sufficient.
  2. It probably makes sense to provide convenience methods like wait_for_call, but I would like to hear the opinion of the reviewers.

CC: @vstinner @tirkarthi @mariocj89

https://bugs.python.org/issue17013

@Kentzo
Copy link
Contributor Author

Kentzo commented Nov 13, 2019

Probably one method that the object should definitely have is wait_for_call(self, call, /, skip=0, timeout=None).

@Kentzo Kentzo force-pushed the bpo-17013 branch 3 times, most recently from 54b7b06 to c740cb8 Compare November 14, 2019 07:26
New methods allow tests to wait for calls executing in other threads.
@mariocj89
Copy link
Contributor

Thanks @Kentzo for putting this through. From conversations with @voidspace, @lisroach, @tirkarthi and in the issue tracker it seems that people wanted to go with having a different mock just for this use case, rather than trying to fold it into the existing one. Similar to what was done with AsyncMock.

Happy to re-evaluate it, but I guess that should be a conversation in the issue tracker.

@cjw296
Copy link
Contributor

cjw296 commented Jan 24, 2020

Closing this PR for now, as it looks like @mariocj89 will be putting in another PR to take over from this.

@cjw296 cjw296 closed this Jan 24, 2020
@Kentzo
Copy link
Contributor Author

Kentzo commented Jan 28, 2020

@cjw296 Hmm, where was that discussion?

@cjw296
Copy link
Contributor

cjw296 commented Jan 28, 2020

https://bugs.python.org/issue17013#msg352211
#16094

Looks like @mariocj89 's PR actually predates this one, I don't have a strong opinion on either, so happy to re-open this PR if you, @mariocj89, @tirkarthi, @voidspace and @lisroach want to discuss which PR to take forward.

@mariocj89
Copy link
Contributor

Hi @Kentzo, this was initially discussed in https://bugs.python.org/issue17013#msg352211 and offlne in the core dev sprints with @voidspace, @lisroach and @tirkarthi when we put forward the other PR. @voidspace preferred to go down the path of having specialized mocks rather than having a mock have all the functionality. Similar to the asyncmock.

@Kentzo
Copy link
Contributor Author

Kentzo commented Jan 29, 2020

At some point I was asked to move the discussion to bugs.python. For the benefit of the future developers I believe it makes sense to point out the benefits of the new "class approach" there as well.

@mariocj89
Copy link
Contributor

At some point I was asked to move the discussion to bugs.python. For the benefit of the future developers I believe it makes sense to point out the benefits of the new "class approach" there as well.

Absolutely, that's why I suggested that. The issue lays on the availability of committer time though. It might be a busy time right now.

@Kentzo
Copy link
Contributor Author

Kentzo commented Apr 27, 2020

Added implementation of the wait_for_call convenience method that can be used like this:

mock.method_1.called.wait_for_call(call(1, a=1), timeout=0.01)

@cjw296
Copy link
Contributor

cjw296 commented Apr 27, 2020

@Kentzo - it's unclear where you've added this?

@Kentzo
Copy link
Contributor Author

Kentzo commented Apr 27, 2020

I guess PR is not updated because it's closed. The corresponding branch in my repo got updated.

@Kentzo
Copy link
Contributor Author

Kentzo commented Jun 7, 2020

Any chance for this PR to get reviewed?

@cjw296
Copy link
Contributor

cjw296 commented Jun 8, 2020

Doesn't look like I can re-open this PR, so please submit a new one if you want anything reviewed. Please comment here with a link to the new one.

@mariocj89 - where did you get to with your approach?

@mariocj89
Copy link
Contributor

There is an open PR that was reviewed but never re-reviewed after proposed changes here: #16094

@Kentzo
Copy link
Contributor Author

Kentzo commented Jun 9, 2020

@cjw296 #20759

@Kentzo
Copy link
Contributor Author

Kentzo commented Oct 21, 2020

Just noticed that I put wrong link above 🤦🏻‍♂️, fixed the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants