Fixed #29082 -- Made test Client appropriately encode JSON. #9645

NDevox · 2018-01-31T16:41:28Z

https://code.djangoproject.com/ticket/29082
(continued from #9636)

NDevox · 2018-01-31T16:42:35Z

@felixxm @carltongibson

Fixed git. One commit now. Let me know if there is anything else worth changing.

carltongibson

OK. This looks right.

@NDevox: In general, we could have saved the original PR (but not once you deleted the repo... 🙂)

I included a few changes from the previous feedback from @felixxm. I will squash the commits when merging.

Thanks for the effort.

// @felixxm: If you're happy I'll merge.

carltongibson · 2018-02-01T07:41:58Z

tests/test_client/tests.py

        self.assertContains(response, 'This is a test')
        self.assertEqual(response.context['var'], '\xf2')
        self.assertEqual(response.templates[0].name, 'GET Template')

    def test_get_post_view(self):
-        "GET a view that normally expects POSTs"
+        """GET a view that normally expects POSTs"""


This and the few following cleanups are strictly unrelated to this patch.

Presumably, they're here because of the feedback for similar items on #9636. There are lots of inconsistencies left in this file, around both docstrings and formatting of simple (e.g.) post_data dicts.

Despite being strictly unrelated, I'm inclined to leave these few changes as they are here, and include them.

Yep I'm starting to think I'll make a ticket to clean up this file.

Mind if I revert these back to how they were for that ticket instead?

@NDevox I'd be very happy for you to do that. 🙂

(If it's just a single file cleanup, you can just make a PR, without the Trac ticket)

carltongibson · 2018-02-01T07:53:17Z

Hmmm. Now there's an isort failure (which looks totally unrelated...). Coffee first.

felixxm · 2018-02-01T08:02:33Z

@carltongibson I created separate PR #9649 to fix imports per isort 4.3.0.

NDevox · 2018-02-01T08:10:27Z

Sorry about the delete yesterday 😞, long day for me that one and things weren't going well, probably should have left it for the morning.

I squashed the commits, mainly to prove to myself I can use git.

carltongibson · 2018-02-01T08:36:34Z

@NDevox No problem. It happens. 🙂

Looking at it, I think your issue probably arose from not fetching the latest updates and rebasing on that. (This morning I realised this branch too wasn't up to date and needed to rebase.) If you're careful about that it usually goes smoothly...

OK. I merged in the isort fix. This should be good to go...

NDevox · 2018-02-01T11:09:43Z

@carltongibson removed the irrelevant docstring/comment changes and am happy to merge.

carltongibson · 2018-02-01T11:11:37Z

OK. Thanks @NDevox!

timgraham · 2018-02-01T13:09:35Z

django/test/client.py

@@ -309,6 +312,15 @@ def _encode_data(self, data, content_type):
                charset = settings.DEFAULT_CHARSET
            return force_bytes(data, encoding=charset)

+    def _encode_json(self, data, encoder):
+        """
+        Return correctly encoded JSON if we have a dict.


Avoid use of "we" in comments per our style guide.

timgraham · 2018-02-01T13:12:44Z

tests/test_client/tests.py

@@ -85,6 +87,69 @@ def test_post(self):
        self.assertEqual(response.templates[0].name, 'POST Template')
        self.assertContains(response, 'Data received')

+    def test_json_post(self):


Try refactoring the tests to using a loop and subTest and it doesn't look nice to duplicate so much code..

As mentioned before I find doing this obfuscates what the tests are doing - especially when they are testing different things.

However if this is the standard in the lib I will do so.

Hmmm to do this I'd have to pass in the test methods with some matching parameters which doesn't look elegant to me.

I've put the assertion code into a method and call that instead which means all these methods are now have 2 lines of code in them instead of repeating assertions.

timgraham · 2018-02-01T13:13:01Z

tests/test_client/views.py

+    A view which expects a request with the header 'application/json' and
+    parseable json data to go with it.
+
+    We expect a value named 'value' in the data.


Avoid use of "we" in comments per our style guide.

timgraham · 2018-02-01T13:15:00Z

docs/topics/testing/tools.txt

+        If you provide ``content_type`` as :mimetype:`application/json` the
+        POST data will be presented as a binary string like so::
+
+            {"name": "fred", "passwd": "secret"}


I don't understand what this is demonstrating... this doesn't look like a binary string.

Was split between representing it as an actual binary string, or what the binary string represents. I'll switch it to look like a binary string.

timgraham · 2018-02-01T13:16:31Z

django/test/client.py

@@ -331,6 +343,8 @@ def get(self, path, data=None, secure=False, **extra):
    def post(self, path, data=None, content_type=MULTIPART_CONTENT,
             secure=False, **extra):
        """Construct a POST request."""
+        if content_type == JSON_CONTENT:


It might be better to put the content type check in _encode_json for consistency with _encode_data and to avoid duplicating that in every put/patch/etc. method.

Pointed this out on the ticket, my only issue with it is I don't think it's clear that this check will happen with this method name.

rsalmaso · 2018-02-01T16:55:30Z

django/test/client.py

@@ -260,7 +262,8 @@ class RequestFactory:
    Once you have a request object you can pass it to any view function,
    just as if that view had been hooked up using a URLconf.
    """
-    def __init__(self, **defaults):
+    def __init__(self, json_encoder=DjangoJSONEncoder, **defaults):


What about using keyword-only args?

def __init__(self, *, json_encoder=DjangoJSONEncoder, **defaults):

Is it worth it?

TBH I'm already doing all the doc changes and I'm pro this change so happy to do it.

I think it is better because it is more explicit and future proof if you want to add other params.
But wait for other comments :)

rsalmaso · 2018-02-01T16:56:35Z

django/test/client.py

@@ -418,8 +438,8 @@ class Client(RequestFactory):
    contexts and templates produced by a view, rather than the
    HTML rendered to the end-user.
    """
-    def __init__(self, enforce_csrf_checks=False, **defaults):
-        super().__init__(**defaults)
+    def __init__(self, enforce_csrf_checks=False, json_encoder=DjangoJSONEncoder, **defaults):


Same as above use keyword-only args

def __init__(self, enforce_csrf_checks=False, *, json_encoder=DjangoJSONEncoder, **defaults):

felixxm · 2018-02-02T21:43:46Z

tests/test_client/tests.py

@@ -85,6 +87,52 @@ def test_post(self):
        self.assertEqual(response.templates[0].name, 'POST Template')
        self.assertContains(response, 'Data received')

+    def assert_json_response(self, response, method):


We use CamelCase for assert methods to follow unittest convention, e.g. assertJSONResponse. Maybe it will be better to pass method and data instead of response, e.g.:

def assertJSONResponse(method, data, method_name): response = method('/json_view/', data, content_type=JSON_CONTENT) self.assertEqual(response.status_code, 200) self.assertEqual(response.context['data'], 37) self.assertEqual(response.templates[0].name, '{} Template'.format(method)) self.assertContains(response, 'Viewing {} page.'.format(method)) def test_json_post(self): assertJSONResponse(self.client.post, {'value': 37}, 'POST') def test_json_put(self): assertJSONResponse(self.client.put, {'value': 37}, 'PUT') ...

Please also try to avoid temporary variables like post_data, put_data, etc.

NDevox · 2018-02-05T10:37:03Z

@felixxm @carltongibson All ready now.

Anything more?

adamchainz · 2018-02-06T17:08:39Z

docs/releases/2.1.txt

 * Added test :class:`~django.test.Client` support for 307 and 308 redirects.

+


this newline shouldn't be here :)

Don't worry about that, I'm making edits to the patch.

timgraham

I amended the first commit with some of my changes by mistake but most of the revisions are in the second one. Let me know if anything looks off.

timgraham · 2018-02-06T18:01:50Z

tests/test_client/tests.py

+        """DELETE some JSON data to a view"""
+        self.assertJSONResponse(self.client.delete, {'value': 37}, 'DELETE')
+
+    def test_encoded_json_post(self):


I wouldn't include this test in the PR because it's testing existing behavior. But it doesn't seem needed as there's a test in test_client_regress that fails if the isinstance(data, dict) check is _encode_json is removed.

NDevox · 2018-02-06T20:40:04Z

@timgraham looks fine to me.

Want me to squash or happy as is?

carltongibson approved these changes Feb 1, 2018

View reviewed changes

carltongibson requested a review from felixxm February 1, 2018 07:45

carltongibson force-pushed the master branch from 1878e62 to fb34ff8 Compare February 1, 2018 07:57

NDevox force-pushed the master branch from fb34ff8 to fcf7983 Compare February 1, 2018 08:09

carltongibson force-pushed the master branch from fcf7983 to e22516c Compare February 1, 2018 08:33

carltongibson changed the title ~~Fixes #29082 -- Encodes JSON appropriately in the test Client.~~ Fixes #29082 -- Made test Client appropriately encode JSON Feb 1, 2018

felixxm changed the title ~~Fixes #29082 -- Made test Client appropriately encode JSON~~ Fixed #29082 -- Made test Client appropriately encode JSON. Feb 1, 2018

NDevox force-pushed the master branch from e22516c to 50e6027 Compare February 1, 2018 11:08

timgraham reviewed Feb 1, 2018

View reviewed changes

rsalmaso reviewed Feb 1, 2018

View reviewed changes

NDevox force-pushed the master branch 2 times, most recently from 0c5f204 to c91c32c Compare February 1, 2018 17:39

felixxm reviewed Feb 2, 2018

View reviewed changes

NDevox force-pushed the master branch 2 times, most recently from cbeb700 to 38d4e4a Compare February 5, 2018 10:22

adamchainz reviewed Feb 6, 2018

View reviewed changes

django deleted a comment from NDevox Feb 6, 2018

timgraham force-pushed the master branch from 8f10f5d to ee5f609 Compare February 6, 2018 18:03

timgraham reviewed Feb 6, 2018

View reviewed changes

Fixed #29082 -- Allowed the test client to encode JSON request data.

4726824

timgraham force-pushed the master branch from ee5f609 to 4726824 Compare February 6, 2018 23:29

timgraham merged commit 4726824 into django:master Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #29082 -- Made test Client appropriately encode JSON. #9645

Fixed #29082 -- Made test Client appropriately encode JSON. #9645

NDevox commented Jan 31, 2018 •

edited by timgraham

Loading

NDevox commented Jan 31, 2018

carltongibson left a comment

carltongibson Feb 1, 2018

NDevox Feb 1, 2018

carltongibson Feb 1, 2018

carltongibson commented Feb 1, 2018

felixxm commented Feb 1, 2018

NDevox commented Feb 1, 2018

carltongibson commented Feb 1, 2018

NDevox commented Feb 1, 2018

carltongibson commented Feb 1, 2018

timgraham Feb 1, 2018

timgraham Feb 1, 2018

NDevox Feb 1, 2018

NDevox Feb 1, 2018

timgraham Feb 1, 2018

timgraham Feb 1, 2018

NDevox Feb 1, 2018

timgraham Feb 1, 2018

NDevox Feb 1, 2018

rsalmaso Feb 1, 2018

NDevox Feb 1, 2018

NDevox Feb 1, 2018

rsalmaso Feb 1, 2018

rsalmaso Feb 1, 2018

felixxm Feb 2, 2018

NDevox Feb 5, 2018

NDevox commented Feb 5, 2018

adamchainz Feb 6, 2018

timgraham Feb 6, 2018

timgraham left a comment

timgraham Feb 6, 2018

NDevox commented Feb 6, 2018

		* Added test :class:`~django.test.Client` support for 307 and 308 redirects.

Fixed #29082 -- Made test Client appropriately encode JSON. #9645

Fixed #29082 -- Made test Client appropriately encode JSON. #9645

Conversation

NDevox commented Jan 31, 2018 • edited by timgraham Loading

NDevox commented Jan 31, 2018

carltongibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carltongibson commented Feb 1, 2018

felixxm commented Feb 1, 2018

NDevox commented Feb 1, 2018

carltongibson commented Feb 1, 2018

NDevox commented Feb 1, 2018

carltongibson commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NDevox commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NDevox commented Feb 6, 2018

NDevox commented Jan 31, 2018 •

edited by timgraham

Loading