Skip to content

child_process: exec's promisify impl now includes stdout/err in error #13388

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.

Sign up for GitHub

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

Closed
giltayar wants to merge 4 commits into from
Closed

child_process: exec's promisify impl now includes stdout/err in error #13388

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e44a057
child_process: exec's promisify impl now includes stdout/err in error
giltayar Jun 2, 2017
e2182e5
child_process: refactorings after review comments
giltayar Jun 2, 2017
6adbd98
child_process: fixed bug in promisified exec test
giltayar Jun 2, 2017
e37cb7e
child_process: fixed win compat bug in test
giltayar Jun 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
child_process: refactorings after review comments
  • Loading branch information
@giltayar
giltayar committed Jun 2, 2017
commit e2182e5688a6a660e15e4d9698b801f358c6712d
29 changes: 12 additions & 17 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,11 @@ exports.exec = function(command /*, options, callback*/) {
opts.callback);
};

Object.defineProperty(exports.exec, util.promisify.custom, {
enumerable: false, value: function(...args) {
const customPromiseExecFunction = (orig) => {
return (...args) => {
const promise = createPromise();
exports.exec.call(this, ...args, (err, stdout, stderr) => {

orig(...args, (err, stdout, stderr) => {
if (err !== null) {
err.stdout = stdout;
err.stderr = stderr;
Expand All @@ -153,7 +154,12 @@ Object.defineProperty(exports.exec, util.promisify.custom, {
}
});
return promise;
}
};
};

Object.defineProperty(exports.exec, util.promisify.custom, {
enumerable: false,
value: customPromiseExecFunction(exports.exec)
});

exports.execFile = function(file /*, args, options, callback*/) {
Expand Down Expand Up @@ -351,19 +357,8 @@ exports.execFile = function(file /*, args, options, callback*/) {
};

Object.defineProperty(exports.execFile, util.promisify.custom, {
enumerable: false, value: function(...args) {
const promise = createPromise();
exports.execFile.call(this, ...args, (err, stdout, stderr) => {
if (err !== null) {
err.stdout = stdout;
err.stderr = stderr;
promiseReject(promise, err);
} else {
promiseResolve(promise, { stdout, stderr });
}
});
return promise;
}
enumerable: false,
value: customPromiseExecFunction(exports.execFile)
});
Copy link
Copy Markdown
Member

@addaleax addaleax Jun 2, 2017

Choose a reason for hiding this comment

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

These two functions are nearly identical. It should be okay, but it shouldn’t be too hard to merge these, right?

Copy link
Copy Markdown
Contributor Author

@giltayar giltayar Jun 2, 2017

Choose a reason for hiding this comment

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

Will do


const _deprecatedCustomFds = deprecate(
Expand Down
10 changes: 5 additions & 5 deletions test/parallel/test-child-process-promisified.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,21 @@ const execFile = promisify(child_process.execFile);
}));
}
const failingCodeWithStdoutErr =
'console.log("out");console.error("err");process.exit(1)';
'console.log(42);console.error(42);process.exit(1)';
{
exec(`${process.execPath} -e '${failingCodeWithStdoutErr}'`)
Copy link
Copy Markdown
Member

@addaleax addaleax Jun 2, 2017

Choose a reason for hiding this comment

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

Does this work on Windows? I think there was some weirdness with single/double quotes. (If you don’t know, that’s okay; CI will tell us.)

Copy link
Copy Markdown
Contributor Author

@giltayar giltayar Jun 2, 2017

Choose a reason for hiding this comment

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

"A wise man does not fall into a hole that a smart man can get out of" :-). I'll change it to output numbers and then use double quotes like the other tests.

.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 1);
assert.strictEqual(err.stdout, 'out\n');
assert.strictEqual(err.stderr, 'err\n');
assert.strictEqual(err.stdout, '42\n');
assert.strictEqual(err.stderr, '43\n');
}));
}

{
execFile(process.execPath, ['-e', failingCodeWithStdoutErr])
.catch(common.mustCall((err) => {
assert.strictEqual(err.code, 1);
assert.strictEqual(err.stdout, 'out\n');
assert.strictEqual(err.stderr, 'err\n');
assert.strictEqual(err.stdout, '42\n');
assert.strictEqual(err.stderr, '43\n');
}));
}