Skip to content

test: Improve test-child-process-stdout-flush #8581

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.

Sign up for GitHub

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

Jump to bottom
Closed
wietsevenema wants to merge 1 commit into from
Closed

test: Improve test-child-process-stdout-flush #8581

wietsevenema wants to merge 1 commit into from

Conversation

@wietsevenema
Copy link

@wietsevenema wietsevenema commented Sep 17, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Changed vars to const / let, functions to lambdas and a mustCall where appropriate.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016
@eljefedelrodeodeljefe
Copy link
Contributor

eljefedelrodeodeljefe commented Sep 17, 2016

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 mscdex added the child_process Issues and PRs related to the child_process subsystem. label Sep 17, 2016
eljefedelrodeodeljefe
eljefedelrodeodeljefe suggested changes Sep 17, 2016
View reviewed changes
Copy link
Contributor

@eljefedelrodeodeljefe eljefedelrodeodeljefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

wietsevenema commented Sep 17, 2016

Changed commit message

cjihrig
cjihrig approved these changes Sep 17, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lpinca
lpinca approved these changes Sep 19, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

princejwesley
princejwesley approved these changes Sep 19, 2016
View reviewed changes
Copy link
Contributor

@princejwesley princejwesley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@imyller
Copy link
Member

imyller commented Sep 20, 2016

@eljefedelrodeodeljefe Could you review again? I believe the changes you've requested have been made. Thank you.

@imyller
Copy link
Member

imyller commented Sep 20, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/4153/

@imyller imyller self-assigned this Sep 20, 2016
santigimeno
santigimeno reviewed Sep 20, 2016
View reviewed changes
test/parallel/test-child-process-stdout-flush.js Outdated
Copy link
Member

@santigimeno santigimeno Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use common.fail here: child.stderr.on('data', common.fail);

test/parallel/test-child-process-stdout-flush.js Outdated
Copy link
Member

@santigimeno santigimeno Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the multiple console.log

test/parallel/test-child-process-stdout-flush.js Outdated
Copy link
Member

@santigimeno santigimeno Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The close event listener has two arguments: code and signal. I would check that code is 0 and signal is not defined.

@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

@wietsevenema ... I think this is almost there. Once this comments are addressed this should be good to go.

@wietsevenema
Copy link
Author

wietsevenema commented Sep 20, 2016

@jasnell Sure thing. Updated the commit and rebased with upstream

santigimeno
santigimeno reviewed Sep 20, 2016
View reviewed changes
test/parallel/test-child-process-stdout-flush.js Outdated
Copy link
Member

@santigimeno santigimeno Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this log

Copy link
Member

@imyller imyller Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. This should be removed.

Copy link
Author

@wietsevenema wietsevenema Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✓

@santigimeno
Copy link
Member

santigimeno commented Sep 20, 2016

LGTM with a nit

@santigimeno
Copy link
Member

santigimeno commented Sep 20, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/4169/

jasnell
jasnell approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

test: Improve test-child-process-stdout-flush
62854f7 
Changed vars to const / let, functions to lambdas and a mustCall where appropriate.
imyller
imyller approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jasnell pushed a commit that referenced this pull request Sep 20, 2016
@jasnell
test: improve test-child-process-stdout-flush
57a5136 
Changed vars to const / let, functions to arrow functions and a
mustCall where appropriate.

PR-URL: #8581
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

Landed in 57a5136! Thank you!

@jasnell jasnell closed this Sep 20, 2016
@imyller imyller removed their assignment Sep 20, 2016
@imyller
Copy link
Member

imyller commented Sep 20, 2016

@jasnell, looks like we were both landing the same PR at the same time :)
I've now removed my assignment from this.

Thank you for your contribution, @wietsevenema

@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

Sorry @imyller! I actually didn't see your assignment on there or I would have left it for you!

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@Fishrock123
test: improve test-child-process-stdout-flush
c664109 
Changed vars to const / let, functions to arrow functions and a
mustCall where appropriate.

PR-URL: #8581
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@santigimeno santigimeno santigimeno approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

+3 more reviewers

@eljefedelrodeodeljefe eljefedelrodeodeljefe eljefedelrodeodeljefe requested changes

@princejwesley princejwesley princejwesley approved these changes

@imyller imyller imyller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.