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

bpo-32972: Async test case #13386

Merged
merged 19 commits into from
May 29, 2019
Merged

Conversation

asvetlov
Copy link
Contributor

@asvetlov asvetlov commented May 17, 2019

Add explicit asyncSetUp and asyncTearDown methods.
The rest is the same as for #13228

AsyncTestCase create a loop instance for every test for the sake of test isolation.
Sometimes a loop shared between all tests can speed up tests execution time a lot but it requires control of closed resources after every test finish. Basically, it requires nested supervisors support that was discussed with @1st1 many times. Sorry, asyncio supervisors have no chance to land on Python 3.8.

The PR intentionally does not provide API for changing the used event loop or getting the test loop: use asyncio.set_event_loop_policy() and asyncio.get_event_loop() instead.

The PR adds four overridable methods to base unittest.TestCase class:

    def _callSetUp(self):
        self.setUp()

    def _callTestMethod(self, method):
        method()

    def _callTearDown(self):
        self.tearDown()

    def _callCleanup(self, function, /, *args, **kwargs):
        function(*args, **kwargs)

It allows using asyncio facilities with minimal influence on the unittest code.

The last but not least: the PR respects contextvars. The context variable installed by asyncSetUp is available on test, tearDown and a coroutine scheduled by addCleanup.

https://bugs.python.org/issue32972

@asvetlov asvetlov changed the title Async test case, second attempt bpo-32972: Async test case May 17, 2019
@asvetlov asvetlov marked this pull request as ready for review May 17, 2019 19:00
@asvetlov asvetlov requested a review from matrixise May 17, 2019 19:00
self.tearDown()

def _callCleanup(self, function, *args, **kwargs):
self._callMaybeAsync(function, *args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we have an explicit addAsyncCleanup?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, we have an explicit async method in AsyncExitStack: https://docs.python.org/3/library/contextlib.html#contextlib.AsyncExitStack.push_async_callback

Even though we toyed with the idea of just reusing the push_callback method and checking if the returned value is an awaitable.

I'm more in favour of addAsyncCleanup in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add addAsyncCleanup() method, it makes sense.
The implementation should push a callback into the same cleanup queue as used for addCleanup() though, to keep cleanups order.
I think running all sync cleanups before asyncs and vice versa is a bad idea.
Cleanups should be executed exactly in the reverse adding order.
Also, self.doCleanups() should execute all sync and async cleanups altogether.
Do you agree?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Lib/unittest/async_case.py Outdated Show resolved Hide resolved
@auvipy
Copy link

auvipy commented May 26, 2019

this would be great to see in 3.8

Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add addAsyncCleanup() before merging.

@asvetlov
Copy link
Contributor Author

addAsyncCleanup() is added

@miss-islington miss-islington merged commit 4dd3e3f into python:master May 29, 2019
@asvetlov asvetlov deleted the async-test-case2 branch September 12, 2019 12:42
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
Add explicit `asyncSetUp` and `asyncTearDown` methods.
The rest is the same as for python#13228

`AsyncTestCase` create a loop instance for every test for the sake of test isolation.
Sometimes a loop shared between all tests can speed up tests execution time a lot but it requires control of closed resources after every test finish. Basically, it requires nested supervisors support that was discussed with @1st1 many times. Sorry, asyncio supervisors have no chance to land on Python 3.8.

The PR intentionally does not provide API for changing the used event loop or getting the test loop: use `asyncio.set_event_loop_policy()` and `asyncio.get_event_loop()` instead.

The PR adds four overridable methods to base `unittest.TestCase` class:
```
    def _callSetUp(self):
        self.setUp()

    def _callTestMethod(self, method):
        method()

    def _callTearDown(self):
        self.tearDown()

    def _callCleanup(self, function, /, *args, **kwargs):
        function(*args, **kwargs)
```
It allows using asyncio facilities with minimal influence on the unittest code.

The last but not least: the PR respects contextvars. The context variable installed by `asyncSetUp` is available on test, `tearDown` and a coroutine scheduled by `addCleanup`.


https://bugs.python.org/issue32972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants