Make 'async def' tests work #2583

Strilanc · 2019-11-20T23:32:58Z

Add pytest-asyncio dev dependency
Detect undecorated async def methods (which would otherwise vacuously pass) via conftest.py/pytest_pyfunc_call
Refactor existing async tests into async def test methods
Remove cirq.testing.assert_asyncio_will_have_result
Remove cirq.testing.assert_asyncio_will_raise
Remove cirq.testing.assert_asyncio_still_running
Add cirq.testing.asyncio_not_finishing with forgot-to-await detection

- Add pytest-asyncio dev dependency - Detect undecorated async def methods (which would otherwise vacuously pass) via conftest.py/pytest_pyfunc_call - Refactor existing async tests into async def test methods - Remove cirq.testing.assert_asyncio_will_have_result - Remove cirq.testing.assert_asyncio_will_raise - Remove cirq.testing.assert_asyncio_still_running - Add cirq.testing.asyncio_not_finishing with forgot-to-await detection

maffoo · 2019-11-21T00:09:46Z

cirq/sim/simulator_test.py

@@ -299,7 +300,7 @@ def _run(self, circuit, param_resolver, repetitions):

    q = cirq.LineQubit(0)
    f = MockSimulator().run_async(cirq.Circuit(cirq.measure(q)), repetitions=10)
-    result = cirq.testing.assert_asyncio_will_have_result(f)
+    result = await f


Does the asyncio decorator handle timeouts?

It doesn't, which sucks. I didn't want to add a custom cirq-specific async test decorator if possible. There is pytest-timeout plugin that might potentially work.

maffoo · 2019-11-22T00:45:47Z

cirq/testing/asynchronous.py


+def asyncio_not_finishing(future: Union[Awaitable, asyncio.Future, Coroutine],


I would suggest having this return True if a future does complete, just to avoid the negation in the name (and the double negative if you want to assert that a future finishes). Could call this asyncio_finishes_soon, say, and then users can call it like:

assert await asyncio_finishes_soon(f) assert not await asyncio_finishes_soon(g)

This method is essentially always used in the context of expecting it to return true, so it makes sense to not invert. The name could potentially be changed, but the logic should stay the same.

asyncio_still_running
asyncio_pending
or something like that

Renamed to asyncio_pending.

maffoo · 2019-11-22T00:48:47Z

cirq/testing/asynchronous.py

+    def __bool__(self):
+        raise RuntimeError(
+            'You forgot the "await" in '
+            '"assert await cirq.testing.asyncio_not_finishing(...)".')


maffoo · 2019-11-22T00:51:12Z

cirq/testing/asynchronous_test.py

-    e = asyncio.Future()
-    e.set_exception(ValueError('test fail'))
-    cirq.testing.assert_asyncio_will_raise(e, ValueError, match="test fail")
+    f = cirq.testing.asyncio_not_finishing(f)


nit: use a different variable name for the return value here, to distinguish from the future itself.

maffoo · 2019-11-22T00:52:52Z

cirq/work/collector_test.py

@@ -52,7 +54,7 @@ def on_job_result(self, job, result):
    completion = TestCollector().collect_async(sampler=cirq.Simulator(),
                                               max_total_samples=100,
                                               concurrency=5)
-    cirq.testing.assert_asyncio_will_have_result(completion, None)
+    assert await completion is None


I would suggest avoiding assert await wherever possible:

result = await TestCollector().collect_async(...) assert result is None

As written it's not clear that await binds as assert (await completion) is None.

Acknowledged, but the alternative interpretation results in a runtime error because bool is not awaitable. Rule of thumb is that await binds super tightly. Tighter than x**y, not as tight as x[y] or x.y.

maffoo · 2019-11-22T00:53:48Z

cirq/work/pauli_sum_collector_test.py

@@ -24,18 +27,19 @@ def test_pauli_string_sample_collector():
                               4 * cirq.Z(a) * cirq.Z(b),
                               samples_per_term=100)
    completion = p.collect_async(sampler=cirq.Simulator())
-    cirq.testing.assert_asyncio_will_have_result(completion, None)
+    assert await completion is None


Here too, can avoid assert await:

result = await p.collect_async(...) assert result is None

Also below.

Acknowledged.

maffoo · 2019-11-22T00:55:15Z

cirq/work/sampler_test.py

@@ -153,5 +153,5 @@ def run_sweep(self, *args, **kwargs):

    a = S().run_sweep_async(cirq.Circuit(), params=None)
    assert not ran
-    cirq.testing.assert_asyncio_will_have_result(a, [])
+    assert await a == []


result = await a assert result == []

Acknowledged.

maffoo · 2019-11-22T01:02:16Z

cirq/work/work_pool_test.py

    pool = CompletionOrderedAsyncWorkPool()
    work = asyncio.Future()
    pool.include_work(work)
    result = pool.__anext__()
-    cirq.testing.assert_asyncio_still_running(result)
+    assert await cirq.testing.asynchronous.asyncio_not_finishing(result)


I was thinking assert await foo and assert not await foo are maybe ok, but I definitely think assert await foo == bar is hard too parse. One possibility would be too absorb the assertion into the cirq.testing.asynchronous function, like in numpy.testing.assert_allclose and friends. Then you could just do

await cirq.testing.asynchronous.assert_not_finishing(result)

You'd need a different function to assert that a future does finish, but maybe it's worth it. Might be good to get some other people's opinions about what would be least confusing here.

Acknowledged. The function actually used to work that way. I changed it intentionally, to move the assert to the outside. IMO the only reason not to assert on the outside is for utility methods that give significantly better error messages on failure.

Strilanc · 2019-11-22T05:30:33Z

@maffoo We can discuss the style choices tomorrow if you'd like. My personal inclination is to not worry too much about internal method styling when people disagree, and to make tools that enforce things if people really disagree.

maffoo

Thanks for the explanations. LGTM

maffoo · 2019-11-22T16:32:32Z

cirq/testing/asynchronous.py

-    Works by running the asyncio event loop for a short amount of time.
+    This method is used in tests checking that a future actually depends on some
+    given event having happened. The test can assert, before the event, that the
+    future is not finishing and then assert, after the event, that the future


nit: "not finishing" -> "still pending"

maffoo · 2019-11-22T16:33:17Z

cirq/testing/asynchronous.py

-        AssertError: The future completed or failed within the timeout.
+    Returns:
+        True if the future did not complete despite being given a bit of time to
+        do so. False if the future did complete (or fail) or was already


nit: "True if the future is still pending after the timeout elapses. False if the future did complete..."

Strilanc requested review from maffoo and viathor November 20, 2019 23:32

googlebot added the cla: yes Makes googlebot stop complaining. label Nov 20, 2019

Strilanc added 2 commits November 21, 2019 13:19

Merge branch 'master' of github.com:quantumlib/Cirq into asynctest

7432e9a

fix

d68d34c

maffoo reviewed Nov 22, 2019

View reviewed changes

Strilanc added 2 commits November 21, 2019 21:28

rename

5d33329

Merge branch 'master' of github.com:quantumlib/Cirq into asynctest

40a0706

maffoo approved these changes Nov 22, 2019

View reviewed changes

Strilanc added 2 commits November 22, 2019 14:52

Merge branch 'master' of github.com:quantumlib/Cirq into asynctest

835973c

comments

d171d33

Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 22, 2019

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 22, 2019

CirqBot merged commit 55c1590 into master Nov 22, 2019

CirqBot deleted the asynctest branch November 22, 2019 23:10

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Nov 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make 'async def' tests work #2583

Make 'async def' tests work #2583

Strilanc commented Nov 20, 2019

maffoo Nov 21, 2019

Strilanc Nov 22, 2019

maffoo Nov 22, 2019

Strilanc Nov 22, 2019

Strilanc Nov 22, 2019

maffoo Nov 22, 2019

maffoo Nov 22, 2019

Strilanc Nov 22, 2019

maffoo Nov 22, 2019

Strilanc Nov 22, 2019

maffoo Nov 22, 2019

Strilanc Nov 22, 2019

maffoo Nov 22, 2019

Strilanc Nov 22, 2019

maffoo Nov 22, 2019

Strilanc Nov 22, 2019

Strilanc commented Nov 22, 2019 •

edited

maffoo left a comment

maffoo Nov 22, 2019

Strilanc Nov 22, 2019

maffoo Nov 22, 2019

Strilanc Nov 22, 2019


		def asyncio_not_finishing(future: Union[Awaitable, asyncio.Future, Coroutine],

Make 'async def' tests work #2583

Make 'async def' tests work #2583

Conversation

Strilanc commented Nov 20, 2019

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

Strilanc commented Nov 22, 2019 • edited

maffoo 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

Strilanc commented Nov 22, 2019 •

edited