Repository: sindresorhus/execa
Pull request #655 (2024-01-06 17:09:48)

Improve some tests
This PR improves some tests related to the `forceKillAfterTimeout` option, mostly removing some code duplication, and ensuring any child process's promise is awaited.


Patch:
diff --git a/index.js b/index.js
index bd940ba..512ebee 100644
--- a/index.js
+++ b/index.js
@@ -136,49 +136,7 @@ export function execa(rawFile, rawArgs, rawOptions) {
 	spawned.kill = spawnedKill.bind(null, spawned.kill.bind(spawned));
 	spawned.cancel = spawnedCancel.bind(null, spawned, context);
 
-	const handlePromise = async () => {
-		const [{error, exitCode, signal, timedOut}, stdoutResult, stderrResult, allResult] = await getSpawnedResult(spawned, options, stdioStreams, processDone);
-		const stdout = handleOutput(options, stdoutResult);
-		const stderr = handleOutput(options, stderrResult);
-		const all = handleOutput(options, allResult);
-
-		if (error || exitCode !== 0 || signal !== null) {
-			const returnedError = makeError({
-				error,
-				exitCode,
-				signal,
-				stdout,
-				stderr,
-				all,
-				command,
-				escapedCommand,
-				options,
-				timedOut,
-				isCanceled: context.isCanceled || (options.signal ? options.signal.aborted : false),
-			});
-
-			if (!options.reject) {
-				return returnedError;
-			}
-
-			throw returnedError;
-		}
-
-		return {
-			command,
-			escapedCommand,
-			exitCode: 0,
-			stdout,
-			stderr,
-			all,
-			failed: false,
-			timedOut: false,
-			isCanceled: false,
-			isTerminated: false,
-		};
-	};
-
-	const handlePromiseOnce = onetime(handlePromise);
+	const handlePromiseOnce = onetime(handlePromise.bind(undefined, {spawned, options, context, stdioStreams, command, escapedCommand, processDone}));
 
 	pipeOutputAsync(spawned, stdioStreams);
 
@@ -189,6 +147,54 @@ export function execa(rawFile, rawArgs, rawOptions) {
 	return spawned;
 }
 
+const handlePromise = async ({spawned, options, context, stdioStreams, command, escapedCommand, processDone}) => {
+	const [
+		{error, exitCode, signal, timedOut},
+		stdoutResult,
+		stderrResult,
+		allResult,
+	] = await getSpawnedResult(spawned, options, stdioStreams, processDone);
+	const stdout = handleOutput(options, stdoutResult);
+	const stderr = handleOutput(options, stderrResult);
+	const all = handleOutput(options, allResult);
+
+	if (error || exitCode !== 0 || signal !== null) {
+		const isCanceled = context.isCanceled || Boolean(options.signal?.aborted);
+		const returnedError = makeError({
+			error,
+			exitCode,
+			signal,
+			stdout,
+			stderr,
+			all,
+			command,
+			escapedCommand,
+			options,
+			timedOut,
+			isCanceled,
+		});
+
+		if (!options.reject) {
+			return returnedError;
+		}
+
+		throw returnedError;
+	}
+
+	return {
+		command,
+		escapedCommand,
+		exitCode: 0,
+		stdout,
+		stderr,
+		all,
+		failed: false,
+		timedOut: false,
+		isCanceled: false,
+		isTerminated: false,
+	};
+};
+
 export function execaSync(rawFile, rawArgs, rawOptions) {
 	const {file, args, command, escapedCommand, options} = handleArguments(rawFile, rawArgs, rawOptions);
 
diff --git a/lib/promise.js b/lib/promise.js
index a4773f3..73b0171 100644
--- a/lib/promise.js
+++ b/lib/promise.js
@@ -1,3 +1,5 @@
+import {once} from 'node:events';
+
 // eslint-disable-next-line unicorn/prefer-top-level-await
 const nativePromisePrototype = (async () => {})().constructor.prototype;
 
@@ -19,18 +21,7 @@ export const mergePromise = (spawned, promise) => {
 };
 
 // Use promises instead of `child_process` events
-export const getSpawnedPromise = spawned => new Promise((resolve, reject) => {
-	spawned.on('exit', (exitCode, signal) => {
-		resolve({exitCode, signal});
-	});
-
-	spawned.on('error', error => {
-		reject(error);
-	});
-
-	if (spawned.stdin) {
-		spawned.stdin.on('error', error => {
-			reject(error);
-		});
-	}
-});
+export const getSpawnedPromise = async spawned => {
+	const [exitCode, signal] = await once(spawned, 'exit');
+	return {exitCode, signal};
+};
diff --git a/lib/stream.js b/lib/stream.js
index 7c571be..4871a61 100644
--- a/lib/stream.js
+++ b/lib/stream.js
@@ -1,7 +1,8 @@
+import {Buffer} from 'node:buffer';
 import {once} from 'node:events';
 import {finished} from 'node:stream/promises';
 import process from 'node:process';
-import getStream, {getStreamAsBuffer, getStreamAsArrayBuffer} from 'get-stream';
+import getStream, {getStreamAsArrayBuffer} from 'get-stream';
 import mergeStreams from '@sindresorhus/merge-streams';
 
 // `all` interleaves `stdout` and `stderr`
@@ -24,40 +25,40 @@ export const makeAllStream = (spawned, {all}) => {
 // On failure, `result.stdout|stderr|all` should contain the currently buffered stream
 // They are automatically closed and flushed by Node.js when the child process exits
 // We guarantee this by calling `childProcess.kill()`
-const getBufferedData = async (stream, streamPromise) => {
-	// When `buffer` is `false`, `streamPromise` is `undefined` and there is no buffered data to retrieve
-	if (!stream || streamPromise === undefined) {
-		return;
-	}
-
+// When `buffer` is `false`, `streamPromise` is `undefined` and there is no buffered data to retrieve
+const getBufferedData = async (streamPromise, encoding) => {
 	try {
 		return await streamPromise;
 	} catch (error) {
-		return error.bufferedData;
+		return error.bufferedData === undefined ? undefined : applyEncoding(error.bufferedData, encoding);
 	}
 };
 
-const getStreamPromise = (stream, {encoding, buffer, maxBuffer}) => {
+const getStreamPromise = async (stream, {encoding, buffer, maxBuffer}) => {
 	if (!stream || !buffer) {
 		return;
 	}
 
-	// eslint-disable-next-line unicorn/text-encoding-identifier-case
-	if (encoding === 'utf8' || encoding === 'utf-8') {
-		return getStream(stream, {maxBuffer});
+	const contents = isUtf8Encoding(encoding)
+		? await getStream(stream, {maxBuffer})
+		: await getStreamAsArrayBuffer(stream, {maxBuffer});
+	return applyEncoding(contents, encoding);
+};
+
+const applyEncoding = (contents, encoding) => {
+	if (isUtf8Encoding(encoding)) {
+		return contents;
 	}
 
 	if (encoding === 'buffer') {
-		return getStreamAsArrayBuffer(stream, {maxBuffer}).then(arrayBuffer => new Uint8Array(arrayBuffer));
+		return new Uint8Array(contents);
 	}
 
-	return applyEncoding(stream, maxBuffer, encoding);
+	return Buffer.from(contents).toString(encoding);
 };
 
-const applyEncoding = async (stream, maxBuffer, encoding) => {
-	const buffer = await getStreamAsBuffer(stream, {maxBuffer});
-	return buffer.toString(encoding);
-};
+// eslint-disable-next-line unicorn/text-encoding-identifier-case
+const isUtf8Encoding = encoding => encoding === 'utf8' || encoding === 'utf-8';
 
 // Some `stdout`/`stderr` options create a stream, e.g. when passing a file path.
 // The `.pipe()` method automatically ends that stream when `childProcess.stdout|stderr` ends.
@@ -77,6 +78,10 @@ const waitForStreamEnd = ({value, type, direction}, processDone) => {
 		: Promise.race([processDone, throwOnStreamError(value)]);
 };
 
+const throwIfStreamError = (stream, processDone) => stream === null
+	? undefined
+	: Promise.race([processDone, throwOnStreamError(stream)]);
+
 const throwOnStreamError = async stream => {
 	const [error] = await once(stream, 'error');
 	throw error;
@@ -110,15 +115,16 @@ export const getSpawnedResult = async (
 			stdoutPromise,
 			stderrPromise,
 			allPromise,
+			throwIfStreamError(spawned.stdin, processDone),
 			...stdioStreams.map(stdioStream => waitForStreamEnd(stdioStream, processDone)),
 		]);
 	} catch (error) {
 		spawned.kill();
 		const results = await Promise.all([
 			{error, signal: error.signal, timedOut: error.timedOut},
-			getBufferedData(spawned.stdout, stdoutPromise),
-			getBufferedData(spawned.stderr, stderrPromise),
-			getBufferedData(spawned.all, allPromise),
+			getBufferedData(stdoutPromise, encoding),
+			getBufferedData(stderrPromise, encoding),
+			getBufferedData(allPromise, encoding),
 		]);
 		cleanupStdioStreams(stdioStreams, error);
 		return results;


Test patch:
diff --git a/test/kill.js b/test/kill.js
index 6c1bc37..498455b 100644
--- a/test/kill.js
+++ b/test/kill.js
@@ -31,49 +31,37 @@ if (process.platform !== 'win32') {
 
 		t.true(isRunning(subprocess.pid));
 		subprocess.kill('SIGKILL');
-	});
-
-	test('`forceKillAfterTimeout: number` should kill after a timeout', async t => {
-		const subprocess = execa('no-killable.js', {stdio: ['ipc']});
-		await pEvent(subprocess, 'message');
-
-		subprocess.kill('SIGTERM', {forceKillAfterTimeout: 50});
 
 		const {signal} = await t.throwsAsync(subprocess);
 		t.is(signal, 'SIGKILL');
 	});
 
-	test('`forceKillAfterTimeout: true` should kill after a timeout', async t => {
+	const testForceKill = async (t, killArguments) => {
 		const subprocess = execa('no-killable.js', {stdio: ['ipc']});
 		await pEvent(subprocess, 'message');
 
-		subprocess.kill('SIGTERM', {forceKillAfterTimeout: true});
+		subprocess.kill(...killArguments);
 
 		const {signal} = await t.throwsAsync(subprocess);
 		t.is(signal, 'SIGKILL');
-	});
-
-	test('kill() with no arguments should kill after a timeout', async t => {
-		const subprocess = execa('no-killable.js', {stdio: ['ipc']});
-		await pEvent(subprocess, 'message');
+	};
 
-		subprocess.kill();
+	test('`forceKillAfterTimeout: number` should kill after a timeout', testForceKill, ['SIGTERM', {forceKillAfterTimeout: 50}]);
+	test('`forceKillAfterTimeout: true` should kill after a timeout', testForceKill, ['SIGTERM', {forceKillAfterTimeout: true}]);
+	test('kill("SIGTERM") should kill after a timeout', testForceKill, ['SIGTERM']);
+	test('kill() with no arguments should kill after a timeout', testForceKill, []);
 
-		const {signal} = await t.throwsAsync(subprocess);
-		t.is(signal, 'SIGKILL');
-	});
-
-	test('`forceKillAfterTimeout` should not be NaN', t => {
+	const testInvalidForceKill = async (t, forceKillAfterTimeout) => {
+		const childProcess = execa('noop.js');
 		t.throws(() => {
-			execa('noop.js').kill('SIGTERM', {forceKillAfterTimeout: Number.NaN});
+			childProcess.kill('SIGTERM', {forceKillAfterTimeout});
 		}, {instanceOf: TypeError, message: /non-negative integer/});
-	});
+		const {signal} = await t.throwsAsync(childProcess);
+		t.is(signal, 'SIGTERM');
+	};
 
-	test('`forceKillAfterTimeout` should not be negative', t => {
-		t.throws(() => {
-			execa('noop.js').kill('SIGTERM', {forceKillAfterTimeout: -1});
-		}, {instanceOf: TypeError, message: /non-negative integer/});
-	});
+	test('`forceKillAfterTimeout` should not be NaN', testInvalidForceKill, Number.NaN);
+	test('`forceKillAfterTimeout` should not be negative', testInvalidForceKill, -1);
 }
 
 test('execa() returns a promise with kill()', t => {
diff --git a/test/node.js b/test/node.js
index 1311fb9..13c4da4 100644
--- a/test/node.js
+++ b/test/node.js
@@ -109,4 +109,7 @@ test('node\'s forked script has a communication channel', async t => {
 	t.is(message, 'pong');
 
 	subprocess.kill();
+
+	const {signal} = await t.throwsAsync(subprocess);
+	t.is(signal, 'SIGTERM');
 });
diff --git a/test/stream.js b/test/stream.js
index 852c4e6..cb8370d 100644
--- a/test/stream.js
+++ b/test/stream.js
@@ -102,6 +102,21 @@ const testMaxBuffer = async (t, streamName) => {
 test('maxBuffer affects stdout', testMaxBuffer, 'stdout');
 test('maxBuffer affects stderr', testMaxBuffer, 'stderr');
 
+test('maxBuffer works with encoding buffer', async t => {
+	const {stdout} = await t.throwsAsync(
+		execa('max-buffer.js', ['stdout', '11'], {maxBuffer: 10, encoding: 'buffer'}),
+	);
+	t.true(stdout instanceof Uint8Array);
+	t.is(Buffer.from(stdout).toString(), '.'.repeat(10));
+});
+
+test('maxBuffer works with other encodings', async t => {
+	const {stdout} = await t.throwsAsync(
+		execa('max-buffer.js', ['stdout', '11'], {maxBuffer: 10, encoding: 'hex'}),
+	);
+	t.is(stdout, Buffer.from('.'.repeat(10)).toString('hex'));
+});
+
 const testNoMaxBuffer = async (t, streamName) => {
 	const promise = execa('max-buffer.js', [streamName, '10'], {buffer: false});
 	const [result, output] = await Promise.all([
diff --git a/test/test.js b/test/test.js
index 0693099..995b9dd 100644
--- a/test/test.js
+++ b/test/test.js
@@ -1,5 +1,6 @@
 import path from 'node:path';
 import process from 'node:process';
+import {setTimeout} from 'node:timers/promises';
 import {fileURLToPath, pathToFileURL} from 'node:url';
 import test from 'ava';
 import isRunning from 'is-running';
@@ -124,10 +125,17 @@ const testExecPath = async (t, mapPath) => {
 test.serial('execPath option', testExecPath, identity);
 test.serial('execPath option can be a file URL', testExecPath, pathToFileURL);
 
-test('stdin errors are handled', async t => {
-	const subprocess = execa('noop.js');
+const emitStdinError = async subprocess => {
+	await setTimeout(0);
 	subprocess.stdin.emit('error', new Error('test'));
-	await t.throwsAsync(subprocess, {message: /test/});
+};
+
+test('stdin errors are handled', async t => {
+	const subprocess = execa('forever.js');
+	await Promise.all([
+		t.throwsAsync(subprocess, {message: /test/}),
+		emitStdinError(subprocess),
+	]);
 });
 
 test('child process errors are handled', async t => {