Skip to content
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: include all stdio strings for fork() #11783

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 10, 2017

test-child-process-fork-stdio-string-variant was only testing 'pipe' for
its stdio value. Add inherit and ignore.

Also added a common.mustCall() to verify that the message event is
triggered.

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

test child_process

@Trott Trott added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Mar 10, 2017

assert.throws(() => fork(childScript, malFormedOpts), errorRegexp);

const child = fork(childScript, stringOpts);
function test(stringVariant) {
const child = fork(childScript, {stdio: 'pipe'});
Copy link
Member

Choose a reason for hiding this comment

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

The options object should be {stdio: stringVariant}.

Copy link
Member Author

Choose a reason for hiding this comment

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

<facepalm>...fixing...

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 with @lpinca's nit addressed.

test-child-process-fork-stdio-string-variant was only testing 'pipe' for
its `stdio` value. Add `inherit` and `ignore`.

Also added a `common.mustCall()` to verify that the `message` event is
triggered.
@Trott
Copy link
Member Author

Trott commented Mar 10, 2017

@Trott
Copy link
Member Author

Trott commented Mar 13, 2017

Landed in 78cdd4b

@Trott Trott closed this Mar 13, 2017
Trott added a commit to Trott/io.js that referenced this pull request Mar 13, 2017
test-child-process-fork-stdio-string-variant was only testing 'pipe' for
its `stdio` value. Add `inherit` and `ignore`.

Also added a `common.mustCall()` to verify that the `message` event is
triggered.

PR-URL: nodejs#11783
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@italoacasas
Copy link
Contributor

This is not landing clearly in v7 staging, @Trott can backport?

@Trott
Copy link
Member Author

Trott commented Mar 14, 2017

@italoacasas This depends on #10866 which is semver-major. Adding dont-land-on-* labels.

jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
test-child-process-fork-stdio-string-variant was only testing 'pipe' for
its `stdio` value. Add `inherit` and `ignore`.

Also added a `common.mustCall()` to verify that the `message` event is
triggered.

PR-URL: nodejs#11783
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@jasnell jasnell mentioned this pull request Apr 4, 2017
@Trott Trott deleted the ignore branch January 13, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants