-
Notifications
You must be signed in to change notification settings - Fork 300
Allow json api renderer to be used as test request renderer class #396
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
Conversation
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 this is a great idea, @sliverc. I really like how this cleans up the test code. I have just a couple of questions before I'm ready to approve the PR. Thanks for working on this.
@@ -40,7 +40,7 @@ def get_resource_name(context, expand_polymorphic_types=False): | |||
|
|||
# Sanity check to make sure we have a view. | |||
if not view: | |||
raise APIException(_('Could not find view.')) |
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.
Why was this switched to returning None instead of raising an exception?
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.
DRF uses the test renderer to parse data passed on to APIClient
. In such a case it won't have a renderer context and therefore there is no view provided.
The json api renderer should still not fail in such a case and render data as is. Hence I had to remove this exception.
Interesting enough is though that the json api renderer was already aware of resource_name
with value 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.
ok, if I understand this correctly based on what you wrote, missing a view should not be possible in a non-test setting? Does that sound true?
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.
yep this is true...
example/tests/utils.py
Outdated
@@ -1,21 +1,7 @@ | |||
import json | |||
|
|||
from django.utils.encoding import force_bytes, force_text | |||
from django.utils.encoding import force_text | |||
|
|||
|
|||
def load_json(data): |
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.
Many of the code changes removed load_json
. Is it still used somewhere or could it be deleted too?
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 would like to remove it as well. Unfortunalty there is one test which still needs it.
Reason being is that it uses RequestFactory
and not APIClient
. In such a case response doesn't seem to provide a .json()
function.
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.
Would it be possible to convert the test to use APIClient
instead?
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.
Possibly. I have a look.
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 have found an ok way to convert it to APIClient
. Now also removed example/tests/utils.py
.
Codecov Report
@@ Coverage Diff @@
## master #396 +/- ##
==========================================
- Coverage 91.97% 91.91% -0.07%
==========================================
Files 56 55 -1
Lines 2917 2882 -35
==========================================
- Hits 2683 2649 -34
+ Misses 234 233 -1
Continue to review full report at Codecov.
|
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 appreciate your extra effort to remove load_json
entirely. Thanks!
This allows to define json api format as default in tests. Following needs to be configured:
With this change tests can be simplified and there is no need to specifically assign json api content type and dump resp. load data as json. This PR changes tests accordingly.
More detail documentation on this rest framework feature can be found here