-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
[WIP] bpo-17013: Implement WaitableMock to create Mock objects that can wait until called #12818
Changes from 7 commits
826a69e
aa73fbc
0f0482e
f51f5b7
a941ca1
3031310
c67715b
5b59f34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,16 +18,19 @@ | |
'NonCallableMagicMock', | ||
'mock_open', | ||
'PropertyMock', | ||
'WaitableMock', | ||
'seal', | ||
) | ||
|
||
|
||
__version__ = '1.0' | ||
|
||
|
||
from collections import defaultdict | ||
import inspect | ||
import pprint | ||
import sys | ||
import threading | ||
import builtins | ||
from types import ModuleType | ||
from unittest.util import safe_repr | ||
|
@@ -2482,6 +2485,52 @@ def __set__(self, obj, val): | |
self(val) | ||
|
||
|
||
class WaitableMock(MagicMock): | ||
""" | ||
A mock that can be used to wait until it was called. | ||
|
||
`event_class` - Class to be used to create event object. | ||
Defaults to Threading.Event and can take values like multiprocessing.Event. | ||
""" | ||
|
||
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove default timeout. There is no such "good default" timeout. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know. 1.0 seconds seems like a good timeout to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. It also prolongs the test suite as more tests are added. @mariocj89 would be creating There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
"""Wait until the mock object is called. | ||
|
||
`timeout` - time to wait for in seconds. Defaults to 1. | ||
""" | ||
return self._event.wait(timeout=timeout) | ||
|
||
def wait_until_called_with(self, *args, timeout=1.0): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following the conversation of how to have a configurable timeout and still allow for I don't really like this idea, but what do you think about: I don't like that is inconsistent with Alternative: Use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The more I think it the more I am convinced 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 Thoughts @cjw296 @vstinner @voidspace @pablogsal ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Another thing is that currently kwargs are not supported and I use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Agree on they should not have a number, what about timeout defaults that are
Indeed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :)
Sounds reasonable to me as it goes along with other wait APIs.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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: 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't work since 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) |
||
"""Wait until the mock object is called with given args. | ||
|
||
`timeout` - time to wait for in seconds. Defaults to 1. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I would wait if someone has a less conflicting name or perhaps receive |
||
""" | ||
if args not in self._expected_calls: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You dont need this anymore thanks to the default dict, these 3 lines are identical to just |
||
event = self._event_class() | ||
self._expected_calls[args] = event | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just event = self._expected_calls[args]? It would allow to remove self._event_class attribute, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, initial code was written without |
||
else: | ||
event = self._expected_calls[args] | ||
|
||
return event.wait(timeout=timeout) | ||
|
||
|
||
def seal(mock): | ||
"""Disable the automatic generation of child mocks. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,144 @@ | ||
import threading | ||
import time | ||
import unittest | ||
|
||
from test.support import start_threads | ||
from unittest.mock import patch, WaitableMock, call | ||
|
||
|
||
class Something: | ||
|
||
def method_1(self): | ||
pass | ||
|
||
def method_2(self): | ||
pass | ||
|
||
|
||
class TestWaitableMock(unittest.TestCase): | ||
|
||
|
||
def _call_after_delay(self, func, *args, delay): | ||
time.sleep(delay) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
func(*args) | ||
|
||
|
||
def _create_thread(self, func, *args, **kwargs): | ||
thread = threading.Thread(target=self._call_after_delay, | ||
args=(func,) + args, kwargs=kwargs) | ||
return thread | ||
|
||
|
||
def test_instance_check(self): | ||
waitable_mock = WaitableMock(event_class=threading.Event) | ||
|
||
with patch(f'{__name__}.Something', waitable_mock): | ||
something = Something() | ||
|
||
self.assertIsInstance(something.method_1, WaitableMock) | ||
self.assertIsInstance( | ||
something.method_1().method_2(), WaitableMock) | ||
|
||
|
||
def test_side_effect(self): | ||
waitable_mock = WaitableMock(event_class=threading.Event) | ||
|
||
with patch(f'{__name__}.Something', waitable_mock): | ||
something = Something() | ||
something.method_1.side_effect = [1] | ||
|
||
self.assertEqual(something.method_1(), 1) | ||
|
||
|
||
def test_spec(self): | ||
waitable_mock = WaitableMock( | ||
event_class=threading.Event, spec=Something) | ||
|
||
with patch(f'{__name__}.Something', waitable_mock) as m: | ||
something = m() | ||
|
||
self.assertIsInstance(something.method_1, WaitableMock) | ||
self.assertIsInstance( | ||
something.method_1().method_2(), WaitableMock) | ||
|
||
with self.assertRaises(AttributeError): | ||
m.test | ||
|
||
|
||
def test_wait_until_called(self): | ||
waitable_mock = WaitableMock(event_class=threading.Event) | ||
|
||
with patch(f'{__name__}.Something', waitable_mock): | ||
something = Something() | ||
thread = self._create_thread(something.method_1, delay=0.5) | ||
|
||
with start_threads([thread]): | ||
something.method_1.wait_until_called() | ||
something.method_1.assert_called_once() | ||
|
||
|
||
def test_wait_until_called_magic_method(self): | ||
waitable_mock = WaitableMock(event_class=threading.Event) | ||
|
||
with patch(f'{__name__}.Something', waitable_mock): | ||
something = Something() | ||
thread = self._create_thread(something.method_1.__str__, delay=0.5) | ||
|
||
with start_threads([thread]): | ||
something.method_1.__str__.wait_until_called() | ||
something.method_1.__str__.assert_called_once() | ||
|
||
|
||
def test_wait_until_called_timeout(self): | ||
waitable_mock = WaitableMock(event_class=threading.Event) | ||
|
||
with patch(f'{__name__}.Something', waitable_mock): | ||
something = Something() | ||
thread = self._create_thread(something.method_1, delay=0.5) | ||
|
||
with start_threads([thread]): | ||
something.method_1.wait_until_called(timeout=0.1) | ||
something.method_1.assert_not_called() | ||
|
||
something.method_1.wait_until_called() | ||
something.method_1.assert_called_once() | ||
|
||
|
||
def test_wait_until_called_with(self): | ||
waitable_mock = WaitableMock(event_class=threading.Event) | ||
|
||
with patch(f'{__name__}.Something', waitable_mock): | ||
something = Something() | ||
thread_1 = self._create_thread(something.method_1, 1, delay=0.5) | ||
thread_2 = self._create_thread(something.method_2, 1, delay=0.1) | ||
thread_3 = self._create_thread(something.method_2, 2, delay=0.1) | ||
|
||
with start_threads([thread_1, thread_2, thread_3]): | ||
something.method_1.wait_until_called_with(1, timeout=0.1) | ||
something.method_1.assert_not_called() | ||
|
||
something.method_1.wait_until_called(timeout=2.0) | ||
something.method_1.assert_called_once_with(1) | ||
self.assertEqual(something.method_1.mock_calls, [call(1)]) | ||
something.method_2.assert_has_calls( | ||
[call(1), call(2)], any_order=True) | ||
|
||
|
||
def test_wait_until_called_with_no_argument(self): | ||
waitable_mock = WaitableMock(event_class=threading.Event) | ||
|
||
with patch(f'{__name__}.Something', waitable_mock): | ||
something = Something() | ||
something.method_1(1) | ||
|
||
something.method_1.assert_called_once_with(1) | ||
self.assertFalse( | ||
something.method_1.wait_until_called_with(timeout=0.1)) | ||
|
||
thread_1 = self._create_thread(something.method_1, delay=0.1) | ||
with start_threads([thread_1]): | ||
self.assertTrue(something.method_1.wait_until_called_with()) | ||
|
||
|
||
if __name__ == "__main__": | ||
unittest.main() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Add `WaitableMock` to :mod:`unittest.mock` that can be used to create Mock | ||
objects that can wait until they are called. Patch by Karthikeyan | ||
Singaravelan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool that the event creation can be customised with any callable :).