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.

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

support django 2.1+ test client json data automatically serialized #6511

Merged

Conversation

terencehonles
Copy link
Contributor

@terencehonles terencehonles commented Mar 15, 2019

Description

  • Support Django 2.1+ test client auto serializing data if the content type is a json type
  • Ensure the result of _encode_data is always Tuple[bytes, str] rather than Tuple[Union[bytes,str], str]

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

to have a better review need to add a test for the proposed change.

rpkilby
rpkilby previously requested changes May 9, 2019
Copy link
Member

@rpkilby rpkilby left a comment

Choose a reason for hiding this comment

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

I'm definitely going to agree with @auvipy here. A test case demonstrating the behavior change would be required for this to be accepted.

My immediate reaction is "why aren't our tests failing under Django 2.1+?"

@terencehonles terencehonles force-pushed the automatically-serialize-test-client-json branch from 4607f28 to 4ed1e0e Compare May 11, 2019 01:24
@terencehonles
Copy link
Contributor Author

@rpkilby updated with a test

@terencehonles terencehonles force-pushed the automatically-serialize-test-client-json branch 2 times, most recently from d368885 to 0a99e02 Compare October 8, 2020 14:47
@stale
Copy link

stale bot commented May 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 1, 2022
@terencehonles terencehonles force-pushed the automatically-serialize-test-client-json branch from 0a99e02 to 9ee6d46 Compare May 6, 2022 17:55
@stale stale bot removed the stale label May 6, 2022
@terencehonles
Copy link
Contributor Author

Updated to master

@terencehonles terencehonles force-pushed the automatically-serialize-test-client-json branch from 9ee6d46 to 09fafd2 Compare May 6, 2022 20:32
@stale
Copy link

stale bot commented Jul 10, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 10, 2022
@terencehonles terencehonles force-pushed the automatically-serialize-test-client-json branch from 09fafd2 to d28724e Compare July 14, 2022 01:24
@terencehonles
Copy link
Contributor Author

bumped, I think the failed test was unrelated but this re-run the test suite

@stale stale bot removed the stale label Jul 14, 2022
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

also, please rebase, and I think we might get rid of some checks to eleminate older version check

tests/test_testing.py Outdated Show resolved Hide resolved
@auvipy auvipy self-assigned this Dec 10, 2022
@auvipy auvipy added this to the 3.15 milestone Dec 10, 2022
@auvipy auvipy removed this from the 3.15 milestone Jul 11, 2023
@stale
Copy link

stale bot commented Sep 17, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 17, 2023
@terencehonles terencehonles force-pushed the automatically-serialize-test-client-json branch from d28724e to d579cc1 Compare October 4, 2023 20:29
@stale stale bot removed the stale label Oct 4, 2023
@terencehonles
Copy link
Contributor Author

The failing test should be fixed by #9129

@auvipy auvipy changed the title support django 2.1 test client json data automatically serialized support django 2.1+ test client json data automatically serialized Oct 5, 2023
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you rebase? and check if any document or test update is needed to get this merged? also a reference to django docs related to this would be really helpful

@auvipy auvipy added this to the 3.15 milestone Oct 5, 2023
@terencehonles terencehonles force-pushed the automatically-serialize-test-client-json branch from d579cc1 to 803e089 Compare October 5, 2023 08:53
@terencehonles
Copy link
Contributor Author

terencehonles commented Oct 5, 2023

can you rebase? and check if any document or test update is needed to get this merged? also a reference to django docs related to this would be really helpful

It's mentioned here https://docs.djangoproject.com/en/4.2/topics/testing/tools/#django.test.Client.post (specifically), and it's mentioned in the 2.1 release notes https://docs.djangoproject.com/en/4.2/releases/2.1/#tests (point 2)

I'm not sure if you're asking for this to be included in the source, since there's not specifically a good place to put it (maybe the tests?), or if you're just asking for the release notes.

@terencehonles
Copy link
Contributor Author

@auvipy friendly ping, I can rebase, but wondering about ^^^

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

looks good to me. should we also mention this in DRF docs?

auvipy
auvipy previously approved these changes Dec 14, 2024
@terencehonles
Copy link
Contributor Author

Sure, I'll rebase and mention it.

One thing that I realized with this request factory, when testing something yesterday, is that it returns a Django not a DRF request. This means that trying to access the request.data doesn't work. I had to wrap it in a DRF request manually so I switched back to the native factory to get the encoding and not to confuse my peers since this PR still hasn't landed.

I will look at the code, but would it be acceptable to return a DRF request instead? It proxies the native request so it's not a breaking change. I will look at the code, but from memory I think I can actually initialize it in way that would make sense. I will also look at the test client to make sure it does not rely on the factory or that if it does it works correctly.

@terencehonles terencehonles force-pushed the automatically-serialize-test-client-json branch from 803e089 to 4579eae Compare December 15, 2024 20:43
@terencehonles
Copy link
Contributor Author

I've left out changing the return type to Response, so I've just rebased and updated the docs.

@auvipy
Copy link
Member

auvipy commented Dec 19, 2024

Thanks, will review it again soon

@auvipy auvipy dismissed rpkilby’s stale review December 28, 2024 10:21

review comment addressed

@auvipy auvipy merged commit 089f6a6 into encode:master Dec 28, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants