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
Changes from 12 commits
69e81e8
7223d4d
31fef06
cd3d842
f3eea0a
0d902ca
362c03a
fdb645c
3100055
466389f
900cf7f
d00c026
873217d
4be1085
bdfc9f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -189,6 +189,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 | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @euri10 this is introduced on this PR, jfyk There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the corresponding pytest-asyncio fixture 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We do, the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes, well if it passes tests then the ressources might be closed properly !
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, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
we could avoid using the @@ -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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The loop is closed twice here (once in the fixture, once in Does the protocol create tasks? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes uvicorn/uvicorn/protocols/http/h11_impl.py Lines 234 to 237 in 4be1085
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an explicit test dependency on
anyio
, since we now explicitly import it?