-
-
Notifications
You must be signed in to change notification settings - Fork 421
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?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4142 +/- ##
==========================================
- Coverage 98.37% 98.35% -0.02%
==========================================
Files 348 348
Lines 15845 15861 +16
Branches 1749 1750 +1
==========================================
+ Hits 15587 15600 +13
- Misses 123 127 +4
+ Partials 135 134 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/4142 |
any idea how to cover this ? I dont understand why it pops now though |
|
||
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 comment
The 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 comment
The 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 comment
The 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.
so I created a BaseClientTransport that essentially takes care of request/response and added a ASGITestClientTransport that inherits httpx ASGIBaseTransport and overrides handle_async_request while TestClientTransport overrides handle_request, the BaseClientTransport is what's common between the 2 transports.
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 think so, this added test is just the mcve from the issue and it fails before the changes, is there something I should add ?
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 comment
The 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 start_blocking_portal
creates a new event loop.
This should even be the same loop with different client instances, right?
by different client instances you meant a AsyncTestClient and a TestClient ?
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.
No I mean different instances AsyncClient
. But actually, I think the test should probably just show that the client uses the running loop from the context it's created in.
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
with self.client.portal() as portal:
return portal.call(self.handle_async_request, request)
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?
I don't think the behaviour is fully fixed yet, as the async def test_lifespan_loop() -> None:
mock = MagicMock()
@contextlib.asynccontextmanager
async def lifespan(app: Litestar) -> AsyncGenerator[None, None]:
mock(asyncio.get_running_loop())
yield
app = Litestar(lifespan=[lifespan])
async with AsyncTestClient(app):
pass
mock.assert_called_once_with(asyncio.get_running_loop()) |
Description
Closes
fixes #1920