From 4e58274ed0fd2dd29d3c8d6c7c47f37a37dc0f0f Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 19 Feb 2021 20:39:12 -0800 Subject: [PATCH] Do not print error banner for shell proxy commands There are a few commands (exec, run-script, and the run-script proxies) where essentially npm is acting like a very fancy shell. It is peculiar and noisy for npm to print a verbose error banner at the end of these commands, since presumably the command itself already did whatever it had to do to report the error appropriately. For example, `npm test` runs a test script, usually outputting test results. Having npm then tell me that my tests failed with exit status 1 and print a debug log, is unnecessary and unwanted. When the error encountered for these commands does not have a non-zero numeric 'code', then we still print the standard npm error reporting messages, because presumably something went wrong OTHER than a process exiting with a non-zero status code. PR-URL: https://github.com/npm/cli/pull/2742 Credit: @isaacs Close: #2742 Reviewed-by: @nlf --- lib/npm.js | 5 ++ lib/utils/cmd-list.js | 14 +++++ lib/utils/error-handler.js | 14 ++++- .../test-lib-utils-cmd-list.js-TAP.test.js | 8 +++ test/lib/npm.js | 1 + test/lib/utils/error-handler.js | 62 +++++++++++++++++++ 6 files changed, 101 insertions(+), 3 deletions(-) diff --git a/lib/npm.js b/lib/npm.js index 40aa9bbd9b506..85dc67a78aac6 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -49,6 +49,7 @@ const makeCmd = cmd => { } const { types, defaults, shorthands } = require('./utils/config.js') +const { shellouts } = require('./utils/cmd-list.js') let warnedNonDashArg = false const _runCmd = Symbol('_runCmd') @@ -81,6 +82,10 @@ const npm = module.exports = new class extends EventEmitter { this.updateNotification = null } + get shelloutCommands () { + return shellouts + } + deref (c) { return deref(c) } diff --git a/lib/utils/cmd-list.js b/lib/utils/cmd-list.js index 4e088c12d43f6..336e4dd547255 100644 --- a/lib/utils/cmd-list.js +++ b/lib/utils/cmd-list.js @@ -136,10 +136,24 @@ const cmdList = [ ] const plumbing = ['birthday', 'help-search'] + +// these commands just shell out to something else or handle the +// error themselves, so it's confusing and weird to write out +// our full error log banner when they exit non-zero +const shellouts = [ + 'exec', + 'run-script', + 'test', + 'start', + 'stop', + 'restart', +] + module.exports = { aliases: Object.assign({}, shorthands, affordances), shorthands, affordances, cmdList, plumbing, + shellouts, } diff --git a/lib/utils/error-handler.js b/lib/utils/error-handler.js index f9685c91d7212..1fc31df44ffb9 100644 --- a/lib/utils/error-handler.js +++ b/lib/utils/error-handler.js @@ -105,8 +105,7 @@ const exit = (code, noLog) => { if (code && !noLog) writeLogFile() - else - reallyExit() + reallyExit() } const errorHandler = (er) => { @@ -130,7 +129,16 @@ const errorHandler = (er) => { cbCalled = true if (!er) return exit(0) - if (typeof er === 'string') { + + // if we got a command that just shells out to something else, then it + // will presumably print its own errors and exit with a proper status + // code if there's a problem. If we got an error with a code=0, then... + // something else went wrong along the way, so maybe an npm problem? + const isShellout = npm.shelloutCommands.includes(npm.command) + const quietShellout = isShellout && typeof er.code === 'number' && er.code + if (quietShellout) + return exit(er.code, true) + else if (typeof er === 'string') { log.error('', er) return exit(1, true) } else if (!(er instanceof Error)) { diff --git a/tap-snapshots/test-lib-utils-cmd-list.js-TAP.test.js b/tap-snapshots/test-lib-utils-cmd-list.js-TAP.test.js index 1c91975c7a152..55ed4e66465ee 100644 --- a/tap-snapshots/test-lib-utils-cmd-list.js-TAP.test.js +++ b/tap-snapshots/test-lib-utils-cmd-list.js-TAP.test.js @@ -173,6 +173,14 @@ Object { "birthday", "help-search", ], + "shellouts": Array [ + "exec", + "run-script", + "test", + "start", + "stop", + "restart", + ], "shorthands": Object { "c": "config", "cit": "install-ci-test", diff --git a/test/lib/npm.js b/test/lib/npm.js index 8494af6bb8098..9fd9888b97b75 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -79,6 +79,7 @@ t.test('not yet loaded', t => { set: Function, }, version: String, + shelloutCommands: Array, }) t.throws(() => npm.config.set('foo', 'bar')) t.throws(() => npm.config.get('foo')) diff --git a/test/lib/utils/error-handler.js b/test/lib/utils/error-handler.js index 0b896fee4f1fe..b1d3e2ca7ca1a 100644 --- a/test/lib/utils/error-handler.js +++ b/test/lib/utils/error-handler.js @@ -43,6 +43,7 @@ const config = { const npm = { version: '1.0.0', config, + shelloutCommands: ['exec', 'run-script'], } const npmlog = { @@ -525,3 +526,64 @@ t.test('use exitCode when emitting exit event', (t) => { process.emit('exit') }) + +t.test('do no fancy handling for shellouts', t => { + const { exit } = process + const { command } = npm + const { log } = npmlog + const LOG_RECORD = [] + t.teardown(() => { + npmlog.log = log + process.exit = exit + npm.command = command + }) + + npmlog.log = function (level, ...args) { + log.call(this, level, ...args) + LOG_RECORD.push(npmlog.record[npmlog.record.length - 1]) + } + + npm.command = 'exec' + + let EXPECT_EXIT = 0 + process.exit = code => { + t.equal(code, EXPECT_EXIT, 'got expected exit code') + EXPECT_EXIT = 0 + } + t.beforeEach((cb) => { + LOG_RECORD.length = 0 + cb() + }) + + const loudNoises = () => LOG_RECORD + .filter(({ level }) => ['warn', 'error'].includes(level)) + + t.test('shellout with a numeric error code', t => { + EXPECT_EXIT = 5 + errorHandler(Object.assign(new Error(), { code: 5 })) + t.equal(EXPECT_EXIT, 0, 'called process.exit') + // should log no warnings or errors, verbose/silly is fine. + t.strictSame(loudNoises(), [], 'no noisy warnings') + t.end() + }) + + t.test('shellout without a numeric error code (something in npm)', t => { + EXPECT_EXIT = 1 + errorHandler(Object.assign(new Error(), { code: 'banana stand' })) + t.equal(EXPECT_EXIT, 0, 'called process.exit') + // should log some warnings and errors, because something weird happened + t.strictNotSame(loudNoises(), [], 'bring the noise') + t.end() + }) + + t.test('shellout with code=0 (extra weird?)', t => { + EXPECT_EXIT = 1 + errorHandler(Object.assign(new Error(), { code: 0 })) + t.equal(EXPECT_EXIT, 0, 'called process.exit') + // should log some warnings and errors, because something weird happened + t.strictNotSame(loudNoises(), [], 'bring the noise') + t.end() + }) + + t.end() +})