Skip to content

http2: fix refs to status 205, add tests#15153

Closed
apapirovski wants to merge 1 commit intonodejs:masterfrom
apapirovski:patch-http2-respond-errors-and-statuses
Closed

http2: fix refs to status 205, add tests#15153
apapirovski wants to merge 1 commit intonodejs:masterfrom
apapirovski:patch-http2-respond-errors-and-statuses

Conversation

@apapirovski
Copy link
Contributor

@apapirovski apapirovski commented Sep 2, 2017

This started out as just tests for throwing errors within respondWithFile & respondWithFD and ended up adding a bit extra because of finding an incorrect reference for HTTP_STATUS_CONTENT_RESET in http2/core.

  • fixes references within http2 core to HTTP_STATUS_CONTENT_RESET to point to the correct HTTP_STATUS_RESET_CONTENT
  • adds tests for status 204, 205 & 304 in respond, respondWithFD & respondWithFile
  • adds general error tests (type errors for options, etc.) for respondWithFD & respondWithFile

Let me know if there's anything I can adjust. Thanks.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http2, test

@nodejs-github-bot nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Sep 2, 2017
@apapirovski apapirovski force-pushed the patch-http2-respond-errors-and-statuses branch 4 times, most recently from 1f43abc to 166bad4 Compare September 2, 2017 22:09
benjamingr
benjamingr approved these changes Sep 2, 2017
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Good work, tests LGTM

@benjamingr
Copy link
Member

benjamingr commented Sep 2, 2017

Reference from spec:

The server has fulfilled the request and the user agent SHOULD reset the document view which caused the request to be sent. This response is primarily intended to allow input for actions to take place via user input, followed by a clearing of the form in which the input is given so that the user can easily initiate another input action. The response MUST NOT include an entity.

claudiorodriguez
claudiorodriguez approved these changes Sep 3, 2017
View reviewed changes
Copy link
Contributor

@claudiorodriguez claudiorodriguez left a comment

Choose a reason for hiding this comment

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

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

cjihrig
cjihrig approved these changes Sep 3, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

This needs some common.mustCall()s added throughout. I didn't comment on all of them.

test/parallel/test-http2-respond-file-errors.js Outdated
Copy link
Contributor

@cjihrig cjihrig Sep 3, 2017

Choose a reason for hiding this comment

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

common.mustCall()?

test/parallel/test-http2-respond-file-errors.js Outdated
Copy link
Contributor

@cjihrig cjihrig Sep 3, 2017

Choose a reason for hiding this comment

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

common.mustCall() here please.

@apapirovski
Copy link
Contributor Author

apapirovski commented Sep 4, 2017

Thanks @cjihrig. Fixed now.

@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

Getting a consistent failure on test-http2-respond-no-data.js now

@apapirovski
http2: fix refs to status 205, add tests
9b21d7f 
Fix references within http2 core to HTTP_STATUS_CONTENT_RESET to point
to the correct HTTP_STATUS_RESET_CONTENT. Add tests for status 204,
205 & 304 in respond, respondWithFD & respondWithFile. Add general
error tests for respondWithFD & respondWithFile.
@apapirovski apapirovski force-pushed the patch-http2-respond-errors-and-statuses branch from 04011b5 to 9b21d7f Compare September 7, 2017 11:29
@apapirovski
Copy link
Contributor Author

apapirovski commented Sep 7, 2017

My bad. Fixed now. Thanks for the review @jasnell!

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2017

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

@apapirovski
Copy link
Contributor Author

apapirovski commented Sep 8, 2017

All green this time, awesome. Thanks for the reviews everyone.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Sep 11, 2017
@apapirovski @BridgeAR
http2: fix refs to status 205, add tests
45357d0 
Fix references within http2 core to HTTP_STATUS_CONTENT_RESET to point
to the correct HTTP_STATUS_RESET_CONTENT. Add tests for status 204,
205 & 304 in respond, respondWithFD & respondWithFile. Add general
error tests for respondWithFD & respondWithFile.

PR-URL: nodejs#15153
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

BridgeAR commented Sep 11, 2017

Landed in 45357d0

@BridgeAR BridgeAR closed this Sep 11, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@apapirovski @MylesBorins
http2: fix refs to status 205, add tests
0343ece 
Fix references within http2 core to HTTP_STATUS_CONTENT_RESET to point
to the correct HTTP_STATUS_RESET_CONTENT. Add tests for status 204,
205 & 304 in respond, respondWithFD & respondWithFile. Add general
error tests for respondWithFD & respondWithFile.

PR-URL: #15153
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Sep 12, 2017
Merged
This was referenced Sep 12, 2017
Closed
Closed
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
@apapirovski @addaleax
http2: fix refs to status 205, add tests
5e41a02 
Fix references within http2 core to HTTP_STATUS_CONTENT_RESET to point
to the correct HTTP_STATUS_RESET_CONTENT. Add tests for status 204,
205 & 304 in respond, respondWithFD & respondWithFile. Add general
error tests for respondWithFD & respondWithFile.

PR-URL: nodejs#15153
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@apapirovski apapirovski deleted the patch-http2-respond-errors-and-statuses branch September 15, 2017 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+1 more reviewer

@claudiorodriguez claudiorodriguez claudiorodriguez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http2 Issues or PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@apapirovski @benjamingr @jasnell @BridgeAR @cjihrig @claudiorodriguez @MylesBorins @nodejs-github-bot