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

test: Verify request payload is uploaded consistently #34066

Merged
merged 1 commit into from
May 12, 2024

Conversation

awwright
Copy link
Contributor

@awwright awwright commented Jun 26, 2020

This adds several HTTP tests testing for the behavior identified in #27880:

  • test-http-27880-bug.js — shows how, if you make a DELETE request with a payload, the payload will actually be written to the server where a second HTTP/1.1 request is expected; if the payload is a valid HTTP message, it will be interpreted as a second request by the server. (But the first request is sent with Connection: close so shouldn't the server close the connection by this point?)
  • test-http-27880-chunked.js — tests that the default behavior for ClientRequest#write should be a chunked encoding.
  • test-http-27880-content-length.js — tests that the default behavior for ClientRequest#end should be a standard message, with an automatically computed Content-Length header.
  • test-http-27880-empty.js — tests that the default behavior for ClientRequest#end should have no payload headers if there's no response body

Naming for the test files will probably need to be reconsidered; suggestions welcome.

Refs: #27880

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 26, 2020
@ronag ronag added the http Issues or PRs related to the http subsystem. label Jun 26, 2020
@awwright awwright force-pushed the issue-27880 branch 2 times, most recently from 3a9b24a to 02d39b8 Compare June 26, 2020 08:02
@aduh95
Copy link
Contributor

aduh95 commented Oct 14, 2020

@awwright there are some failing tests which keep this from landing. Do you still want to work on this?

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Oct 14, 2020
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@awwright
Copy link
Contributor Author

@aduh95 Those failed tests are bugs in the HTTP implementation that need to be fixed. However I'm not sure what the process is for doing this, exactly.

@aduh95 aduh95 removed the stalled Issues and PRs that are stalled. label Oct 14, 2020
@aduh95
Copy link
Contributor

aduh95 commented Oct 14, 2020

I think the process is to move those tests to test/known_issues.

@awwright
Copy link
Contributor Author

@aduh95 I rebased and moved the tests, I'm hoping that is sufficient?

@aduh95
Copy link
Contributor

aduh95 commented Oct 15, 2020

Regarding the naming of the file, what about:

  • test-http-request-method-delete-payload.js
  • test-http-clientrequest-write-chunked.js
  • test-http-clientrequest-end-contentlength.js
  • test-http-clientrequest-end-empty-response-body.js

@awwright
Copy link
Contributor Author

That seems reasonable.

It's not obvious in how the files are written, but I'm trying to test the behavior in both directions, too. I actually forget off-hand if the bug is in requests only, or both request and responses.

@aduh95
Copy link
Contributor

aduh95 commented Nov 8, 2020

@awwright Do you want to rename those files so we can move forward with this?

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Nov 8, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2020

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2021

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Jan 9, 2021
@ronag
Copy link
Member

ronag commented Apr 25, 2021

@aduh95 @dnlup @awwright any chance of revival?

@ronag
Copy link
Member

ronag commented Apr 25, 2021

@nodejs/http

@ronag ronag reopened this Apr 25, 2021
@ronag ronag added the review wanted PRs that need reviews. label Apr 25, 2021
@dnlup
Copy link
Contributor

dnlup commented Apr 26, 2021

I am a little swamped atm. I don't think I am going to be able to work on it now. Sorry.

@Trott Trott removed the stalled Issues and PRs that are stalled. label Dec 15, 2021
@awwright
Copy link
Contributor Author

awwright commented Oct 3, 2022

What else exactly does this need? Can we not merge a test that's showing an actual, live bug?

Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 merged commit d4e365f into nodejs:main May 12, 2024
53 checks passed
@aduh95
Copy link
Contributor

aduh95 commented May 12, 2024

Landed in d4e365f

targos pushed a commit that referenced this pull request May 12, 2024
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <[email protected]>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: nodejs#34066
Refs: nodejs#27880
Reviewed-By: James M Snell <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Node.js seems to change how it is uploaded based on the method,
but HTTP doesn't make any distinction.

Co-authored-by: Austin Wright <[email protected]>
Co-authored-by: Lenvin Gonsalves <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: nodejs#34066
Refs: nodejs#27880
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. review wanted PRs that need reviews. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants