-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: adjust error types, increase test coverage #15109
http2: adjust error types, increase test coverage #15109
Conversation
Change error types emitted from request and validatePriorityOptions to be TypeError rather than the incorrect RangeError. Add tests to confirm that all errors are thrown as expected (weight, parent, exclusive, silent, endStream and getTrailers). Add test for method CONNECT throwing error on missing :authority or superfluous :scheme and :path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
() => client.request({ | ||
[HTTP2_HEADER_METHOD]: 'CONNECT' | ||
}), | ||
common.expectsError({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note for a future test: you can write common.expectsError(throwingFn, errorObj)
instead of assert.throws(throwingFn, common.expectsError(errorObj))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks @BridgeAR! Will use in the future.
@apapirovski Thanks a lot for helping out with improving the code coverage and cleaning up the issues you find while doing so! Much appreciated. |
Landed in e289540 |
Change error types emitted from request and validatePriorityOptions to be TypeError rather than the incorrect RangeError. Add tests to confirm that all errors are thrown as expected (weight, parent, exclusive, silent, endStream and getTrailers). Add test for method CONNECT throwing error on missing :authority or superfluous :scheme and :path. PR-URL: nodejs#15109 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Change error types emitted from request and validatePriorityOptions to be TypeError rather than the incorrect RangeError. Add tests to confirm that all errors are thrown as expected (weight, parent, exclusive, silent, endStream and getTrailers). Add test for method CONNECT throwing error on missing :authority or superfluous :scheme and :path. PR-URL: nodejs/node#15109 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Change error types emitted from request and validatePriorityOptions to be TypeError rather than the incorrect RangeError. Add tests to confirm that all errors are thrown as expected (weight, parent, exclusive, silent, endStream and getTrailers). Add test for method CONNECT throwing error on missing :authority or superfluous :scheme and :path. PR-URL: #15109 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Change error types emitted from request and validatePriorityOptions to be TypeError rather than the incorrect RangeError. Add tests to confirm that all errors are thrown as expected (weight, parent, exclusive, silent, endStream and getTrailers). Add test for method CONNECT throwing error on missing :authority or superfluous :scheme and :path. PR-URL: #15109 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Change error types emitted from request and validatePriorityOptions to be TypeError rather than the incorrect RangeError. Add tests to confirm that all errors are thrown as expected (weight, parent, exclusive, silent, endStream and getTrailers). Add test for method CONNECT throwing error on missing :authority or superfluous :scheme and :path. PR-URL: #15109 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
A few small things:
request
andvalidatePriorityOptions
(when validating options provided) to beTypeError
rather thanRangeError
request
) throwing errors on missing:authority
or superfluous:scheme
and:path
Please let me know if there's anything I can adjust. Thanks!
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
http2, test