test : updated test to use common.mustCall #17437
Conversation
| res.write('string'); | ||
| // write should accept buffer | ||
| res.write(Buffer.from('asdf')); | ||
| const countdown = new Countdown(2, () => res.end(Buffer.from('asdf'))); |
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
| @@ -23,41 +23,37 @@ | |||
| require('../common'); | |||
| const assert = require('assert'); | |||
| const http = require('http'); | |||
| const common = require('../common'); | |||
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 : 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 <[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]>
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