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: add missing console.error to exec-maxBuffer #14796

Closed
wants to merge 1 commit into
base: master
from

Conversation

@BethGriggs
Member

BethGriggs commented Aug 12, 2017

While looking at this test I noticed that the final cmd calls console.('${unicode}'). To me, it wasn't clear if this was intentional or if it should be console.error('${unicode}').

https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-exec-maxBuffer.js#L39

If it was intentional I am happy for this to be closed and/or I can add a comment to the test.

/cc @Trott as author of the test case

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

test: add missing console.error to exec-maxBuffer
Adds the missing console.error to test-child-process-exec-maxBuffer
@vsemozhetbyt

This comment has been minimized.

Show comment
Hide comment
@vsemozhetbyt

vsemozhetbyt Aug 12, 2017

Member

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

EDIT(gibfahn): CI was green except for an ARM flake.

Member

vsemozhetbyt commented Aug 12, 2017

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

EDIT(gibfahn): CI was green except for an ARM flake.

@gibfahn

LGTM, but I'd appreciate confirmation from @Trott

@gibfahn gibfahn requested a review from Trott Aug 12, 2017

@Trott

Trott approved these changes Aug 13, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 13, 2017

Member

Sure looks like an error on my part. Thanks for catching it and fixing it.

Member

Trott commented Aug 13, 2017

Sure looks like an error on my part. Thanks for catching it and fixing it.

@aqrln

aqrln approved these changes Aug 13, 2017

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Aug 16, 2017

Member

Landed in 4f1eddf, thank you! 🎉

Member

tniessen commented Aug 16, 2017

Landed in 4f1eddf, thank you! 🎉

@tniessen tniessen closed this Aug 16, 2017

tniessen added a commit that referenced this pull request Aug 16, 2017

test: add missing console.error to exec-maxBuffer
Adds the missing console.error to test-child-process-exec-maxBuffer

PR-URL: #14796
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MSLaguana added a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017

test: add missing console.error to exec-maxBuffer
Adds the missing console.error to test-child-process-exec-maxBuffer

PR-URL: nodejs/node#14796
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins added a commit that referenced this pull request Sep 10, 2017

test: add missing console.error to exec-maxBuffer
Adds the missing console.error to test-child-process-exec-maxBuffer

PR-URL: #14796
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 20, 2017

test: add missing console.error to exec-maxBuffer
Adds the missing console.error to test-child-process-exec-maxBuffer

PR-URL: #14796
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins added a commit that referenced this pull request Sep 20, 2017

test: add missing console.error to exec-maxBuffer
Adds the missing console.error to test-child-process-exec-maxBuffer

PR-URL: #14796
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

@BethGriggs BethGriggs deleted the BethGriggs:fix-child-process-exec-maxbuff branch Feb 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment