[Django] #37205: Tests asserting result isinstance of HttpResponse do not fail if result is a subclass

[Django]

#37205: Tests asserting result isinstance of HttpResponse do not fail if result is
a subclass
-------------------------------------+-------------------------------------
Reporter: Jonathan | Owner: Jonathan Biemond
Biemond |
Type: | Status: assigned
Cleanup/optimization |
Component: | Version: dev
Uncategorized |
Severity: Normal | Keywords: tests, internal
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Tests in the Django test suite that use `self.assertIsInstance()` to check
if a response is an `HttpResponse` will pass for any object that is a
subclass of `HttpResponse`, such as `HttpResponseNotAllowed`. This can
lead to uncaught errors when the expectation is that the result is only
and exactly `HttpResponse`.

For example the following test will continue to pass after these changes:
{{{#!div
{{{#!diff
def test_require_http_methods_methods(self):
- @require_http_methods(["GET", "PUT"])
+ @require_http_methods(["PUT"])
def my_view(request):
return HttpResponse("OK")

request = HttpRequest()
request.method = "GET"
self.assertIsInstance(my_view(request), HttpResponse)
request.method = "POST"
self.assertIsInstance(my_view(request), HttpResponseNotAllowed)
}}}
}}}

Both responses are `HttpResponseNotAllowed` and both assert statements are
true.
Majority of occurrences may be found here:
https://github.com/django/django/blob/main/tests/decorators/test_http.py
--
Ticket URL: <https://code.djangoproject.com/ticket/37205>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37205: Tests asserting result isinstance of HttpResponse do not fail if result is
a subclass
-------------------------------------+-------------------------------------

Reporter: Jonathan Biemond | Owner: Jonathan
Type: | Biemond
Cleanup/optimization | Status: assigned
Component: Uncategorized | Version: dev
Severity: Normal | Resolution:

Keywords: tests, internal | Triage Stage:

| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jonathan Biemond):

I propose we explicitly assert against the HTTP status code instead. This
has the downside of breaking tests if a HttpResponse's status code ever
changes, but that seems unlikely.
{{{#!div
{{{#!diff
def test_require_http_methods_methods(self):

@require_http_methods(["GET", "PUT"])

def my_view(request):
return HttpResponse("OK")

request = HttpRequest()
request.method = "GET"

- self.assertIsInstance(my_view(request), HttpResponse)
+ self.assertEqual(my_view(request).status_code, HTTPStatus.OK)
request.method = "POST"
- self.assertIsInstance(my_view(request), HttpResponseNotAllowed)
+ self.assertEqual(my_view(request).status_code,
HTTPStatus.METHOD_NOT_ALLOWED)

}}}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37205#comment:1>