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

http2: improve tests and docs #42858

Merged
merged 3 commits into from
May 24, 2022

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Apr 25, 2022

This commit documents the event parameters and http2stream.respond, and
adds some tests to ensure the actual behaviors are aligned with the docs.

Testing the Http2Server.sessionError event is added by updating
test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js.
The event seemingly has not been tested so far.

ServerHttp2Session is exported to validate the session event and the
sessionError event.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Apr 25, 2022
@daeyeon daeyeon force-pushed the master.doc-220425.Mon.5754 branch 2 times, most recently from 870e7a5 to 31dd9d6 Compare April 25, 2022 06:00
@daeyeon daeyeon changed the title doc: improve doc for http2 http2: improve tests and docs Apr 25, 2022
@daeyeon daeyeon force-pushed the master.doc-220425.Mon.5754 branch from 31dd9d6 to eda399a Compare April 25, 2022 09:06
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@daeyeon daeyeon force-pushed the master.doc-220425.Mon.5754 branch 7 times, most recently from 4ba9b20 to f5120d0 Compare April 28, 2022 00:40
@daeyeon
Copy link
Member Author

daeyeon commented May 14, 2022

@mcollina @ShogunPanda @RafaelGSS This PR needs a Jenkins CI test, doesn't it? Can someone get it started? Thank you!

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label May 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 14, 2022
@nodejs-github-bot
Copy link
Collaborator

@daeyeon
Copy link
Member Author

daeyeon commented May 18, 2022

Need a rerun of the Jenkins CI. The failures are seemingly unrelated to this PR.

@nodejs-github-bot
Copy link
Collaborator

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2022
@nodejs-github-bot
Copy link
Collaborator

@daeyeon
Copy link
Member Author

daeyeon commented May 24, 2022

Pls help me re-run the CI.

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 24, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 24, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/42858
✔  Done loading data for nodejs/node/pull/42858
----------------------------------- PR info ------------------------------------
Title      http2: improve tests and docs (#42858)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     daeyeon:master.doc-220425.Mon.5754 -> nodejs:master
Labels     doc, http2, commit-queue-squash
Commits    3
 - http2: improve tests and docs
 - fix: add client.on('error') and drop :status
 - test: update test-http2-server-sessionerror.js
Committers 1
 - Daeyeon Jeong 
PR-URL: https://github.com/nodejs/node/pull/42858
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: Rafael Gonzaga 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42858
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: Rafael Gonzaga 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - http2: improve tests and docs
   ⚠  - fix: add client.on('error') and drop :status
   ⚠  - test: update test-http2-server-sessionerror.js
   ℹ  This PR was created on Mon, 25 Apr 2022 02:42:16 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/42858#pullrequestreview-952377018
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/42858#pullrequestreview-953188072
   ✔  - Rafael Gonzaga (@RafaelGSS): https://github.com/nodejs/node/pull/42858#pullrequestreview-953315250
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-05-24T07:42:37Z: https://ci.nodejs.org/job/node-test-pull-request/44145/
- Querying data for job/node-test-pull-request/44145/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2377372947

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 24, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2022
@nodejs-github-bot nodejs-github-bot merged commit 714e2d7 into nodejs:master May 24, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 714e2d7

@daeyeon
Copy link
Member Author

daeyeon commented May 24, 2022

Thanks!! @mcollina @RafaelGSS @ShogunPanda

@daeyeon daeyeon deleted the master.doc-220425.Mon.5754 branch May 24, 2022 11:41
bengl pushed a commit that referenced this pull request May 30, 2022
This commit documents the event parameters and `http2stream.respond`,
and adds some tests to ensure the actual behaviors are aligned with
the docs.

Testing the 'Http2Server.sessionError' event is added by updating
`test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js`.
The event seemingly has not been tested so far.

`ServerHttp2Session` is exported to validate the `session` event
and the `sessionError` event.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #42858
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@bengl bengl mentioned this pull request May 31, 2022
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
This commit documents the event parameters and `http2stream.respond`,
and adds some tests to ensure the actual behaviors are aligned with
the docs.

Testing the 'Http2Server.sessionError' event is added by updating
`test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js`.
The event seemingly has not been tested so far.

`ServerHttp2Session` is exported to validate the `session` event
and the `sessionError` event.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #42858
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
This commit documents the event parameters and `http2stream.respond`,
and adds some tests to ensure the actual behaviors are aligned with
the docs.

Testing the 'Http2Server.sessionError' event is added by updating
`test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js`.
The event seemingly has not been tested so far.

`ServerHttp2Session` is exported to validate the `session` event
and the `sessionError` event.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #42858
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
This commit documents the event parameters and `http2stream.respond`,
and adds some tests to ensure the actual behaviors are aligned with
the docs.

Testing the 'Http2Server.sessionError' event is added by updating
`test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js`.
The event seemingly has not been tested so far.

`ServerHttp2Session` is exported to validate the `session` event
and the `sessionError` event.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #42858
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
This commit documents the event parameters and `http2stream.respond`,
and adds some tests to ensure the actual behaviors are aligned with
the docs.

Testing the 'Http2Server.sessionError' event is added by updating
`test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js`.
The event seemingly has not been tested so far.

`ServerHttp2Session` is exported to validate the `session` event
and the `sessionError` event.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: nodejs/node#42858
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants