test : updated test to use common.mustCall#17437
test : updated test to use common.mustCall#17437mithunsasidharan wants to merge 1 commit intonodejs:masterfrom mithunsasidharan:pr_7
Conversation
There was a problem hiding this comment.
This will no longer do what it claims to as the countdown is redeclared on each request. It will never go below 1 which means the callback will never fire.
In general, this test would be better if rewritten without Countdown and instead by using .once('request') with common.mustCall for the first test that would then declare on('request') with common.mustCall for the 2nd test.
Something like this basically:
server.once('request', common.mustCall((req, res) => {
server.on('request', common.mustCall((req, res) => {
res.end(Buffer.from('asdf'));
}));
// 1st test case code here
}));There was a problem hiding this comment.
@apapirovski : Thanks for the feedback. I've updated the PR with the changes and also shortened the commit message. Kindly review the PR now. Thanks !
mithunsasidharan
changed the title
There was a problem hiding this comment.
See line 23 -- common has already been required, but just not set to a variable.
There was a problem hiding this comment.
@maclover7 : Thanks for the feedback. I've updated the PR with the changes. Kindly review now and update the CI Checks. Thanks !
Trott
left a comment
Trott
left a comment
There was a problem hiding this comment.
The inclusion of common must be the first module loaded. Otherwise, it does not protect against leaked globals from other modules.
Trott
left a comment
Trott
left a comment
There was a problem hiding this comment.
LGTM once common is moved to be the first module loaded and if CI is OK.
|
@Trott : Thanks for the feedback. I've moved up |
maclover7
added
the
author ready
|
@maclover7 : Curious why |
|
@mithunsasidharan It looks like it's an unrelated failure, and the rest of the CI is green, so things should be okay. |
|
@apapirovski : Can you please land this ? Thanks a lot ! |
|
Landed in bb59063 Thank you for your contribution! |
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax
removed
the
author ready
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
11 participants
refactored test case in
test-http-res-write-end-dont-take-arrayto usecommon.mustCallas per issue #17169Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test