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
unittest.TestCase coroutine support #77153
Comments
This makes unittest TestCase classes run |
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). |
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. |
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 And you should use the |
Instead of a separate base class, what about an overridable |
How is that better? |
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 |
It's good to see others with the same idea as me. I just wrote 4d7e183 . 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. |
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.
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. |
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 |
I'm with Yuri: subclassing is under better control. |
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. |
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 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). |
Why is a metaclass required? Playing with a modified version of John's PR5938, it doesn't seem necessary. |
Class decorators are also worth considering in cases where you find yourself reaching for a metaclass. |
That code does not seem to work for me: @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:
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. |
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. |
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. |
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.
Zachary Ware proposed In case of limiting build-in 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. |
I've updated my pull request to do the following:
Thoughts? |
John: inspect.iscoroutinefunction(meth) does not work if the method is decorated with e.g. unittest.mock.patch. |
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. |
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 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 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 We then add a concrete So what will 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, A few open questions:
[1] #6051 |
You also still need to add tests.
Andrew: Judging by your questions in msg313481 I think my description of 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 :) |
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 To give my answers to Yury's open questions:
|
+1.
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. |
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 (dave-shawley#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? |
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. |
I added a different implementation for consideration (#10296). |
Hi everyone, I'm trying to reboot conversation on this issue since I would love for this to land in Python 3.8. At the recommendation of Terry Jan Reedy, here is my summary of where I think that the discussion is currently. If anything is misrepresented, incorrectly linked, or if I misspelled anyones name, I apologize. Feel free to correct any mistakes that you notice. New subclass of TestCase versus enhancing unittest.TestCase This was one of the primary discussion points since the start of this BPO. I believe that Petter S (msg313454), Yury Selivanov (msg313695), and Andrew Svetlov (msg313481) were +1 on having a new sub-class of unittest.TestCase whereas Zachary Ware (msg313696) and R. David Murray (msg313413) would prefer that unittest.TestCase be enhanced to support async methods directly. This is (in my opinion) the most contentious of the issues. It also is the one with the largest impact on the implementation. I feel that it is still largely up in the air amongst the core developers. Lifecycle of the loop Whether the loop should live for the entire execution of a TestCase or for the execution of an individual test method came up a few times. Nathaniel Smith (msg313455) was concerned about callbacks leaking between tests. Yury Selivanov and Zachary Ware agreed that having a single event loop per class was acceptable (msg313696 and msg313700). Support for other loops Supporting 3rd party loop implementations (e.g., Tornado, Twisted, curio/trio) was discussed briefly. The conclusion that I drew from the discussion is that the built-in testing class was not required to offer direct support to non-asyncio compatible loops. Most notably msg313481 from Andrew Svetlov influenced my implementation. Where should support live? This is only relevant if we are not enhancing unittest.TestCase. Petter S favored that the separate test case implementation live in asyncio instead of unittest. I don't believe that anyone else had a strong opinion on this issue. Should we have explicit methods for async behavior? Yury Selivanov was most outspoken on explicitly named "async" methods like "asyncSetup", "asyncAddCallback", and the like. Particularly in msg313695 and msg313700. Zachary Ware seemed to agree that the separation was necessary but the functionality could be implemented by calling the async methods from the existing setUp and setUpClass methods (msg313699). Did I miss anything? |
PR 10296 is my implementation of a unittest.TestCase subclass solution to this issue. This comment explains the approach and rationale in detail. Let's discuss this and see if the implementation meets expectations or should be abandoned. I refactored unittest.TestCase to isolate the running of test methods into a new helper method named _runTest and add a new hook named _terminateTest. The _runTest method is used to customize test method execution in sub-classes. The _terminateTest method is called from within the finally block in unittest.TestCase.run. It is an empty method in unittest.TestCase. This was the only change to unittest.TestCase. A new class unittest.AsyncioTestCase was added that implements async-based testing. It is a direct sub-class of unittest.TestCase that:
Rationale I believe that creating an destroying the loop with each test method execution is the safest approach for managing the lifecycle. I view having the loop exist at the class level is an unnecessary optimization. I also ensure that code under test that calls asyncio.get_event_loop or asyncio.get_running_loop will receive the loop by calling asyncio.set_event_loop with the new loop. This came up in PR review with Petter S. The management of the loop is isolated into a property which makes it possible to create custom sub-classes that instantiate 3rd party loops that are asyncio compatible. This is the only concession that my implementation makes to supporting other loop classes. If it is not clear, I believe that a new sub-class of unittest.TestCase is necessary for a clean implementation. It preserves unittest.TestCase so the risk of breaking existing code is minimized and the async functionality is isolated in a new class that is explicitly meant to test async code. This is also necessary if the implementation should exist in the asyncio module. |
#13386 is a new attempt to solve the feature request |
Done. |
I did an initial try at the documentation. I think most of the functions are very similar to the TestCase API and I just copied them to add the async related information. I guess it could be improved to keep the length down with still retaining the link to synchronous API. I added an example of a coroutine test case with the order of setup and teardown calls. I also realized during the docs that there is no async versions of class level setup and teardown calls. They are executed in a slightly different manner in unittest.suite module unlike per test setup and teardown. Is this an explicit decision to not support them? Is it worth documenting them? Feedback welcome. |
Thanks! The test case re-creates a loop per test for the sake of test isolation (scheduled activities from test_a() don't interleave with test_b()). Unfortunately, on the class level level (setUpClass()/tearDownClass()) the loop doesn't exits. There are other reasons that prevent adding them properly, at least not in 3.8. Maybe later we can provide more rich support. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: