refac: refactor test-http-response-multiheaders.js test to use countdown#17419
refac: refactor test-http-response-multiheaders.js test to use countdown#17419LEQADA wants to merge 3 commits intonodejs:masterfrom
Conversation
There was a problem hiding this comment.
Could we store 2 in a const or something? Potentially this could also be rewritten to utilize the countdown more fully (loop could be based on countdown being > 0, etc.) but at the very least we should remove magic numbers.
There was a problem hiding this comment.
@apapirovski might be a good idea. Do you have any variable name preferences? Maybe runs = 2 or runCount = 2?
There was a problem hiding this comment.
Either of those sound good 👍
LEQADA
force-pushed
the
use-countdown-test-http-response-multiheaders
branch
from
a80a543 to
6e9f719
Compare
|
Changed commit messages to have less than 72 chars. |
maclover7
left a comment
maclover7
left a comment
There was a problem hiding this comment.
One quick comment, then LGTM
| server.listen(0, common.mustCall(function() { | ||
| let count = 0; | ||
| for (let n = 1; n <= 2; n++) { | ||
| const runCount = 2; |
There was a problem hiding this comment.
Can you move runCount to live outside of the server.listen?
There was a problem hiding this comment.
I believe so -- although the linter may want an extra blank line (you can check this via make lint-js :))
There was a problem hiding this comment.
@maclover7 it says Running JS linter... and then exits. Does it mean that everything is ok? :)
There was a problem hiding this comment.
@LEQADA Yep -- if no errors show up then all good. You can also see the linter CI job passing at https://ci.nodejs.org/job/node-test-linter/14156/
maclover7
added
the
author ready
|
Landing... |
|
Landed in 35c01d8, congrats on your first PR to Node.js! |
PR-URL: #17419 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>
addaleax
removed
the
author ready
PR-URL: #17419 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>
PR-URL: #17419 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>
PR-URL: #17419 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>
PR-URL: #17419 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>
PR-URL: #17419 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>
8 participants
Refs #17169 to fix
test/parallel/test-http-response-multiheaders.jsChecklist
make -j4 test(UNIX), orvcbuild test(Windows) passes