test: update test-http-timeout-overflow to use common.mustCall #17528
Conversation
apapirovski
left a comment
apapirovski
left a comment
There was a problem hiding this comment.
Hi @collin5 — thank you for taking this on! this particular test would be better with using common.mustCall for both the function within http.createServer and within res.on('end'). Then the process.on('exit') block can also be removed.
Countdown is better when used for tests where n > 1.
| response.end('OK'); | ||
| }); | ||
|
|
||
| const server = http.createServer(function(req, res) { |
There was a problem hiding this comment.
This could be common.mustCall(function(req, res) { /* function body */ }) which would then confirm that it executes during the duration of the test. Then no Countdown is needed.
There was a problem hiding this comment.
@apapirovski Right, that actually makes more sense. Let me do it ASAP 👍
| }, function(res) { | ||
| req.clearTimeout(callback); | ||
|
|
||
| res.on('end', function() { |
There was a problem hiding this comment.
This can also use common.mustCall so common.mustCall(function() { /* function body */ }).
|
@apapirovski Thanks for the feedback, I've pushed the changes 👍. |
apapirovski
left a comment
apapirovski
left a comment
There was a problem hiding this comment.
LGTM. Thanks for making the changes!
apapirovski
changed the title
PR-URL: #17528 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
|
@apapirovski Thank you too 👍 |
PR-URL: #17528 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: #17528 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: #17528 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: #17528 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: #17528 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
4 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
Updated tests in /test/parallel/test-http-timeout-overflow.js to use Countdown module.
Ref issue #17169