test: Improve test-child-process-stdout-flush #8581
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
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. |
|
|
||
| child.stderr.setEncoding('utf8'); | ||
| child.stderr.on('data', function(data) { | ||
| child.stderr.on('data', (data) => { | ||
| console.log('parent stderr: ' + data); | ||
| assert.ok(false); | ||
| }); |
There was a problem hiding this comment.
I would use common.fail here: child.stderr.on('data', common.fail);
| console.log('parent stderr: ' + data); | ||
| assert.ok(false); | ||
| }); | ||
|
|
||
| child.stdout.setEncoding('utf8'); | ||
| child.stdout.on('data', function(data) { | ||
| child.stdout.on('data', (data) => { | ||
| count += data.length; | ||
| console.log(count); |
There was a problem hiding this comment.
I would remove the multiple console.log
| console.log('okay'); | ||
| }); | ||
| })); |
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
| child.on('close', common.mustCall((code, signal) => { | ||
| assert.strictEqual(code, 0); | ||
| assert.strictEqual(signal, null); | ||
| assert.strictEqual(n, count); | ||
| console.log('okay'); |
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>
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.