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

Replace pytest-asyncio marker by anyio #1155

Merged
merged 15 commits into from Jun 17, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions requirements-pypa.in
@@ -0,0 +1,3 @@
pip
setuptools
wheel
20 changes: 20 additions & 0 deletions requirements-pypa.txt
@@ -0,0 +1,20 @@
#
# This file is autogenerated by pip-compile with python 3.9
# To update, run:
#
# pip-compile --allow-unsafe --generate-hashes --output-file=requirements-pypa.txt requirements-pypa.in
#
wheel==0.37.0 \
--hash=sha256:21014b2bd93c6d0034b6ba5d35e4eb284340e09d63c59aef6fc14b0f346146fd \
--hash=sha256:e2ef7239991699e3355d54f8e968a21bb940a1dbf34a4d226741e64462516fad
# via -r requirements-pypa.in

# The following packages are considered to be unsafe in a requirements file:
pip==21.2.4 \
--hash=sha256:0eb8a1516c3d138ae8689c0c1a60fde7143310832f9dc77e11d8a4bc62de193b \
--hash=sha256:fa9ebb85d3fd607617c0c44aca302b1b45d87f9c2a1649b46c26167ca4296323
# via -r requirements-pypa.in
setuptools==57.4.0 \
--hash=sha256:6bac238ffdf24e8806c61440e755192470352850f3419a52f26ffe0a1a64f465 \
--hash=sha256:a49230977aa6cfb9d933614d2f7b79036e9945c4cdd7583163f4e920b83418d6
# via -r requirements-pypa.in
3 changes: 2 additions & 1 deletion requirements.txt
Expand Up @@ -21,7 +21,6 @@ trustme
cryptography
coverage
httpx>=0.18.2
pytest-asyncio==0.14.*
async_generator; python_version < '3.7'


Expand All @@ -31,3 +30,5 @@ mkdocs-material

# py3.10 workarounds
https://github.com/aaugustin/websockets/archive/4e1dac362a3639b9cf0e5bcf382601e6e32cfede.tar.gz#egg=websockets; python_version >= '3.10'

anyio @ git+https://github.com/graingert/anyio.git@b907c72ffae4e89c09fc9d7d81ff8c45fee43ccd
Kludex marked this conversation as resolved.
Show resolved Hide resolved
7 changes: 4 additions & 3 deletions scripts/install
Expand Up @@ -10,9 +10,10 @@ set -x

if [ -z "$GITHUB_ACTIONS" ]; then
"$PYTHON" -m venv "$VENV"
PIP="$VENV/bin/pip"
PIP="$VENV/bin/python -m pip"
else
PIP="pip"
PIP="$PYTHON -m pip"
fi

"$PIP" install -r "$REQUIREMENTS"
$PIP install -r "requirements-pypa.txt"
$PIP install -r "$REQUIREMENTS"
6 changes: 6 additions & 0 deletions tests/conftest.py
@@ -1,4 +1,5 @@
import ssl
from typing import Dict, Tuple

import pytest
import trustme
Expand Down Expand Up @@ -128,3 +129,8 @@ def reload_directory_structure(tmp_path_factory: pytest.TempPathFactory):
ext_file.touch()

yield root


@pytest.fixture
def anyio_backend() -> Tuple[str, Dict[str, object]]:
return ("asyncio", {})
8 changes: 4 additions & 4 deletions tests/middleware/test_debug.py
Expand Up @@ -4,7 +4,7 @@
from uvicorn.middleware.debug import DebugMiddleware


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_debug_text():
async def app(scope, receive, send):
raise RuntimeError("Something went wrong")
Expand All @@ -24,7 +24,7 @@ async def app(scope, receive, send):
assert "RuntimeError" in response.text


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_debug_html():
async def app(scope, receive, send):
raise RuntimeError("Something went wrong")
Expand All @@ -43,7 +43,7 @@ async def app(scope, receive, send):
assert "RuntimeError" in response.text


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_debug_after_response_sent():
async def app(scope, receive, send):
await send({"type": "http.response.start", "status": 204, "headers": []})
Expand All @@ -63,7 +63,7 @@ async def app(scope, receive, send):
assert response.content == b""


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_debug_not_http():
async def app(scope, send, receive):
raise RuntimeError("Something went wrong")
Expand Down
12 changes: 6 additions & 6 deletions tests/middleware/test_logging.py
Expand Up @@ -27,7 +27,7 @@ async def app(scope, receive, send):
await send({"type": "http.response.body", "body": b"", "more_body": False})


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_trace_logging(caplog):
config = Config(app=app, log_level="trace")
with caplog_for_logger(caplog, "uvicorn.asgi"):
Expand All @@ -46,7 +46,7 @@ async def test_trace_logging(caplog):
assert "ASGI [2] Completed" in messages.pop(0)


@pytest.mark.asyncio
@pytest.mark.anyio
@pytest.mark.parametrize("http_protocol", [("h11"), ("httptools")])
async def test_trace_logging_on_http_protocol(http_protocol, caplog):
config = Config(app=app, log_level="trace", http=http_protocol)
Expand All @@ -64,7 +64,7 @@ async def test_trace_logging_on_http_protocol(http_protocol, caplog):
assert any(" - HTTP connection lost" in message for message in messages)


@pytest.mark.asyncio
@pytest.mark.anyio
@pytest.mark.parametrize("ws_protocol", [("websockets"), ("wsproto")])
async def test_trace_logging_on_ws_protocol(ws_protocol, caplog):
async def websocket_app(scope, receive, send):
Expand Down Expand Up @@ -95,7 +95,7 @@ async def open_connection(url):
assert any(" - WebSocket connection lost" in message for message in messages)


@pytest.mark.asyncio
@pytest.mark.anyio
@pytest.mark.parametrize("use_colors", [(True), (False), (None)])
async def test_access_logging(use_colors, caplog):
config = Config(app=app, use_colors=use_colors)
Expand All @@ -113,7 +113,7 @@ async def test_access_logging(use_colors, caplog):
assert '"GET / HTTP/1.1" 204' in messages.pop()


@pytest.mark.asyncio
@pytest.mark.anyio
@pytest.mark.parametrize("use_colors", [(True), (False)])
async def test_default_logging(use_colors, caplog):
config = Config(app=app, use_colors=use_colors)
Expand All @@ -134,7 +134,7 @@ async def test_default_logging(use_colors, caplog):
assert "Shutting down" in messages.pop(0)


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_unknown_status_code(caplog):
async def app(scope, receive, send):
assert scope["type"] == "http"
Expand Down
4 changes: 2 additions & 2 deletions tests/middleware/test_message_logger.py
Expand Up @@ -5,7 +5,7 @@
from uvicorn.middleware.message_logger import MessageLoggerMiddleware


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_message_logger(caplog):
async def app(scope, receive, send):
await receive()
Expand All @@ -27,7 +27,7 @@ async def app(scope, receive, send):
assert sum(["ASGI [1] Raised exception" in message for message in messages]) == 0


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_message_logger_exc(caplog):
async def app(scope, receive, send):
raise RuntimeError()
Expand Down
6 changes: 3 additions & 3 deletions tests/middleware/test_proxy_headers.py
Expand Up @@ -20,7 +20,7 @@ async def app(
await response(scope, receive, send)


@pytest.mark.asyncio
@pytest.mark.anyio
@pytest.mark.parametrize(
("trusted_hosts", "response_text"),
[
Expand Down Expand Up @@ -50,7 +50,7 @@ async def test_proxy_headers_trusted_hosts(
assert response.text == response_text


@pytest.mark.asyncio
@pytest.mark.anyio
@pytest.mark.parametrize(
("trusted_hosts", "response_text"),
[
Expand Down Expand Up @@ -87,7 +87,7 @@ async def test_proxy_headers_multiple_proxies(
assert response.text == response_text


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_proxy_headers_invalid_x_forwarded_for() -> None:
app_with_middleware = ProxyHeadersMiddleware(app, trusted_hosts="*")
async with httpx.AsyncClient(
Expand Down
8 changes: 4 additions & 4 deletions tests/middleware/test_wsgi.py
Expand Up @@ -49,7 +49,7 @@ def return_exc_info(environ: Environ, start_response: StartResponse) -> List[byt
return [output]


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_wsgi_get() -> None:
app = WSGIMiddleware(hello_world)
async with httpx.AsyncClient(app=app, base_url="http://testserver") as client:
Expand All @@ -58,7 +58,7 @@ async def test_wsgi_get() -> None:
assert response.text == "Hello World!\n"


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_wsgi_post() -> None:
app = WSGIMiddleware(echo_body)
async with httpx.AsyncClient(app=app, base_url="http://testserver") as client:
Expand All @@ -67,7 +67,7 @@ async def test_wsgi_post() -> None:
assert response.text == '{"example": 123}'


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_wsgi_exception() -> None:
# Note that we're testing the WSGI app directly here.
# The HTTP protocol implementations would catch this error and return 500.
Expand All @@ -77,7 +77,7 @@ async def test_wsgi_exception() -> None:
await client.get("/")


@pytest.mark.asyncio
@pytest.mark.anyio
async def test_wsgi_exc_info() -> None:
# Note that we're testing the WSGI app directly here.
# The HTTP protocol implementations would catch this error and return 500.
Expand Down
6 changes: 6 additions & 0 deletions tests/protocols/test_http.py
Expand Up @@ -185,6 +185,12 @@ def get_connected_protocol(app, protocol_cls, event_loop, **kwargs):
asyncio._set_running_loop(None)


@pytest.fixture
def event_loop():
with contextlib.closing(asyncio.new_event_loop()) as loop:
yield loop
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

@euri10 this is introduced on this PR, jfyk

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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? 🤔

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@graingert graingert Jun 14, 2022

Choose a reason for hiding this comment

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

yes

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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

Copy link
Member Author

Choose a reason for hiding this comment

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

as promised, here's the followup: #1520



@pytest.mark.parametrize("protocol_cls", HTTP_PROTOCOLS)
def test_get_request(protocol_cls, event_loop):
app = Response("Hello, world", media_type="text/plain")
Expand Down