#34342 closed Bug (fixed)
The test async_client is not consuming async StreamingResponse generators properly
Reported by: | Alexandre Spaeth | Owned by: | Alexandre Spaeth |
---|---|---|---|
Component: | Testing framework | Version: | 4.2 |
Severity: | Release blocker | Keywords: | async |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
It seems that the way the async_client
behaves with StreamingResponse
makes it not play nicely with the fixes done in 4.2 to support async generators.
I think the issue is in closing_iterator_wrapper
: we are naively calling sync_to_async
on it, but I don’t believe this is possible with a proper async generator.
Here I tried to write a test to reproduce the issue:
https://github.com/Alexerson/django/commit/b05e524bfadfb0d553438d0375bad0009439fade
Looking at the codebase, I’m also believing that there are other parts that might break, for instance assertContains
seems to expect a well behaved sync generator.
Change History (8)
comment:2 by , 21 months ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
Thanks for the report. Eyeballing it looks correct. I will investigate this morning.
Bug in 0bd2c0c9015b53c41394a1c0989afbfd94dc2830
I’m also believing that there are other parts that might break, for instance assertContains seems to expect a well behaved sync generator.
Yes, maybe. There are a few places where the testing assertion and context managers aren't async compatible (yet). We can address those separately.
comment:3 by , 21 months ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:4 by , 21 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
I pushed my observations I tiny bit further and I think something as simple as this is a proper fix for the issue:
https://github.com/django/django/pull/16559
Also, I confirm the issue for assertContains, but it could be harder to fix, because we will need an async version of assertContains just for this, and this is probably overkill. In a case like that, some documentations might be enough to solve the issue.