-
Notifications
You must be signed in to change notification settings - Fork 31k
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
child_process: restore exec{File}Sync error props #16060
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,7 @@ | |
const common = require('../common'); | ||
const assert = require('assert'); | ||
|
||
const { execFileSync, execSync } = require('child_process'); | ||
const { execFileSync, execSync, spawnSync } = require('child_process'); | ||
|
||
const TIMER = 200; | ||
const SLEEP = 2000; | ||
|
@@ -112,6 +112,8 @@ assert.strictEqual(ret, `${msg}\n`); | |
// Verify the execFileSync() behavior when the child exits with a non-zero code. | ||
{ | ||
const args = ['-e', 'process.exit(1)']; | ||
const spawnSyncResult = spawnSync(process.execPath, args); | ||
const spawnSyncKeys = Object.keys(spawnSyncResult); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: const spawnSyncKeys = Object.keys(spawnSyncResult).sort();
assert.deepStrictEqual(spawnSyncKeys, [ /* ... */ ]); Right now you check that the corresponding properties exist on the error object but that won't catch regressions where the result of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
assert.throws(() => { | ||
execFileSync(process.execPath, args); | ||
|
@@ -121,6 +123,11 @@ assert.strictEqual(ret, `${msg}\n`); | |
assert(err instanceof Error); | ||
assert.strictEqual(err.message, msg); | ||
assert.strictEqual(err.status, 1); | ||
assert.strictEqual(typeof err.pid, 'number'); | ||
spawnSyncKeys.forEach((key) => { | ||
if (key === 'pid') return; | ||
assert.deepStrictEqual(err[key], spawnSyncResult[key]); | ||
}); | ||
return true; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this logic being lost by moving to
Object.assign()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but I don't think this logic should be there. It was also added in #13601.
The documentation says: "The Error object will contain the entire result from child_process.spawnSync()". If we want to translate the status, it should be done in
spawnSync
andexecSync
will inherit it.