test : updated test to use common.mustCall#17437
Uh oh!
There was an error while loading. Please reload this page.
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 
apapirovski commented Dec 3, 2017
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.
mithunsasidharan commented Dec 4, 2017
@Trott : Thanks for the feedback. I've moved up |
maclover7 commented Dec 5, 2017
maclover7 added the author ready mithunsasidharan commented Dec 5, 2017
@maclover7 : Curious why |
maclover7 commented Dec 5, 2017
@mithunsasidharan It looks like it's an unrelated failure, and the rest of the CI is green, so things should be okay. |
mithunsasidharan commented Dec 6, 2017
@apapirovski : Can you please land this ? Thanks a lot ! |
apapirovski commented Dec 6, 2017
Landed in bb59063 Thank you for your contribution! |
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax removed the author ready 
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #17437 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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