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 <gar+gh@danger.computer>

4 files changed, 77 insertions, 7 deletions

diff --git a/lib/cli.js b/lib/cli.js
index 46859f150..f42132f94 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 38c3ed306..2c2a1b6ab 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 aa8977316..932799bc5 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 <command>
+
+Usage:
+
+npm install        install all the dependencies in your project
+npm install <foo>  add the <foo> dependency to your project
+npm test           run this project's tests
+npm run <foo>      run the script named <foo>
+npm <command> -h   quick help on <command>
+npm -l             display usage info for all commands
+npm help <term>    search for help on <term>
+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 <command> --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 40da77bf4..28e44394e 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
   })