classification
Title: unittest.TestCase coroutine support
Type: enhancement Stage: patch review
Components: asyncio, Tests Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Petter S, asvetlov, dave-shawley, njs, pdxjohnny, r.david.murray, yselivanov, zach.ware
Priority: normal Keywords: patch

Created on 2018-02-28 18:41 by pdxjohnny, last changed 2018-11-02 11:26 by dave-shawley.

Pull Requests
URL Status Linked Edit
PR 5938 closed pdxjohnny, 2018-02-28 18:41
PR 6051 open Petter S, 2018-03-10 11:11
PR 10296 open dave-shawley, 2018-11-02 11:26
Messages (31)
msg313063 - (view) Author: John Andersen (pdxjohnny) * Date: 2018-02-28 18:41
This makes unittest TestCase classes run `test_` methods which are async
msg313065 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-02-28 19:16
In order for this to be actually useful, I think we also need a mechanism to set up and run async cleanup methods.  In my own project I accomplish this by putting the run_until_complete in a try/finally loop and have an asyncAddCleanup method that just appends coroutines to a list that are then run in reversed order in the finally clause.

Whatever we come up with needs tests and docs, and before that, I'm sure, more discussion about the API.  Probably a topic for python-ideas?  Or the testing sig?

I really do think we should have async test support in the stdlib, though.  Basic support isn't that hard (it's 15 lines in the test suite I'm using currently).
msg313066 - (view) Author: John Andersen (pdxjohnny) * Date: 2018-02-28 19:48
More discussion indeed. I figured I was not alone in my desire to see async test support in the stdlib. Let me know what other changes would be good.
msg313347 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-03-06 18:05
I think the right approach would be to add an new base TestCase class: AsyncioTestCase.  There other frameworks that have different event loop implementations, so we can't assume that an `async def` test should always be executed by asyncio.

And you should use the `asyncio.run` function to run the coroutine.
msg313349 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-03-06 18:11
Instead of a separate base class, what about an overridable `coroutine_runner` attribute that defaults to `asyncio.run`?
msg313350 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-03-06 18:12
> Instead of a separate base class, what about an overridable `coroutine_runner` attribute that defaults to `asyncio.run`?

How is that better?
msg313352 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-03-06 18:20
How is a separate base class better? :)

Having a default coroutine runner on the standard TestCase means all you have to do to add async tests is tack an `async` onto the front of your method definition, and everything just works as it always has.  Making it a separate base class means you have to know about that base class, what it's called, and remember to derive your test class from it.  If you accidentally add `async` to the front of a test method in a TestCase-derived test class, you get mostly-silent success with an easily-ignored warning about a coroutine not being awaited.
msg313358 - (view) Author: Petter S (Petter S) * Date: 2018-03-06 19:14
It's good to see others with the same idea as me. I just wrote https://github.com/python/cpython/pull/6005/commits/4d7e1837f235687c875e985e97701609fc1ac458 .

In my opinion, there is no need for another test class. I completely agree with Zachary Ware that the current TestCase class is more or less broken (or at least easily misused) with respect to coroutines.

Fixing TestCase would not break existing code. The customization of event loop would not be hard to add.
msg313359 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-03-06 19:26
> How is a separate base class better? :)

It's very explicit that way.

Also, I personally subclassed TestCase in many of my projects specifically to add async support.  To do that you have to use a metaclass to scan class' namespace for 'async def' functions.

Currently, unittest.TestCase doesn't have a metaclass.  If you add one to it, it might break all packages that were subclassing TestCase with a metaclass.


> If you accidentally add `async` to the front of a test method in a TestCase-derived test class, you get mostly-silent success with an easily-ignored warning about a coroutine not being awaited.

Well, what if you use trio or curio?  You add an 'async' keyword and get a cryptic error that some framework internals just broke.

So I'm strong -1 on the coroutine_runner attribute idea.
msg313360 - (view) Author: Petter S (Petter S) * Date: 2018-03-06 19:34
> Also, I personally subclassed TestCase in many of my projects specifically to add async support.  To do that you have to use a metaclass to scan class' namespace for 'async def' functions.

Doesn't that break when, for example, test methods are decorated with unittest.mock.patch?
msg313361 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-03-06 19:38
> Doesn't that break when, for example, test methods are decorated with unittest.mock.patch?

No, it shouldn't break them if you wrap async methods carefully.

Here's a metaclass that I wrote recently doing just that: https://github.com/Azure/azure-functions-python-worker/blob/9ed3f8acd45a264927ce11af64f5b495f0404032/azure/worker/testutils.py#L44-L63
msg313400 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-03-07 22:26
I'm with Yuri: subclassing is under better control.
The need for a new subclass is relative rare, for the whole asyncio world AsyncioTestCase should be enough.
Another async framework most likely require a different test tool with another game rules.
msg313413 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-03-08 00:45
I agree with what Zach said in msg313352.  I don't see how having a separate class affects the problem of what to use to run an async def test case.  If "an AsyncTestCase should be enough" but "other frameworks will require a different test tool", why wouldn't support in TestCase for running the test under asyncio also be enough?  I'm really not seeing what a separate class buys you.
msg313414 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-03-08 01:47
> I'm really not seeing what a separate class buys you.

I already mentioned in my previous comments that adding async support to unittest.TestCase would require us to add a metaclass to it, which is potentially a backwards incompatible change.

Moreover, I'm not even sure that `AsyncioTestCase` should be in the unittest module and not in asyncio.  Any non-trivial async package I've worked with usually requires quite a complicated setup to prepare an async test environment.  Usually a test case needs to test against different event loop policies or different event loops.  Which means that we need to support asynchronous versions of setUp, setUpClass, etc.  And I'm not sure we should just detect when they are asynchronous, perhaps we should make a set of new methods: asyncSetUp, asyncSetUpClass, etc.  This is something that needs more discussion.

So far I see that two core asyncio devs are against the proposed idea of changing unittest.TestCase to support async transparently.  IMO it gives almost no benefits but will complicate the implementation of TestCase and TestRunner (which are already quite hairy).
msg313416 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-03-08 02:12
Why is a metaclass required?  Playing with a modified version of John's PR5938, it doesn't seem necessary.
msg313429 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-03-08 09:38
Class decorators are also worth considering in cases where you find yourself reaching for a metaclass.
msg313438 - (view) Author: Petter S (Petter S) * Date: 2018-03-08 13:13
> No, it shouldn't break them if you wrap async methods carefully.
> Here's a metaclass that I wrote recently doing just that

That code does not seem to work for me:
https://gist.github.com/PetterS/f684095a09fd1d8164a4d8b28ce3932d
I get "RuntimeWarning: coroutine 'test_async_with_mock' was never awaited"

@mock.patch needs to work correctly for test methods.

>> I'm really not seeing what a separate class buys you.
> I already mentioned in my previous comments that adding async support to unittest.TestCase would require us to add a metaclass to it, which is potentially a backwards incompatible change.

No, unittest.TestCase can handle this, as demonstrated by two PRs in this bug. This would not change the behavior of existing (working) code.
msg313451 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-03-08 17:40
> That code does not seem to work for me:
> https://gist.github.com/PetterS/f684095a09fd1d8164a4d8b28ce3932d
> I get "RuntimeWarning: coroutine 'test_async_with_mock' was never awaited"

> @mock.patch needs to work correctly for test methods.
> [..]
> No, unittest.TestCase can handle this, as demonstrated by two PRs in this bug. This would not change the behavior of existing (working) code.

Correct, and while it's possible to fix my little helper to work with mocks, you're completely right that we can, indeed, implement async support in TestCase without a metaclass.  My mistake here, thanks for correcting!

In any case, while I think we can now talk about augmenting TestCase, I'd still want to first discuss few other issues:

1. Do we need support for async versions of setUp, setUpClass, etc?  In my opinion: yes.

2. There're a few options how to implement (1):

a) Add async support to TestCase. Automatically detect when a test is async, or when 'setUp' and friends are async and run them all with 'asyncio.run'.

b) Add async support to TestCase.  Add asyncSetUp, asyncSetUpClass, etc magic methods.  The argument in favour of this approach over (a) is that it's possible to safely use 'super()' calls when you have multiply inherited TestCases.

c) Add a new AsyncioTestCase specifically for working with asyncio (combine with option (b)).

I still don't like (a) because it involves too much implicit logic.  I think that it's simple enough to inherit your testcase from unittest.AsyncioTestCase and read a separate documentation specifically about it and not about generic TestCase.
msg313454 - (view) Author: Petter S (Petter S) * Date: 2018-03-08 19:04
> 1. Do we need support for async versions of setUp, setUpClass, etc?  In my opinion: yes.

I completely agree. I would imagine many or most real-world tests requiring async setUp. There is also the question on how a custom loop etc. can be used in the unit test class.

How about this: unittest.TestCase gets a very small refactor in which a overridable helper method is used to run the test method. This helper method can then be changed to run async methods in a subclass AsyncioTestCase (that probably should live in asyncio).

Pull request 6005 contained such a helper method, but the async part could be implemented in the subclass instead.
msg313455 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-03-08 20:03
You should also think about loop lifecycle: right now it's using the same loop for all test cases, so callbacks can leak between tests. Twisted actually goes a step further and explicitly checks for left over callbacks and fails the test if any are found.
msg313481 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-03-09 12:08
1. We have an agreement that we need `asyncSetUp` and all family. I strongly support it. `super()` call is crucial argument I think.

2. Should async setup/teardown methods be a part of `unittest.TestCase` or we need a new AsyncTestCase class?

I believe adding async methods to TestCase makes no sense: without instructions how to run async code these methods are pretty useless and error-prone. Implementation of async startup/teardown without providing async runner (read about a runner below) looks like a error. If we mix everything into TestCase we should support virtually both modes in the single TestCase class: one for sync-only test case and other for async tests with async setup/teardown.

3. The next question is: should we support *every not existing yet* async framework by our tools or asyncio-compatible only. 
*Not existing yet* means we have no idea how to create a test loop, prepare it, run a test by loop, tear down everything.
Sure, we can invite a `async function runner` concept but I request a PEP for such significant addition with full specification.

Zachary Ware proposed `coroutine_runner` property in `TestCase`.
What is `coroutine_runner`? How should it work? How to run a coroutine? Subscribe for a UNIX signal or spawn a subprocess (asyncio has many tricks for signal/subprocess support, pretty sure they are not compatible with curio/trio approach)? Should we expose unified timer/socket/file API?
Is it ok that correct asyncio code will fail or even hang if `coroutine_runner` is not compatible with asyncio? If no -- that is benefit of *unified runner* if we need to use concrete runner family for writing tests?

In case of limiting build-in `AsyncioTestCase` to asyncio only we narrow down to very well defined asyncio API. Tornado and Twisted declared asyncio compatibility, they should work with the test case class.

curio/trio explicitly declared incompatibility with asyncio. That's fine, we should not support them by standard unittest classes too because we have no way to write tests for non-specified-yet contracts.
msg313658 - (view) Author: John Andersen (pdxjohnny) * Date: 2018-03-12 15:49
I've updated my pull request to do the following:

1. Provide a new AsyncTestCase class which is a subclass of TestCase
2. Run coroutines with a coroutineRunner property.
  a. In 3.6 this calls get_evet_loop.run_until_complete
  b. In 3.7 > this calls asyncio.run
3. setUp, testMethod s, and tearDown can be either async or not

Thoughts?
msg313687 - (view) Author: Petter S (Petter S) * Date: 2018-03-12 20:00
John: inspect.iscoroutinefunction(meth) does not work if the method is decorated with e.g. unittest.mock.patch.
msg313692 - (view) Author: Petter S (Petter S) * Date: 2018-03-12 20:22
Personally, I think John's PR is fine. (but the test class could arguably live in asyncio) I like that setUp, tearDown and test methods all can be async.

But if setUp and tearDown should never be async as Yury commented above, they don't need runners. That is what I went for in my PR.

Introducing three new public methods to TestCase is too much. I originally made the runner private, but Yury told me to make it public and document it.
msg313695 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-03-12 20:27
First, John and Peter, let's not have two competing PRs.  I'd prefer to have only one to make things easier to review.  So far it looks like Peter's is slightly more developed.  And this also seems to be a complex issue, so there's plenty of work to be done here and you can work on it jointly if you want.

I think a lot of Andrew's recommendations (see [1]) about doSetUp etc make sense.

The problem is how to marry methods like `asyncSetUpClass`, `asyncSetUp` and `runTest` together: all are async, all should be executed *within* one event loop.  They should also be executed *before* you call the actual test method, so it's *incorrect* to call the test case and decide if async set ups should be called or not.  This is another argument to have a separate AsyncioTestCase base class.

BTW, we can use asyncio.run() to execute only *one* coroutine.  If you have more than one, they will be executed in *different* event loops!  This is why the "coroutine runner" proposal is fundamentally incompatible with having `asyncSetUp` methods.

Speaking of asyncSetUp, let me re-iterate why we can't *just* reuse setUp:

  class B:

    async def setUp(self):
        print('bar')

  class A(B):

    def setUp(self):
        super().setUp()
        print('foo')

If we execute tests in A, 'bar' will never be printed.

So I'm suggesting to try another approach: have a generic ABC `unittets.AsyncTestCase(ABC)` with one abstract method: `makeAsyncExecutor`.  This will be a class intended to be used by framework developers.

We then add a concrete `unittest.AsyncioTestCase(unittest.AsyncTestCase)` implementation class to unittest (or should it be asyncio? I'm fine either way).

So what will `makeAsyncExecutor` do?  It should return an instance of `unittest.AsyncExecutor(ABC)` object which roughly would look like the following (this is just an example design, we need to discuss it further):

  class AsyncExecutor(abc.ABC):

      def init(self)

      def runAsync(self, coro: typing.Coroutine)

      def shutdown(self)

Why do we need a proxy object?  Well, due to some historical reasons, `asyncio.loop.close()` just closes the loop.  The actual correct shutdown protocol for an asyncio program is a little bit more elaborate, you can take a look at how asyncio.run is implemented in Python 3.7: [2].  Correct initialization of an event loop might also require extra work.  And keep in mind, we want to have a more-or-less generic and reusable implementation here so that frameworks like Trio could use this new API too (that's not the main goal, but would be nice to have).

A few open questions: 

- Should we have 'asyncSetUpClass'?

- Should we have an event loop per one test case method, or per one class?  Should this be configurable?  In my experience both things are needed from time to time.

- If we have 'asyncSetUp' should it be called before or after 'setUp'?


[1] https://github.com/python/cpython/pull/6051
[2] https://github.com/python/cpython/blob/master/Lib/asyncio/runners.py#L8
msg313696 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-03-12 20:30
1. I'm still -1 on a separate subclass, especially since the subclass should still be compatible with the base class.

2. This is only in consideration for 3.8 (even 3.7 is past feature freeze at this point), so the version-dependent behavior is unnecessary.

3. I agree that the `super().setUp()` idiom precludes allowing `setUp` and `tearDown` to be synchronous or asynchronous unless the idiom becomes `self.runMethod(super().setUp)`, but that's longer and harder to remember.  `setUpClass` and `tearDownClass` also need to be considered.

You also still need to add tests.

`addCleanup` is an open question; I think it could handle sync or async on its own but could be convinced otherwise.


Andrew: Judging by your questions in msg313481 I think my description of `coroutine_runner` was not specific enough.  The basic idea is that unittest.case.TestCase is defined with `coroutine_runner = asyncio.run`, and if you need something else then you do:

class MyTest(TestCase):

    # I assume trio has something like this :)
    coroutine_runner = trio.run

    async def test_something_in_trio(self):
        self.assertTrue(1)


asyncio gets the special treatment of having its runner set by default by virtue of being in the standard library.

I'm certainly open to better naming :)
msg313699 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-03-12 20:45
Ok, Yury clarified a few points before I got my message submitted there, so some of my last message may be a bit misguided.  In particular, the problems with just using `asyncio.run` are clearer to me now.

To give my answers to Yury's open questions:

- We should have an async setUpClass capability, one way or another.

- I would say event loop per class.  If someone really needs event loop per method, they can create separate classes per method.  It's ugly, but effective.

- We should have an async setUp capability.  Maybe we could add a helper method to be called from setUp rather than adding a whole new asyncSetUp into the protocol?  That eliminates the problem of which goes first.
msg313700 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-03-12 20:52
> - I would say event loop per class.  If someone really needs event loop per method, they can create separate classes per method.  It's ugly, but effective.

+1.

- We should have an async setUp capability.  Maybe we could add a helper method to be called from setUp rather than adding a whole new asyncSetUp into the protocol?  That eliminates the problem of which goes first.

I'd rather have a protocol :)  Protocols are easy to document and it's possible to statically validate them (with linters/metaclasses).  Calling some method from setUp to call asyncSetUp would be a common gotcha IMO.

We can always call synchronous setUp before asyncSetUp, I think it's intuitive enough.

Speaking of addCallback, we should have a distinct addAsyncCallback.  It's OK to have an object with both __call__ and __await__ methods -- in which case it's not clear which one you should call.

In general, while we will be adding a new subclass and a few 'async'-prefixed methods, it should still be relatively straightforward for people to write and, more importantly, read the code that uses them.
msg328892 - (view) Author: Dave Shawley (dave-shawley) * Date: 2018-10-30 11:02
Hi all, I took a slightly different direction for adding async/await support to the unittest library.  I summarized the approach that I took in a message to python-ideas (https://mail.python.org/pipermail/python-ideas/2018-October/054331.html) and a branch is available for preview in my fork of cpython (https://github.com/dave-shawley/cpython/pull/1).

My question is whether I should reboot this bpo with my PR or create a new one since the implementation is different than what was already discussed?
msg328918 - (view) Author: Petter S (Petter S) * Date: 2018-10-30 13:45
As far as I am concerned, the discussion can continue in this issue.

I still think a reasonable first step is to add a run hook to the regular TestCase class, like in PR 6051. But building consensus is a bit tricky.
msg329125 - (view) Author: Dave Shawley (dave-shawley) * Date: 2018-11-02 11:26
I added a different implementation for consideration (https://github.com/python/cpython/pull/10296).
History
Date User Action Args
2018-11-02 11:26:43dave-shawleysetmessages: + msg329125
pull_requests: + pull_request9606
2018-10-30 13:45:32Petter Ssetmessages: + msg328918
2018-10-30 11:02:36dave-shawleysetnosy: + dave-shawley
messages: + msg328892
2018-03-12 20:52:46yselivanovsetmessages: + msg313700
2018-03-12 20:45:01zach.waresetmessages: + msg313699
2018-03-12 20:30:08zach.waresetmessages: + msg313696
2018-03-12 20:27:42yselivanovsetmessages: + msg313695
2018-03-12 20:22:01Petter Ssetmessages: + msg313692
2018-03-12 20:00:01Petter Ssetmessages: + msg313687
2018-03-12 15:49:06pdxjohnnysetmessages: + msg313658
2018-03-10 11:11:47Petter Ssetkeywords: + patch
stage: patch review
pull_requests: + pull_request5812
2018-03-09 12:08:41asvetlovsetmessages: + msg313481
2018-03-08 20:03:57njssetmessages: + msg313455
2018-03-08 19:04:00Petter Ssetmessages: + msg313454
2018-03-08 17:40:35yselivanovsetmessages: + msg313451
2018-03-08 13:13:01Petter Ssetmessages: + msg313438
2018-03-08 09:38:00njssetnosy: + njs
messages: + msg313429
2018-03-08 02:12:49zach.waresetmessages: + msg313416
2018-03-08 01:47:36yselivanovsetmessages: + msg313414
2018-03-08 00:45:15r.david.murraysetmessages: + msg313413
2018-03-07 22:26:03asvetlovsetmessages: + msg313400
2018-03-06 19:38:08yselivanovsetmessages: + msg313361
2018-03-06 19:34:20Petter Ssetmessages: + msg313360
2018-03-06 19:26:14yselivanovsetmessages: + msg313359
2018-03-06 19:14:50Petter Ssetnosy: + Petter S
messages: + msg313358
2018-03-06 18:20:07zach.waresetmessages: + msg313352
2018-03-06 18:12:52yselivanovsetmessages: + msg313350
2018-03-06 18:11:39zach.waresetnosy: + zach.ware
messages: + msg313349
2018-03-06 18:05:57yselivanovsetmessages: + msg313347
2018-03-06 18:03:41yselivanovlinkissue32992 superseder
2018-02-28 19:48:04pdxjohnnysetmessages: + msg313066
2018-02-28 19:16:35r.david.murraysetversions: - Python 3.6, Python 3.7
nosy: + r.david.murray

messages: + msg313065

type: behavior -> enhancement
2018-02-28 18:41:57pdxjohnnycreate