-
Notifications
You must be signed in to change notification settings - Fork 30k
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
test: refactor test-http-hostname-typechecking #13993
Conversation
* Use common.mustCall() to confirm callback is invoked. * Change spacing of require statements to conform to test-writing guide.
Landed in 5bbae25 |
* Use common.mustCall() to confirm callback is invoked. * Change spacing of require statements to conform to test-writing guide. PR-URL: #13993 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
http.request({hostname: v}).on('error', common.noop).end(); | ||
http.request({host: v}).on('error', common.noop).end(); | ||
http.request({hostname: v}).on('error', common.mustCall()).end(); | ||
http.request({host: v}).on('error', common.mustCall()).end(); |
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.
I think I ran into an issue like this before; the thing is, this is not guaranteed to fail, since the machine running the tests may have an HTTP server running on port 80.
@Trott Any suggestions? Can/should we revert this? The next-best alternative would be trying to allocate a port by creating a server, then closing it and trying to use that as the por value (or actually run an HTTP server that serves proper responses?).
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.
@addaleax My inclination would be to change common.mustCall()
to () => {}
to preserve the previous behavior. Perhaps bonus points for rethrowing the error if it's not ECONNREFUSED
.
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.
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.
(Oh! Reverting this one won't work because common.noop is no longer A Thing. So #14065 is probably the closest thing to a revert.)
* Use common.mustCall() to confirm callback is invoked. * Change spacing of require statements to conform to test-writing guide. PR-URL: #13993 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* Use common.mustCall() to confirm callback is invoked. * Change spacing of require statements to conform to test-writing guide. PR-URL: #13993 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* Use common.mustCall() to confirm callback is invoked. * Change spacing of require statements to conform to test-writing guide. PR-URL: #13993 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test http