Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support content attribute for a Testing Response. #968

Merged
merged 1 commit into from
May 26, 2022
Merged

Support content attribute for a Testing Response. #968

merged 1 commit into from
May 26, 2022

Conversation

quinox
Copy link
Contributor

@quinox quinox commented May 25, 2022 •
edited
Loading

I have made yet another thing!

content is a documented attribute of a testing response which isn't part of the parent HttpResponseBase, thus we need to add it to our MonkeyPatched types.

Without this attribute the example code from the documentation will fail typing:

>>> from django.test import Client
>>> c = Client()
>>> response = c.post('/login/', {'username': 'john', 'password': 'smith'})
>>> response.status_code
200
>>> response = c.get('/customer/details/')
>>> response.content
b'<!DOCTYPE html...'

This worked on 1.10.1 but broke after upgrading to 1.11.0 in my own code:

project/status/tests.py:28:26: error: "_MonkeyPatchedWSGIResponse" has no attribute "content"; maybe "context"?  [attr-defined]
            self.assertEqual(response.content, b'example_fixed_value EXAMPLE_VALUE\n')

Related issues

None that I am aware of.

@quinox
Support content attribute for a Testing Response.
a37fab6 
`content` is a documented attribute of a testing response which isn't part
of the parent HttpResponseBase:

https://docs.djangoproject.com/en/4.0/topics/testing/tools/#testing-responses
@intgr
Copy link
Collaborator

intgr commented May 25, 2022

This was discussed with @sterliakov in #909 (comment)

TL;DR: Use the getvalue() method instead of content, which is more universal.

The response class hierarchy is the following:

HttpResponseBase
├── HttpResponse
│   ├── HttpResponseRedirectBase
│   └── JsonResponse
└── StreamingHttpResponse
    └── FileResponse

Only HttpResponse defines the content method, but views can also return StreamingHttpResponse, which doesn't have it. Therefore accessing content is not strictly type-safe. However, both subclasses of HttpResponseBase do define the getvalue() method.

Although I haven't verified how StreamingHttpResponse behaves with the test client. Maybe it's monkey patched after all, like other attributes are?

@blueyed blueyed mentioned this pull request May 25, 2022
Closed
@intgr
Copy link
Collaborator

intgr commented May 25, 2022 •
edited
Loading

Although I haven't verified how StreamingHttpResponse behaves with the test client.

Indeed I checked, and the content attribute does not work.

def dummy_view(_request: HttpRequest) -> HttpResponseBase:
    return FileResponse(__file__)


urlpatterns: list[URLPattern] = [
    path("dummy", dummy_view),
]


@pytest.mark.urls("tests_module")
def test_dummy(client: Client) -> None:
    resp = client.get("/dummy")
    print(resp.content)

Fails with

AttributeError: This FileResponse instance has no `content` attribute. Use `streaming_content` instead.
at .../site-packages/django/http/response.py:437: AttributeError

@intgr
Copy link
Collaborator

intgr commented May 25, 2022

So arguably Django documentation should be updated to recommend getvalue().

@quinox
Copy link
Contributor Author

quinox commented May 25, 2022 •
edited
Loading

This was discussed with @sterliakov in #909 (comment)

TL;DR: Use the getvalue() method instead of content, which is more universal.

Thanks for the details, I'll update my code accordingly.

It's a bit unfortunate that code suggested by the official Django documentation fails on this. Is the MyPy plugin setup flexible enough to let django-stubs give a suggestion in this case?

@intgr
Copy link
Collaborator

intgr commented May 25, 2022

It's a bit unfortunate that code suggested by the official Django documentation fails on this.

Indeed. After thinking a bit, I'm not opposed to merging this. I'd like to see more opinions.

The lack of content attribute may cause unnecessary churn for users who don't really care about the distinction, as the vast majority of views don't use streaming response. This only affects test code anyway, and Django provides a helpful error message when they try accessing this attribute on a streaming response.

@fizyk
Copy link

fizyk commented May 25, 2022

I'm here because the lack of both url and content properties on the MonkeyPatched type stub bit me.

If the response type would be a Union of various MonkeyPatched types (HTTP, Stream etc), then all the end developer would have to do to settle would be to assert a specific type for mypy to recognize accordingly.

It would be kind of both typesafe and almost in the spirit of Django's docs.

@quinox
Copy link
Contributor Author

quinox commented May 26, 2022

After learning why the current behavior is the way it is I personally I agree with @intgr's reasoning: having the MonkeyPatched objects have attributes that are only true in 99% of the cases is fine since the other 1% will result at runtime in a helpful AttributeError, and it will only affect testing code (which won't affect production, and if run automatically won't stay out-of-sight).

For me MyPy is doing its work on a best-effort basis, and trading type-correctness for usability seems a sensible thing in this particular case. Having to constantly assert the outcome of client.post is sub-optimal for me (lots of boilerplate code), getvalue() is an adequate work-around if we don't want to merge this PR.

@sobolevn
Copy link
Member

This also hit me:

tests/test_apps/test_main/test_views/test_index_view.py:10: error: "_MonkeyPatchedWSGIResponse" has no attribute "content"; maybe "context"?
tests/test_apps/test_main/test_views/test_index_view.py:18: error: "_MonkeyPatchedWSGIResponse" has no attribute "content"; maybe "context"?
tests/test_server/test_urls.py:39: error: "_MonkeyPatchedWSGIResponse" has no attribute "content"; maybe "context"?

https://github.com/wemake-services/wemake-django-template/runs/6601307942?check_suite_focus=true

@sobolevn
Copy link
Member

Maybe we can add content to _MonkeyPatchedWSGIResponse only?

sobolevn
sobolevn approved these changes May 26, 2022
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exaclty what you did :)

@sobolevn sobolevn merged commit 42d8e18 into typeddjango:master May 26, 2022
@quinox quinox deleted the testing-response-content branch May 26, 2022 10:43
@PIG208 PIG208 mentioned this pull request Jun 8, 2022
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn sobolevn approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@quinox @intgr @fizyk @sobolevn