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

Adds a Realistic WSGI Server for Testing #5915

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

theGOTOguy
Copy link
Contributor

@theGOTOguy theGOTOguy commented Aug 27, 2021

Sending of chunked-encoded data is not currently tested to be correct anywhere in requests' tests, and is called out as a needed improvement in #5906. This is a hurdle to accepting #5664, a change which would both simplify requests' code and consistently use urllib3 for retrying errors.

The existing test server is a simple socket that can send and receive data and is not aware of HTTP protocols. Therefore, using it to verify correct sending of chunked-encoded data, for instance, would ultimately become a change-detector test.

This change seeks to remedy that while also taking a step towards both simplifying and strengthening requests' tests.

We strengthen requests' tests by introducing the popular Werkzeug library as an explicit test dependency (Flask, which uses Werkzeug, is already a test dependency). By doing so, we leverage their authority as an implementer of PEP 3333 to validate our own implementation of chunked encoding. This implicitly future-proofs against any hypothetical breaking changes to the standard by testing against Werkzeug's trusted implementation.

We also simplify requests' tests, since the implementation of the test WerkzeugServer here is substantially more concise than the current test server. If this change is accepted, in another pull request I will refactor all of requests' tests to use the WerkzeugServer rather than the current test server.

@theGOTOguy theGOTOguy closed this Aug 27, 2021
@theGOTOguy
Copy link
Contributor Author

theGOTOguy commented Aug 27, 2021

Finally got the test server to verify that it is actually ready to accept connections in a robust way! Sorry about holding the test queue up for so long.

@theGOTOguy theGOTOguy reopened this Aug 27, 2021
@theGOTOguy
Copy link
Contributor Author

This PR has been sitting here for a while and I'm wondering if there's still any interest from the maintainers with regard to improving your tests.

If I can get some feedback that you would prefer to test against a realistic WSGI server instead of the existing simple socket and that you'd accept the PR, I'll refactor all of your tests to use the WerkzeugServer implemented here, simplify your code, and hopefully make it easier to make forward progress in the future because of the greater confidence imparted by more realistic tests.

If there is no interest, I'll close this PR.

@sigmavirus24
Copy link
Contributor

There's interest in adding a WSGI server not in replacing it entirely. There are tests that a socket server is still incredibly valuable for and tests that a higher level server would be better suited to.

@theGOTOguy
Copy link
Contributor Author

@sigmavirus24 Sure, I buy that. In the interest of closing this PR and making things better, I need clarification on how you want to proceed:

  • We could merge this as a body of work and proceed to add WSGI tests piecemeal going forward on a case-by-case basis.
  • I could make a separate WSGI version of all tests, so that there would be both a socket and WSGI version of every existing test.
  • You could clarify which specific tests you would prefer a WSGI version of and I can refactor only that subset.

@theGOTOguy
Copy link
Contributor Author

I'd still like to see requests move towards more realistic testing against an actual implementation of PEP 3333. If any specific guidance can be provided on what the expectations are in order to merge this PR, I will make the necessary modifications or refactor additional tests.

@theGOTOguy
Copy link
Contributor Author

Hello! This pull request is very old, but I still feel that adding realistic tests against a reference implementation of PEP 3333 would be worthwhile.

If any guidance can be provided as to what the maintainers' expectations are in order to merge this change, then I will be happy to conform to those expectations and also to refactor any desired subset of the existing tests.

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.

None yet

2 participants