#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 , 22 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 , 22 months ago
| Has patch: | set |
|---|---|
| Owner: | changed from to |
| Status: | new → assigned |
comment:4 by , 22 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/Alexerson/django/commit/0f5439d158278393584904f4a88846dae27dad92
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.