refactor : test-http-request-dont-override-options to use common.mustCall #17438
Conversation
mithunsasidharan
changed the title
mithunsasidharan
changed the title
|
@apapirovski : Thanks for the feedback. I've updated the PR to include the recommended changes. Kindly review the PR now. |
|
|
||
| http.createServer(function(req, res) { | ||
| const server = http.createServer(function(req, res) { |
There was a problem hiding this comment.
common.mustCall should be added to this function.
| }).listen(0, function() { | ||
| server.listen(0); | ||
|
|
||
| server.on('listening', common.mustCall(function() { |
There was a problem hiding this comment.
We don't need to call on here since listen (line 13) accepts a callback.
| @@ -33,13 +34,11 @@ http.createServer(function(req, res) { | |||
| }).end(); | |||
There was a problem hiding this comment.
You could just put server.close() after res.resume() and then validate the assert conditions there (lines 37-42). That way we don't need the process.on('exit').
| assert.strictEqual(options.host, undefined); | ||
| assert.strictEqual(options.hostname, common.localhostIPv4); | ||
| assert.strictEqual(options.port, undefined); | ||
| assert.strictEqual(options.defaultPort, undefined); | ||
| assert.strictEqual(options.path, undefined); | ||
| assert.strictEqual(options.method, undefined); | ||
| }); | ||
| }).unref(); | ||
| })).unref(); |
There was a problem hiding this comment.
This can be removed if server.close() is added somewhere as per above.
|
@apapirovski : Thanks for the feedback. I've updated the PR addressing the review comments. Kindly review the PR. Thanks! |
| }).listen(0, function() { | ||
| server.listen(0, nextTest); | ||
|
|
||
| function nextTest() { |
There was a problem hiding this comment.
Can this be just inlined above? The name isn't very descriptive (and could confuse someone in the future) since there's only 1 test, not many.
server.listen(0, () => {...});
There was a problem hiding this comment.
@apapirovski : Thanks for the feedback. I've updated the PR as per the comment and also shortened the commit message. Kindly review the PR now. Thanks !
maclover7
added
the
author ready
|
Landing... |
|
Landed in 00c5b06 |
…stCall PR-URL: nodejs#17438 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
addaleax
removed
the
author ready
…stCall PR-URL: #17438 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
…stCall PR-URL: #17438 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
…stCall PR-URL: #17438 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
…stCall PR-URL: #17438 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
…stCall PR-URL: #17438 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
refactor :
test-http-request-dont-override-optionsto usecommon.mustCallas per issue #17169Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test