Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not print error banner for shell proxy commands #2742

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/npm.js
Expand Up @@ -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')
Expand Down Expand Up @@ -81,6 +82,10 @@ const npm = module.exports = new class extends EventEmitter {
this.updateNotification = null
}

get shelloutCommands () {
return shellouts
}

deref (c) {
return deref(c)
}
Expand Down
14 changes: 14 additions & 0 deletions lib/utils/cmd-list.js
Expand Up @@ -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,
}
14 changes: 11 additions & 3 deletions lib/utils/error-handler.js
Expand Up @@ -105,8 +105,7 @@ const exit = (code, noLog) => {

if (code && !noLog)
writeLogFile()
else
reallyExit()
reallyExit()
}

const errorHandler = (er) => {
Expand All @@ -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)) {
Expand Down
8 changes: 8 additions & 0 deletions tap-snapshots/test-lib-utils-cmd-list.js-TAP.test.js
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions test/lib/npm.js
Expand Up @@ -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'))
Expand Down
62 changes: 62 additions & 0 deletions test/lib/utils/error-handler.js
Expand Up @@ -43,6 +43,7 @@ const config = {
const npm = {
version: '1.0.0',
config,
shelloutCommands: ['exec', 'run-script'],
}

const npmlog = {
Expand Down Expand Up @@ -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()
})