From c911c67c44a5aeed6553d1a6d37be71fb169c060 Mon Sep 17 00:00:00 2001 From: "Kent C. Dodds" Date: Fri, 2 Sep 2016 19:15:36 -0600 Subject: [PATCH] fix(error): when an error occurs, stop processes This is a significant refactor to make it so when an error occurs in one parallel script, the others are killed and also to ensure that an error in one serial script will result in the others not being executed. Closes #48 --- other/ERRORS_AND_WARNINGS.md | 18 +++ package.json | 4 +- src/bin/p-s.js | 9 +- src/get-logger.js | 7 +- src/index.js | 116 +++++++++++++---- src/index.test.js | 246 ++++++++++++++++++++++++++++------- 6 files changed, 319 insertions(+), 81 deletions(-) diff --git a/other/ERRORS_AND_WARNINGS.md b/other/ERRORS_AND_WARNINGS.md index 8e2dbe0..aca3c3b 100644 --- a/other/ERRORS_AND_WARNINGS.md +++ b/other/ERRORS_AND_WARNINGS.md @@ -27,3 +27,21 @@ This happens when you use the `--require` flag and the module you specify cannot 1. Check that you spelled the module correctly 2. Check that the module you wish to require is require-able + +## Failed with exit code + +This means that one of the scripts p-s tried to run resulted in a non-zero exit code (a failing exit code) + +### To Fix: + +Try to run the script without `p-s` and verify that the script is working. If not, fix that. If it's working without `p-s` it could be a problem with `p-s`. Please file an issue. + +## Emitted an error + +This means that the child process for the specified script emitted an error. + +### To Fix: + +Look at the error and try to figure out why the script would be failing. + +Try to run the script without `p-s` and verify that the script is working. If not, fix that. If it's working without `p-s` it could be a problem with `p-s`. Please file an issue. diff --git a/package.json b/package.json index 749d3d3..b6de649 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "license": "MIT", "dependencies": { "arrify": "1.0.1", - "async": "2.0.1", + "bluebird": "3.4.6", "colors": "1.1.2", "commander": "2.9.0", "find-up": "1.1.2", @@ -30,7 +30,7 @@ "omelette": "0.3.1", "prefix-matches": "0.0.9", "shell-escape": "0.2.0", - "spawn-command": "0.0.2" + "spawn-command-with-kill": "1.0.0" }, "devDependencies": { "all-contributors-cli": "3.0.6", diff --git a/src/bin/p-s.js b/src/bin/p-s.js index 54efb9a..7ee0dbe 100755 --- a/src/bin/p-s.js +++ b/src/bin/p-s.js @@ -60,12 +60,9 @@ function loadAndRun(scriptsAndArgs, psConfig) { parallel: scriptsAndArgs.parallel, logLevel: program.logLevel, }), - }, result => { - if (result.error) { - log.error(result.error) - process.exit(FAIL_CODE) - } - process.exit(result.code) + }).catch(error => { + log.error(error) + process.exitCode = error.code || FAIL_CODE }) } diff --git a/src/get-logger.js b/src/get-logger.js index 9d37e0b..d0885bd 100644 --- a/src/get-logger.js +++ b/src/get-logger.js @@ -31,7 +31,12 @@ function getLogger(logLevel) { function getMessage(first, ...rest) { if (isPlainObject(first) && first.message && first.ref) { - return [...arrify(first.message), getLink(first.ref), ...rest] + return [ + ...arrify(first.message), + getLink(first.ref), + first.error, + ...rest, + ].filter(i => !!i) } else { return [first, ...rest] } diff --git a/src/index.js b/src/index.js index 23a94b2..3fb39d5 100644 --- a/src/index.js +++ b/src/index.js @@ -1,7 +1,7 @@ -import spawn from 'spawn-command' -import async from 'async' +import spawn from 'spawn-command-with-kill' +import Promise from 'bluebird' import colors from 'colors/safe' -import {isString, find, clone} from 'lodash' +import {isString, clone} from 'lodash' import {sync as findUpSync} from 'find-up' import managePath from 'manage-path' import arrify from 'arrify' @@ -9,49 +9,117 @@ import getScriptToRun from './get-script-to-run' import getScriptsFromConfig from './get-scripts-from-config' import getLogger from './get-logger' -const noop = () => {} // eslint-disable-line func-style, no-empty-function +const NON_ERROR = 0 export default runPackageScripts -function runPackageScripts({scriptConfig, scripts, args, options = {}}, callback = noop) { +function runPackageScripts({scriptConfig, scripts, args, options = {}}) { if (scripts.length === 0) { scripts = ['default'] } const scriptNames = arrify(scripts) - const method = options.parallel ? 'map' : 'mapSeries' - async[method](scriptNames, (scriptName, cb) => { - const child = runPackageScript({scriptConfig, options, scriptName, args}) - if (child.on) { - child.on('exit', exitCode => cb(null, exitCode)) - } else { - cb(child) - } - }, (err, results) => { - if (err) { - callback({error: err}) - } else { - const NON_ERROR = 0 - const result = find(results, r => r !== NON_ERROR) - callback({code: result}) + if (options.parallel) { + return runParallel() + } else { + return runSeries() + } + + function runSeries() { + return scriptNames.reduce((res, scriptName) => { + return res.then(() => ( + runPackageScript({scriptConfig, options, scriptName, args}) + )) + }, Promise.resolve()) + } + + function runParallel() { + const results = scriptNames.map(script => ({script, code: undefined})) + let aborted = false + + const promises = scriptNames.map(scriptName => { + return runPackageScript({scriptConfig, options, scriptName, args}) + }) + + const allPromise = Promise.all(promises.map((promise, index) => { + return promise.then(code => { + if (!aborted) { + results[index].code = code + } + }) + })).then(() => results) + + allPromise.catch(() => { + /* istanbul ignore if */ + if (aborted) { + // this is very unlikely to happen + } else { + abortAll() + } + }) + + return allPromise + + function abortAll() { + aborted = true + promises.forEach(p => p.abort()) } - }) + } } + function runPackageScript({scriptConfig, options, scriptName, args}) { const scripts = getScriptsFromConfig(scriptConfig, scriptName) const script = getScriptToRun(scripts, scriptName) if (!isString(script)) { - return { + return Promise.reject({ message: colors.red( `Scripts must resolve to strings. There is no script that can be resolved from "${scriptName}"` ), ref: 'missing-script', - } + }) } const command = [script, args].join(' ').trim() const log = getLogger(getLogLevel(options)) log.info(colors.gray('p-s executing: ') + colors.green(command)) - return spawn(command, {stdio: 'inherit', env: getEnv()}) + let child + const promise = new Promise((resolve, reject) => { + child = spawn(command, {stdio: 'inherit', env: getEnv()}) + + child.on('error', error => { + child = null + reject({ + message: colors.red( + `The script called "${scriptName}" which runs "${command}" emitted an error` + ), + ref: 'emitted-an-error', + error, + }) + }) + + child.on('close', code => { + child = null + if (code === NON_ERROR) { + resolve(code) + } else { + reject({ + message: colors.red( + `The script called "${scriptName}" which runs "${command}" failed with exit code ${code}` + ), + ref: 'failed-with-exit-code', + code, + }) + } + }) + }) + + promise.abort = function abort() { + if (child !== null) { + child.kill() + child = null + } + } + + return promise } function getLogLevel({silent, logLevel}) { diff --git a/src/index.test.js b/src/index.test.js index fbf5e48..862e1fe 100644 --- a/src/index.test.js +++ b/src/index.test.js @@ -1,119 +1,256 @@ import {resolve} from 'path' +import Promise from 'bluebird' import test from 'ava' import {spy} from 'sinon' import color from 'colors/safe' import {clone} from 'lodash' import managePath from 'manage-path' import proxyquire from 'proxyquire' -import async from 'async' proxyquire.noCallThru() test('spawn called with the parent process.env + npm path', t => { - const {options: {env}} = testSpawnCallWithDefaults(t) - const copy = clone(process.env) - managePath(copy).unshift(resolve('../node_modules/.bin')) - t.deepEqual(env, copy) + return testSpawnCallWithDefaults(t).then(({options: {env}}) => { + const copy = clone(process.env) + managePath(copy).unshift(resolve('../node_modules/.bin')) + t.deepEqual(env, copy) + }) }) test('does not blow up if there is no node_modules/.bin', t => { - const {options: {env}} = testSpawnCallWithDefaults(t, undefined, { + return testSpawnCallWithDefaults(t, undefined, { 'find-up': {sync: () => null}, + }).then(({options: {env}}) => { + t.deepEqual(env, process.env) }) - t.deepEqual(env, process.env) }) test('spawn called with the expected command', t => { const lintCommand = 'eslint .' - const {command} = testSpawnCall(t, {lint: lintCommand}, 'lint') - t.is(command, lintCommand) + return testSpawnCall(t, {lint: lintCommand}, 'lint').then(({command}) => { + t.is(command, lintCommand) + }) }) -test('spawn.on called with "exit"', t => { - const {onSpy} = testSpawnCallWithDefaults(t) - t.true(onSpy.calledOnce) +test('spawn.on called with "close" and "error"', t => { + return testSpawnCallWithDefaults(t).then(({onSpy}) => { + t.true(onSpy.calledTwice) + t.true(onSpy.calledWith('close')) + t.true(onSpy.calledWith('error')) + }) }) -test.cb('returns a log object when no script is found', t => { +test('returns a log object when no script is found', t => { const {runPackageScript} = setup() const scriptConfig = {lint: {script: 42}} - runPackageScript({scriptConfig, scripts: ['lint']}, ({error}) => { + return runPackageScript({scriptConfig, scripts: ['lint']}).catch(error => { t.deepEqual(error, { message: color.red('Scripts must resolve to strings. There is no script that can be resolved from "lint"'), ref: 'missing-script', }) - t.end() }) }) test('options: silent sets the logLevel to disable', t => { const options = {silent: true} - const {getLoggerSpy} = testSpawnCallWithDefaults(t, options) - t.true(getLoggerSpy.calledOnce) - t.true(getLoggerSpy.calledWith('disable')) + return testSpawnCallWithDefaults(t, options).then(({getLoggerSpy}) => { + t.true(getLoggerSpy.calledOnce) + t.true(getLoggerSpy.calledWith('disable')) + }) }) test('options: logLevel sets the log level', t => { const options = {logLevel: 'warn'} - const {getLoggerSpy} = testSpawnCallWithDefaults(t, options) - t.true(getLoggerSpy.calledOnce) - t.true(getLoggerSpy.calledWith('warn')) + return testSpawnCallWithDefaults(t, options).then(({getLoggerSpy}) => { + t.true(getLoggerSpy.calledOnce) + t.true(getLoggerSpy.calledWith('warn')) + }) }) test('passes on additional arguments', t => { const lintCommand = 'eslint' const argsToPassOn = 'src/ scripts/' - const {command} = testSpawnCall(t, {lint: lintCommand}, 'lint', {}, argsToPassOn) - t.is(command, `${lintCommand} ${argsToPassOn}`) + return testSpawnCall(t, {lint: lintCommand}, 'lint', {}, argsToPassOn).then(({command}) => { + t.is(command, `${lintCommand} ${argsToPassOn}`) + }) }) -test.cb('runs scripts serially if given an array of input without parallel', t => { +test('runs scripts serially if given an array of input without parallel', t => { const lintCommand = 'eslint' const buildCommand = 'babel' const args = 'src/ scripts/' - const mapSeriesSpy = spy(async.mapSeries) + const resolveSpy = spy(Promise, 'resolve') const {runPackageScript, spawnStubSpy} = setup({ - async: {mapSeries: mapSeriesSpy}, + bluebird: Promise, }) const scripts = ['lint', 'build'] const scriptConfig = {build: buildCommand, lint: lintCommand} const options = {} - runPackageScript({scriptConfig, scripts, args, options}, () => { - t.true(mapSeriesSpy.calledOnce) + runPackageScript({scriptConfig, scripts, args, options}).then(() => { + t.true(resolveSpy.calledOnce) t.true(spawnStubSpy.calledTwice) const [command1] = spawnStubSpy.firstCall.args const [command2] = spawnStubSpy.secondCall.args t.is(command1, 'eslint src/ scripts/') t.is(command2, 'babel src/ scripts/') - t.end() }) }) -test.cb('runs scripts in parallel if given an array of input', t => { +test('stops running scripts when running serially if any given script fails', t => { + const FAIL_CODE = 1 + const badCommand = 'bad' + const goodCommand = 'good' + const scripts = ['badS', 'goodS'] + const scriptConfig = { + goodS: goodCommand, + badS: badCommand, + } + function spawnStub(cmd) { + return { + on(event, cb) { + if (event === 'close') { + if (cmd === badCommand) { + cb(FAIL_CODE) + } else { + cb(0) + } + } + }, + kill() { + }, + } + } + const spawnStubSpy = spy(spawnStub) + const {runPackageScript} = setup({ + 'spawn-command-with-kill': spawnStubSpy, + }) + return runPackageScript({scriptConfig, scripts}).then(() => { + t.fail('the promise should be rejected') + }, ({message, ref, code}) => { + t.is(code, FAIL_CODE) + t.true(typeof ref === 'string') + t.true(typeof message === 'string') + t.true(spawnStubSpy.calledOnce) // only called with the bad script, not for the good one + const [command1] = spawnStubSpy.firstCall.args + t.is(command1, 'bad') + }) +}) + +test('runs scripts in parallel if given an array of input', t => { const lintCommand = 'eslint' const buildCommand = 'babel' const args = 'src/ scripts/' - const mapSpy = spy(async.map) - const {runPackageScript, spawnStubSpy} = setup({ - async: {map: mapSpy}, - }) + const {runPackageScript, spawnStubSpy} = setup() const scripts = ['lint', 'build'] const scriptConfig = {build: buildCommand, lint: lintCommand} const options = {parallel: true} - runPackageScript({scriptConfig, scripts, args, options}, () => { - t.true(mapSpy.calledOnce) + return runPackageScript({scriptConfig, scripts, args, options}).then(() => { t.true(spawnStubSpy.calledTwice) const [command1] = spawnStubSpy.firstCall.args const [command2] = spawnStubSpy.secondCall.args t.is(command1, 'eslint src/ scripts/') t.is(command2, 'babel src/ scripts/') - t.end() }) }) test('runs the default script if no scripts provided', t => { - const {command} = testSpawnCall(t, {default: 'echo foo'}, []) - t.is(command, `echo foo`) + return testSpawnCall(t, {default: 'echo foo'}, []).then(({command}) => { + t.is(command, `echo foo`) + }) +}) + +test('an non-zero exit code from a script will abort other scripts that are still running', t => { + const FAIL_CODE = 1 + const goodCommand = 'goodButLong' + const badCommand = 'bad' + const longCommand = 'long' + const scripts = ['goodS', 'badS', 'longS'] + const scriptConfig = { + goodS: goodCommand, + badS: badCommand, + longS: longCommand, + } + const killSpy = spy() + function spawnStub(cmd) { + return { + on(event, cb) { + if (event === 'close') { + if (cmd === longCommand) { + // verifies that the promise callback wont be invoked when it's been aborted + setTimeout(() => cb(0)) + } else if (cmd === badCommand) { + cb(FAIL_CODE) + } else { + cb(0) + } + } + }, + kill() { + killSpy(cmd) + }, + } + } + const {runPackageScript} = setup({ + 'spawn-command-with-kill': spawnStub, + }) + const options = {parallel: true} + return runPackageScript({scriptConfig, scripts, options}).then(() => { + t.fail('the promise should be rejected') + }, ({code, ref, message}) => { + t.is(code, FAIL_CODE) + t.true(typeof ref === 'string') + t.true(typeof message === 'string') + t.true(killSpy.calledOnce) // and only once, just for the longCommand + t.is(killSpy.firstCall.args[0], longCommand) + }) +}) + +test('an error event from a script will abort other scripts', t => { + const ERROR_STRING = 'error string' + const goodCommand = 'good' + const badCommand = 'bad' + const longCommand = 'long' + const scripts = ['goodS', 'badS', 'longS'] + const scriptConfig = { + goodS: goodCommand, + badS: badCommand, + longS: longCommand, + } + const killSpy = spy() + function spawnStub(cmd) { + return { + on(event, cb) { + if (event === 'close') { + if (cmd === longCommand) { + // never call the callback + // it'll get aborted anyway. + } else if (cmd === badCommand) { + // do nothing in this case because we're going to call the error cb + } else { + cb(0) + } + } else if (event === 'error' && cmd === badCommand) { + cb(ERROR_STRING) + } + }, + kill() { + killSpy(cmd) + }, + } + } + const {runPackageScript} = setup({ + 'spawn-command-with-kill': spawnStub, + }) + const options = {parallel: true} + return runPackageScript({scriptConfig, scripts, options}).then(() => { + t.fail('the promise should be rejected') + }, ({error, ref, message}) => { + t.is(error, ERROR_STRING) + t.true(typeof ref === 'string') + t.true(typeof message === 'string') + t.true(killSpy.calledOnce) // and only once, just for the longCommand + t.is(killSpy.firstCall.args[0], longCommand) + }) }) // util functions @@ -124,22 +261,35 @@ function testSpawnCallWithDefaults(t, options, stubOverrides) { function testSpawnCall(t, scriptConfig = {build: 'webpack'}, scripts = 'build', psOptions, args, stubOverrides) { /* eslint max-params:[2, 6] */ // TODO: refactor const {runPackageScript, spawnStubSpy, ...otherRet} = setup(stubOverrides) - runPackageScript({scriptConfig, options: psOptions, scripts, args}) - t.true(spawnStubSpy.calledOnce) - const [command, options] = spawnStubSpy.firstCall.args - return {command, options, spawnStubSpy, ...otherRet} + return runPackageScript({scriptConfig, options: psOptions, scripts, args}) + .then(result => { + t.true(spawnStubSpy.calledOnce) + const [command, options] = spawnStubSpy.firstCall.args + return Promise.resolve({result, command, options, spawnStubSpy, ...otherRet}) + }, error => { + t.true(spawnStubSpy.calledOnce) + const [command, options] = spawnStubSpy.firstCall.args + return Promise.reject({error, command, options, spawnStubSpy, ...otherRet}) + }) } -function setup(stubOverrides = {}) { - const onSpy = spy((event, cb) => cb()) - const spawnStub = () => ({on: onSpy}) // eslint-disable-line func-style +function setup( + stubOverrides = {}, + onSpy = spy((event, cb) => { + if (event === 'close') { + cb(0) + } + }) +) { + const killSpy = spy() + const spawnStub = () => ({on: onSpy, kill: killSpy}) // eslint-disable-line func-style const infoSpy = spy() const getLoggerSpy = spy(() => ({info: infoSpy})) const spawnStubSpy = spy(spawnStub) const runPackageScript = proxyquire('./index', { - 'spawn-command': spawnStubSpy, + 'spawn-command-with-kill': spawnStubSpy, './get-logger': getLoggerSpy, ...stubOverrides, }).default - return {spawnStubSpy, infoSpy, getLoggerSpy, onSpy, runPackageScript} + return {spawnStubSpy, infoSpy, getLoggerSpy, onSpy, killSpy, runPackageScript} }