test: Use mustCall in http test#17487
Conversation
apapirovski
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Could you update to use common.mustCall instead since it only runs 1 iteration? Countdown is better for tests where n > 1.
(Also, the commit message will need to be updated and should be 50 chars or less.)
Thanks!
There was a problem hiding this comment.
This test would be better if it used common.mustCall here instead of Countdown since it's just 1 iteration. The server.close() doesn't need to be conditional.
maclover7
left a comment
There was a problem hiding this comment.
LGTM once @apapirovski's comment is addressed
52bd113 to
e55bba6
Compare
|
@apapirovski Thanks for the feedback. Modified as per your suggestion. Please, have a look. |
apapirovski
left a comment
There was a problem hiding this comment.
Thanks for making the changes. Some feedback below. Also, the process.on('exit'); block can also be removed since there's now a common.mustCall which verifies that the test runs long enough.
There was a problem hiding this comment.
This line can be removed, we can safely call server.close() multiple times.
There was a problem hiding this comment.
Thanks. Done!!
There was a problem hiding this comment.
This should be common.mustCall which specifies COUNT as the 2nd argument, that way it will make sure that this function is called the right number of times. Something like:
common.mustCall((req, res) => {
// test code here
}, COUNT);There was a problem hiding this comment.
Thanks for explanation. Its Done.
e55bba6 to
de44a99
Compare
|
Landed in f81bb7b Thanks for the contribution @sreepurnajasti! 👍 |
PR-URL: #17487 Refs: #17169 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
PR-URL: #17487 Refs: #17169 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
PR-URL: #17487 Refs: #17169 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
PR-URL: #17487 Refs: #17169 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
PR-URL: #17487 Refs: #17169 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
PR-URL: #17487 Refs: #17169 Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Refs: #17169
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)