django-stubs: Fix HttpResponse missing attribute error.

[PIG208]

This is a part of #18777.

As Django monkey patch attributes on the HttpResponse the test client returns. This does not really make the patched attributes available on the response object of the test client, but instead avoids accessing these attributes by taking a detour.

It refactors away response.json() with response.assert_json_success and a new helper response.assert_json_error_data manually and with regexes.

A flag use_zulip_api_format is added for response.assert_json_error_data to properly test test_scim.py.

Self-review checklist

• Self-reviewed the changes for clarity and maintainability
  (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

• Explains differences from previous plans (e.g., issue description).
• Highlights technical choices and bugs encountered.
• Calls out remaining decisions and concerns.
• Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

• Each commit is a coherent idea.
• Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

• Visual appearance of the changes.
• Responsiveness and internationalization.
• Strings and tooltips.
• End-to-end functionality of buttons, interactions and flows.
• Corner cases, error conditions, and easily imagined bugs.

[andersk]

The use_zulip_error_format flag is not worth its complexity. The five tests that set it to False would be simpler and more readable without it.

-        response_dict = self.assert_json_error_data(result, use_zulip_error_format=False)
+        self.assertEqual(result.status_code, 400)
         self.assertEqual(
-            response_dict,
+            orjson.loads(result.content),
             {
                 "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"],
                 "detail": "Must specify name.formatted, name.givenName or name.familyName when creating a new user",
                 "status": 400,
             },
         )

[timabbott]

I think result_dict or response_dict would probably be a better conventional variable name for these objects than success.

[andersk]

I’m fine with success or response_dict I guess, but result_dict is strictly worse. (Requests have responses; computations have results. We have a lot of result variables of type HttpResponse that should be named response.)

My justification for success is the same as the one in the OpenAPI schema, where JsonResponse is the type of all API responses and JsonSuccess is the more specific subtype of all successful API responses.

[timabbott]

response_dict WFM. I don't like success because the object it a dictionary containing the data returned in the response -- sans headers and the like. success_dict would be fine too, but I think it's cleaner to have the name directly associated with response.

Also agreed that it would be a nice refactor to s/result/response/ in the tests.

[PIG208]

The PR is updated to go with response_dict.

[andersk]

I noticed that django-stubs actually knows about response.json(). We’re just not declaring our ZulipTestCase.client_get et al. wrappers with the type that knows about it, _MonkeyPatchedWSGIResponse. (It’s not present on all HttpResponse objects, only the ones returned by the test client.)

[PIG208]

I noticed that django-stubs actually knows about response.json(). We’re just not declaring our ZulipTestCase.client_get et al. wrappers with the type that knows about it, _MonkeyPatchedWSGIResponse. (It’s not present on all HttpResponse objects, only the ones returned by the test client.)

Yes. We might need to change the return type of the wrapped test client to _MonkeyPatchedWSGIResponse so that it's properly typed. This requires us to import it with django-stubs installed. This causes type errors wherever we have HttpResponse in our test code, and it will be a larger migration than this one.

[andersk]

You can actually write this today, without django-stubs installed (in which case mypy will treat it as Any, like it treats HttpResponse now).

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from django.test.client import _MonkeyPatchedWSGIResponse

def client_get(…) -> "_MonkeyPatchedWSGIResponse"
    …

Even after we do install django-stubs, we’ll still need that TYPE_CHECKING conditional, since _MonkeyPatchedWSGIResponse doesn’t exist in the real django at runtime.

[timabbott]

Makes sense. Though I think it's probably a cleanup to the tests to not have tons of them doing the .json() access directly, so we probably still want to do a version of this PR? Or are you proposing we don't want to do that?

[andersk]

This is still basically a fine cleanup. If we have an assert_json_success we might as well use its return value.

The commit has two changes, though: use the assert_json_success return value, and add assert_json_error_data. The commit message only mentions the latter. (Unintentional squash perhaps?) It should be split into two commits. Or perhaps assert_json_error_data should be dropped—it has only two callers.

[timabbott]

This is great, merged, thanks @PIG208!