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

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

Closed
wants to merge 8 commits into from
49 changes: 49 additions & 0 deletions Lib/unittest/mock.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Copy link
Contributor

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 :).

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()
Copy link
Member

Choose a reason for hiding this comment

The 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):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 EventMock superseding the PR. Would be happy to know his approach on asserting events and timeouts.

Copy link
Member

Choose a reason for hiding this comment

The 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):
Copy link
Contributor

@mariocj89 mariocj89 Apr 14, 2019

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor

@mariocj89 mariocj89 Apr 14, 2019

Choose a reason for hiding this comment

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

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 ?

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 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.

Copy link
Member

Choose a reason for hiding this comment

The 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().

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 :)

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

"""Wait until the mock object is called with given args.

`timeout` - time to wait for in seconds. Defaults to 1.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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, 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.

"""
if args not in self._expected_calls:
Copy link
Contributor

Choose a reason for hiding this comment

The 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._expected_calls[args].

event = self._event_class()
self._expected_calls[args] = event
Copy link
Member

Choose a reason for hiding this comment

The 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?

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, initial code was written without defaultdict and I missed updating this after defaultdict was used as suggested by @mariocj89 . Thanks this removes boilerplate.

else:
event = self._expected_calls[args]

return event.wait(timeout=timeout)


def seal(mock):
"""Disable the automatic generation of child mocks.

Expand Down
144 changes: 144 additions & 0 deletions Lib/unittest/test/testmock/testwaitablemock.py
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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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 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.