-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
test: Use mustCall in http test #17487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Use mustCall in http test #17487
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed, we can safely call server.close() multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Done!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)