-
-
Notifications
You must be signed in to change notification settings - Fork 463
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.
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
fix: Add context_data
attribute to _MonkeyPatchedW/ASGIResponse.
#2509
fix: Add context_data
attribute to _MonkeyPatchedW/ASGIResponse.
#2509
Conversation
django-stubs/test/client.pyi
Outdated
# `context_data` only exists if the response was successful too as the return type changes. | ||
# `HTTPResponse` when API failed and `TemplateResponse` when successful. | ||
# https://docs.djangoproject.com/en/stable/topics/testing/tools/#django.test.Response.context | ||
context: ContextList | dict[str, Any] | None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this can ruin valid use, basically you would have to do:
assert response.context
assert response.context[key]
Right?
This attribute is only populated when using the [DjangoTemplates](https://docs.djangoproject.com/en/5.1/topics/templates/#django.template.backends.django.DjangoTemplates) backend. If you’re using another template engine, [context_data](https://docs.djangoproject.com/en/5.1/ref/template-response/#django.template.response.SimpleTemplateResponse.context_data) may be a suitable alternative on responses with that attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have a custom Template BE then context is null and you have to use context_data instead. From testing this out, I found context was always null on a 200 response regardless of template BE using Django 4.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not against adding context_data
, but I don't think we should add None
type to these values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, I see your point. I only added None type as that's what I saw during some TDD, but like you mentioned this might do more harm than good. Happy to remove the None types here 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed None
now from both context
and context_data
.
2fe59e8
to
7f678b1
Compare
As detailed in Django's documentation [0], when a non-standard template backend is being used, the `context_data` attribute is populated instead of `context` when the test API call was successful. In reality, I've seen this to be a populated attribute regardless of the backend template engine being used. Regardless, this near hidden attribute is missing and should be included to help improve unit testing when validating the response data contains the expected data.
7f678b1
to
2ae18a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
As detailed in Django's documentation [0], when a non-standard template backend is used, the
context_data
attribute is populated instead ofcontext
when the test API call is successful. I've seen this to be a populated attribute regardless of the backend template engine being used. Regardless, this near-hidden attribute is missing and should be included to help improve unit testing when validating that the response data contains the expected data.[0] https://docs.djangoproject.com/en/stable/topics/testing/tools/#django.test.Response.context
Related issues
None.