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: #2742
Credit: @isaacs
Close: #2742
Reviewed-by: @nlf

Author: isaacs

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)
   }

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,
 }

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)) {

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",

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'))

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()
+})