[WIP] bpo-17013: Implement WaitableMock to create Mock objects that can wait until called #12818

tirkarthi · 2019-04-13T15:35:59Z

This is an initial implementation with preliminary docs and tests to see if it's worthy enough of addition.

Some notes :

Currently, it doesn't support waiting for calls with keyword arguments.
It seems the calls to event object are also recorded in mock_calls . I think these should be filtered out or maybe I am doing something wrong.
There is per call event object and per mock object event to track if mock was called with given arguments and to store just mock is called or not. Is there a better way to store this?

https://bugs.python.org/issue17013

wait_until_called and wait_until_called_with are supported. This stores a dictionary with args as key and corresponding event object which is set once there is a call to it. In case of call not present we only want to know if the function was called and hence a per mock event object is also present which is set for no args case.

mariocj89

I think this is going to be really useful!

mariocj89 · 2019-04-13T15:39:21Z

Lib/unittest/mock.py

+    def __init__(self, *args, event_class=threading.Event, **kwargs):
+        _safe_super(WaitableMock, self).__init__(*args, **kwargs)
+        self._event = event_class()
+        self._expected_calls = {}


This might need to be a default dict of the event class the user passed.
Otherwise the following situation might fail:
Some calls ‘wait_untill_called_with’ after te call has been performed (it should not block)

Thanks Mario, I have added a test and used defaultdict which simplifies the code.

mariocj89 · 2019-04-13T15:40:27Z

Lib/unittest/mock.py

+        ret_value = _safe_super(WaitableMock, self)._mock_call(*args, **kwargs)
+
+        for call in self._mock_mock_calls:
+            event = self._expected_calls.get(call.args)


With the default dict proposed above this can be changed by just assuming it is there and calling set on it. No need to check is set, isn’t it?

mariocj89 · 2019-04-13T15:41:34Z

Lib/unittest/mock.py

+
+    def wait_until_called_with(self, *args, timeout=1.0):
+        """Wait until the mock object is called with given args.
+        If args is empty then it waits for the mock object to be called.


I would allow for arts to be empty, which means called with no args. Otherwise users have no way to wait for that.

If they want to wait for any call they can use the other method.

Makes sense. I have removed this line and code to default to self._event during empty args. Previously, calling the function without args would create an empty tuple as key in self._expected_calls and set the event along with self._event. So it was set before too but now as per suggestion instead of using self._event the appropriate event object at self._expected_calls[()] is used .

mariocj89 · 2019-04-13T15:42:22Z

Lib/unittest/mock.py

+        """Wait until the mock object is called with given args.
+        If args is empty then it waits for the mock object to be called.
+
+        `timeout` - time to wait for in seconds. Defaults to 1.


I don’t have a great alternative, but feels a pity if users cannot wait for calls that had a timeout parameter as discussed in the issue.

Yes, I would wait if someone has a less conflicting name or perhaps receive kwargs_dict as a dictionary instead of (**kwargs, timeout) that fixes conflict but the API would be nice to pass keyword arguments as **kwargs itself.

tirkarthi · 2019-04-13T16:39:52Z

Another problem is that inside wait_until_called_with I create event objects and calling self._event_class and so on seems to record the calls to the Mock object too. But this is not a problem with wait_until_called where not such attribute access is done. I guess there is some flaw in my approach of attribute access with self leading to this.

import threading
import time
from unittest.mock import WaitableMock, patch, call

def call_after_sometime(func, *args, delay=1):
    time.sleep(delay)
    func(*args)

def foo(*args):
    pass

def bar(*args):
    pass

with patch('__main__.foo', WaitableMock(event_class=threading.Event)):
    with patch('__main__.bar', WaitableMock(event_class=threading.Event)):
        threading.Thread(target=call_after_sometime, args=(foo, 1), kwargs={'delay': 1}).start()
        threading.Thread(target=call_after_sometime, args=(bar, 2), kwargs={'delay': 1}).start()
        print("bar called with 1 ", bar.wait_until_called_with(2, timeout=2))
        print(bar.mock_calls)
        bar.assert_called_once_with(2)
        print("foo called with 1 ", foo.wait_until_called_with(timeout=2))
        print(foo.mock_calls)

bar called with 1  <WaitableMock name='mock._event_class().is_set()' id='4461405584'>
[call.expected_calls.__contains__((2,)),
 call._event_class(),
 call._event_class().is_set(),
 call._event_class().is_set().__bool__(),
 call._event_class().is_set().__str__()]
Traceback (most recent call last):
  File "../backups/bpo17013_mock_1.py", line 25, in <module>
    bar.assert_called_once_with(2)
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 852, in assert_called_once_with
    raise AssertionError(msg)
AssertionError: Expected 'mock' to be called once. Called 0 times.
Calls: [call.expected_calls.__contains__((2,)),
 call._event_class(),
 call._event_class().is_set(),
 call._event_class().is_set().__bool__(),
 call._event_class().is_set().__str__()].

For foo it would work fine :

foo called with 1  True
[call(1)]

calls to foo return true and correct call objects. Meanwhile calls to bar with wait_until_called_with cause irrelevant calls to be recorded. Is there something I am missing or some setup in configuring the mock that I have missed?

I think there is a flaw that I have added time.sleep in my examples and calls are made misleading me and perhaps the mock object never waits for wait_until_called_with calls and the assertion error is telling me the same? If I add sleep it works fine.

mariocj89 · 2019-04-13T16:43:55Z

Lib/unittest/mock.py

+    def __init__(self, *args, event_class=threading.Event, **kwargs):
+        _safe_super(WaitableMock, self).__init__(*args, **kwargs)
+        self._event = event_class()
+        self._expected_calls = defaultdict(lambda: event_class())


You can probably simplify this to be just ‘defaultdict(event_class)’. Without the lambda.

Removed. Thanks 👍

mariocj89 · 2019-04-13T16:44:59Z

Lib/unittest/mock.py

+    """
+    A mock that can be used to wait until it was called.
+
+    `event_class` - Class to be used to create event object.


Really cool that the event creation can be customised with any callable :).

tirkarthi · 2019-04-13T17:48:52Z

Regarding my above comment on irrelevant calls. I got bitten by mock's flexibility :)

I was using self.expected_calls in the for loop that should be self._expected_class and self._event_class without storing the event class thus for attributes not found mock gave me set of WaitableMock. I have fixed them and now mock_calls only has the actual calls made. I added an assertion for the same too.

…tion

mariocj89 · 2019-04-14T09:42:08Z

Lib/unittest/mock.py

+
+        `timeout` - time to wait for in seconds. Defaults to 1.
+        """
+        if args not in self._expected_calls:


You dont need this anymore thanks to the default dict, these 3 lines are identical to just event = self._expected_calls[args].

mariocj89 · 2019-04-14T09:46:39Z

Lib/unittest/mock.py

+        """
+        return self._event.wait(timeout=timeout)
+
+    def wait_until_called_with(self, *args, timeout=1.0):


Following the conversation of how to have a configurable timeout and still allow for timeout to be a keyword arg to be matched:

I don't really like this idea, but what do you think about:
def wait_until_called_with(self, *args, **kwargs, mock_wait_timeout=1.0):?

I don't like that is inconsistent with wait_until_called (maybe change it as well?) and might seem unnecessarily verbose, but we need to allow for timeout to be used as one of the keywords. The main issue with inconsistency is that users might still pass timeout here which might be interpreted as an argument to match.

Alternative: Use timeout at the constructor. This might be the simplest for users. Even if it restricts the Mock to have the same timeout for all calls validation, I doubt that is often a problem and I think it is the simplest to use.

The more I think it the more I am convinced timeout in the init is probably the best approach:

m = WaitableMock(timeout=5) m("a positional arg", timeout="kwarg") # This will match m.wait_until_called_with( "a positional arg", timeout="kwarg" )

Removes confusion from the users (compared to different arguments) and allows timeout to be used as a keyword arg in the function to be matched.

Thoughts @cjw296 @vstinner @voidspace @pablogsal ?

I am fine with moving it to constructor if the trade-off is worth it. Initially I liked the idea of different arguments having different timeout but if it conflicts with kwargs and different per call timeouts are not a frequent use case then we can change it. For custom timeout per call users might try to set wait_mock.timeout for the object as needed.

Another thing is that currently kwargs are not supported and I use args as the key to event object for initial PoC. Continuing to use it feels like a hack so if there is a way better way to store it then it will be better.

I dislike having to pass the timeout to the mock constructor, but to me, it's perfectly legit to pass a timeout to the mock call. Like mocking threading.Lock.acquire for example.

IMHO timeout must not have a default, so what about putting the timeout as the first position? I heard that a PEP 570 has been merged and it allows to write "def wait_until_called_with(self, timeout, /, *args, **kwargs):" which accepts timeout in kwargs ;-)

You must add **kwargs to mimick Mock.assert_called_with().

Maybe just skip timeout in this case and ask the user to pass timeout to constructor and use it here? But that would mean that wait_until_called can custom timeout meanwhile this couldn't so remove that too. I don't have a good answer to where the trade-off should be made on passing it per call or in the constructor which I hope was the point @mariocj89 was also making to just make this constructor field despite losing some flexibility.

Yeah, I also really don't like the timeout in the constructor, but could not come with a better idea. As much as PEP570 is great, having timeout it as positional only would make the signature not symmetrical with the one the Mock is mocking as @tirkarthi explained here. It might be an acceptable tradeoff though.
Another option would be to request to pass a "call-like" object, though it would make it harder to use.

IMHO timeout must not have a default

Agree on they should not have a number, what about timeout defaults that are None and you just assume to wait forever? Just as Event.wait or thread.join.

You must add **kwargs to mimick Mock.assert_called_with().

Indeed.

If needed then we could resort to discuss.python.org or python-dev to see if someone else has a better idea. But I find this to be one that may have more back and forth to give broader opinions before coming at a reasonable API based on how one views flexibility in using the API :)

Agree on they should not have a number, what about timeout defaults that are None and you just assume to wait forever? Just as Event.wait or thread.join.

Sounds reasonable to me as it goes along with other wait APIs.

You must add **kwargs to mimick Mock.assert_called_with().

Is there a data structure to use to store both *args and **kwargs that is hashable to map the event objects. kwargs is a dict making it unhashable for storage.

Is there a data structure to use to store both *args and **kwargs that is hashable to map the event objects. kwargs is a dict making it unhashable for storage.

Good point, you might need to find a different solution here. Also, does this work if one of the arguments passed is a list? Can you add a test doing: assert_called_with([],[])?

A non-ideal option might be to have to use something like two lists and map them index based, but this would add a decent amount of complexity (as you will also need to check for uniqueness. Let's see if someone else in this thread has a better idea.

It doesn't work since [] is not hashable. call objects are comparable and support contains check. Hence one approach based on this would be to have self._call_index and self._event_index that is a list but pretends like a defaultdict implementation mapping index of _call to event object.

def _get_or_create_event(self, _call): try: # Try to see if it's already in the list and get corresponding event position index = self._call_index.index(_call) event = self._event_index[index] except ValueError: # If not present create an event object and append it to an index event = self._event_class() self._call_index.append(_call) self._event_index.append(event) return event def _mock_call(self, *args, **kwargs): ret_value = _safe_super(WaitableMock, self)._mock_call(*args, **kwargs) for _call in self._mock_mock_calls: event = self._get_or_create_event(_call) event.set() self._event.set() return ret_value def wait_until_called_with(self, *args, timeout, **kwargs): _call = call(*args, **kwargs) event = self._get_or_create_event(_call) return event.is_set() or event.wait(timeout=timeout)

mariocj89 · 2019-04-14T10:01:04Z

Lib/unittest/test/testmock/testwaitablemock.py

+
+
+    def _call_after_delay(self, func, *args, delay):
+        time.sleep(delay)


This might be painful but it might be better to use events to wait for things to happen rather than relying in sleep. Otherwise, this might be adding considerable time to the test suit.

I'd say wait for a core dev to comment about it, it might be fine though.

I have kept the delays 0.1-0.5s to simulate latency of the call. Currently, it runs on 2.5 seconds on my local machine and might increase once I add more tests. I am new to using event objects so if there is a better way to simulate latency then I can refactor my tests.

vstinner · 2019-04-14T10:11:06Z

Lib/unittest/mock.py

+        """
+        if args not in self._expected_calls:
+            event = self._event_class()
+            self._expected_calls[args] = event


Why not just event = self._expected_calls[args]? It would allow to remove self._event_class attribute, no?

Yes, initial code was written without defaultdict and I missed updating this after defaultdict was used as suggested by @mariocj89 . Thanks this removes boilerplate.

vstinner · 2019-04-15T09:17:58Z

Lib/unittest/mock.py

+
+        return ret_value
+
+    def wait_until_called(self, timeout=1.0):


Please remove default timeout. There is no such "good default" timeout.

I don't know. 1.0 seconds seems like a good timeout to me.

Agreed. It also prolongs the test suite as more tests are added. @mariocj89 would be creating EventMock superseding the PR. Would be happy to know his approach on asserting events and timeouts.

IMHO it's a bad idea to hardcode a default timeout. We cannot make assumption on the hardware performance. Please have a look at https://bugs.python.org/issue36369 : Python is slow on a Raspberry Pi Zero W. Does it surprise anyone? Why would Python expect that it's always run on fast x86-64?

I suggest to either block by default (make the timeout optional) or to require a timeout value.

In the worst case, please make at least the default configurable.

I spent like 5 years to fix the Python test suite because too many tests used hardcoded timeouts which fit well for a fast desktop computer, but not for our slowest buildbot workers. Get a test failure only because the timeout is too short is annoying.

vstinner · 2019-04-15T09:21:44Z

Lib/unittest/mock.py

+    def __init__(self, *args, event_class=threading.Event, **kwargs):
+        _safe_super(WaitableMock, self).__init__(*args, **kwargs)
+        self._event_class = event_class
+        self._event = event_class()


The usage self._event vs self._expected_calls which contains events is non obvious. Can add a comment to explain where each event is used and how.

vstinner · 2019-04-15T09:28:32Z

Lib/unittest/mock.py

+        """
+        return self._event.wait(timeout=timeout)
+
+    def wait_until_called_with(self, *args, timeout=1.0):


I dislike having to pass the timeout to the mock constructor, but to me, it's perfectly legit to pass a timeout to the mock call. Like mocking threading.Lock.acquire for example.

IMHO timeout must not have a default, so what about putting the timeout as the first position? I heard that a PEP 570 has been merged and it allows to write "def wait_until_called_with(self, timeout, /, *args, **kwargs):" which accepts timeout in kwargs ;-)

You must add **kwargs to mimick Mock.assert_called_with().

bedevere-bot · 2019-04-15T09:28:49Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

tirkarthi · 2019-04-15T09:48:06Z

IMHO timeout must not have a default, so what about putting the timeout as the first position? I heard that a PEP 570 has been merged and it allows to write "def wait_until_called_with(self, timeout, /, *args, **kwargs):" which accepts timeout in kwargs ;-)

@vstinner This slightly makes the API confusing to me . When I need to check if the mock is called with ('a', 'b') under timeout of 1.0 then I would do mock.wait_until_called_with(1.0, 'a', 'b') . Here 1.0 is timeout passed as positional only argument but it reads like wait until mock is called with (1.0, 'a', 'b') . This should be modeled on API like mock.assert_called_with with *args and **kwargs along with timeout. If the user has a timeout parameter and also wants custom timeout this can lead to more confusion like something.method_1.wait_until_called_with(1, timeout=0.1).

Maybe just skip timeout in this case and ask the user to pass timeout to constructor and use it here? But that would mean that wait_until_called can custom timeout meanwhile this couldn't so remove that too. I don't have a good answer to where the trade-off should be made on passing it per call or in the constructor which I hope was the point @mariocj89 was also making to just make this constructor field despite losing some flexibility.

You must add **kwargs to mimick Mock.assert_called_with().

Yes, currently I have used args as the dictionary key there should be a way to have a unique key combination of args and kwargs to store the corresponding event object.

mariocj89 · 2019-09-13T12:07:51Z

On the PR I am going to open (today ideally), I have block as default as many other API do. I am having some issues getting this to work for multiprocessing. I’ll open it WIP and we can iterate.

…

On Fri, 13 Sep 2019 at 12:55, Victor Stinner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Lib/unittest/mock.py <#12818 (comment)>: > + self._event_class = event_class + self._event = event_class() + self._expected_calls = defaultdict(event_class) + + def _mock_call(self, *args, **kwargs): + ret_value = _safe_super(WaitableMock, self)._mock_call(*args, **kwargs) + + for call in self._mock_mock_calls: + event = self._expected_calls[call.args] + event.set() + + self._event.set() + + return ret_value + + def wait_until_called(self, timeout=1.0): IMHO it's a bad idea to hardcode a default timeout. We cannot make assumption on the hardware performance. Please have a look at https://bugs.python.org/issue36369 : Python is slow on a Raspberry Pi Zero W. Does it surprise anyone? Why would Python expect that it's always run on fast x86-64? I suggest to either block by default (make the timeout optional) or to require a timeout value. In the worst case, please make at least the default configurable. I spent like 5 years to fix the Python test suite because too many tests used hardcoded timeouts which fit well for a fast desktop computer, but not for our slowest buildbot workers. Get a test failure only because the timeout is too short is annoying. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12818?email_source=notifications&email_token=AA4JV43VRS5SYCEXU3GF3FTQJN5SVA5CNFSM4HFYN2QKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEVA66Q#discussion_r324157816>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4JV47OUXKX2ZPJT5U5KQTQJN5SVANCNFSM4HFYN2QA> .

tirkarthi · 2019-10-19T17:12:49Z

Closing in favor of #16094 by Mario. Thanks all for the suggestions on the API.

tirkarthi added 3 commits April 13, 2019 13:01

Add tests and docs

aa73fbc

Add NEWS entry

0f0482e

the-knights-who-say-ni added the CLA signed label Apr 13, 2019

bedevere-bot added the awaiting review label Apr 13, 2019

mariocj89 suggested changes Apr 13, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Apr 13, 2019

Use defaultdict for event object creation

f51f5b7

mariocj89 reviewed Apr 13, 2019

View reviewed changes

Use proper attributes and assert for mock_calls

a941ca1

tirkarthi added 2 commits April 13, 2019 23:19

Remove unused lambda for defaultdict

3031310

Don't use default event for empty args and use helper for thread crea…

c67715b

…tion

mariocj89 reviewed Apr 14, 2019

View reviewed changes

vstinner reviewed Apr 14, 2019

View reviewed changes

Allow defaultdict to take care of missing event objects

5b59f34

vstinner requested changes Apr 15, 2019

View reviewed changes

bedevere-bot removed the awaiting core review label Apr 15, 2019

bedevere-bot added the awaiting changes label Apr 15, 2019

tirkarthi changed the title ~~bpo-17013: Implement WaitableMock to create Mock objects that can wait until called~~ [WIP] bpo-17013: Implement WaitableMock to create Mock objects that can wait until called Apr 15, 2019

csabella requested a review from vstinner June 1, 2019 15:17

mariocj89 mentioned this pull request Sep 13, 2019

gh-61215: New mock to wait for multi-threaded events to happen #16094

Merged

tirkarthi closed this Oct 19, 2019

pitrou mentioned this pull request Apr 10, 2022

Allow waiting on a mock #61215

Open



		def _call_after_delay(self, func, *args, delay):
		time.sleep(delay)

[WIP] bpo-17013: Implement WaitableMock to create Mock objects that can wait until called #12818

[WIP] bpo-17013: Implement WaitableMock to create Mock objects that can wait until called #12818

Conversation

tirkarthi commented Apr 13, 2019 • edited by bedevere-bot Loading

mariocj89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirkarthi commented Apr 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirkarthi commented Apr 13, 2019

Choose a reason for hiding this comment

mariocj89 Apr 14, 2019 • edited Loading

Choose a reason for hiding this comment

mariocj89 Apr 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Apr 15, 2019

tirkarthi commented Apr 15, 2019

mariocj89 commented Sep 13, 2019 via email

tirkarthi commented Oct 19, 2019

tirkarthi commented Apr 13, 2019 •

edited by bedevere-bot

Loading

mariocj89 Apr 14, 2019 •

edited

Loading

mariocj89 Apr 14, 2019 •

edited

Loading