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

unittest.TestCase coroutine support #77153

Closed
pdxjohnny mannequin opened this issue Feb 28, 2018 · 40 comments
Closed

unittest.TestCase coroutine support #77153

pdxjohnny mannequin opened this issue Feb 28, 2018 · 40 comments
Labels
3.8 only security fixes tests Tests in the Lib/test dir topic-asyncio type-feature A feature request or enhancement

Comments

@pdxjohnny
Copy link
Mannequin

pdxjohnny mannequin commented Feb 28, 2018

BPO 32972
Nosy @bitdancer, @njsmith, @asvetlov, @zware, @1st1, @miss-islington, @pdxjohnny, @PetterS, @tirkarthi, @dave-shawley
PRs
  • bpo-32972: Add coroutine support to unittest #5938
  • bpo-32972: Unittest: Add a runTestMethod to TestCase (don't merge) #6051
  • bpo-32972: Add unittest.AsyncioTestCase #10296
  • bpo-32972: Async test case #13386
  • bpo-32972: Document IsolatedAsyncioTestCase of unittest module #15878
  • [3.8] bpo-32972: Document IsolatedAsyncioTestCase of unittest module (GH-15878) #15918
  • 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:

    assignee = None
    closed_at = <Date 2019-09-11.11:18:08.785>
    created_at = <Date 2018-02-28.18:41:56.987>
    labels = ['3.8', 'type-feature', 'tests', 'expert-asyncio']
    title = 'unittest.TestCase coroutine support'
    updated_at = <Date 2019-09-11.11:18:08.784>
    user = 'https://github.com/pdxjohnny'

    bugs.python.org fields:

    activity = <Date 2019-09-11.11:18:08.784>
    actor = 'asvetlov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-09-11.11:18:08.785>
    closer = 'asvetlov'
    components = ['Tests', 'asyncio']
    creation = <Date 2018-02-28.18:41:56.987>
    creator = 'pdxjohnny'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32972
    keywords = ['patch']
    message_count = 40.0
    messages = ['313063', '313065', '313066', '313347', '313349', '313350', '313352', '313358', '313359', '313360', '313361', '313400', '313413', '313414', '313416', '313429', '313438', '313451', '313454', '313455', '313481', '313658', '313687', '313692', '313695', '313696', '313699', '313700', '328892', '328918', '329125', '335016', '335018', '342757', '343875', '343876', '351715', '351741', '351819', '351825']
    nosy_count = 11.0
    nosy_names = ['r.david.murray', 'njs', 'asvetlov', 'THRlWiTi', 'zach.ware', 'yselivanov', 'miss-islington', 'pdxjohnny', 'Petter S', 'xtreak', 'dave-shawley']
    pr_nums = ['5938', '6051', '10296', '13386', '15878', '15918']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue32972'
    versions = ['Python 3.8']

    @pdxjohnny
    Copy link
    Mannequin Author

    pdxjohnny mannequin commented Feb 28, 2018

    This makes unittest TestCase classes run test_ methods which are async

    @pdxjohnny pdxjohnny mannequin added type-bug An unexpected behavior, bug, or error 3.7 (EOL) end of life 3.8 only security fixes tests Tests in the Lib/test dir topic-asyncio labels Feb 28, 2018
    @bitdancer
    Copy link
    Member

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

    @bitdancer bitdancer added type-feature A feature request or enhancement and removed 3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error labels Feb 28, 2018
    @pdxjohnny
    Copy link
    Mannequin Author

    pdxjohnny mannequin commented Feb 28, 2018

    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.

    @1st1
    Copy link
    Member

    1st1 commented Mar 6, 2018

    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.

    @zware
    Copy link
    Member

    zware commented Mar 6, 2018

    Instead of a separate base class, what about an overridable coroutine_runner attribute that defaults to asyncio.run?

    @1st1
    Copy link
    Member

    1st1 commented Mar 6, 2018

    Instead of a separate base class, what about an overridable coroutine_runner attribute that defaults to asyncio.run?

    How is that better?

    @zware
    Copy link
    Member

    zware commented Mar 6, 2018

    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.

    @PetterS
    Copy link
    Mannequin

    PetterS mannequin commented Mar 6, 2018

    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.

    @1st1
    Copy link
    Member

    1st1 commented Mar 6, 2018

    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.

    @PetterS
    Copy link
    Mannequin

    PetterS mannequin commented Mar 6, 2018

    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?

    @1st1
    Copy link
    Member

    1st1 commented Mar 6, 2018

    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

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Mar 7, 2018

    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.

    @bitdancer
    Copy link
    Member

    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.

    @1st1
    Copy link
    Member

    1st1 commented Mar 8, 2018

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

    @zware
    Copy link
    Member

    zware commented Mar 8, 2018

    Why is a metaclass required? Playing with a modified version of John's PR5938, it doesn't seem necessary.

    @njsmith
    Copy link
    Contributor

    njsmith commented Mar 8, 2018

    Class decorators are also worth considering in cases where you find yourself reaching for a metaclass.

    @PetterS
    Copy link
    Mannequin

    PetterS mannequin commented Mar 8, 2018

    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.

    @1st1
    Copy link
    Member

    1st1 commented Mar 8, 2018

    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.

    @PetterS
    Copy link
    Mannequin

    PetterS mannequin commented Mar 8, 2018

    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.

    @njsmith
    Copy link
    Contributor

    njsmith commented Mar 8, 2018

    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.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Mar 9, 2018

    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.

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

    @pdxjohnny
    Copy link
    Mannequin Author

    pdxjohnny mannequin commented Mar 12, 2018

    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?

    @PetterS
    Copy link
    Mannequin

    PetterS mannequin commented Mar 12, 2018

    John: inspect.iscoroutinefunction(meth) does not work if the method is decorated with e.g. unittest.mock.patch.

    @PetterS
    Copy link
    Mannequin

    PetterS mannequin commented Mar 12, 2018

    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.

    @1st1
    Copy link
    Member

    1st1 commented Mar 12, 2018

    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] #6051
    [2] https://github.com/python/cpython/blob/master/Lib/asyncio/runners.py#L8

    @zware
    Copy link
    Member

    zware commented Mar 12, 2018

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

    @zware
    Copy link
    Member

    zware commented Mar 12, 2018

    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.

    @1st1
    Copy link
    Member

    1st1 commented Mar 12, 2018

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

    @dave-shawley
    Copy link
    Mannequin

    dave-shawley mannequin commented Oct 30, 2018

    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?

    @PetterS
    Copy link
    Mannequin

    PetterS mannequin commented Oct 30, 2018

    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.

    @dave-shawley
    Copy link
    Mannequin

    dave-shawley mannequin commented Nov 2, 2018

    I added a different implementation for consideration (#10296).

    @dave-shawley
    Copy link
    Mannequin

    dave-shawley mannequin commented Feb 7, 2019

    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?

    @dave-shawley
    Copy link
    Mannequin

    dave-shawley mannequin commented Feb 7, 2019

    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:

    • uses a @Property named loop to lazily create an event loop instance
    • destroys the event loop instance in _terminateTest
    • re-implements _runTest to call new asynchronous hook methods
    • adds asyncSetUp and asyncTearDown methods that simply call the synchronous methods
    • re-implements doCleanups to call co-routines asynchronously

    Rationale
    ---------
    I used asyncio.iscoroutinefunction to detect if test methods or callbacks are co-routines. You explicitly opt-in to async behavior using the async marker on things that you want to run on the loop. This will cause problems with using the patch decorator on test methods since they will no not be detected as co-routines. I took this approach primarily to simplify the code and enforce explicitness. Since the implementation is a new sub-class, it cannot break existing code and new code can place the patch inside of the test method instead of decorating the method.

    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.

    @asvetlov
    Copy link
    Contributor

    #13386 is a new attempt to solve the feature request

    @miss-islington
    Copy link
    Contributor

    New changeset 4dd3e3f by Miss Islington (bot) (Andrew Svetlov) in branch 'master':
    bpo-32972: Async test case (GH-13386)
    4dd3e3f

    @asvetlov
    Copy link
    Contributor

    Done.
    Let's keep the issue open until the documentation will be committed

    @tirkarthi
    Copy link
    Member

    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.

    @asvetlov
    Copy link
    Contributor

    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.

    @miss-islington
    Copy link
    Contributor

    New changeset 6a9fd66 by Miss Islington (bot) (Xtreak) in branch 'master':
    bpo-32972: Document IsolatedAsyncioTestCase of unittest module (GH-15878)
    6a9fd66

    @asvetlov
    Copy link
    Contributor

    New changeset 0ba5dbd by Andrew Svetlov (Miss Islington (bot)) in branch '3.8':
    bpo-32972: Document IsolatedAsyncioTestCase of unittest module (GH-15878) (GH-15918)
    0ba5dbd

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes tests Tests in the Lib/test dir topic-asyncio type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants