FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

child_process: do not extend result for *Sync() by mscdex · Pull Request #13601 · nodejs/node · GitHub

/ node Public

child_process: do not extend result for *Sync() - #13601

Merged
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:child_process-no-extend-result
Jun 13, 2017
ConversationCommits (1)ChecksFiles changed
Merged

child_process: do not extend result for *Sync()#13601
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:child_process-no-extend-result

Conversation

mscdex commented Jun 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

It appears that when the synchronous child_process methods were added, additional information such as user-provided/parsed options, etc. was being copied to the returned object. Not only is this undocumented/unexpected, but it looks like this may have only been done for the purposes of tests (to test normalized/default options for example).

This PR extracts the actual sync spawning into an internal function which can then be monkey-patched as needed by tests.

I have also changed the errors returned by the exec*Sync() methods so that it matches that of the Error object mutation done for the async exec*() methods (e.g. err.result contains the libuv error name for status codes less than 0).

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

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

mscdex added child_process Issues and PRs related to the child_process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 10, 2017
nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jun 10, 2017

cjihrig commented Jun 10, 2017

Copy link
Copy Markdown
Contributor

Not only is this undocumented/unexpected, but it looks like this may have only been done for the purposes of tests (to test normalized/default options for example).

I agree that it is unexpected. It looks like this was the behavior going all the way back to at least 0.12.0. The tests referenced in this PR are all newer than that, so I'm not sure that the two things are related. @bnoordhuis or @sam-github do either of you know why the inputs were originally attached to the output?

refack approved these changes Jun 10, 2017
View reviewed changes
Comment thread test/parallel/test-child-process-spawnsync-shell.js Outdated

refack Jun 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

tiny nit: common.isWindows...

mscdex Jun 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think it really matters much since we're already explicitly passing 'win32' to this function below here.

mscdex force-pushed the child_process-no-extend-result branch from a19761a to 14f88fe Compare June 10, 2017 19:44

mscdex commented Jun 10, 2017

Copy link
Copy Markdown
Contributor Author

Forgot to re-add a removed test. New CI: https://ci.nodejs.org/job/node-test-pull-request/8594/

mscdex commented Jun 12, 2017

Copy link
Copy Markdown
Contributor Author

/cc @nodejs/ctc

mcollina approved these changes Jun 12, 2017
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mscdex force-pushed the child_process-no-extend-result branch from 14f88fe to 1ed6ece Compare June 13, 2017 00:45

jasnell commented Jun 13, 2017

Copy link
Copy Markdown
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/875/

sam-github commented Jun 13, 2017

Copy link
Copy Markdown
Contributor

Sorry, I don't know why they were added. Getting rid of them seems like a good idea.

refack commented Jun 13, 2017

Copy link
Copy Markdown
Contributor

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/875/

CITGM clean

jasnell commented Jun 13, 2017

Copy link
Copy Markdown
Member

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

child_process: do not extend result for *Sync()
448c4c6 
PR-URL: nodejs#13601
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
mscdex force-pushed the child_process-no-extend-result branch from 1ed6ece to 448c4c6 Compare June 13, 2017 23:20
mscdex merged commit 448c4c6 into nodejs:master Jun 13, 2017
mscdex deleted the child_process-no-extend-result branch June 13, 2017 23:22
targos mentioned this pull request Oct 6, 2017
Closed
3 tasks
targos mentioned this pull request Oct 7, 2017
Closed
3 tasks
targos added a commit to targos/node that referenced this pull request Oct 21, 2017
child_process: restore exec{File}Sync error props
c88256b 
In PR [1], a bunch of properties were removed from the error thrown by
execSync and execFileSync. It turns out that some of those were still
supposed to be there, as the documentation states that the error
contains the entire result from the spawnSync call.

[1] nodejs#13601
targos added a commit that referenced this pull request Oct 23, 2017
child_process: restore exec{File}Sync error props
556ebab 
In PR [1], a bunch of properties were removed from the error thrown by
execSync and execFileSync. It turns out that some of those were still
supposed to be there, as the documentation states that the error
contains the entire result from the spawnSync call.

[1] #13601

PR-URL: #16060
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
child_process: restore exec{File}Sync error props
fccd192 
In PR [1], a bunch of properties were removed from the error thrown by
execSync and execFileSync. It turns out that some of those were still
supposed to be there, as the documentation states that the error
contains the entire result from the spawnSync call.

[1] nodejs/node#13601

PR-URL: nodejs/node#16060
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell mentioned this pull request Oct 30, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
child_process: restore exec{File}Sync error props
a8b2622 
In PR [1], a bunch of properties were removed from the error thrown by
execSync and execFileSync. It turns out that some of those were still
supposed to be there, as the documentation states that the error
contains the entire result from the spawnSync call.

[1] nodejs/node#13601

PR-URL: nodejs/node#16060
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
refack refack 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. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL