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: add child_process customFds test #9307

Merged
merged 2 commits into from Oct 29, 2016
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 27, 2016

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

test, child_process

Description of change

This PR adds a test for spawn()'s deprecated customFds option.

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Oct 27, 2016
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

@santigimeno
Copy link
Member

This commit adds a test for spawn()'s deprecated customFds option.

PR-URL: nodejs#9307
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
_convertCustomFds() is only called from normalizeSpawnArguments(),
which always provides an options object. Therefore, there is no
need to check for options in _convertCustomFds().

PR-URL: nodejs#9307
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig cjihrig merged commit ec7c27f into nodejs:master Oct 29, 2016
@cjihrig cjihrig deleted the customfds branch October 29, 2016 17:35
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
This commit adds a test for spawn()'s deprecated customFds option.

PR-URL: #9307
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
_convertCustomFds() is only called from normalizeSpawnArguments(),
which always provides an options object. Therefore, there is no
need to check for options in _convertCustomFds().

PR-URL: #9307
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Member

@cjihrig should this be backported? I definitely think the test should be backported, but a bit iffy on the unreachable code with a review of v4 and v6

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 22, 2016

I think both commits should be good to backport, as normalizeSpawnArguments() sends back an options object on v4 and v6.

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
This commit adds a test for spawn()'s deprecated customFds option.

PR-URL: #9307
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
_convertCustomFds() is only called from normalizeSpawnArguments(),
which always provides an options object. Therefore, there is no
need to check for options in _convertCustomFds().

PR-URL: #9307
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Member

@cjihrig I've cherry-picked to v6.x. When landing on v4.x it is failing tests. Can you manually backport?

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
This commit adds a test for spawn()'s deprecated customFds option.

PR-URL: #9307
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
_convertCustomFds() is only called from normalizeSpawnArguments(),
which always provides an options object. Therefore, there is no
need to check for options in _convertCustomFds().

PR-URL: #9307
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 5, 2017

@MylesBorins it looks like some of the necessary APIs don't exist that far back, so let's not backport to v4.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants