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.

Sign up for GitHub

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

Jump to bottom

test: Verify request payload is uploaded consistently #34066

Merged
merged 1 commit into from
May 12, 2024
Merged

test: Verify request payload is uploaded consistently #34066

merged 1 commit into from
May 12, 2024

Conversation

awwright
Copy link
Contributor

@awwright awwright commented Jun 26, 2020 •
edited by aduh95
Loading

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
@awwright awwright mentioned this pull request Jun 26, 2020
Open
@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 awwright requested a review from jasnell October 15, 2020 01:11
@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
@98lenvi 98lenvi mentioned this pull request Sep 26, 2022
Closed
@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?
@awwright awwright mentioned this pull request Nov 30, 2023
Closed
@aduh95 aduh95 force-pushed the issue-27880 branch 2 times, most recently from 7cb53b7 to b753797 Compare May 11, 2024 17:20
@awwright @98lenvi @aduh95
test: verify request payload is uploaded consistently
1f659d4 
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 <aaa@bzfx.net>
Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@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

CI: https://ci.nodejs.org/job/node-test-pull-request/59125/
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59142/
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59176/
@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
@awwright @98lenvi
@aduh95 @targos
test: verify request payload is uploaded consistently
578868d 
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 <aaa@bzfx.net>
Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request May 13, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@awwright @98lenvi
@aduh95 @marco-ippolito
test: verify request payload is uploaded consistently
38b3a5a 
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 <aaa@bzfx.net>
Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <jasnell@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@awwright @98lenvi
@aduh95 @marco-ippolito
test: verify request payload is uploaded consistently
f40788b 
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 <aaa@bzfx.net>
Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@awwright @98lenvi
@aduh95 @marco-ippolito
test: verify request payload is uploaded consistently
1d01325 
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 <aaa@bzfx.net>
Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #34066
Refs: #27880
Reviewed-By: James M Snell <jasnell@gmail.com>
eliphazb pushed a commit to eliphazb/node that referenced this pull request Jun 20, 2024
@awwright @98lenvi
@aduh95 @eliphazb
test: verify request payload is uploaded consistently
760dd94 
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 <aaa@bzfx.net>
Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#34066
Refs: nodejs#27880
Reviewed-By: James M Snell <jasnell@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@awwright @98lenvi
@aduh95 @bmeck
test: verify request payload is uploaded consistently
3f17bd2 
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 <aaa@bzfx.net>
Co-authored-by: Lenvin Gonsalves <lenvingonsalves@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#34066
Refs: nodejs#27880
Reviewed-By: James M Snell <jasnell@gmail.com>
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.
7 participants
@awwright @aduh95 @ronag @dnlup @nodejs-github-bot @jasnell @Trott