From 9dd2ed5189b6f283094664e9e192cf1598ec3f79 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Mon, 29 Mar 2021 11:00:44 -0400 Subject: [PATCH] fix: empty newline printed to stderr Starting in v7.7.0 running `npm` (no args) is printing an empty newline to stderr. This fixes that by correctly exiting via errorHandler and avoiding hitting the cb() never called error and adds a test to make sure we avoid that regression moving forward. Fixes: https://github.com/nodejs/node/pull/37678#issuecomment-808734374 Co-authored-by: Gar --- lib/cli.js | 4 +- smoke-tests/index.js | 14 +++++++ .../smoke-tests-index.js-TAP.test.js | 37 +++++++++++++++++++ test/lib/cli.js | 29 +++++++++++++-- 4 files changed, 77 insertions(+), 7 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index 46859f150e3b9..f42132f944390 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -61,9 +61,6 @@ module.exports = (process) => { impl(npm.argv, errorHandler) else { try { - // I don't know why this is needed but we get a cb() not called if we - // omit it - npm.log.level = 'silent' if (cmd) { const didYouMean = require('./utils/did-you-mean.js') const suggestions = await didYouMean(npm, npm.localPrefix, cmd) @@ -71,6 +68,7 @@ module.exports = (process) => { } else npm.output(npm.usage) process.exitCode = 1 + return errorHandler() } catch (err) { errorHandler(err) } diff --git a/smoke-tests/index.js b/smoke-tests/index.js index 38c3ed306e5f1..2c2a1b6ab9543 100644 --- a/smoke-tests/index.js +++ b/smoke-tests/index.js @@ -12,6 +12,7 @@ t.cleanSnapshot = s => s.split(cwd).join('{CWD}') .split(process.cwd()).join('{CWD}') .replace(/\\+/g, '/') .replace(/\r\n/g, '\n') + .replace(/\ \(in a browser\)/g, '') // setup server const registryServer = require('./server.js') @@ -55,6 +56,19 @@ t.test('npm init', async t => { t.equal(pkg.version, '1.0.0', 'should have expected generated version') }) +t.test('npm (no args)', async t => { + const cmd = `"${process.execPath}" "${npmLocation}" --no-audit --no-update-notifier` + const cmdRes = await execAsync(cmd, { cwd: localPrefix, env }) + .catch(err => { + t.equal(err.code, 1, 'should exit with error code') + return err + }) + + t.equal(cmdRes.stderr, '', 'should have no stderr output') + t.matchSnapshot(String(cmdRes.stdout), + 'should have expected no args output') +}) + t.test('npm install prodDep@version', async t => { const cmd = `${npmBin} install abbrev@1.0.4` const cmdRes = await exec(cmd) diff --git a/tap-snapshots/smoke-tests-index.js-TAP.test.js b/tap-snapshots/smoke-tests-index.js-TAP.test.js index aa8977316b1c8..932799bc5a47b 100644 --- a/tap-snapshots/smoke-tests-index.js-TAP.test.js +++ b/tap-snapshots/smoke-tests-index.js-TAP.test.js @@ -5,6 +5,43 @@ * Make sure to inspect the output below. Do not ignore changes! */ 'use strict' +exports[`smoke-tests/index.js TAP npm (no args) > should have expected no args output 1`] = ` +npm + +Usage: + +npm install install all the dependencies in your project +npm install add the dependency to your project +npm test run this project's tests +npm run run the script named +npm -h quick help on +npm -l display usage info for all commands +npm help search for help on +npm help npm more involved overview + +All commands: + + access, adduser, audit, bin, bugs, cache, ci, completion, + config, dedupe, deprecate, diff, dist-tag, docs, doctor, + edit, exec, explain, explore, find-dupes, fund, get, help, + hook, init, install, install-ci-test, install-test, link, + ll, login, logout, ls, org, outdated, owner, pack, ping, + prefix, profile, prune, publish, rebuild, repo, restart, + root, run-script, search, set, set-script, shrinkwrap, star, + stars, start, stop, team, test, token, uninstall, unpublish, + unstar, update, version, view, whoami + +Specify configs in the ini-formatted file: + {CWD}/smoke-tests/index/.npmrc +or on the command line via: npm --key=value + +More configuration info: npm help config +Configuration fields: npm help 7 config + +npm@7.7.5 {CWD} + +` + exports[`smoke-tests/index.js TAP npm diff > should have expected diff output 1`] = ` diff --git a/package.json b/package.json index v1.0.4..v1.1.1 100644 diff --git a/test/lib/cli.js b/test/lib/cli.js index 40da77bf44e3d..28e44394e16dc 100644 --- a/test/lib/cli.js +++ b/test/lib/cli.js @@ -172,17 +172,37 @@ t.test('gracefully handles error printing usage', t => { t.teardown(() => { npmock.output = output errorHandlerCb = null + errorHandlerCalled = null }) const proc = { - argv: ['node', 'npm', 'asdf'], + argv: ['node', 'npm'], on: () => {}, } npmock.argv = [] - npmock.output = (msg) => { - throw new Error('test exception') + errorHandlerCb = () => { + t.match(errorHandlerCalled, [], 'should call errorHandler with no args') + t.end() + } + cli(proc) +}) + +t.test('handles output error', t => { + const { output } = npmock + t.teardown(() => { + npmock.output = output + errorHandlerCb = null + errorHandlerCalled = null + }) + const proc = { + argv: ['node', 'npm'], + on: () => {}, + } + npmock.argv = [] + npmock.output = () => { + throw new Error('ERR') } errorHandlerCb = () => { - t.match(errorHandlerCalled, /test exception/) + t.match(errorHandlerCalled, /ERR/, 'should call errorHandler with error') t.end() } cli(proc) @@ -191,6 +211,7 @@ t.test('gracefully handles error printing usage', t => { t.test('load error calls error handler', t => { t.teardown(() => { errorHandlerCb = null + errorHandlerCalled = null LOAD_ERROR = null })