Repository: sindresorhus/execa
Pull request #227 (2019-05-10 10:32:39)

Improve `error.killed`
This changes the conditions behind `error.killed`.

### a) `childProcess.kill()` was successfully called

`true` in core Node.js, `true` in `execa`. Unchanged.

### b) `process.kill(childProcess.pid)` was called

`false` in core Node.js, `false` in `execa`. Unchanged.

Ideally this should be `true`, but I am not sure that's possible. The issue is that Windows uses `exitCode: 1` and no `signal`, while Unix uses `exitCode: null` and `signal: 'SIGTERM'`. The difference is due to Node.js emulating termination signals on Windows. On Windows (with just `exitCode: 1`) it seems impossible to know whether the child process was killed externally.

One consequence of this is: while `process.kill(childProcess.pid)` will set `error.killed` to `false` on both Windows and Unix, the error message will be different: `was killed with SIGTERM` on Unix, `failed with exit code 1` on Windows.

### c) `timeout` was reached

`true` in core Node.js, `true` in `execa`. Changed to `false` with this PR. We have `error.timedOut` for this. This makes those boolean error properties orthogonal to each other.

### d) `maxBuffer` was reached

`true` in core Node.js, `false` in `execa`. Unchanged. We should add a property similar to `error.timedOut` but for `maxBuffer` instead. I can do it in a separate PR.

Note: this not does change `childProcess.killed`, which is a separate property and is `true` when `childProcess()` was successfully called (in both core Node.js and `execa`).

sindresorhus: Can you fix the merge conflict?
sindresorhus: > On Windows (with just exitCode: 1) it seems impossible to know whether the child process was killed externally.

Should we open an Node.js issue about that?
ehmicky: Merge conflict fixed.

I have opened [an issue on Node](https://github.com/nodejs/node/issues/27642). However I think this PR could still be merged regardless: if that issue resolves, we can create a new PR so that `error.killed` is `true` on external signal termination.

Patch:
diff --git a/index.js b/index.js
index dc0f983..beeacdd 100644
--- a/index.js
+++ b/index.js
@@ -173,7 +173,7 @@ function getStream(process, stream, {encoding, buffer, maxBuffer}) {
 function makeError(result, options) {
 	const {stdout, stderr, code, signal} = result;
 	let {error} = result;
-	const {joinedCommand, timedOut, isCanceled, parsed: {options: {timeout}}} = options;
+	const {joinedCommand, timedOut, isCanceled, killed, parsed: {options: {timeout}}} = options;
 
 	const [exitCodeName, exitCode] = getCode(result, code);
 
@@ -196,8 +196,9 @@ function makeError(result, options) {
 	// it to `undefined`
 	error.signal = signal || undefined;
 	error.command = joinedCommand;
-	error.timedOut = Boolean(timedOut);
+	error.timedOut = timedOut;
 	error.isCanceled = isCanceled;
+	error.killed = killed && !timedOut;
 
 	if ('all' in result) {
 		error.all = result.all;
@@ -364,14 +365,10 @@ const execa = (command, args, options) => {
 					joinedCommand,
 					parsed,
 					timedOut,
-					isCanceled
+					isCanceled,
+					killed: spawned.killed
 				});
 
-				// TODO: missing some timeout logic for killed
-				// https://github.com/nodejs/node/blob/master/lib/child_process.js#L203
-				// error.killed = spawned.killed || killed;
-				error.killed = error.killed || spawned.killed;
-
 				if (!parsed.options.reject) {
 					return error;
 				}
@@ -449,7 +446,9 @@ module.exports.sync = (command, args, options) => {
 	if (result.error || result.status !== 0 || result.signal !== null) {
 		const error = makeError(result, {
 			joinedCommand,
-			parsed
+			parsed,
+			timedOut: false,
+			isCanceled: false
 		});
 
 		if (!parsed.options.reject) {
@@ -466,7 +465,9 @@ module.exports.sync = (command, args, options) => {
 		exitCode: 0,
 		exitCodeName: 'SUCCESS',
 		failed: false,
+		killed: false,
 		command: joinedCommand,
-		timedOut: false
+		timedOut: false,
+		isCanceled: false
 	};
 };


Test patch:
diff --git a/test.js b/test.js
index 5d7a4b4..ecdc7c6 100644
--- a/test.js
+++ b/test.js
@@ -293,7 +293,6 @@ test('error.killed is true if process was killed directly', async t => {
 	t.true(error.killed);
 });
 
-// TODO: Should this really be the case, or should we improve on child_process?
 test('error.killed is false if process was killed indirectly', async t => {
 	const cp = execa('forever');
 
@@ -305,6 +304,16 @@ test('error.killed is false if process was killed indirectly', async t => {
 	t.false(error.killed);
 });
 
+test('result.killed is false if not killed', async t => {
+	const result = await execa('noop');
+	t.false(result.killed);
+});
+
+test('result.killed is false if not killed, in sync mode', t => {
+	const result = execa.sync('noop');
+	t.false(result.killed);
+});
+
 if (process.platform === 'darwin') {
 	test.cb('sanity check: child_process.exec also has killed.false if killed indirectly', t => {
 		const cp = childProcess.exec('forever', error => {
@@ -362,6 +371,7 @@ test('error.code is 4', code, 4);
 
 test('timeout kills the process if it times out', async t => {
 	const error = await t.throwsAsync(execa('forever', {timeout: 1, message: TIMEOUT_REGEXP}));
+	t.false(error.killed);
 	t.true(error.timedOut);
 });
 
@@ -375,6 +385,11 @@ test('timedOut is false if no timeout was set', async t => {
 	t.false(result.timedOut);
 });
 
+test('timedOut will be false if no timeout was set and zero exit code in sync mode', t => {
+	const result = execa.sync('noop');
+	t.false(result.timedOut);
+});
+
 async function errorMessage(t, expected, ...args) {
 	await t.throwsAsync(execa('exit', args), {message: expected});
 }
@@ -578,11 +593,26 @@ test('cancel method kills the subprocess', t => {
 	t.true(subprocess.killed);
 });
 
-test('result.isCanceled is false when spawned.cancel isn\'t called', async t => {
+test('result.isCanceled is false when spawned.cancel() isn\'t called (success)', async t => {
 	const result = await execa('noop');
 	t.false(result.isCanceled);
 });
 
+test('result.isCanceled is false when spawned.cancel() isn\'t called (failure)', async t => {
+	const error = await t.throwsAsync(execa('fail'));
+	t.false(error.isCanceled);
+});
+
+test('result.isCanceled is false when spawned.cancel() isn\'t called in sync mode (success)', t => {
+	const result = execa.sync('noop');
+	t.false(result.isCanceled);
+});
+
+test('result.isCanceled is false when spawned.cancel() isn\'t called in sync mode (failure)', t => {
+	const error = t.throws(() => execa.sync('fail'));
+	t.false(error.isCanceled);
+});
+
 test('calling cancel method throws an error with message "Command was canceled"', async t => {
 	const subprocess = execa('noop');
 	subprocess.cancel();