Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 byanyio
#1155Replace
pytest-asyncio
marker byanyio
#1155Changes from 5 commits
69e81e8
7223d4d
31fef06
cd3d842
f3eea0a
0d902ca
362c03a
fdb645c
3100055
466389f
900cf7f
d00c026
873217d
4be1085
bdfc9f1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@euri10 this is introduced on this PR, jfyk
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.
the corresponding pytest-asyncio fixture
event_loop
is herehttps://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 ?
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.
We do, the
contextlib.closing
callsloop.close()
. Is that what you mean? 🤔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.
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 comment
The 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,
is it an anti-pattern, idk :)
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.
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
MockLoop
doing something like this, is that what you mean ?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.
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?
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.
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 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 reviewThere 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.
as promised, here's the followup: #1520