Skip to content

Conversation

@cjihrig
Copy link
Contributor

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-botnodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Oct 27, 2016
Copy link
Member

@TrottTrott 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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
_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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
@cjihrigcjihrig merged commit ec7c27f into nodejs:masterOct 29, 2016
@cjihrigcjihrig 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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
@evanlucasevanlucas mentioned this pull request Nov 3, 2016
@MylesBorins
Copy link
Contributor

@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
ContributorAuthor

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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

@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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 21, 2016
@cjihrig
Copy link
ContributorAuthor

@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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

child_processIssues and PRs related to the child_process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@cjihrig@santigimeno@MylesBorins@jasnell@Trott@nodejs-github-bot