fix: cleanup star/unstar (#4868)

It was querying whoami once for every package you starred/unstarred, and
incorrectly trying to determine if you weren't logged in.  In fact the
function throws a descriptive message if you're not logged in already.
The whoami check was also racing with the fetch of the packument for
each package you were starring/unstarring meaning you could also get a
random 401 for a private package instead of the 'you need to log in'
message.

unstar was setting an undocumented config item to get the
shared code to unstar.  The command already has a name attribute that
tells us what action we are doing so we can just use that.

Finally, the duplicated (and differing) params between the two commands
were consolidated.

5 files changed, 132 insertions, 159 deletions

diff --git a/test/fixtures/mock-registry.js b/test/fixtures/mock-registry.js
index 01d43ba3d..a62890b72 100644
--- a/test/fixtures/mock-registry.js
+++ b/test/fixtures/mock-registry.js
@@ -183,6 +183,15 @@ class MockRegistry {
     }
   }
 
+  star (manifest, users) {
+    const spec = npa(manifest.name)
+    this.nock = this.nock.put(`/${spec.escapedName}`, {
+      _id: manifest._id,
+      _rev: manifest._rev,
+      users,
+    }).reply(200, { ...manifest, users })
+  }
+
   async package ({ manifest, times = 1, query, tarballs }) {
     let nock = this.nock
     const spec = npa(manifest.name)
@@ -206,7 +215,7 @@ class MockRegistry {
   // either pass in packuments if you need to set specific attributes besides version,
   // or an array of versions
   // the last packument in the packuments or versions array will be tagged latest
-  manifest ({ name = 'test-package', packuments, versions } = {}) {
+  manifest ({ name = 'test-package', users, packuments, versions } = {}) {
     packuments = this.packuments(packuments, name)
     const latest = packuments.slice(-1)[0]
     const manifest = {
@@ -220,6 +229,9 @@ class MockRegistry {
       'dist-tags': { latest: latest.version },
       ...latest,
     }
+    if (users) {
+      manifest.users = users
+    }
     if (versions) {
       packuments = versions.map(version => ({ version }))
     }
diff --git a/test/lib/commands/deprecate.js b/test/lib/commands/deprecate.js
index 03177cb7b..8a925fc2a 100644
--- a/test/lib/commands/deprecate.js
+++ b/test/lib/commands/deprecate.js
@@ -85,7 +85,7 @@ t.test('undeprecate', async t => {
     name: 'foo',
     versions,
   })
-  registry.package({ manifest, query: { write: true } })
+  await registry.package({ manifest, query: { write: true } })
   registry.nock.put('/foo', body => {
     for (const version of versions) {
       if (body.versions[version].deprecated !== '') {
@@ -110,7 +110,7 @@ t.test('deprecates given range', async t => {
     name: 'foo',
     versions,
   })
-  registry.package({ manifest, query: { write: true } })
+  await registry.package({ manifest, query: { write: true } })
   const message = 'test deprecation message'
   registry.nock.put('/foo', body => {
     if (body.versions['1.0.1'].deprecated) {
@@ -136,7 +136,7 @@ t.test('deprecates all versions when no range is specified', async t => {
     name: 'foo',
     versions,
   })
-  registry.package({ manifest, query: { write: true } })
+  await registry.package({ manifest, query: { write: true } })
   const message = 'test deprecation message'
   registry.nock.put('/foo', body => {
     for (const version of versions) {
diff --git a/test/lib/commands/owner.js b/test/lib/commands/owner.js
index 800d5b96a..f8ab7feef 100644
--- a/test/lib/commands/owner.js
+++ b/test/lib/commands/owner.js
@@ -46,7 +46,7 @@ function registryPackage (t, registry, name) {
     name,
     packuments: [{ maintainers, version: '1.0.0' }],
   })
-  mockRegistry.package({ manifest })
+  return mockRegistry.package({ manifest })
 }
 
 t.test('owner no args', async t => {
@@ -73,7 +73,7 @@ t.test('owner ls no args', async t => {
     name: packageName,
     packuments: [{ maintainers, version: '1.0.0' }],
   })
-  registry.package({ manifest })
+  await registry.package({ manifest })
 
   await npm.exec('owner', ['ls'])
   t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
@@ -137,7 +137,7 @@ t.test('owner ls <pkg>', async t => {
     name: packageName,
     packuments: [{ maintainers, version: '1.0.0' }],
   })
-  registry.package({ manifest })
+  await registry.package({ manifest })
 
   await npm.exec('owner', ['ls', packageName])
   t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
@@ -153,7 +153,7 @@ t.test('owner ls <pkg> no maintainers', async t => {
     name: packageName,
     versions: ['1.0.0'],
   })
-  registry.package({ manifest })
+  await registry.package({ manifest })
 
   await npm.exec('owner', ['ls', packageName])
   t.equal(joinedOutput(), 'no admin found')
@@ -173,7 +173,7 @@ t.test('owner add <user> <pkg>', async t => {
     packuments: [{ maintainers, version: '1.0.0' }],
   })
   registry.couchuser({ username })
-  registry.package({ manifest })
+  await registry.package({ manifest })
   registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
     t.match(body, {
       _id: manifest._id,
@@ -206,7 +206,7 @@ t.test('owner add <user> cwd package', async t => {
     packuments: [{ maintainers, version: '1.0.0' }],
   })
   registry.couchuser({ username })
-  registry.package({ manifest })
+  await registry.package({ manifest })
   registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
     t.match(body, {
       _id: manifest._id,
@@ -236,7 +236,7 @@ t.test('owner add <user> <pkg> already an owner', async t => {
     packuments: [{ maintainers, version: '1.0.0' }],
   })
   registry.couchuser({ username })
-  registry.package({ manifest })
+  await registry.package({ manifest })
   await npm.exec('owner', ['add', username, packageName])
   t.equal(joinedOutput(), '')
   t.match(
@@ -273,7 +273,7 @@ t.test('owner add <user> <pkg> fails to PUT updates', async t => {
     packuments: [{ maintainers, version: '1.0.0' }],
   })
   registry.couchuser({ username })
-  registry.package({ manifest })
+  await registry.package({ manifest })
   registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`).reply(404, {})
   await t.rejects(
     npm.exec('owner', ['add', username, packageName]),
@@ -295,7 +295,7 @@ t.test('owner add <user> <pkg> no previous maintainers property from server', as
     packuments: [{ maintainers: undefined, version: '1.0.0' }],
   })
   registry.couchuser({ username })
-  registry.package({ manifest })
+  await registry.package({ manifest })
   registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
     t.match(body, {
       _id: manifest._id,
@@ -351,7 +351,7 @@ t.test('owner rm <user> <pkg>', async t => {
     packuments: [{ maintainers, version: '1.0.0' }],
   })
   registry.couchuser({ username })
-  registry.package({ manifest })
+  await registry.package({ manifest })
   registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
     t.match(body, {
       _id: manifest._id,
@@ -378,7 +378,7 @@ t.test('owner rm <user> <pkg> not a current owner', async t => {
     packuments: [{ maintainers, version: '1.0.0' }],
   })
   registry.couchuser({ username })
-  registry.package({ manifest })
+  await registry.package({ manifest })
   await npm.exec('owner', ['rm', username, packageName])
   t.match(logs.info, [['owner rm', `Not a package owner: ${username}`]])
 })
@@ -400,7 +400,7 @@ t.test('owner rm <user> cwd package', async t => {
     packuments: [{ maintainers, version: '1.0.0' }],
   })
   registry.couchuser({ username })
-  registry.package({ manifest })
+  await registry.package({ manifest })
   registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
     t.match(body, {
       _id: manifest._id,
@@ -430,7 +430,7 @@ t.test('owner rm <user> only user', async t => {
     packuments: [{ maintainers: maintainers.slice(0, 1), version: '1.0.0' }],
   })
   registry.couchuser({ username })
-  registry.package({ manifest })
+  await registry.package({ manifest })
   await t.rejects(
     npm.exec('owner', ['rm', username]),
     {
@@ -486,7 +486,7 @@ t.test('workspaces', async t => {
         'process.cwd': () => path.join(prefix, 'workspace-a'),
       }),
     })
-    registryPackage(t, npm.config.get('registry'), 'workspace-a')
+    await registryPackage(t, npm.config.get('registry'), 'workspace-a')
     await npm.exec('owner', ['ls'])
     t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
   })
@@ -499,7 +499,7 @@ t.test('workspaces', async t => {
       }),
     })
     npm.config.set('workspace', ['workspace-a'])
-    registryPackage(t, npm.config.get('registry'), 'workspace-a')
+    await registryPackage(t, npm.config.get('registry'), 'workspace-a')
     await npm.exec('owner', ['ls'])
     t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
   })
@@ -511,7 +511,7 @@ t.test('workspaces', async t => {
         'process.cwd': () => path.join(prefix, 'workspace-a'),
       }),
     })
-    registryPackage(t, npm.config.get('registry'), packageName)
+    await registryPackage(t, npm.config.get('registry'), packageName)
     await npm.exec('owner', ['ls', packageName])
     t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
   })
@@ -524,7 +524,7 @@ t.test('workspaces', async t => {
       }),
     })
     npm.config.set('workspace', ['workspace-a'])
-    registryPackage(t, npm.config.get('registry'), packageName)
+    await registryPackage(t, npm.config.get('registry'), packageName)
     await npm.exec('owner', ['ls', packageName])
     t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
   })
@@ -543,7 +543,7 @@ t.test('workspaces', async t => {
       name: 'workspace-a',
       packuments: [{ maintainers, version: '1.0.0' }],
     })
-    registry.package({ manifest })
+    await registry.package({ manifest })
     registry.couchuser({ username })
     registry.nock.put(`/workspace-a/-rev/${manifest._rev}`, body => {
       t.match(body, {
@@ -572,7 +572,7 @@ t.test('workspaces', async t => {
       name: 'workspace-a',
       packuments: [{ maintainers, version: '1.0.0' }],
     })
-    registry.package({ manifest })
+    await registry.package({ manifest })
     registry.couchuser({ username })
     registry.nock.put(`/workspace-a/-rev/${manifest._rev}`, body => {
       t.match(body, {
@@ -603,7 +603,7 @@ t.test('workspaces', async t => {
       name: 'workspace-a',
       packuments: [{ maintainers, version: '1.0.0' }],
     })
-    registry.package({ manifest })
+    await registry.package({ manifest })
     registry.couchuser({ username })
     registry.nock.put(`/workspace-a/-rev/${manifest._rev}`, body => {
       t.match(body, {
@@ -649,7 +649,7 @@ t.test('completion', async t => {
       name: packageName,
       packuments: [{ maintainers, version: '1.0.0' }],
     })
-    registry.package({ manifest })
+    await registry.package({ manifest })
     const res = await owner.completion({ conf: { argv: { remain: ['npm', 'owner', 'rm'] } } })
     t.strictSame(res, maintainers.map(m => m.name), 'should return list of current owners')
   })
@@ -683,7 +683,7 @@ t.test('completion', async t => {
       name: packageName,
       packuments: [{ maintainers: [], version: '1.0.0' }],
     })
-    registry.package({ manifest })
+    await registry.package({ manifest })
 
     const res = await owner.completion({ conf: { argv: { remain: ['npm', 'owner', 'rm'] } } })
     t.strictSame(res, [], 'should return no owners if not found')
diff --git a/test/lib/commands/star.js b/test/lib/commands/star.js
index 5b79c0776..ce9d258be 100644
--- a/test/lib/commands/star.js
+++ b/test/lib/commands/star.js
@@ -1,133 +1,61 @@
 const t = require('tap')
-const { fake: mockNpm } = require('../../fixtures/mock-npm')
+const { load: loadMockNpm } = require('../../fixtures/mock-npm.js')
+const MockRegistry = require('../../fixtures/mock-registry.js')
 
-let result = ''
-
-const noop = () => null
-const config = {
-  unicode: false,
-  'star.unstar': false,
-}
-const npm = mockNpm({
-  config,
-  output: (...msg) => {
-    result += msg.join('\n')
-  },
-})
-const npmFetch = { json: noop }
-const log = { error: noop, info: noop, verbose: noop }
-const mocks = {
-  'proc-log': log,
-  'npm-registry-fetch': npmFetch,
-  '../../../lib/utils/get-identity.js': async () => 'foo',
-}
-
-const Star = t.mock('../../../lib/commands/star.js', mocks)
-const star = new Star(npm)
-
-t.afterEach(() => {
-  config.unicode = false
-  config['star.unstar'] = false
-  log.info = noop
-  result = ''
-})
+const pkgName = '@npmcli/test-package'
+const authToken = 'test-auth-token'
+const username = 'test-user'
+const auth = { '//registry.npmjs.org/:_authToken': authToken }
 
 t.test('no args', async t => {
+  const { npm } = await loadMockNpm(t)
   await t.rejects(
-    star.exec([]),
+    npm.exec('star', []),
     { code: 'EUSAGE' },
     'should throw usage error'
   )
 })
 
-t.test('star a package', async t => {
-  t.plan(4)
-  const pkgName = '@npmcli/arborist'
-  npmFetch.json = async (uri, opts) => {
-    return {
-      _id: pkgName,
-      _rev: 'hash',
-      users: (
-        opts.method === 'PUT'
-          ? { foo: true }
-          : {}
-      ),
-    }
-  }
-  log.info = (title, msg, id) => {
-    t.equal(title, 'star', 'should use expected title')
-    t.equal(msg, 'starring', 'should use expected msg')
-    t.equal(id, pkgName, 'should use expected id')
-  }
-  await star.exec([pkgName])
-  t.equal(
-    result,
-    '(*) @npmcli/arborist',
-    'should output starred package msg'
-  )
-})
+t.test('first person to star a package unicode:false', async t => {
+  const { npm, joinedOutput } = await loadMockNpm(t, {
+    config: { unicode: false, ...auth },
+  })
+  const registry = new MockRegistry({
+    tap: t,
+    registry: npm.config.get('registry'),
+    authorization: authToken,
+  })
+  const manifest = registry.manifest({ name: pkgName })
+  await registry.package({ manifest, query: { write: true } })
+  registry.whoami({ username })
+  registry.star(manifest, { [username]: true })
 
-t.test('unstar a package', async t => {
-  t.plan(4)
-  const pkgName = '@npmcli/arborist'
-  config['star.unstar'] = true
-  npmFetch.json = async (uri, opts) => {
-    return {
-      _id: pkgName,
-      _rev: 'hash',
-      ...(opts.method === 'PUT'
-        ? {}
-        : { foo: true }
-      ),
-    }
-  }
-  log.info = (title, msg, id) => {
-    t.equal(title, 'unstar', 'should use expected title')
-    t.equal(msg, 'unstarring', 'should use expected msg')
-    t.equal(id, pkgName, 'should use expected id')
-  }
-  await star.exec([pkgName])
+  await npm.exec('star', [pkgName])
   t.equal(
-    result,
-    '( ) @npmcli/arborist',
-    'should output unstarred package msg'
+    joinedOutput(),
+    '(*) @npmcli/test-package',
+    'should output starred package msg'
   )
 })
 
-t.test('unicode', async t => {
-  t.test('star a package', async t => {
-    config.unicode = true
-    npmFetch.json = async (uri, opts) => ({})
-    await star.exec(['pkg'])
-    t.equal(
-      result,
-      '\u2605  pkg',
-      'should output unicode starred package msg'
-    )
+t.test('second person to star a package unicode:true', async t => {
+  const { npm, joinedOutput } = await loadMockNpm(t, {
+    config: { unicode: true, ...auth },
   })
-
-  t.test('unstar a package', async t => {
-    config.unicode = true
-    config['star.unstar'] = true
-    npmFetch.json = async (uri, opts) => ({})
-    await star.exec(['pkg'])
-    t.equal(
-      result,
-      '\u2606  pkg',
-      'should output unstarred package msg'
-    )
+  const registry = new MockRegistry({
+    tap: t,
+    registry: npm.config.get('registry'),
+    authorization: authToken,
   })
-})
+  const manifest = registry.manifest({ name: pkgName, users: { otheruser: true } })
+  await registry.package({ manifest, query: { write: true } })
+  registry.whoami({ username })
+  registry.star(manifest, { otheruser: true, [username]: true })
 
-t.test('logged out user', async t => {
-  const Star = t.mock('../../../lib/commands/star.js', {
-    ...mocks,
-    '../../../lib/utils/get-identity.js': async () => undefined,
-  })
-  const star = new Star(npm)
-  await t.rejects(
-    star.exec(['@npmcli/arborist']),
-    /You need to be logged in/,
-    'should throw login required error'
+  await npm.exec('star', [pkgName])
+  t.equal(
+    joinedOutput(),
+    '★  @npmcli/test-package',
+    'should output starred package msg'
   )
 })
diff --git a/test/lib/commands/unstar.js b/test/lib/commands/unstar.js
index fb3c269b7..85c33d279 100644
--- a/test/lib/commands/unstar.js
+++ b/test/lib/commands/unstar.js
@@ -1,29 +1,62 @@
 const t = require('tap')
+const { load: loadMockNpm } = require('../../fixtures/mock-npm.js')
+const MockRegistry = require('../../fixtures/mock-registry.js')
 
-t.test('unstar', async t => {
-  t.plan(3)
+const pkgName = '@npmcli/test-package'
+const authToken = 'test-auth-token'
+const username = 'test-user'
+const auth = { '//registry.npmjs.org/:_authToken': authToken }
 
-  class Star {
-    constructor (npm) {
-      this.npm = npm
-    }
+t.test('no args', async t => {
+  const { npm } = await loadMockNpm(t)
+  await t.rejects(
+    npm.exec('unstar', []),
+    { code: 'EUSAGE' },
+    'should throw usage error'
+  )
+})
+
+t.test('unstar a package unicode:false', async t => {
+  const { npm, joinedOutput } = await loadMockNpm(t, {
+    config: { unicode: false, ...auth },
+  })
+  const registry = new MockRegistry({
+    tap: t,
+    registry: npm.config.get('registry'),
+    authorization: authToken,
+  })
+  const manifest = registry.manifest({ name: pkgName, users: { [username]: true } })
+  await registry.package({ manifest, query: { write: true } })
+  registry.whoami({ username })
+  registry.star(manifest, {})
+
+  await npm.exec('unstar', [pkgName])
+  t.equal(
+    joinedOutput(),
+    '( ) @npmcli/test-package',
+    'should output unstarred package msg'
+  )
+})
 
-    async exec (args) {
-      t.same(args, ['pkg'], 'should forward packages')
-    }
-  }
-  const Unstar = t.mock('../../../lib/commands/unstar.js', {
-    '../../../lib/commands/star.js': Star,
+t.test('unstar a package unicode:true', async t => {
+  const { npm, joinedOutput } = await loadMockNpm(t, {
+    config: { unicode: true, ...auth },
   })
 
-  const unstar = new Unstar({
-    config: {
-      set: (key, value) => {
-        t.equal(key, 'star.unstar', 'should set unstar config value')
-        t.equal(value, true, 'should set a truthy value')
-      },
-    },
+  const registry = new MockRegistry({
+    tap: t,
+    registry: npm.config.get('registry'),
+    authorization: authToken,
   })
+  const manifest = registry.manifest({ name: pkgName, users: { [username]: true } })
+  await registry.package({ manifest, query: { write: true } })
+  registry.whoami({ username })
+  registry.star(manifest, {})
 
-  await unstar.exec(['pkg'])
+  await npm.exec('unstar', [pkgName])
+  t.equal(
+    joinedOutput(),
+    '☆  @npmcli/test-package',
+    'should output unstarred package msg'
+  )
 })