Replace pytest-asyncio marker by anyio

[graingert]

uvicorn currently depends on both pytest-asyncio and anyio in tests, so we can save a dep by swapping pytest-asyncio for anyio.

[florimondmanca]

It doesn't seem like we "already" depend on anyio, do we? We're adding it as a new dependency here.

If so, would the rationale better be to prepare the ground for future trio support?

[graingert]

It doesn't seem like we "already" depend on anyio, do we? We're adding it as a new dependency here.

no the anyio dep is already present

[Kludex]

@graingert where is it present? 🤔

[graingert]

@graingert where is it present? 🤔

The test suite depends on httpx depends on anyio

[Kludex]

@euri10 Please check if you agree with this PR as well.

I've removed all the unnecessary changes from this PR, and focused on what it proposes, jfyk @graingert

[Kludex]

@euri10 this is introduced on this PR, jfyk

[euri10]

the corresponding pytest-asyncio fixture event_loop is here

https://github.com/pytest-dev/pytest-asyncio/blob/4c7da65d6fcf9d725eccba28ad1ed2083524ee16/tests/async_fixtures/test_async_fixtures_with_finalizer.py#L19-L25

With this change, we don't close the loop after the yield, I'm surprised then pytest does not raise Resources not closed warnings, would be probably better to do it anyways so we dont get caught further down the road with such warning that are insanely hard to debug

and the scope is different, I'm not sure we need a loop for each test in that module, do we ?

[Kludex]

With this change, we don't close the loop after the yield ...

We do, the contextlib.closing calls loop.close(). Is that what you mean? 🤔

[agronholm]

What is this fixture even used for here? It feels like an anti-pattern.

[euri10]

We do, the contextlib.closing calls loop.close(). Is that what you mean? thinking

yes, well if it passes tests then the ressources might be closed properly !

What is this fixture even used for here? It feels like an anti-pattern.

iirc we use this pytest_asyncio fixture only in those "protocol" tests to create a bunch of mocks, mocked loop, mocked transport and then we create a protocol we interact with and look at its buffer,
is it an anti-pattern, idk :)

[euri10]

I might be misunderstanding something, we use pytest-asyncio event_loop fixture, which is to me a real event loop

@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()

we could avoid using the MockLoop doing something like this, is that what you mean ?

@@ -175,18 +175,15 @@ class MockTask:
 
 @contextlib.contextmanager
 def get_connected_protocol(app, protocol_cls, event_loop, **kwargs):
-    loop = MockLoop(event_loop)
-    asyncio._set_running_loop(loop)
     transport = MockTransport()
     config = Config(app=app, **kwargs)
     server_state = ServerState()
-    protocol = protocol_cls(config=config, server_state=server_state, _loop=loop)
+    protocol = protocol_cls(config=config, server_state=server_state, _loop=event_loop)
     protocol.connection_made(transport)
     try:
         yield protocol
     finally:
         protocol.loop.close()
-        asyncio._set_running_loop(None)
 
 
 @pytest.mark.parametrize("protocol_cls", HTTP_PROTOCOLS)
@@ -195,7 +192,8 @@ def test_get_request(protocol_cls, event_loop):
 
     with get_connected_protocol(app, protocol_cls, event_loop) as protocol:
         protocol.data_received(SIMPLE_GET_REQUEST)
-        protocol.loop.run_one()
+        tasks = asyncio.all_tasks(loop=event_loop)
+        protocol.loop.run_until_complete(*tasks)
         assert b"HTTP/1.1 200 OK" in protocol.transport.buffer
         assert b"Hello, world" in protocol.transport.buffer

[agronholm]

The loop is closed twice here (once in the fixture, once in get_connected_protocol(). Looks like the latter could be made into a fixture instead.

Does the protocol create tasks?

[graingert]

yes

uvicorn/uvicorn/protocols/http/h11_impl.py

Lines 234 to 237 in 4be1085

	task = self.loop.create_task(self.cycle.run_asgi(app))
	task.add_done_callback(self.tasks.discard)
	self.tasks.add(task)

[graingert]

@euri10 I just pushed a fix to make MockLoop wrap the currently running loop get_connected_protocol can be made into a plain function but it makes the PR more difficult to review

[graingert]

as promised, here's the followup: #1520

[euri10]

We do, the contextlib.closing calls loop.close(). Is that what you mean? thinking

yes, well if it passes tests then the ressources might be closed properly !

What is this fixture even used for here? It feels like an anti-pattern.

iirc we use this pytest_asyncio fixture only in those "protocol" tests to create a bunch of mocks, mocked loop, mocked transport and then we create a protocol we interact with and look at its buffer,
is it an anti-pattern, idk :)

discussion still happening

[euri10]

I like those changes

it's just test_http whose diff is larger, but in any case I find it way cleaner than before.

I think we can give it a shot this way, all good on my side, if you prefer to separate the rework on the MockLoop from the anyio switch I'd be ok too but this way is also ok to me :)

[florimondmanca]

LGTM 👍

Looks like I had already viewed this back when it was initially opened. Changes outside test_http.py are straightforward. I reviewed that file as well and the changes are in fact pretty systematic too, so giving my LGTM, though I haven't checked if there's nitty-gritty discussion still ongoing. Nice one!

[florimondmanca]

Should we add an explicit test dependency on anyio, since we now explicitly import it?

anyio==3.6.1