From 80c2ac995170a05b26856a2b72fe9c8163b2c999 Mon Sep 17 00:00:00 2001
From: Gar <gar+gh@danger.computer>
Date: Wed, 17 Feb 2021 10:31:34 -0800
Subject: [PATCH] chore(tests): refactor publish tests

Less mocking, more intentional testing of things

PR-URL: https://github.com/npm/cli/pull/2717
Credit: @wraithgar
Close: #2717
Reviewed-by: @nlf
---
 test/lib/publish.js | 356 +++++++++++++++++++++++---------------------
 1 file changed, 183 insertions(+), 173 deletions(-)

diff --git a/test/lib/publish.js b/test/lib/publish.js
index 6d5cebf540698..5243b5254201e 100644
--- a/test/lib/publish.js
+++ b/test/lib/publish.js
@@ -1,40 +1,28 @@
 const t = require('tap')
 const requireInject = require('require-inject')
+const fs = require('fs')
+
+// The way we set loglevel is kind of convoluted, and there is no way to affect
+// it from these tests, which only interact with lib/publish.js, which assumes
+// that the code that is requiring and calling lib/publish.js has already
+// taken care of the loglevel
+const log = require('npmlog')
+log.level = 'silent'
 
 // mock config
 const {defaults} = require('../../lib/utils/config.js')
-const credentials = {
-  'https://unauthed.registry': {
-    email: 'me@example.com',
-  },
-  'https://scope.specific.registry': {
-    token: 'some.registry.token',
-    alwaysAuth: false,
-  },
-  'https://some.registry': {
-    token: 'some.registry.token',
-    alwaysAuth: false,
-  },
-  'https://registry.npmjs.org/': {
-    token: 'npmjs.registry.token',
-    alwaysAuth: false,
-  },
-}
+
 const config = {
   list: [defaults],
 }
 
-const registryCredentials = (t, registry) => {
-  return (uri) => {
-    t.same(uri, registry, 'gets credentials for expected registry')
-    return credentials[uri]
-  }
-}
-
-const fs = require('fs')
+t.afterEach(cb => {
+  log.level = 'silent'
+  cb()
+})
 
-t.test('should publish with libnpmpublish, respecting publishConfig', (t) => {
-  t.plan(6)
+t.test('should publish with libnpmpublish, passing through flatOptions and respecting publishConfig.registry', (t) => {
+  t.plan(7)
 
   const registry = 'https://some.registry'
   const publishConfig = { registry }
@@ -49,25 +37,16 @@ t.test('should publish with libnpmpublish, respecting publishConfig', (t) => {
   const publish = requireInject('../../lib/publish.js', {
     '../../lib/npm.js': {
       flatOptions: {
-        json: true,
-        defaultTag: 'latest',
-        registry,
+        customValue: true,
       },
       config: {
         ...config,
-        getCredentialsByURI: registryCredentials(t, registry),
+        getCredentialsByURI: (uri) => {
+          t.same(uri, registry, 'gets credentials for expected registry')
+          return { token: 'some.registry.token' }
+        },
       },
     },
-    '../../lib/utils/tar.js': {
-      getContents: () => ({
-        id: 'someid',
-      }),
-      logTar: () => {},
-    },
-    '../../lib/utils/output.js': () => {},
-    '../../lib/utils/otplease.js': (opts, fn) => {
-      return Promise.resolve().then(() => fn(opts))
-    },
     // verify that we do NOT remove publishConfig if it was there originally
     // and then removed during the script/pack process
     libnpmpack: async () => {
@@ -82,7 +61,8 @@ t.test('should publish with libnpmpublish, respecting publishConfig', (t) => {
         t.match(manifest, { name: 'my-cool-pkg', version: '1.0.0' }, 'gets manifest')
         t.isa(tarData, Buffer, 'tarData is a buffer')
         t.ok(opts, 'gets opts object')
-        t.same(opts.registry, publishConfig.registry, 'publishConfig is passed through')
+        t.same(opts.customValue, true, 'flatOptions values are passed through')
+        t.same(opts.registry, registry, 'publishConfig.registry is passed through')
       },
     },
   })
@@ -94,7 +74,7 @@ t.test('should publish with libnpmpublish, respecting publishConfig', (t) => {
   })
 })
 
-t.test('re-loads publishConfig if added during script process', (t) => {
+t.test('re-loads publishConfig.registry if added during script process', (t) => {
   t.plan(6)
   const registry = 'https://some.registry'
   const publishConfig = { registry }
@@ -107,26 +87,14 @@ t.test('re-loads publishConfig if added during script process', (t) => {
 
   const publish = requireInject('../../lib/publish.js', {
     '../../lib/npm.js': {
-      flatOptions: {
-        json: true,
-        defaultTag: 'latest',
-        registry: 'https://registry.npmjs.org/',
-      },
       config: {
         ...config,
-        getCredentialsByURI: registryCredentials(t, registry),
+        getCredentialsByURI: (uri) => {
+          t.same(uri, registry, 'gets credentials for expected registry')
+          return { token: 'some.registry.token' }
+        },
       },
     },
-    '../../lib/utils/tar.js': {
-      getContents: () => ({
-        id: 'someid',
-      }),
-      logTar: () => {},
-    },
-    '../../lib/utils/output.js': () => {},
-    '../../lib/utils/otplease.js': (opts, fn) => {
-      return Promise.resolve().then(() => fn(opts))
-    },
     libnpmpack: async () => {
       fs.writeFileSync(`${testDir}/package.json`, JSON.stringify({
         name: 'my-cool-pkg',
@@ -140,7 +108,7 @@ t.test('re-loads publishConfig if added during script process', (t) => {
         t.match(manifest, { name: 'my-cool-pkg', version: '1.0.0' }, 'gets manifest')
         t.isa(tarData, Buffer, 'tarData is a buffer')
         t.ok(opts, 'gets opts object')
-        t.same(opts.registry, registry, 'publishConfig is passed through')
+        t.same(opts.registry, registry, 'publishConfig.registry is passed through')
       },
     },
   })
@@ -152,10 +120,9 @@ t.test('re-loads publishConfig if added during script process', (t) => {
   })
 })
 
-t.test('should not log if silent (dry run)', (t) => {
-  t.plan(2)
+t.test('if loglevel=info and json, should not output package contents', (t) => {
+  t.plan(4)
 
-  const registry = 'https://registry.npmjs.org'
   const testDir = t.testdir({
     'package.json': JSON.stringify({
       name: 'my-cool-pkg',
@@ -163,41 +130,81 @@ t.test('should not log if silent (dry run)', (t) => {
     }, null, 2),
   })
 
+  log.level = 'info'
   const publish = requireInject('../../lib/publish.js', {
     '../../lib/npm.js': {
       flatOptions: {
-        json: false,
-        defaultTag: 'latest',
-        dryRun: true,
-        registry,
+        json: true,
       },
       config: {
         ...config,
-        getCredentialsByURI: () => {
-          throw new Error('should not call getCredentialsByURI in dry run')
+        getCredentialsByURI: (uri) => {
+          t.same(uri, defaults.registry, 'gets credentials for expected registry')
+          return { token: 'some.registry.token' }
         },
       },
     },
+    '../../lib/utils/output.js': () => {
+      t.pass('output is called')
+    },
     '../../lib/utils/tar.js': {
-      getContents: () => ({}),
+      getContents: () => ({
+        id: 'someid',
+      }),
       logTar: () => {
-        t.pass('called logTar (but nothing actually printed)')
+        t.pass('logTar is called')
+      },
+    },
+    libnpmpublish: {
+      publish: () => {
+        t.pass('publish called')
       },
     },
-    '../../lib/utils/otplease.js': (opts, fn) => {
-      return Promise.resolve().then(() => fn(opts))
+  })
+
+  return publish([testDir], (er) => {
+    if (er)
+      throw er
+    t.pass('got to callback')
+  })
+})
+
+t.test('if loglevel=silent and dry-run, should not output package contents or publish or validate credentials, should log tarball contents', (t) => {
+  t.plan(2)
+
+  const testDir = t.testdir({
+    'package.json': JSON.stringify({
+      name: 'my-cool-pkg',
+      version: '1.0.0',
+    }, null, 2),
+  })
+
+  log.level = 'silent'
+  const publish = requireInject('../../lib/publish.js', {
+    '../../lib/npm.js': {
+      flatOptions: {
+        dryRun: true,
+      },
+      config: {
+        ...config,
+        getCredentialsByURI: () => {
+          throw new Error('should not call getCredentialsByURI in dry run')
+        },
+      },
     },
     '../../lib/utils/output.js': () => {
-      throw new Error('should not output in silent mode')
+      throw new Error('should not output in dry run mode')
     },
-    npmlog: {
-      verbose: () => {},
-      notice: () => {},
-      level: 'silent',
+    '../../lib/utils/tar.js': {
+      getContents: () => ({
+        id: 'someid',
+      }),
+      logTar: () => {
+        t.pass('logTar is called')
+      },
     },
-    libnpmpack: async () => '',
     libnpmpublish: {
-      publish: (manifest, tarData, opts) => {
+      publish: () => {
         throw new Error('should not call libnpmpublish in dry run')
       },
     },
@@ -210,10 +217,9 @@ t.test('should not log if silent (dry run)', (t) => {
   })
 })
 
-t.test('should log tarball contents (dry run)', (t) => {
+t.test('if loglevel=info and dry-run, should not publish, should log package contents and log tarball contents', (t) => {
   t.plan(3)
 
-  const registry = 'https://registry.npmjs.org'
   const testDir = t.testdir({
     'package.json': JSON.stringify({
       name: 'my-cool-pkg',
@@ -221,13 +227,11 @@ t.test('should log tarball contents (dry run)', (t) => {
     }, null, 2),
   })
 
+  log.level = 'info'
   const publish = requireInject('../../lib/publish.js', {
     '../../lib/npm.js': {
       flatOptions: {
-        json: false,
-        defaultTag: 'latest',
         dryRun: true,
-        registry,
       },
       config: {
         ...config,
@@ -246,10 +250,6 @@ t.test('should log tarball contents (dry run)', (t) => {
     '../../lib/utils/output.js': () => {
       t.pass('output fn is called')
     },
-    '../../lib/utils/otplease.js': (opts, fn) => {
-      return Promise.resolve().then(() => fn(opts))
-    },
-    libnpmpack: async () => '',
     libnpmpublish: {
       publish: () => {
         throw new Error('should not call libnpmpublish in dry run')
@@ -266,16 +266,7 @@ t.test('should log tarball contents (dry run)', (t) => {
 
 t.test('shows usage with wrong set of arguments', (t) => {
   t.plan(1)
-  const publish = requireInject('../../lib/publish.js', {
-    '../../lib/npm.js': {
-      flatOptions: {
-        json: false,
-        defaultTag: '0.0.13',
-        registry: 'https://registry.npmjs.org/',
-      },
-      config,
-    },
-  })
+  const publish = requireInject('../../lib/publish.js')
 
   return publish(['a', 'b', 'c'], (er) => t.matchSnapshot(er, 'should print usage'))
 })
@@ -283,14 +274,10 @@ t.test('shows usage with wrong set of arguments', (t) => {
 t.test('throws when invalid tag', (t) => {
   t.plan(1)
 
-  const registry = 'https://registry.npmjs.org'
-
   const publish = requireInject('../../lib/publish.js', {
     '../../lib/npm.js': {
       flatOptions: {
-        json: false,
         defaultTag: '0.0.13',
-        registry,
       },
       config,
     },
@@ -306,8 +293,8 @@ t.test('throws when invalid tag', (t) => {
 t.test('can publish a tarball', t => {
   t.plan(4)
 
-  const registry = 'https://registry.npmjs.org/'
   const testDir = t.testdir({
+    tarball: {},
     package: {
       'package.json': JSON.stringify({
         name: 'my-cool-tarball',
@@ -318,37 +305,21 @@ t.test('can publish a tarball', t => {
   const tar = require('tar')
   tar.c({
     cwd: testDir,
-    file: `${testDir}/package.tgz`,
+    file: `${testDir}/tarball/package.tgz`,
     sync: true,
   }, ['package'])
 
-  // no cheating!  read it from the tarball.
-  fs.unlinkSync(`${testDir}/package/package.json`)
-  fs.rmdirSync(`${testDir}/package`)
-
-  const tarFile = fs.readFileSync(`${testDir}/package.tgz`)
+  const tarFile = fs.readFileSync(`${testDir}/tarball/package.tgz`)
   const publish = requireInject('../../lib/publish.js', {
     '../../lib/npm.js': {
-      flatOptions: {
-        json: true,
-        defaultTag: 'latest',
-        registry,
-      },
       config: {
         ...config,
-        getCredentialsByURI: registryCredentials(t, registry),
+        getCredentialsByURI: (uri) => {
+          t.same(uri, defaults.registry, 'gets credentials for expected registry')
+          return { token: 'some.registry.token' }
+        },
       },
     },
-    '../../lib/utils/tar.js': {
-      getContents: () => ({
-        id: 'someid',
-      }),
-      logTar: () => {},
-    },
-    '../../lib/utils/output.js': () => {},
-    '../../lib/utils/otplease.js': (opts, fn) => {
-      return Promise.resolve().then(() => fn(opts))
-    },
     libnpmpublish: {
       publish: (manifest, tarData, opts) => {
         t.match(manifest, {
@@ -360,32 +331,49 @@ t.test('can publish a tarball', t => {
     },
   })
 
-  return publish([`${testDir}/package.tgz`], (er) => {
+  return publish([`${testDir}/tarball/package.tgz`], (er) => {
     if (er)
       throw er
     t.pass('got to callback')
   })
 })
 
-t.test('throw if not logged in', async t => {
+t.test('should check auth for default registry', async t => {
   t.plan(2)
-  const registry = 'https://unauthed.registry'
-
   const publish = requireInject('../../lib/publish.js', {
-    '../../lib/utils/tar.js': {
-      getContents: () => ({
-        id: 'someid',
-      }),
-      logTar: () => {},
+    '../../lib/npm.js': {
+      config: {
+        ...config,
+        getCredentialsByURI: (uri) => {
+          t.same(uri, defaults.registry, 'gets credentials for expected registry')
+          return {}
+        },
+      },
     },
+  })
+
+  return publish([], (err) => {
+    t.match(err, {
+      message: 'This command requires you to be logged in.',
+      code: 'ENEEDAUTH',
+    }, 'throws when not logged in')
+  })
+})
+
+t.test('should check auth for configured registry', async t => {
+  t.plan(2)
+  const registry = 'https://some.registry'
+  const publish = requireInject('../../lib/publish.js', {
     '../../lib/npm.js': {
       flatOptions: {
-        json: false,
         registry,
       },
       config: {
         ...config,
-        getCredentialsByURI: registryCredentials(t, registry),
+        getCredentialsByURI: (uri) => {
+          t.same(uri, registry, 'gets credentials for expected registry')
+          return {}
+        },
       },
     },
   })
@@ -398,44 +386,69 @@ t.test('throw if not logged in', async t => {
   })
 })
 
-t.test('read registry only from publishConfig', t => {
-  t.plan(4)
-
+t.test('should check auth for scope specific registry', async t => {
+  t.plan(2)
   const registry = 'https://some.registry'
-  const publishConfig = { registry }
   const testDir = t.testdir({
     'package.json': JSON.stringify({
-      name: 'my-cool-pkg',
+      name: '@npm/my-cool-pkg',
       version: '1.0.0',
-      publishConfig,
     }, null, 2),
   })
 
   const publish = requireInject('../../lib/publish.js', {
     '../../lib/npm.js': {
       flatOptions: {
-        json: false,
+        '@npm:registry': registry,
       },
       config: {
         ...config,
-        getCredentialsByURI: registryCredentials(t, registry),
+        getCredentialsByURI: (uri) => {
+          t.same(uri, registry, 'gets credentials for expected registry')
+          return {}
+        },
       },
     },
-    '../../lib/utils/tar.js': {
-      getContents: () => ({
-        id: 'someid',
-      }),
-      logTar: () => {},
+  })
+
+  return publish([testDir], (err) => {
+    t.match(err, {
+      message: 'This command requires you to be logged in.',
+      code: 'ENEEDAUTH',
+    }, 'throws when not logged in')
+  })
+})
+
+t.test('should use auth for scope specific registry', t => {
+  t.plan(4)
+  const registry = 'https://some.registry'
+  const testDir = t.testdir({
+    'package.json': JSON.stringify({
+      name: '@npm/my-cool-pkg',
+      version: '1.0.0',
+    }, null, 2),
+  })
+
+  const publish = requireInject('../../lib/publish.js', {
+    '../../lib/npm.js': {
+      flatOptions: {
+        '@npm:registry': registry,
+      },
+      config: {
+        ...config,
+        getCredentialsByURI: (uri) => {
+          t.same(uri, registry, 'gets credentials for expected registry')
+          return { token: 'some.registry.token' }
+        },
+      },
     },
-    '../../lib/utils/output.js': () => {},
     libnpmpublish: {
       publish: (manifest, tarData, opts) => {
-        t.match(manifest, { name: 'my-cool-pkg', version: '1.0.0' }, 'gets manifest')
-        t.same(opts.registry, registry, 'publishConfig is passed through')
+        t.ok(opts, 'gets opts object')
+        t.same(opts['@npm:registry'], registry, 'scope specific registry is passed through')
       },
     },
   })
-
   return publish([testDir], (er) => {
     if (er)
       throw er
@@ -443,40 +456,37 @@ t.test('read registry only from publishConfig', t => {
   })
 })
 
-t.test('should check auth for scope specific registry', t => {
+t.test('read registry only from publishConfig', t => {
+  t.plan(4)
+
+  const registry = 'https://some.registry'
+  const publishConfig = { registry }
   const testDir = t.testdir({
     'package.json': JSON.stringify({
-      name: '@npm/my-cool-pkg',
+      name: 'my-cool-pkg',
       version: '1.0.0',
+      publishConfig,
     }, null, 2),
   })
 
-  const registry = 'https://scope.specific.registry'
   const publish = requireInject('../../lib/publish.js', {
     '../../lib/npm.js': {
-      flatOptions: {
-        json: false,
-        '@npm:registry': registry,
-      },
       config: {
         ...config,
-        getCredentialsByURI: registryCredentials(t, registry),
+        getCredentialsByURI: (uri) => {
+          t.same(uri, registry, 'gets credentials for expected registry')
+          return { token: 'some.registry.token' }
+        },
       },
     },
-    '../../lib/utils/tar.js': {
-      getContents: () => ({
-        id: 'someid',
-      }),
-      logTar: () => {},
-    },
-    '../../lib/utils/output.js': () => {},
-    '../../lib/utils/otplease.js': (opts, fn) => {
-      return Promise.resolve().then(() => fn(opts))
-    },
     libnpmpublish: {
-      publish: () => '',
+      publish: (manifest, tarData, opts) => {
+        t.match(manifest, { name: 'my-cool-pkg', version: '1.0.0' }, 'gets manifest')
+        t.same(opts.registry, registry, 'publishConfig is passed through')
+      },
     },
   })
+
   return publish([testDir], (er) => {
     if (er)
       throw er