Fix event_loop is run after other fixtures

[simonfagerholm]

Resolves #154, resolves #157, resolves #158
Also adds unittests for the use case.

Not ready for merge, breaks test_asyncio_marker_without_loop, which was added in #64.

pytest-asyncio/tests/test_simple.py

Lines 116 to 128 in a7e5795

	class TestUnexistingLoop:
	@pytest.fixture
	def remove_loop(self):
	old_loop = asyncio.get_event_loop()
	asyncio.set_event_loop(None)
	yield
	asyncio.set_event_loop(old_loop)
	@pytest.mark.asyncio
	async def test_asyncio_marker_without_loop(self, remove_loop):
	"""Test the asyncio pytest marker in a Test class."""
	ret = await async_coro()
	assert ret == 'ok'

Discussion for possible fix of broken test below

[simonfagerholm]

I did something that seems to fix the test, however I'm really not sure the behaviour is wanted:

diff --git a/pytest_asyncio/plugin.py b/pytest_asyncio/plugin.py
index 9860e04..c0b65da 100644
--- a/pytest_asyncio/plugin.py
+++ b/pytest_asyncio/plugin.py
@@ -125,14 +125,18 @@ def pytest_pyfunc_call(pyfuncitem):
     if 'asyncio' in pyfuncitem.keywords:
         if getattr(pyfuncitem.obj, 'is_hypothesis_test', False):
             pyfuncitem.obj.hypothesis.inner_test = wrap_in_sync(
-                pyfuncitem.obj.hypothesis.inner_test
+                pyfuncitem.obj.hypothesis.inner_test,
+                _loop=pyfuncitem.funcargs['event_loop']
             )
         else:
-            pyfuncitem.obj = wrap_in_sync(pyfuncitem.obj)
+            pyfuncitem.obj = wrap_in_sync(
+                pyfuncitem.obj,
+                _loop=pyfuncitem.funcargs['event_loop']
+            )
     yield


-def wrap_in_sync(func):
+def wrap_in_sync(func, _loop):
     """Return a sync wrapper around an async function executing it in the
     current event loop."""

@@ -140,9 +144,15 @@ def wrap_in_sync(func):
     def inner(**kwargs):
         coro = func(**kwargs)
         if coro is not None:
-            task = asyncio.ensure_future(coro)
             try:
-                asyncio.get_event_loop().run_until_complete(task)
+                loop = asyncio.get_event_loop()
+            except RuntimeError as exc:
+                if 'no current event loop' not in str(exc):
+                    raise
+                loop = _loop
+            task = asyncio.ensure_future(coro, loop=loop)
+            try:
+                loop.run_until_complete(task)
             except BaseException:
                 # run_until_complete doesn't get the result from exceptions
                 # that are not subclasses of `Exception`. Consume all

Basically the wrapper will attach the loop from the event_loop if get_event_loop fails because no current event loop is available.

[rsebille]

Hi,

I think the behavior is fine (second time that I look to the source of pytest-asyncio s don't take my word for it :D), but I would take it up a notch to "An asynchronous test function should run on the event loop that was used for its fixtures".

So I tried the following changes, and tests continue to pass but can't keep thinking that is probably too easy :).

diff --git a/pytest_asyncio/plugin.py b/pytest_asyncio/plugin.py
index 9860e04..08cf9d2 100644
--- a/pytest_asyncio/plugin.py
+++ b/pytest_asyncio/plugin.py
@@ -125,14 +125,18 @@ def pytest_pyfunc_call(pyfuncitem):
     if 'asyncio' in pyfuncitem.keywords:
         if getattr(pyfuncitem.obj, 'is_hypothesis_test', False):
             pyfuncitem.obj.hypothesis.inner_test = wrap_in_sync(
-                pyfuncitem.obj.hypothesis.inner_test
+                pyfuncitem.obj.hypothesis.inner_test,
+                _loop=pyfuncitem.funcargs['event_loop'],
             )
         else:
-            pyfuncitem.obj = wrap_in_sync(pyfuncitem.obj)
+            pyfuncitem.obj = wrap_in_sync(
+                pyfuncitem.obj,
+                _loop=pyfuncitem.funcargs['event_loop'],
+            )
     yield
 
 
-def wrap_in_sync(func):
+def wrap_in_sync(func, _loop):
     """Return a sync wrapper around an async function executing it in the
     current event loop."""
 
@@ -140,9 +144,9 @@ def wrap_in_sync(func):
     def inner(**kwargs):
         coro = func(**kwargs)
         if coro is not None:
-            task = asyncio.ensure_future(coro)
+            task = asyncio.ensure_future(coro, loop=_loop)
             try:
-                asyncio.get_event_loop().run_until_complete(task)
+                _loop.run_until_complete(task)
             except BaseException:
                 # run_until_complete doesn't get the result from exceptions
                 # that are not subclasses of `Exception`. Consume all

[simonfagerholm]

@rsebille The behavior and your comment differ I think:
In your change you always attach the loop from event_loop, which may or may not bethe same loop the fixtures run in.
I try to get the current loop first, enabling fixtures to change the loop if needed. As a fallback I use the event_loop, in case there is no loop (which should mean a fixture has set it to None as we know we started the event_loop-fixture before running fixtures).
Thus I think my original suggestion matches your comment more than the code you submitted.

[simonfagerholm]

@rsebille I forgot to add that I appreciate that you took time to look into this and any future support :)

[rsebille]

I totally assume (slaps ensues) that asynchronous fixtures were explicitly run in the event_loop event loop to avoid "different loop" error and that the plugin would not allow changing the event loop without overriding event_loop.

@simonfagerholm No problem, thank to you for taking the time on this.

[Tinche]

Thanks for this, I will look at this over the weekend!

[simonfagerholm]

@Tinche Thats great! In my digging I found out that the test is supposed to test for problems with pytest-aiohttp.
So I used the following as a test bench and discovered that it:

• passes on pytest-asyncio==0.10
• fails on pytest-asyncio==0.11
• passes on my current branch
• passes on my suggested addition to the branch fixing the unittest

As it fails on pytest-asyncio==0.11 and passes on this PR, it seems the unittest did not protect against that problem.

import pytest
from aiohttp import web


async def hello(request):
    return web.Response(body=b'Hello, world')


def create_app(loop):
    app = web.Application(loop=loop)
    app.router.add_route('GET', '/', hello)
    return app


async def test_hello(aiohttp_client):
    client = await aiohttp_client(create_app)
    resp = await client.get('/')
    assert resp.status == 200
    text = await resp.text()
    assert 'Hello, world' in text


@pytest.fixture
async def async_fixture():
    yield 'Hi from async_fixture()!'


@pytest.mark.asyncio
async def test_async_fixture_fixed(async_fixture):
    assert async_fixture == 'Hi from async_fixture()!'

============================= test session starts =============================
platform win32 -- Python 3.7.7, pytest-5.4.1, py-1.8.1, pluggy-0.13.1 -- python.exe
cachedir: .pytest_cache
rootdir: aiohttp_test
plugins: aiohttp-0.3.0, asyncio-0.11.0
collecting ... collected 2 items

test_aiohttp_error.py::test_hello[pyloop] 
test_aiohttp_error.py::test_async_fixture_fixed PASSED                         [ 50%]ERROR                    [100%]
test setup failed
args = (), kwargs = {}
request = <SubRequest 'async_fixture' for <Function test_async_fixture_fixed>>
setup = <function pytest_fixture_setup.<locals>.wrapper.<locals>.setup at 0x0000019B771C0678>
finalizer = <function pytest_fixture_setup.<locals>.wrapper.<locals>.finalizer at 0x0000019B771C0F78>

    def wrapper(*args, **kwargs):
        request = kwargs['request']
        if strip_request:
            del kwargs['request']
    
        gen_obj = generator(*args, **kwargs)
    
        async def setup():
            res = await gen_obj.__anext__()
            return res
    
        def finalizer():
            """Yield again, to finalize."""
            async def async_finalizer():
                try:
                    await gen_obj.__anext__()
                except StopAsyncIteration:
                    pass
                else:
                    msg = "Async generator fixture didn't stop."
                    msg += "Yield only once."
                    raise ValueError(msg)
            asyncio.get_event_loop().run_until_complete(async_finalizer())
    
        request.addfinalizer(finalizer)
>       return asyncio.get_event_loop().run_until_complete(setup())

venv\lib\site-packages\pytest_asyncio\plugin.py:102: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <asyncio.windows_events.WindowsSelectorEventLoopPolicy object at 0x0000019B771580C8>

    def get_event_loop(self):
        """Get the event loop for the current context.
    
        Returns an instance of EventLoop or raises an exception.
        """
        if (self._local._loop is None and
                not self._local._set_called and
                isinstance(threading.current_thread(), threading._MainThread)):
            self.set_event_loop(self.new_event_loop())
    
        if self._local._loop is None:
            raise RuntimeError('There is no current event loop in thread %r.'
>                              % threading.current_thread().name)
E           RuntimeError: There is no current event loop in thread 'MainThread'.

C:\Program Files\Python\Python37\lib\asyncio\events.py:644: RuntimeError

test_aiohttp_error.py::test_async_fixture_fixed ERROR                    [100%]
test_aiohttp_error.py:27 (test_async_fixture_fixed)
def finalizer():
        """Yield again, to finalize."""
        async def async_finalizer():
            try:
                await gen_obj.__anext__()
            except StopAsyncIteration:
                pass
            else:
                msg = "Async generator fixture didn't stop."
                msg += "Yield only once."
                raise ValueError(msg)
>       asyncio.get_event_loop().run_until_complete(async_finalizer())

venv\lib\site-packages\pytest_asyncio\plugin.py:99: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <asyncio.windows_events.WindowsSelectorEventLoopPolicy object at 0x0000019B771580C8>

    def get_event_loop(self):
        """Get the event loop for the current context.
    
        Returns an instance of EventLoop or raises an exception.
        """
        if (self._local._loop is None and
                not self._local._set_called and
                isinstance(threading.current_thread(), threading._MainThread)):
            self.set_event_loop(self.new_event_loop())
    
        if self._local._loop is None:
            raise RuntimeError('There is no current event loop in thread %r.'
>                              % threading.current_thread().name)
E           RuntimeError: There is no current event loop in thread 'MainThread'.

C:\Program Files\Python\Python37\lib\asyncio\events.py:644: RuntimeError


=================================== ERRORS ====================================

[simonfagerholm]

I also tested the examples in #157 and verified that this PR solves #157.

[Tinche]

Yeah, I guess that unittest from #64 was useless.

On @simonfagerholm's branch, and applied the linked diff (which I'm ok with). So I tried installing pytest-aiohttp to run the provided example, and having pytest-aiohttp installed breaks a bunch of our own tests. I guess that can't be avoided? It was this way back on 0.10.0 too.

If you could add a changelog entry, I'll release this, with thanks :)

[simonfagerholm]

@Tinche Pushed a commit of my diff above. Thanks for your help in the investigation, do you have any ideas on how to test the compatibility with aiohttp? I somewhat dislike the inclusion of aiohttp in the tests, but it might not be possible in other ways.
Maybe a "test suite" of compatibility with other packages including aiohttp, that can be separated from the other tests.

[Tinche]

Actually I don't really have a good idea. It would be ideal if they started depending on us, since I feel they are a higher level component...

[Tinche]

Thanks a lot @simonfagerholm !