test: Improve test-child-process-stdout-flush#8581
test: Improve test-child-process-stdout-flush#8581wietsevenema wants to merge 1 commit intonodejs:masterfrom
Conversation
|
Refers to nodejs/code-and-learn#56. Not sure if Anna has mentioned it otherwise, but please edit the commit message to meet the contribution guidelines, by adding a small line to change you made and what it refers to. Otherwise LGTM |
mscdex
added
the
child_process
eljefedelrodeodeljefe
left a comment
eljefedelrodeodeljefe
left a comment
There was a problem hiding this comment.
Refers to nodejs/code-and-learn#56. Not sure if Anna has mentioned it otherwise, but please edit the commit message to meet the contribution guidelines, by adding a small line to change you made and what it refers to.
Otherwise LGTM
wietsevenema
force-pushed
the
master
branch
from
db88c43 to
ad8c02b
Compare
|
Changed commit message |
wietsevenema
force-pushed
the
master
branch
from
ad8c02b to
0902725
Compare
|
@eljefedelrodeodeljefe Could you review again? I believe the changes you've requested have been made. Thank you. |
There was a problem hiding this comment.
I would use common.fail here: child.stderr.on('data', common.fail);
There was a problem hiding this comment.
I would remove the multiple console.log
There was a problem hiding this comment.
The close event listener has two arguments: code and signal. I would check that code is 0 and signal is not defined.
|
@wietsevenema ... I think this is almost there. Once this comments are addressed this should be good to go. |
|
@jasnell Sure thing. Updated the commit and rebased with upstream |
wietsevenema
force-pushed
the
master
branch
from
0902725 to
3083af1
Compare
There was a problem hiding this comment.
I agree. This should be removed.
|
LGTM with a nit |
Changed vars to const / let, functions to lambdas and a mustCall where appropriate.
wietsevenema
force-pushed
the
master
branch
from
3083af1 to
62854f7
Compare
Changed vars to const / let, functions to arrow functions and a mustCall where appropriate. PR-URL: #8581 Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
|
Landed in 57a5136! Thank you! |
|
@jasnell, looks like we were both landing the same PR at the same time :) Thank you for your contribution, @wietsevenema |
|
Sorry @imyller! I actually didn't see your assignment on there or I would have left it for you! |
Changed vars to const / let, functions to arrow functions and a mustCall where appropriate. PR-URL: #8581 Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
11 participants
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change
Changed vars to const / let, functions to lambdas and a mustCall where appropriate.