FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: Verify request payload is uploaded consistently by awwright · Pull Request #34066 · nodejs/node · GitHub

/ node Public

test: Verify request payload is uploaded consistently - #34066

Merged
aduh95 merged 1 commit into
nodejs:mainfrom
awwright:issue-27880
May 12, 2024
ConversationCommits (1)ChecksFiles changed
Merged

test: Verify request payload is uploaded consistently#34066
aduh95 merged 1 commit into
nodejs:mainfrom
awwright:issue-27880

Conversation

awwright commented Jun 26, 2020 •
edited by aduh95
Loading

Copy link
Copy Markdown
Contributor

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 added the test Issues and PRs related to the tests. label Jun 26, 2020
awwright mentioned this pull request Jun 26, 2020
Open
ronag added the http Issues or PRs related to the http subsystem. label Jun 26, 2020
awwright force-pushed the issue-27880 branch 2 times, most recently from 3a9b24a to 02d39b8 Compare June 26, 2020 08:02

aduh95 commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

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

aduh95 added the stalled Issues and PRs that are stalled. label Oct 14, 2020

github-actions Bot commented Oct 14, 2020

Copy link
Copy Markdown
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 commented Oct 14, 2020

Copy link
Copy Markdown
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 removed the stalled Issues and PRs that are stalled. label Oct 14, 2020

aduh95 commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

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

awwright requested a review from jasnell October 15, 2020 01:11

awwright commented Oct 15, 2020

Copy link
Copy Markdown
Contributor Author

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

aduh95 commented Oct 15, 2020

Copy link
Copy Markdown
Contributor

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 commented Oct 15, 2020

Copy link
Copy Markdown
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 commented Nov 8, 2020

Copy link
Copy Markdown
Contributor

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

aduh95 added the stalled Issues and PRs that are stalled. label Nov 8, 2020

github-actions Bot commented Nov 8, 2020

Copy link
Copy Markdown
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.

github-actions Bot commented Jan 9, 2021

Copy link
Copy Markdown
Contributor

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 Bot closed this Jan 9, 2021

ronag commented Apr 25, 2021

Copy link
Copy Markdown
Member

@aduh95 @dnlup @awwright any chance of revival?

ronag commented Apr 25, 2021

Copy link
Copy Markdown
Member

@nodejs/http

ronag reopened this Apr 25, 2021
ronag added the review wanted PRs that need reviews. label Apr 25, 2021

dnlup commented Apr 26, 2021

Copy link
Copy Markdown
Contributor

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

Trott removed the stalled Issues and PRs that are stalled. label Dec 15, 2021
98lenvi mentioned this pull request Sep 26, 2022
Closed

awwright commented Oct 3, 2022

Copy link
Copy Markdown
Contributor Author

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

awwright mentioned this pull request Nov 30, 2023
Closed
aduh95 force-pushed the issue-27880 branch 2 times, most recently from 7cb53b7 to b753797 Compare May 11, 2024 17:20
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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 11, 2024

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59125/

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59142/

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59176/

aduh95 merged commit d4e365f into nodejs:main May 12, 2024

aduh95 commented May 12, 2024

Copy link
Copy Markdown
Contributor

Landed in d4e365f

targos pushed a commit that referenced this pull request May 12, 2024
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 mentioned this pull request May 13, 2024
Merged
marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
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>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
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>
zeexzeex mentioned this pull request Aug 24, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell Awaiting requested review from jasnell

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL