Add the awaited attribute to CoroutineMock. #67

Kentzo · 2017-12-08T01:42:52Z

Refs #64.

Kentzo · 2017-12-27T20:45:28Z

@Martiusweb despite the incompatibility with 3.4 (which I will fix) what do you think?

Do you think it makes sense to always increase await_count even if await failed with exception?

Martiusweb · 2017-12-29T13:39:57Z

test/test_mock.py

@@ -283,6 +283,23 @@ def test_called_CoroutineMock_returns_MagicMock(self):
        mock = asynctest.mock.CoroutineMock()
        self.assertIsInstance(run_coroutine(mock()), asynctest.mock.MagicMock)

+    def test_awaited_CoroutineMock_sets_awaited(self):


Can you add more tests:

when result is awaitable?

when awaiting the result raises an exception?

Martiusweb · 2017-12-29T13:40:22Z

Yes, this is something that should be implemented.
We can add the method assert_awaited(), but it can be done in an other PR.
Do you have examples of cases where an Event is useful for awaited (rather than a simple boolean)?
It can be a good idea but it can be confusing knowning that unittest.mock.Mock.called is a boolean: for instance, someone will probably do something like:

self.assertTrue(coro_mock.awaited)

which will always be ok. I think it should be renamed in awaited_event (or both attributes can be provided, awaited as a boolean, awaited_event as an Event object).

About the exception: the way you implemented it is right. Since you check that the returned result is awaitable, the exception will be raised as the result of the awaited call, not because the object is not awaitable. Thus, I think that we can safely say that the CoroutineMock has been awaited even if an exception is raised.

Kentzo · 2017-12-29T22:14:06Z

I cannot share the entire project, but the use case is for tests whose behavior require to do something at very specific steps of execution flow.

E.g. when a test involves 2 coroutines (client and server) and you want to send some data after server authorizes client. In that test one of server's methods is wrapped with a mock and then test awaits that wrapped mock to be awaited. That is a signal that test can proceed.

It can be a good idea but it can be confusing knowning that unittest.mock.Mock.called is a boolean

What if we replace asyncio.Event with a custom subclass:

class Event(asyncio.Event):
    def __bool__(self):
        return self.is_set()

I think it should be renamed in awaited_event

The reason I did it this way was to make it closer to called / call_count.

Martiusweb · 2018-01-05T15:16:07Z

I recently merged autospec support but it seems that I forgot a few things (handling the mock attributes/delegated properties correctly).
In fact, I didn't use unittest.mock._delegating_property() for is_coroutine and a few other private properties of the mocks, but I should have.

edit: I'll work on this next to unblock this PR.

Kentzo · 2018-01-05T21:28:05Z

Do you know how to address it? I was going to take a look at this next.

In fact, I didn't use unittest.mock._delegating_property() for is_coroutine and a few other private properties of the mocks, but I should have.

I don't think this is necessary: it seems to be useful only for "public" attributes.

Martiusweb · 2018-01-07T15:43:38Z

Your last commit is the right way to fix it. I thought it was caused by how _mock_add_spec is overridden, but I forgot that adding methods/public attributes requires adding it to create_autospec().

I'll add a bit of documentation and tests for the autospec case and merge it.

Martiusweb · 2018-01-07T20:41:47Z

Since I don't have the right to edit your PR, I pushed my changes in the branch GreatFruitOmsk-coroutine-wait:
I rebased master (I fixed the documentation generation which was broken since it used values from setup.py that we moved in setup.cfg), and added 3 commits.

80b799f and 85e454f are cosmetic/doc changes
92bb286 implements tests for the methods with create_autospec and fix them.

Can you review them so we can merge it all? It will be part of the 0.12 release, that I will release once I'm ready to merge #73 and #74.

Kentzo · 2018-01-09T01:36:52Z

I think there is no reason to reference "old" yield from syntax
Perhaps the awaited Event-like object should be extended with the wait_next method: important if test wants to distinguish 1st, 2nd etc awaits

Another thing I just realized is that run_coroutine creates a new loop, while asyncio.Event already belongs to a default one.

Refs Martiusweb#64.

Refs Martiusweb#64

The awaited can be used for boolean comparison. Refs Martiusweb#64

Refs Martiusweb#64

Martiusweb

Your porposal is a good idea, but I think that there are things that need to be changed.

Martiusweb · 2018-01-09T09:44:50Z

asynctest/mock.py

+        return (yield from self.wait_for(predicate))
+
+    @asyncio.coroutine
+    def wait_for(self, predicate):


_AwaitEvent.wait_for() and AwaitEvent.notify() should be private (prefixed with "")

I think github's parser ate the prefix, what did you mean _?

I wanted to keep wait_for public for a user to introduce more complex conditions (e.g. mock is awaited after being called with certain arguments).

Interresting use case. I didn't think of something like this.

I think that we can add this example in the documentation, and tell that the predicate is only checked when the mock is awaited. I don't want users to have the feeling that it can magically awake after a random event.

In fact, I was thinking about this:

def predicate(mock): return mock.await_count > 0 and m.call_args == (("foo", ), {}) t1 = asyncio.ensure_future(mock("foo")) t2 = mock("bar") # not yet scheduled mock.awaited.wait_for(predicate) # will hang forever: mock.call_args is now based on the last call.

The problem is that the predicate is based on the mock (ie coroutine function), not the coroutine that will be awaited itself.

I'm affraid that this will be confusing as it's not possible to wait_for() a given instance of the coroutine function to be awaited. Maybe we should keep this private and let users write their own logic when they want to do complex things (one can use the wraps argument to customize th behavior of the mock).

You should use call_args_list.

This won’t be 100% accurate (you can await in different order) but could be sufficient for some tests.

In other hand for those tests skip would be sufficient as well. Perhaps we should add await_args / await_args_list?

I think that the use case for checking which exact call has been awaited makes sense, but I'm not conviced with generic/public wait_for() method yet.

I think that we should merge this PR with a private _wait_for() as it's already very useful like this. Then we can work on a new PR to implement wait_call() and wait_next_call() or add optional parameters matching in wait() and wait_next().

When I was working on that, ability to await a specific call was only one of use cases I thought of. Another was an ability to await an external consition like setting mock’s attribute to a specific value. Or more general: any modification of mock’s state related to await. I also thought of ability to await external conditions, but couldn’t think of a practical example.

I would opt for having wait_for public as it seems in line with the naming of async primitives. People who would confuse it would probably have problems with wait and wait_next as well. This an advanced method, but it is possible to mark it as such with docs.

What do you say if we keep it public via naming, but exclude from the documentation for now?

Please check GreatFruitOmsk@3839b15, it introduces an independent list of "await args" and among other addresses the issue in your example for wait_for.

Martiusweb · 2018-01-09T09:53:51Z

asynctest/mock.py

+        Wait for await to be called at least ``skip`` number of times.
+        """
+        def predicate(mock):
+            return mock.await_count > skip


The default value of skip should be 1 to be consistent with the meaning of the parameter (and I'd call it "min_await_count" which is more meaningful).

mock = CoroutineMock() await mock.wait(skip=0) # should not block according to doc because it has been called /at least/ 0 times await mock.wait(skip=1) # should block until awaited once

Thus, the condition must be mock.await_count >= min_await_count (greater or equal is consistent with "at least").

There is inconsistency indeed, but I like the skip argument as it appears more to the point: you wait for a call skipping a given number of other calls. Perhaps I could rephrase the doc better.

Yes, it sounds consistent like this:

mock.awaited.wait() # wait the first await mock.awaited.wait(skip=1) # don't resume for the first await

cool :)

Martiusweb · 2018-01-09T09:53:55Z

asynctest/mock.py

+        await_count = self._mock.await_count
+
+        def predicate(mock):
+            return mock.await_count > await_count + skip


Same as for wait(), to be consistent I'd call the argument min_next_wait_count, set 1 as default value, and use >= in the predicate.

I find the skip name more to the point: wait for await skipping # awaits.

Please take a look at the rephrased docstrings. If they are still hard to comprehend, I'll change the name.

Martiusweb · 2018-01-09T09:57:19Z

asynctest/mock.py

@@ -462,16 +510,16 @@ def proxy():
                    try:
                        return (yield from result)
                    finally:
+                        yield from _mock_self.awaited.notify()


calling notify() before incrementing await_count is incorrect with this implementation or the changes I suggest:

mock = CoroutineMock() # await_count is 0 t = ensure_future(mock()) await mock.awaited.wait() # concurrently, notify is called, but await_count is 0 when the , thus the previous condition is not met, the value is incremented, but never notified again.

notify() must be called after the counter has been incremented.

Martiusweb · 2018-01-09T09:57:29Z

asynctest/mock.py

            else:
                @asyncio.coroutine
                def proxy():
                    try:
                        return result
                    finally:
+                        yield from _mock_self.awaited.notify()


same as above.

Martiusweb · 2018-01-09T10:08:18Z

test/test_mock.py

        self.assertEqual(1, mock.a_coroutine.await_count)

+    def test_awaited_wait(self):
+        loop = asyncio.new_event_loop()
+        with replace_loop(loop):


I don't think it is worth changing run_coroutine, which is just a very small helper.
I'd simply use the normal APIs of asyncio and unittest:

loop = asyncio.new_event_loop() self.addCleanup(loop.close) mock = asynctest.mock.CoroutineMock() t = asyncio.ensure_future(mock.awaited.wait()) loop.run_until_complete(mock()) (...)

That's not enough. The new loop is also need to be set and unset (like run_coroutine does) before mock is created, otherwise asyncio.Condition will capture the current loop, not the loop that is running it.

Don't you think this utility function might be convenient as more tests are added? Perhaps it can be further extended (by another PR) into function shared between asynctest and utils?

You're right. We can keep it that way for now, or move these tests in a class extending asynctest.TestCase, since one of its purpose is to handle the loop for each test case.

Allow to skip a given number of awaits and wait for the next await. Refs Martiusweb#64

When condition is created it automatically captures current event loop. Since user should be able to create mock before setting the event loop, creation of the Condition should be delayed before it's needed: - One of wait methods is called - CoroutineMock result is being awaited Tests are simplified to reuse asynctest.TestCase. Addition of replace_loop is reverted being unnecessary. Refs Martiusweb#64

Martiusweb · 2018-01-23T07:37:55Z

Hi,

Sorry for the delay.
I'll do a few cosmetic changes (typos, etc) and merge it. I'll also add a loop argument to CoroutineMock (in an other PR) to be able to control which loop is used in _AwaitedEvent.

I was thinking of something else:
currently a user is notified once the coroutine that is awaited returned, but doesn't it make more sense to notify that the coroutine has been awaited as soon as it starts to be executed?

Basically, it would be something like this:

@asyncio.coroutine
def proxy():
    try:
        yield from (asyncio.sleep(0))
    finally:
        _mock_self.await_count += 1
        yield from _mock_self.awaited._notify()

    return (yield from result)

Kentzo · 2018-01-23T08:08:16Z

Please take a look at the commit I linked. It is not part of this PR, but I’d like to include it.

I was thinking of something else

In that case you likely don’t care about executing the coroutine and plain side_effect would do.

Martiusweb · 2018-01-24T09:14:23Z

I added a bunch of comments, but this is very cool, let's add this commit in the PR!

I changed a few cosmetic things and rebased the PR on top of master (I pushed a change to fix something in python 3.7).

Once the PR is updated with the changes of https://github.com/Martiusweb/asynctest/tree/GreatFruitOmsk-coroutine-wait and the commit for await args is ready, we can merge.

Thank you very much!

Martiusweb · 2018-01-24T10:04:57Z

There still work to do to fix Python 3.7 with PYTHONASYNCIODEBUG=1, I'll ignore this for this PR and work on it later.

It's a bug in the nightly:
https://bugs.python.org/issue32636

Martiusweb · 2018-01-30T21:13:06Z

Finally done!

Thank you very much for this awesome contribution and your patience.

Kentzo · 2018-01-31T06:53:03Z

Just pushed a follow up ;)

Please let me know if anything needs to be changed for other PRs I opened.

Kentzo changed the title ~~Add the awaited attributed to CoroutineMock.~~ Add the awaited attribute to CoroutineMock. Dec 8, 2017

Kentzo force-pushed the coroutine-wait branch 2 times, most recently from 312a6a8 to 4321ab8 Compare December 12, 2017 07:08

Martiusweb reviewed Dec 29, 2017

View reviewed changes

Kentzo force-pushed the coroutine-wait branch from 483a23c to 852531e Compare January 5, 2018 02:08

Kentzo force-pushed the coroutine-wait branch from 852531e to ee988e4 Compare January 5, 2018 22:23

Kentzo and others added 7 commits January 8, 2018 17:47

Add the awaited and await_count attributes to CoroutineMock.

e6d0133

Refs Martiusweb#64.

The only awaitable in 3.4 was a coroutine.

988c757

Refs Martiusweb#64

Add assert_awaited / assert_not_awaited

9498346

The awaited can be used for boolean comparison. Refs Martiusweb#64

Add awaited and related attributes to an autospec function.

d843452

Refs Martiusweb#64

Move isawaitable to the top of the module

ba2b471

Doc of CoroutineMock.awaited

19f702d

Test and fix the support of create_autospec

60aa5a9

Kentzo force-pushed the coroutine-wait branch 2 times, most recently from b785776 to 93852fc Compare January 9, 2018 03:26

Martiusweb requested changes Jan 9, 2018

View reviewed changes

Better control over awaited

e9c157c

Allow to skip a given number of awaits and wait for the next await. Refs Martiusweb#64

Kentzo force-pushed the coroutine-wait branch from 93852fc to e9c157c Compare January 9, 2018 20:20

Martiusweb approved these changes Jan 23, 2018

View reviewed changes

Kentzo and others added 2 commits January 30, 2018 22:04

Fix various warnings found by PyCharm analyser.

a6e7734

Conditional import of _format_callback

eae5b84

Martiusweb merged commit 06e4470 into Martiusweb:master Jan 30, 2018

Kentzo mentioned this pull request Nov 12, 2019

bpo-17013: Extend Mock.called to allow waiting for calls python/cpython#17133

Closed

Kentzo mentioned this pull request Jun 9, 2020

gh-61215: Add Mock.call_event to allow waiting for calls python/cpython#20759

Closed

Add the awaited attribute to CoroutineMock. #67

Add the awaited attribute to CoroutineMock. #67

Conversation

Kentzo commented Dec 8, 2017

Kentzo commented Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Martiusweb commented Dec 29, 2017

Kentzo commented Dec 29, 2017

Martiusweb commented Jan 5, 2018 • edited Loading

Kentzo commented Jan 5, 2018 • edited Loading

Martiusweb commented Jan 7, 2018

Martiusweb commented Jan 7, 2018

Kentzo commented Jan 9, 2018 • edited Loading

Martiusweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kentzo Jan 9, 2018 • 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

Kentzo Jan 10, 2018 • 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

Kentzo Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martiusweb commented Jan 23, 2018

Kentzo commented Jan 23, 2018

Martiusweb commented Jan 24, 2018

Martiusweb commented Jan 24, 2018 • edited Loading

Martiusweb commented Jan 30, 2018

Kentzo commented Jan 31, 2018

Kentzo commented Dec 27, 2017 •

edited

Loading

Martiusweb commented Jan 5, 2018 •

edited

Loading

Kentzo commented Jan 5, 2018 •

edited

Loading

Kentzo commented Jan 9, 2018 •

edited

Loading

Kentzo Jan 9, 2018 •

edited

Loading

Kentzo Jan 10, 2018 •

edited

Loading

Kentzo Jan 9, 2018 •

edited

Loading

Martiusweb commented Jan 24, 2018 •

edited

Loading