-
-
Notifications
You must be signed in to change notification settings - Fork 431
fix: add a AsyncTestClientTransport for the AsyncTestClient #4142
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
base: main
Are you sure you want to change the base?
Changes from 12 commits
8f58dfe
7d756cb
87f20f5
ceeb839
5701857
c6a4b2a
1585099
28fa120
a507e1c
b1d0fcc
13367dd
4dffcb5
eec69c1
18fe44a
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 |
---|---|---|
@@ -1,6 +1,9 @@ | ||
from asyncio import get_event_loop | ||
|
||
import pytest | ||
|
||
from litestar.testing import TestClient | ||
from litestar import Litestar, get | ||
from litestar.testing import AsyncTestClient, TestClient | ||
from litestar.testing.life_span_handler import LifeSpanHandler | ||
from litestar.types import Receive, Scope, Send | ||
|
||
|
@@ -35,3 +38,17 @@ async def app(scope: Scope, receive: Receive, send: Send) -> None: | |
handler = LifeSpanHandler(TestClient(app)) | ||
await handler.wait_shutdown() | ||
handler.close() | ||
|
||
|
||
async def test_multiple_clients_event_loop() -> None: | ||
@get("/") | ||
def return_loop_id() -> dict: | ||
return {"loop_id": id(get_event_loop())} | ||
|
||
app = Litestar(route_handlers=[return_loop_id]) | ||
|
||
async with AsyncTestClient(app) as client_1, AsyncTestClient(app) as client_2: | ||
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. This should even be the same loop with different client instances, right? 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 think so, this added test is just the mcve from the issue and it fails before the changes, is there something I should add ? 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 realize the diff is overly complicate, the PR is simple: before AsyncTestClient used TestClientTransport hence the issue. 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.
Yeah, maybe a test for the transport in particular that shows the client always uses 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. actually if this test was using TestClient instead of AsyncTestClient it wouldn't pass as
by different client instances you meant a AsyncTestClient and a TestClient ? 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. No I mean different instances |
||
response_1 = await client_1.get("/") | ||
response_2 = await client_2.get("/") | ||
|
||
assert response_1.json() == response_2.json() # FAILS |
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.
With this refactoring done, I think you actually don't even need 2 transports. The sync version could just do
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 benefit of having 2 transports is that they respectively inherit their httpx counterparts so I think it's a little bit more mypy friendly but I may be wrong
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.
Okay, that makes sense. Maybe could still move the request handling fn into the common base class?