Opened 3 weeks ago
Last modified 3 weeks ago
#36859 assigned New feature
SimpleTestCase.assertContains can't be called multiple times on a streaming response
| Reported by: | Baptiste Mispelon | Owned by: | Baptiste Mispelon |
|---|---|---|---|
| Component: | Testing framework | Version: | 6.0 |
| Severity: | Normal | Keywords: | assertContains, streaming |
| Cc: | Carlton Gibson | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
I recently got an unexpected test failure when using SimpleTestCase.assertContains():
def test_some_view(self): response = self.client.get("/some/url/") self.assertContains(response, "something") self.assertContains(response, "another thing") # This failed
Even though my view at /some/url/ was returning a response that did include both "something" and "another thing", the test was failing on the second assertion. After some confusion, I eventually figured out that the cause of this failure was that the view was returning a StreamingHttpResponse.
This failure makes sense if you understand the internals of streaming responses, but I'd argue that the test code shouldn't have to care about the nature of the response, and that repeated calls to assertContains should work transparently.
As far as I can tell, this behavior is not documented (one way or another) and has always been present (see 82b3e6ffcb9d810cc0e3ec27d25f89ce1fd525e0).
(PR with test case and tentative fix will be attached to this ticket soon)
Change History (6)
comment:1 by , 3 weeks ago
comment:2 by , 3 weeks ago
| Cc: | added |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
| Type: | New feature → Bug |
Thank you Baptiste, I think it makes sense to expect multiple calls of assertContains to work on a streamed response.
follow-up: 4 comment:3 by , 3 weeks ago
I'm not sure about calling this a bug. It seems very much a new feature. — The existing behaviour is long standing, and will be expected by anyone who's used streaming responses over time. So, if we are going to change it, it would merit a release note, etc.
I'd argue that the test code shouldn't have to care about the nature of the response...
Yeah. Gah. Streaming responses are their own thing. Our test probably should know it's dealing with a streaming response, and perform extra checks accordingly. Collecting the content into a variable and then asserting on that, would be pretty standard.
But 🤷 — I don't have a particular objection if it doesn't break anything. (Only commenting because I was CC'd.)
comment:4 by , 3 weeks ago
| Type: | Bug → New feature |
|---|
Replying to Carlton Gibson:
But 🤷 — I don't have a particular objection if it doesn't break anything. (Only commenting because I was CC'd.)
Thank you Carlton, I cc'd you because I was very keen to knowing your view on this matter. Happy to restore the New Feature type.
comment:6 by , 3 weeks ago
On the bug vs. feature, I'd argue for the latter on the basis that the current behaviour was neither tested nor documented.
I'll also note that assertContains() already handles streaming responses differently than non-streaming ones: https://github.com/django/django/blob/07a16407452f5b62594661ae7ae589eca8cccd4d/django/test/testcases.py#L575
I've added release notes to my PR.
PR here: https://github.com/django/django/pull/20527