chore(tests): consistent error assertions

Our handling of "there should be no error" was not consistent.  It is
preferable to stay within the assertion library of your testing
framework, but tap also does not by default bail out on a test suite on
a failed assertion.  The `bail` flag turns this on for these assertions,
so now we are dealing with them in a consistent way and in a way that
aligns with what people are likely expecting when they see this kind of
assertion (i.e. there should be no error).

Coincidentally this found a subtle bug in at least one test where we
were checking that no error was generated after we had called t.end().
The `if (err) throw err` not using the testing framework meant this was
impossible to catch.

test/lib/access.js

@@ -117,7 +117,7 @@ t.test('access public on scoped package', (t) => {
   access.exec([
     'public',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access public on scoped package')
   })
 })
@@ -202,7 +202,7 @@ t.test('access restricted on scoped package', (t) => {
   access.exec([
     'restricted',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access restricted on scoped package')
   })
 })
@@ -261,7 +261,7 @@ t.test('access grant read-only', (t) => {
     'myorg:myteam',
     '@scoped/another',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access grant read-only')
   })
 })
@@ -285,7 +285,7 @@ t.test('access grant read-write', (t) => {
     'myorg:myteam',
     '@scoped/another',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access grant read-write')
   })
 })
@@ -313,7 +313,7 @@ t.test('access grant current cwd', (t) => {
     'read-write',
     'myorg:myteam',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access grant current cwd')
   })
 })
@@ -370,7 +370,7 @@ t.test('access grant malformed team arg', (t) => {
 })
 
 t.test('access 2fa-required/2fa-not-required', t => {
-  t.plan(2)
+  t.plan(4)
   const Access = t.mock('../../lib/access.js', {
     libnpmaccess: {
       tfaRequired: (spec) => {
@@ -385,14 +385,12 @@ t.test('access 2fa-required/2fa-not-required', t => {
   })
   const access = new Access({})
 
-  access.exec(['2fa-required', '@scope/pkg'], er => {
-    if (er)
-      throw er
+  access.exec(['2fa-required', '@scope/pkg'], err => {
+    t.error(err, { bail: true })
   })
 
-  access.exec(['2fa-not-required', 'unscoped-pkg'], er => {
-    if (er)
-      throw er
+  access.exec(['2fa-not-required', 'unscoped-pkg'], err => {
+    t.error(err, { bail: true })
   })
 })
 
@@ -413,7 +411,7 @@ t.test('access revoke', (t) => {
     'myorg:myteam',
     '@scoped/another',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access revoke')
   })
 })
@@ -465,7 +463,7 @@ t.test('npm access ls-packages with no team', (t) => {
   access.exec([
     'ls-packages',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access ls-packages with no team')
   })
 })
@@ -485,7 +483,7 @@ t.test('access ls-packages on team', (t) => {
     'ls-packages',
     'myorg:myteam',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access ls-packages on team')
   })
 })
@@ -509,7 +507,7 @@ t.test('access ls-collaborators on current', (t) => {
   access.exec([
     'ls-collaborators',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access ls-collaborators on current')
   })
 })
@@ -529,7 +527,7 @@ t.test('access ls-collaborators on spec', (t) => {
     'ls-collaborators',
     'yargs',
   ], (err) => {
-    t.error(err, 'npm access')
+    t.error(err, { bail: true })
     t.ok('should successfully access ls-packages with no team')
   })
 })

test/lib/adduser.js

@@ -83,7 +83,7 @@ t.test('usage', (t) => {
 })
 t.test('simple login', (t) => {
   adduser.exec([], (err) => {
-    t.error(err, 'npm adduser')
+    t.error(err, { bail: true })
 
     t.equal(
       registryOutput,
@@ -154,7 +154,7 @@ t.test('scoped login', (t) => {
   _flatOptions.scope = '@myscope'
 
   adduser.exec([], (err) => {
-    t.error(err, 'npm adduser')
+    t.error(err, { bail: true })
 
     t.same(
       setConfig['@myscope:registry'],
@@ -175,7 +175,7 @@ t.test('scoped login with valid scoped registry config', (t) => {
   _flatOptions.scope = '@myscope'
 
   adduser.exec([], (err) => {
-    t.error(err, 'npm adduser')
+    t.error(err, { bail: true })
 
     t.same(
       setConfig['@myscope:registry'],

test/lib/bin.js

@@ -18,7 +18,7 @@ t.test('bin', (t) => {
   t.match(bin.usage, 'bin', 'usage has command name in it')
 
   bin.exec([], (err) => {
-    t.error(err, 'npm bin')
+    t.error(err, { bail: true })
     t.ok('should have printed directory')
   })
 })
@@ -49,7 +49,7 @@ t.test('bin -g', (t) => {
   const bin = new Bin(npm)
 
   bin.exec([], (err) => {
-    t.error(err, 'npm bin')
+    t.error(err, { bail: true })
     t.ok('should have printed directory')
   })
 })
@@ -79,7 +79,7 @@ t.test('bin -g (not in path)', (t) => {
   const bin = new Bin(npm)
 
   bin.exec([], (err) => {
-    t.error(err, 'npm bin')
+    t.error(err, { bail: true })
     t.ok('should have printed directory')
   })
 })

test/lib/bugs.js

@@ -86,8 +86,7 @@ t.test('open bugs urls & emails', t => {
   keys.forEach(pkg => {
     t.test(pkg, t => {
       bugs.exec([pkg], (er) => {
-        if (er)
-          throw er
+        t.error(er, { bail: true })
         t.equal(opened[expect[pkg]], 1, 'opened expected url', {opened})
         t.end()
       })
@@ -97,8 +96,7 @@ t.test('open bugs urls & emails', t => {
 
 t.test('open default package if none specified', t => {
   bugs.exec([], (er) => {
-    if (er)
-      throw er
+    t.error(er, { bail: true })
     t.equal(opened['https://example.com'], 2, 'opened expected url', {opened})
     t.end()
   })

test/lib/cache.js

@@ -88,7 +88,7 @@ t.test('cache clean (force)', t => {
   })
 
   cache.exec(['clear'], err => {
-    t.error(err)
+    t.error(err, { bail: true })
     t.equal(rimrafPath, path.join(npm.cache, '_cacache'))
     t.end()
   })
@@ -123,7 +123,7 @@ t.test('cache add pkg only', t => {
   })
 
   cache.exec(['add', 'mypkg'], err => {
-    t.error(err)
+    t.error(err, { bail: true })
     t.strictSame(logOutput, [
       ['silly', 'cache add', 'args', ['mypkg']],
       ['silly', 'cache add', 'spec', 'mypkg'],
@@ -142,7 +142,7 @@ t.test('cache add pkg w/ spec modifier', t => {
   })
 
   cache.exec(['add', 'mypkg', 'latest'], err => {
-    t.error(err)
+    t.error(err, { bail: true })
     t.strictSame(logOutput, [
       ['silly', 'cache add', 'args', ['mypkg', 'latest']],
       ['silly', 'cache add', 'spec', 'mypkg@latest'],
@@ -159,7 +159,7 @@ t.test('cache verify', t => {
   })
 
   cache.exec(['verify'], err => {
-    t.error(err)
+    t.error(err, { bail: true })
     t.match(outputOutput, [
       `Cache verified and compressed (${path.join(npm.cache, '_cacache')})`,
       'Content verified: 1 (100 bytes)',
@@ -186,7 +186,7 @@ t.test('cache verify w/ extra output', t => {
   })
 
   cache.exec(['check'], err => {
-    t.error(err)
+    t.error(err, { bail: true })
     t.match(outputOutput, [
       `Cache verified and compressed (~${path.join('/fake/path', '_cacache')})`,
       'Content verified: 1 (100 bytes)',

test/lib/ci.js

@@ -33,8 +33,7 @@ t.test('should ignore scripts with --ignore-scripts', (t) => {
   const ci = new CI(npm)
 
   ci.exec([], er => {
-    if (er)
-      throw er
+    t.error(er, { bail: true })
     t.equal(REIFY_CALLED, true, 'called reify')
     t.strictSame(SCRIPTS, [], 'no scripts when running ci')
     t.end()
@@ -122,8 +121,7 @@ t.test('should use Arborist and run-script', (t) => {
   const ci = new CI(npm)
 
   ci.exec(null, er => {
-    if (er)
-      throw er
+    t.error(er, { bail: true })
     for (const [msg, result] of Object.entries(timers))
       t.notOk(result, `properly resolved ${msg} timer`)
     t.match(timers, { 'npm-ci:rm': false }, 'saw the rimraf timer')
@@ -141,7 +139,6 @@ t.test('should pass flatOptions to Arborist.reify', (t) => {
       this.loadVirtual = () => Promise.resolve(true)
       this.reify = async (options) => {
         t.equal(options.production, true, 'should pass flatOptions to Arborist.reify')
-        t.end()
       }
     },
   })
@@ -153,8 +150,8 @@ t.test('should pass flatOptions to Arborist.reify', (t) => {
   })
   const ci = new CI(npm)
   ci.exec(null, er => {
-    if (er)
-      throw er
+    t.error(er, { bail: true })
+    t.end()
   })
 })
 
@@ -224,7 +221,6 @@ t.test('should remove existing node_modules before installing', (t) => {
         const contents = await readdir(testDir)
         const nodeModules = contents.filter((path) => path.startsWith('node_modules'))
         t.same(nodeModules, ['node_modules'], 'should only have the node_modules directory')
-        t.end()
       }
     },
   })
@@ -238,7 +234,7 @@ t.test('should remove existing node_modules before installing', (t) => {
   const ci = new CI(npm)
 
   ci.exec(null, er => {
-    if (er)
-      throw er
+    t.error(er, { bail: true })
+    t.end()
   })
 })