FIX #162 "attached to a different loop", and add a new test case #164
Conversation
test_async_fixtures_with_finalizer_scope.py: 2) Main problem is due to side effects of asyncio.get_event_loop(). See: https://github.com/python/cpython/blob/3.8/Lib/asyncio/events.py#L636 This method could either return an existing loop or create a new one. This commit replaces all asyncio.get_event_loop() with previous pytest-asyncio code (0.10.0), so plugin uses the loop provided by event_loop fixture (instead of calling asyncio.get_event_loop()) Except following block, that has not been modified in this commit (as it behaves similar to 0.10.0) https://github.com/pytest-dev/pytest-asyncio/blob/v0.12.0/pytest_asyncio/plugin.py#L54-L66 Changes are for using always the new loop provided by event_loop fixture. Instead of calling get_event_loop() that: - either returns global recorded loop (with set_event_loop()) in case there is one, - or otherwise creates and record a new one
| return True | ||
|
|
||
|
|
||
| @contextlib.asynccontextmanager |
There was a problem hiding this comment.
This seems to fail the build on older Python versions. I don't think it is an essential part of the test. So you might get it to run on all required versions by changing the test a bit.
pytest_asyncio/plugin.py
Outdated
|
|
||
| fixturedef.func = wrapper | ||
| elif inspect.iscoroutinefunction(fixturedef.func): | ||
| coro = fixturedef.func | ||
|
|
||
| strip_event_loop = False |
There was a problem hiding this comment.
This code is copy pasted from line 72. Can you refactor it such that it is needed only once? E.g. pull it into a small function.
There was a problem hiding this comment.
Fixed. Included a new class FixtureStripper.
Please review again as I have included a new test case, and this has required additional changes
Thanks in advance.
There was a problem hiding this comment.
Had some minor comments, I am happy overall.
|
Great fix, thank you! Please review and merge this. |
|
Hello, thanks for this! The next few days are jam packed for me so this might wait until the end of the week for me to review. Hopefully the community can also comment in the mean time. |
… on python 3.6 & 3.5 2) An additional test case (test_module_with_get_event_loop_finalizer). Refactoring previous test case I got to another potential issue: Finalizers using get_event_loop() instead of event_loop [on a module scope fixture] To be able to pass this new test case I needed some additional changes on next event_loop fixture block( https://github.com/pytest-dev/pytest-asyncio/blob/v0.12.0/pytest_asyncio/plugin.py#L54-L66): - This block needs to apply on all event_loop fixture scopes (so I remove 'asyncio' in request.keywords, that only apply to function scope) - The get_event_loop() method inside this block has following side effects (See https://github.com/python/cpython/blob/3.8/Lib/asyncio/events.py#L636): - As no loop is still set, then L636 is executed, and so a new event_loop (let's call it L2) is first created and then set. - And then finalizer will restore L2 at the end of test. What has no sense, to restore L2 that wasn't before the test. And additionally this L2 is never closed. So it seeems that get_event_loop() should be removed, and so I see no reasons for any finalizer. 3) DRY: New FixtureStripper to avoid repeating code
Even_loop() fixture properly closes the loop (but as explained below this is not enough to reset behaviour): ``` @pytest.fixture def event_loop(request): """Create an instance of the default event loop for each test case.""" loop = asyncio.get_event_loop_policy().new_event_loop() yield loop loop.close() ``` Subsequent calls to asyncio.get_event_loop() returns the previous closed loop, instead of providing a new loop. This is due to (https://github.com/python/cpython/blob/3.8/Lib/asyncio/events.py#L634) variable _set_called is True, and so even when loop is None, then no new_event_loop is created [I really don´t know the reason of this _set_called behaviour]. The best solution I have found is to set_event_loop_policy(None). This completely resets global variable and so any subsequent call to asyncio.get_event_loop() provides a new event_loop. I have included this in the pytest hook (pytest_fixture_post_finalizer), that is called after fixture teardowns. Note: Same behaviour can be done modifying event_loop() fixture, but I have discarded it as this would force users that have overwritten even_loop fixture to modify its implementation: ``` @pytest.fixture def event_loop(request): """Create an instance of the default event loop for each test case.""" loop = asyncio.get_event_loop_policy().new_event_loop() yield loop loop.close() asyncio.set_event_loop_policy(None) ```
asyncio.create_task has been added in Python 3.7. The low-level asyncio.ensure_future() function can be used instead (works in all Python versions but is less readable). https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task
| def port_finalizer(finalizer): | ||
| async def port_afinalizer(): | ||
| # await task inside get_event_loop() | ||
| # RantimeError is raised if task is created on a different loop |
| async def port1(request, event_loop): | ||
| def port_finalizer(finalizer): | ||
| async def port_afinalizer(): | ||
| # await task inside get_event_loop() |
There was a problem hiding this comment.
This task is not awaited inside get_event_loop(), but in loop.run_until_complete().
| def port_finalizer(finalizer): | ||
| async def port_afinalizer(): | ||
| # await task inside get_event_loop() | ||
| # if loop is different a RuntimeError is raised |
There was a problem hiding this comment.
I would suggest to align the two comments from port1 and port2 and then highlight the difference of the two fixtures (first uses event_loop, second uses asyncio.get_event_loop)
|
OK let's get this in. Could you add a line to the changelog? |
Please review release date
Line added. Let me know if anything else is needed |
|
Thanks a lot, release going out tomorrow! |
@Tinche a friendly ping reminder as you said "tomorrow" some time ago and there's been no release :) |
test_async_fixtures_with_finalizer_scope.py:
Main problem is due to side effects of asyncio.get_event_loop(). See:
https://github.com/python/cpython/blob/3.8/Lib/asyncio/events.py#L636
This method could either return an existing loop or create a new one.
This commit replaces all asyncio.get_event_loop() in order to use the loop provided by event_loop fixture (instead of calling asyncio.get_event_loop())
https://github.com/pytest-dev/pytest-asyncio/blob/v0.12.0/pytest_asyncio/plugin.py#L54-L66
Changes are for using always the new loop provided by event_loop fixture.
Instead of calling get_event_loop() that:
BTW a9e2213 is not on the 0.10.0 changelog