Opened 3 weeks ago

Last modified 41 hours ago

#37205 assigned Cleanup/optimization

Replace assertIsInstance HttpResponse test assertions as do not fail if result is a subclass

Reported by: Jonathan Biemond Owned by: Jonathan Biemond
Component: Core (Other) Version: dev
Severity: Normal Keywords: tests, internal
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

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:

     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

Change History (4)

in reply to:  description comment:1 by Jonathan Biemond, 3 weeks ago

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.

     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)

comment:2 by Sarah Boyce, 3 weeks ago

Component: Uncategorized → Core (Other)
Summary: Tests asserting result isinstance of HttpResponse do not fail if result is a subclass → Replace assertIsInstance HttpResponse test assertions as do not fail if result is a subclass
Triage Stage: Unreviewed → Accepted

Agreed, thank you!

comment:3 by Jonathan Biemond, 3 weeks ago

Has patch: set

Pull request submitted! ​https://github.com/django/django/pull/21591

comment:4 by blighj, 41 hours ago

Triage Stage: Accepted → Ready for checkin
Note: See TracTickets for help on using tickets.
Back to Top