Repository: sindresorhus/execa (js)

Issue:
`extendEnv` does not work when combined with `shell: true`
The `extendEnv` option does not work as intended when combined with the `shell: true` option:

```js
process.env.TEST = 'test'
const result = execa.sync('echo $TEST', { shell: true, env: {}, extendEnv: true })
assert.equal(result.stdout, 'test')
```

`stdout` is an empty string but should be `'test'`

I will work on a PR to fix this.

Pull request description:
Fix `extendEnv` option not working with `shell` option
Fixes #183 

Options parsing was not correct for the `extendEnv` option which made it not work when combined with `shell: true` option.

Patch:
diff --git a/index.js b/index.js
index b4b5e34..07d683d 100644
--- a/index.js
+++ b/index.js
@@ -1,5 +1,6 @@
 'use strict';
 const path = require('path');
+const os = require('os');
 const childProcess = require('child_process');
 const crossSpawn = require('cross-spawn');
 const stripFinalNewline = require('strip-final-newline');
@@ -14,27 +15,31 @@ const stdio = require('./lib/stdio');
 const TEN_MEGABYTES = 1000 * 1000 * 10;
 
 function handleArgs(command, args, options) {
-	options = Object.assign({
-		extendEnv: true,
-		env: {}
-	}, options);
-
-	if (options.extendEnv) {
-		options.env = Object.assign({}, process.env, options.env);
-	}
-
 	const parsed = crossSpawn._parse(command, args, options);
+	command = parsed.command;
+	args = parsed.args;
+	options = parsed.options;
 
 	options = Object.assign({
 		maxBuffer: TEN_MEGABYTES,
 		buffer: true,
 		stripFinalNewline: true,
 		preferLocal: true,
-		localDir: parsed.options.cwd || process.cwd(),
+		localDir: options.cwd || process.cwd(),
 		encoding: 'utf8',
 		reject: true,
 		cleanup: true
-	}, parsed.options, {windowsHide: true});
+	}, options, {
+		windowsHide: true
+	});
+
+	if (options.extendEnv !== false) {
+		options.env = Object.assign({}, process.env, options.env);
+	}
+
+	if (options.preferLocal) {
+		options.env = npmRunPath.env(Object.assign({}, options, {cwd: options.localDir}));
+	}
 
 	// TODO: Remove in the next major release
 	if (options.stripEof === false) {
@@ -43,26 +48,17 @@ function handleArgs(command, args, options) {
 
 	options.stdio = stdio(options);
 
-	if (options.preferLocal) {
-		options.env = npmRunPath.env(Object.assign({}, options, {cwd: options.localDir}));
-	}
-
 	if (options.detached) {
 		// #115
 		options.cleanup = false;
 	}
 
-	if (process.platform === 'win32' && path.basename(parsed.command) === 'cmd.exe') {
+	if (process.platform === 'win32' && path.basename(command) === 'cmd.exe') {
 		// #116
-		parsed.args.unshift('/q');
+		args.unshift('/q');
 	}
 
-	return {
-		command: parsed.command,
-		args: parsed.args,
-		options,
-		parsed
-	};
+	return {command, args, options, parsed};
 }
 
 function handleInput(spawned, input) {
@@ -120,43 +116,67 @@ function getStream(process, stream, {encoding, buffer, maxBuffer}) {
 }
 
 function makeError(result, options) {
-	const {stdout, stderr} = result;
-
+	const {stdout, stderr, code, signal} = result;
 	let {error} = result;
-	const {code, signal} = result;
-
-	const {parsed, joinedCommand} = options;
-	const timedOut = options.timedOut || false;
+	const {joinedCommand, timedOut, parsed: {options: {timeout}}} = options;
 
-	if (!error) {
-		let output = '';
-
-		if (Array.isArray(parsed.options.stdio)) {
-			if (parsed.options.stdio[2] !== 'inherit') {
-				output += output.length > 0 ? stderr : `\n${stderr}`;
-			}
+	const [codeString, codeNumber] = getCode(result, code);
 
-			if (parsed.options.stdio[1] !== 'inherit') {
-				output += `\n${stdout}`;
-			}
-		} else if (parsed.options.stdio !== 'inherit') {
-			output = `\n${stderr}${stdout}`;
-		}
-
-		error = new Error(`Command failed: ${joinedCommand}${output}`);
-		error.code = code < 0 ? errname(code) : code;
+	if (!(error instanceof Error)) {
+		const message = [joinedCommand, stderr, stdout].filter(Boolean).join('\n');
+		error = new Error(message);
 	}
 
+	const prefix = getErrorPrefix({timedOut, timeout, signal, codeString, codeNumber});
+	error.message = `Command ${prefix}: ${error.message}`;
+
+	error.code = codeNumber || codeString;
 	error.stdout = stdout;
 	error.stderr = stderr;
 	error.failed = true;
 	error.signal = signal || null;
 	error.cmd = joinedCommand;
-	error.timedOut = timedOut;
+	error.timedOut = Boolean(timedOut);
 
 	return error;
 }
 
+function getCode({error = {}}, code) {
+	if (error.code) {
+		return [error.code, os.constants.errno[error.code]];
+	}
+
+	if (Number.isInteger(code)) {
+		return [errname(-Math.abs(code)), Math.abs(code)];
+	}
+
+	return [];
+}
+
+function getErrorPrefix({timedOut, timeout, signal, codeString, codeNumber}) {
+	if (timedOut) {
+		return `timed out after ${timeout} milliseconds`;
+	}
+
+	if (signal) {
+		return `was killed with ${signal}`;
+	}
+
+	if (codeString !== undefined && codeNumber !== undefined) {
+		return `failed with exit code ${codeNumber} (${codeString})`;
+	}
+
+	if (codeString !== undefined) {
+		return `failed with exit code ${codeString}`;
+	}
+
+	if (codeNumber !== undefined) {
+		return `failed with exit code ${codeNumber}`;
+	}
+
+	return 'failed';
+}
+
 function joinCommand(command, args) {
 	let joinedCommand = command;
 


Test patch:
diff --git a/test.js b/test.js
index 91fd135..532298d 100644
--- a/test.js
+++ b/test.js
@@ -14,6 +14,11 @@ process.env.FOO = 'foo';
 
 const NO_NEWLINES_REGEXP = /^[^\n]*$/;
 const STDERR_STDOUT_REGEXP = /stderr[^]*stdout/;
+const TIMEOUT_REGEXP = /timed out after/;
+
+function getExitRegExp(exitMessage) {
+	return new RegExp(`failed with exit code ${exitMessage}`);
+}
 
 test('execa()', async t => {
 	const {stdout} = await m('noop', ['foo']);
@@ -45,7 +50,7 @@ test('execa.stderr()', async t => {
 });
 
 test('stdout/stderr available on errors', async t => {
-	const err = await t.throwsAsync(m('exit', ['2']));
+	const err = await t.throwsAsync(m('exit', ['2']), {message: getExitRegExp('2')});
 	t.is(typeof err.stdout, 'string');
 	t.is(typeof err.stderr, 'string');
 });
@@ -99,7 +104,7 @@ test('execa.sync()', t => {
 });
 
 test('execa.sync() throws error if written to stderr', t => {
-	t.throws(() => m.sync('foo'), process.platform === 'win32' ? /'foo' is not recognized as an internal or external command/ : 'spawnSync foo ENOENT');
+	t.throws(() => m.sync('foo'), process.platform === 'win32' ? /'foo' is not recognized as an internal or external command/ : /spawnSync foo ENOENT/);
 });
 
 test('execa.sync() includes stdout and stderr in errors for improved debugging', t => {
@@ -256,9 +261,13 @@ test('skip throwing when using reject option', async t => {
 	t.is(typeof error.stderr, 'string');
 });
 
+test('allow unknown exit code', async t => {
+	await t.throwsAsync(m('exit', ['255']), {message: /exit code 255 \(Unknown system error -255\)/});
+});
+
 test('execa() returns code and failed properties', async t => {
 	const {code, failed} = await m('noop', ['foo']);
-	const error = await t.throwsAsync(m('exit', ['2']), {code: 2});
+	const error = await t.throwsAsync(m('exit', ['2']), {code: 2, message: getExitRegExp('2')});
 	t.is(code, 0);
 	t.false(failed);
 	t.true(error.failed);
@@ -284,7 +293,7 @@ test('error.killed is true if process was killed directly', async t => {
 		cp.kill();
 	}, 100);
 
-	const error = await t.throwsAsync(cp);
+	const error = await t.throwsAsync(cp, {message: /was killed with SIGTERM/});
 	t.true(error.killed);
 });
 
@@ -296,7 +305,9 @@ test('error.killed is false if process was killed indirectly', async t => {
 		process.kill(cp.pid, 'SIGINT');
 	}, 100);
 
-	const error = await t.throwsAsync(cp);
+	// `process.kill()` is emulated by Node.js on Windows
+	const message = process.platform === 'win32' ? /failed with exit code 1/ : /was killed with SIGINT/;
+	const error = await t.throwsAsync(cp, {message});
 	t.false(error.killed);
 });
 
@@ -322,7 +333,7 @@ if (process.platform !== 'win32') {
 			process.kill(cp.pid, 'SIGINT');
 		}, 100);
 
-		const error = await t.throwsAsync(cp);
+		const error = await t.throwsAsync(cp, {message: /was killed with SIGINT/});
 		t.is(error.signal, 'SIGINT');
 	});
 
@@ -333,12 +344,12 @@ if (process.platform !== 'win32') {
 			process.kill(cp.pid, 'SIGTERM');
 		}, 100);
 
-		const error = await t.throwsAsync(cp);
+		const error = await t.throwsAsync(cp, {message: /was killed with SIGTERM/});
 		t.is(error.signal, 'SIGTERM');
 	});
 
 	test('custom error.signal', async t => {
-		const error = await t.throwsAsync(m('delay', ['3000', '0'], {killSignal: 'SIGHUP', timeout: 1500}));
+		const error = await t.throwsAsync(m('delay', ['3000', '0'], {killSignal: 'SIGHUP', timeout: 1500, message: TIMEOUT_REGEXP}));
 		t.is(error.signal, 'SIGHUP');
 	});
 }
@@ -348,12 +359,12 @@ test('result.signal is null for successful execution', async t => {
 });
 
 test('result.signal is null if process failed, but was not killed', async t => {
-	const error = await t.throwsAsync(m('exit', [2]));
+	const error = await t.throwsAsync(m('exit', [2]), {message: getExitRegExp('2')});
 	t.is(error.signal, null);
 });
 
 async function code(t, num) {
-	await t.throwsAsync(m('exit', [`${num}`]), {code: num});
+	await t.throwsAsync(m('exit', [`${num}`]), {code: num, message: getExitRegExp(num)});
 }
 
 test('error.code is 2', code, 2);
@@ -361,14 +372,14 @@ test('error.code is 3', code, 3);
 test('error.code is 4', code, 4);
 
 test('timeout will kill the process early', async t => {
-	const error = await t.throwsAsync(m('delay', ['60000', '0'], {timeout: 1500}));
+	const error = await t.throwsAsync(m('delay', ['60000', '0'], {timeout: 1500, message: TIMEOUT_REGEXP}));
 
 	t.true(error.timedOut);
 	t.not(error.code, 22);
 });
 
 test('timeout will not kill the process early', async t => {
-	const error = await t.throwsAsync(m('delay', ['3000', '22'], {timeout: 30000}), {code: 22});
+	const error = await t.throwsAsync(m('delay', ['3000', '22'], {timeout: 30000}), {code: 22, message: getExitRegExp('22')});
 	t.false(error.timedOut);
 });
 
@@ -378,7 +389,7 @@ test('timedOut will be false if no timeout was set and zero exit code', async t
 });
 
 test('timedOut will be false if no timeout was set and non-zero exit code', async t => {
-	const error = await t.throwsAsync(m('delay', ['1000', '3']));
+	const error = await t.throwsAsync(m('delay', ['1000', '3']), {message: getExitRegExp('3')});
 	t.false(error.timedOut);
 });
 
@@ -388,8 +399,8 @@ async function errorMessage(t, expected, ...args) {
 
 errorMessage.title = (message, expected) => `error.message matches: ${expected}`;
 
-test(errorMessage, /Command failed: exit 2 foo bar/, 2, 'foo', 'bar');
-test(errorMessage, /Command failed: exit 3 baz quz/, 3, 'baz', 'quz');
+test(errorMessage, /Command failed with exit code 2.*: exit 2 foo bar/, 2, 'foo', 'bar');
+test(errorMessage, /Command failed with exit code 3.*: exit 3 baz quz/, 3, 'baz', 'quz');
 
 async function cmd(t, expected, ...args) {
 	const error = await t.throwsAsync(m('fail', args));
@@ -438,10 +449,8 @@ if (process.platform !== 'win32') {
 	test('cleanup false - SIGKILL', spawnAndKill, 'SIGKILL', false);
 }
 
-// See: https://github.com/sindresorhus/execa/issues/56
-const onlyWinFailing = test[process.platform === 'win32' ? 'failing' : 'serial'];
-onlyWinFailing('execa.shell() supports the `shell` option', async t => {
-	const {stdout} = await m.shell('noop foo', {
+test('execa.shell() supports the `shell` option', async t => {
+	const {stdout} = await m.shell('node fixtures/noop foo', {
 		shell: process.platform === 'win32' ? 'cmd.exe' : '/bin/bash'
 	});
 	t.is(stdout, 'foo');
@@ -455,7 +464,7 @@ if (process.platform !== 'win32') {
 			await m(`fast-exit-${process.platform}`, [], {input: 'data'});
 			t.pass();
 		} catch (error) {
-			t.is(error.code, 'EPIPE');
+			t.is(error.code, 32);
 		}
 	});
 }
@@ -478,7 +487,7 @@ test('extend environment variables by default', async t => {
 	]);
 });
 
-test('do not extend environment with `extendEnv` option', async t => {
+test('do not extend environment with `extendEnv: false`', async t => {
 	const result = await m.stdout('environment', [], {env: {BAR: 'bar', PATH: process.env.PATH}, extendEnv: false});
 
 	t.deepEqual(result.split('\n'), [
@@ -487,6 +496,14 @@ test('do not extend environment with `extendEnv` option', async t => {
 	]);
 });
 
+test('use extend environment with `extendEnv: true` and `shell: true`', async t => {
+	process.env.TEST = 'test';
+	const command = process.platform === 'win32' ? 'echo %TEST%' : 'echo $TEST';
+	const stdout = await m.stdout(command, {shell: true, env: {}, extendEnv: true});
+	t.is(stdout, 'test');
+	delete process.env.TEST;
+});
+
 test('do not buffer when streaming', async t => {
 	const result = await getStream(m('max-buffer', ['stdout', '21'], {maxBuffer: 10}).stdout);