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

http2: improve code coverage #14985

Closed
mcollina opened this issue Aug 23, 2017 · 10 comments
Closed

http2: improve code coverage #14985

mcollina opened this issue Aug 23, 2017 · 10 comments
Labels
good first issue Issues that are suitable for first-time contributors. http2 Issues or PRs related to the http2 subsystem.

Comments

@mcollina
Copy link
Member

This is a meta-issue to improve the code coverage of http2.
As you can see in https://coverage.nodejs.org/coverage-342c5f9d4c2eb868/root/internal/http2/index.html, we have 79% code coverage, and we should definitely do better before it leaves experimental status.

Feel free to reach out to @nodejs/http2 if you have any questions on how to test this part of Node.js.
@mcollina mcollina added good first issue Issues that are suitable for first-time contributors. http2 Issues or PRs related to the http2 subsystem. labels Aug 23, 2017
@TimothyGu
Copy link
Member

TimothyGu commented Aug 23, 2017 •
edited
Loading

There's https://github.com/http2/http2-test, which happens to be written to run on Node.js. We should look into integratingporting that.

@mcollina
Copy link
Member Author

@TimothyGu thats fairly old and for the http2 module published on npm. I think we need something specific for core in test/parallel.

@benhalverson
Copy link
Member

@mcollina I'd like to help improve the test coverage. Where should I look to get started on http2?

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

@benhalverson ... reach out any time with any questions you may have.

@ssbrewster
Copy link
Contributor

@jasnell should it be one PR per test file added or multiple test files in one PR (if it matters)?

@mcollina
Copy link
Member Author

@ssbrewster it's ok if it's a PR with 2-3 files if they are connected to the same area that you are testing. If you are adding some tests for sendFile, then it's ok if they all live in the same commit/PR. If they are related to different areas of the API, then fire multiple PRs.

@ssbrewster
Copy link
Contributor

Great, thanks for the clarification @mcollina

apapirovski added a commit to apapirovski/node that referenced this issue Aug 25, 2017
@apapirovski
test: increase coverage for http2 response headers
3a7b684 
Expanded an existing test for setting pseudo-headers on response to
include all pseudo-headers, not just :status.

Refs: nodejs#14985
@apapirovski apapirovski mentioned this issue Aug 25, 2017
Closed
3 tasks
@apapirovski apapirovski mentioned this issue Aug 26, 2017
Closed
3 tasks
apapirovski added a commit to apapirovski/node that referenced this issue Aug 27, 2017
@apapirovski
test: add a test for Expect & checkExpectation
922bbe2 
New test case for Expect header & checkExpectation event based on the
existing http test case.

Refs: nodejs#14985
@apapirovski apapirovski mentioned this issue Aug 27, 2017
Closed
3 tasks
apapirovski added a commit to apapirovski/node that referenced this issue Aug 27, 2017
@apapirovski
test: add http2 test for method CONNECT
260f29d 
Adds test case for default handling of method CONNECT, as well
as the ability to bind a connect listener and handle the request.

Refs: nodejs#14985
@apapirovski apapirovski mentioned this issue Aug 29, 2017
Closed
3 tasks
BridgeAR pushed a commit that referenced this issue Aug 29, 2017
@apapirovski @BridgeAR
test: increase coverage for http2 response headers
3eb0297 
Expanded an existing test for setting pseudo-headers on response to
include all pseudo-headers, not just :status.

PR-URL: #15035
Refs: #14985
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@apapirovski apapirovski mentioned this issue Aug 29, 2017
Closed
3 tasks
apapirovski added a commit to apapirovski/node that referenced this issue Aug 29, 2017
@apapirovski
http2: correct emit error in onConnect, full tests
89ea398 
Correct requestOnConnect to emit session error on
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE rather than stream error. Add full
test suite for the error handling within requestOnConnect.

Refs: nodejs#14985
ghost pushed a commit to ayojs/ayo that referenced this issue Aug 30, 2017
@apapirovski
test: increase coverage for http2 response headers
1d92188 
Expanded an existing test for setting pseudo-headers on response to
include all pseudo-headers, not just :status.

PR-URL: nodejs/node#15035
Refs: nodejs/node#14985
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
ghost pushed a commit to ayojs/ayo that referenced this issue Aug 30, 2017
@apapirovski
test: increase coverage for http2 response headers
fb83c65 
Expanded an existing test for setting pseudo-headers on response to
include all pseudo-headers, not just :status.

PR-URL: nodejs/node#15035
Refs: nodejs/node#14985
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this issue Aug 30, 2017
@apapirovski @BridgeAR
test: add http2 test for method CONNECT
88441f6 
Adds test case for default handling of method CONNECT, as well
as the ability to bind a connect listener and handle the request.

PR-URL: #15052
Refs: #14985
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this issue Aug 30, 2017
@apapirovski @BridgeAR
test: add a test for Expect & checkExpectation
6eeb06f 
New test case for Expect header & checkExpectation event based on the
existing http test case.

PR-URL: #15040
Refs: #14985
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mcollina
Copy link
Member Author

http2 has now 95% statemement coverage, and I think we can consider this issue solved.

mcollina pushed a commit that referenced this issue Oct 17, 2017
@trivikr @mcollina
test: http2 priority stream depends on itself
ff747e3 
This commit add test case where priority() throws ERR_INVALID_OPT_VALUE
when stream depends on itself

Refs: #14985
PR-URL: #16224
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
trivikr added a commit to trivikr/node that referenced this issue Oct 17, 2017
@trivikr
test: http2 util passing array in te header
9a03c2a 
This adds a test case in which array with two values is passed to
param value in isIllegalConnectionSpecificHeader

Refs: nodejs#14985
targos pushed a commit that referenced this issue Oct 18, 2017
@trivikr @targos
test: http2 connectionListener reject client
ae3836a 
This code change modifies connectionListener tests to cover test case
where this.emit('unknownProtocol', socket) returns false

PR-URL: #16080
Ref: #14985
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Oct 18, 2017
@trivikr @targos
test: http2 client operations after destroy
8a172cb 
PR-URL: #16094
Ref: #14985
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 18, 2017
@trivikr @addaleax
test: http2 connectionListener reject client
19eaa17 
This code change modifies connectionListener tests to cover test case
where this.emit('unknownProtocol', socket) returns false

PR-URL: nodejs/node#16080
Ref: nodejs/node#14985
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 18, 2017
@trivikr @addaleax
test: http2 client operations after destroy
3abaa5d 
PR-URL: nodejs/node#16094
Ref: nodejs/node#14985
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 18, 2017
@trivikr @addaleax
test: http2 ERR_INVALID_ARG_TYPE tests
fbb8bd3 
PR-URL: nodejs/node#15766
Ref: nodejs/node#14985
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 18, 2017
@trivikr @addaleax
test: http2 add timeout no callback test case
49fef00 
Refs: nodejs/node#14985
PR-URL: nodejs/node#16082
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 18, 2017
@trivikr @addaleax
test: http2 emitGoAway post shutdown pre destroy
c288f99 
This commit tests use case when emitGoAway is called when client is
shutting down but is not destroyed.

Refs: nodejs/node#14985
PR-URL: nodejs/node#16215
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 18, 2017
@trivikr @addaleax
test: http2 client settings errors
375bd94 
Refs: nodejs/node#14985
PR-URL: nodejs/node#16096
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 18, 2017
@trivikr @addaleax
test: http2 priority stream depends on itself
3fef56e 
This commit add test case where priority() throws ERR_INVALID_OPT_VALUE
when stream depends on itself

Refs: nodejs/node#14985
PR-URL: nodejs/node#16224
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
apapirovski pushed a commit to apapirovski/node that referenced this issue Oct 19, 2017
@trivikr @apapirovski
http2: simplify te header validation, add test
1fd662c 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: nodejs#16246
Refs: nodejs#14985
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
@trivikr @MylesBorins
test: http2 ERR_INVALID_ARG_TYPE tests
1c14ad9 
PR-URL: #15766
Ref: #14985
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
@trivikr @MylesBorins
test: http2 emitGoAway post shutdown pre destroy
327be77 
This commit tests use case when emitGoAway is called when client is
shutting down but is not destroyed.

Refs: #14985
PR-URL: #16215
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
@trivikr @MylesBorins
test: http2 client settings errors
50b4965 
Refs: #14985
PR-URL: #16096
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
@trivikr @MylesBorins
test: http2 priority stream depends on itself
e5cf833 
This commit add test case where priority() throws ERR_INVALID_OPT_VALUE
when stream depends on itself

Refs: #14985
PR-URL: #16224
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
@trivikr @MylesBorins
http2: simplify te header validation, add test
25ef372 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: #16246
Refs: #14985
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
@trivikr @MylesBorins
test: consolidate http2 tests in one file
65c5d0e 
PR-URL: #15624
Refs: #14985
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
@trivikr @MylesBorins
test: http2 session operations after destroy
c07d757 
Refs #14985

PR-URL: #15758
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
@trivikr @MylesBorins
test: http2 add timeout no callback test case
6d16022 
Refs: #14985
PR-URL: #16082
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
@trivikr @addaleax
http2: simplify te header validation, add test
e851661 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: nodejs/node#16246
Refs: nodejs/node#14985
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
@trivikr @addaleax
http2: simplify te header validation, add test
49cbad1 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: nodejs/node#16246
Refs: nodejs/node#14985
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are suitable for first-time contributors. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@mcollina @jasnell @ssbrewster @TimothyGu @btrajkovski @benhalverson @trivikr