fix(tests): move more tests to use real npm

This moves a handful of the smaller tests to using the new npm mock that
uses the real actual npm object.  It also extends the testing surface
area of a few tests back down into the actual `process.spawn` that
results, instead of anything internal to the code.

Some dead code in `lib/test.js` was found during this, as well as an
instance of a module throwing a string instead of an error object.

PR-URL: #3463
Credit: @wraithgar
Close: #3463
Reviewed-by: @nlf

Author: wraithgar

lib/set.js

@@ -22,7 +22,7 @@ class Set extends BaseCommand {
 
   exec (args, cb) {
     if (!args.length)
-      return cb(this.usage)
+      return cb(this.usageError())
     this.npm.commands.config(['set'].concat(args), cb)
   }
 }

lib/test.js

@@ -19,15 +19,5 @@ class Test extends LifecycleCmd {
       'script-shell',
     ]
   }
-
-  exec (args, cb) {
-    super.exec(args, er => {
-      if (er && er.code === 'ELIFECYCLE') {
-        /* eslint-disable standard/no-callback-literal */
-        cb('Test failed.  See above for more details.')
-      } else
-        cb(er)
-    })
-  }
 }
 module.exports = Test

node_modules/.gitignore

@@ -199,6 +199,7 @@
     "eslint-plugin-promise": "^5.1.0",
     "eslint-plugin-standard": "^5.0.0",
     "licensee": "^8.2.0",
+    "spawk": "^1.7.1",
     "tap": "^15.0.9"
   },
   "scripts": {

package-lock.json

@@ -9,6 +9,10 @@ for (const level in npmlog.levels)
 const { title, execPath } = process
 
 const RealMockNpm = (t, otherMocks = {}) => {
+  t.afterEach(() => {
+    outputs.length = 0
+    logs.length = 0
+  })
   t.teardown(() => {
     npm.perfStop()
     npmlog.record.length = 0
@@ -22,6 +26,9 @@ const RealMockNpm = (t, otherMocks = {}) => {
   })
   const logs = []
   const outputs = []
+  const joinedOutput = () => {
+    return outputs.map(o => o.join(' ')).join('\n')
+  }
   const npm = t.mock('../../lib/npm.js', otherMocks)
   const command = async (command, args = []) => {
     return new Promise((resolve, reject) => {
@@ -43,7 +50,7 @@ const RealMockNpm = (t, otherMocks = {}) => {
     }
   }
   npm.output = (...msg) => outputs.push(msg)
-  return { npm, logs, outputs, command }
+  return { npm, logs, outputs, command, joinedOutput }
 }
 
 const realConfig = require('../../lib/utils/config')

package.json

@@ -1,24 +1,26 @@
 const t = require('tap')
 
-const FindDupes = require('../../lib/find-dupes.js')
+const { real: mockNpm } = require('../fixtures/mock-npm')
 
-t.test('should run dedupe in dryRun mode', (t) => {
-  t.plan(3)
-  const findDupesTest = new FindDupes({
-    config: {
-      set: (k, v) => {
-        t.match(k, 'dry-run')
-        t.match(v, true)
-      },
+t.test('should run dedupe in dryRun mode', async (t) => {
+  t.plan(5)
+  const { npm, command } = mockNpm(t, {
+    '@npmcli/arborist': function (args) {
+      t.ok(args, 'gets options object')
+      t.ok(args.path, 'gets path option')
+      t.ok(args.dryRun, 'is called in dryRun mode')
+      this.dedupe = () => {
+        t.ok(true, 'dedupe is called')
+      }
     },
-    commands: {
-      dedupe: (args, cb) => {
-        t.match(args, [])
-        cb()
-      },
+    '../../lib/utils/reify-finish.js': (npm, arb) => {
+      t.ok(arb, 'gets arborist tree')
     },
   })
-  findDupesTest.exec({}, () => {
-    t.end()
-  })
+  await npm.load()
+  // explicitly set to false so we can be 100% sure it's always true when it
+  // hits arborist
+  npm.config.set('dry-run', false)
+  npm.config.set('prefix', 'foo')
+  await command('find-dupes')
 })

test/fixtures/mock-npm.js

@@ -1,16 +1,16 @@
 const t = require('tap')
+const { real: mockNpm } = require('../fixtures/mock-npm')
 
-t.test('should retrieve values from npm.commands.config', (t) => {
-  const Get = t.mock('../../lib/get.js')
-  const get = new Get({
-    commands: {
-      config: ([action, arg]) => {
-        t.equal(action, 'get', 'should use config get action')
-        t.equal(arg, 'foo', 'should use expected key')
-        t.end()
-      },
-    },
-  })
-
-  get.exec(['foo'])
+t.test('should retrieve values from config', async t => {
+  const { joinedOutput, command, npm } = mockNpm(t)
+  const name = 'editor'
+  const value = 'vigor'
+  await npm.load()
+  npm.config.set(name, value)
+  await command('get', [name])
+  t.equal(
+    joinedOutput(),
+    value,
+    'outputs config item'
+  )
 })

test/lib/find-dupes.js

@@ -1,15 +1,24 @@
 const t = require('tap')
 const glob = require('glob')
 const { resolve } = require('path')
+const { real: mockNpm } = require('../fixtures/mock-npm')
 
 const full = process.env.npm_lifecycle_event === 'check-coverage'
 
 if (!full)
   t.pass('nothing to do here, not checking for full coverage')
 else {
-  // some files do config.get() on load, so have to load npm first
-  const npm = require('../../lib/npm.js')
-  t.test('load npm first', t => npm.load(t.end))
+  const { npm } = mockNpm(t)
+
+  t.teardown(() => {
+    const exitHandler = require('../../lib/utils/exit-handler.js')
+    exitHandler.setNpm(npm)
+    exitHandler()
+  })
+
+  t.test('load npm first', async t => {
+    await npm.load()
+  })
 
   t.test('load all the files', t => {
     // just load all the files so we measure coverage for the missing tests
@@ -21,11 +30,4 @@ else {
     t.pass('loaded all files')
     t.end()
   })
-
-  t.test('call the exit handler so we dont freak out', t => {
-    const exitHandler = require('../../lib/utils/exit-handler.js')
-    exitHandler.setNpm(npm)
-    exitHandler()
-    t.end()
-  })
 }

test/lib/get.js

@@ -1,19 +1,13 @@
 const t = require('tap')
+const { real: mockNpm } = require('../fixtures/mock-npm')
 
-t.test('prefix', (t) => {
-  t.plan(3)
-  const dir = '/prefix/dir'
-
-  const Prefix = require('../../lib/prefix.js')
-  const prefix = new Prefix({
-    prefix: dir,
-    output: (output) => {
-      t.equal(output, dir, 'prints the correct directory')
-    },
-  })
-
-  prefix.exec([], (err) => {
-    t.error(err, 'npm prefix')
-    t.ok('should have printed directory')
-  })
+t.test('prefix', async (t) => {
+  const { joinedOutput, command, npm } = mockNpm(t)
+  await npm.load()
+  await command('prefix')
+  t.equal(
+    joinedOutput(),
+    npm.prefix,
+    'outputs npm.prefix'
+  )
 })

test/lib/load-all.js

@@ -1,7 +1,9 @@
 const t = require('tap')
+const { real: mockNpm } = require('../fixtures/mock-npm')
 
-t.test('should prune using Arborist', (t) => {
-  const Prune = t.mock('../../lib/prune.js', {
+t.test('should prune using Arborist', async (t) => {
+  t.plan(4)
+  const { command, npm } = mockNpm(t, {
     '@npmcli/arborist': function (args) {
       t.ok(args, 'gets options object')
       t.ok(args.path, 'gets path option')
@@ -13,16 +15,6 @@ t.test('should prune using Arborist', (t) => {
       t.ok(arb, 'gets arborist tree')
     },
   })
-  const prune = new Prune({
-    prefix: 'foo',
-    flatOptions: {
-      foo: 'bar',
-    },
-  })
-  prune.exec(null, er => {
-    if (er)
-      throw er
-    t.ok(true, 'callback is called')
-    t.end()
-  })
+  await npm.load()
+  await command('prune')
 })

test/lib/prefix.js

@@ -1,16 +1,36 @@
 const t = require('tap')
-let runArgs
-const npm = {
-  commands: {
-    'run-script': (args, cb) => {
-      runArgs = args
-      cb()
-    },
-  },
-}
-const Restart = require('../../lib/restart.js')
-const restart = new Restart(npm)
-restart.exec(['foo'], () => {
-  t.match(runArgs, ['restart', 'foo'])
-  t.end()
+const spawk = require('spawk')
+const { real: mockNpm } = require('../fixtures/mock-npm')
+
+spawk.preventUnmatched()
+t.teardown(() => {
+  spawk.unload()
+})
+
+// TODO this ... smells.  npm "script-shell" config mentions defaults but those
+// are handled by run-script, not npm.  So for now we have to tie tests to some
+// pretty specific internals of runScript
+const makeSpawnArgs = require('@npmcli/run-script/lib/make-spawn-args.js')
+
+t.test('should run stop script from package.json', async t => {
+  const prefix = t.testdir({
+    'package.json': JSON.stringify({
+      name: 'x',
+      version: '1.2.3',
+      scripts: {
+        restart: 'node ./test-restart.js',
+      },
+    }),
+  })
+  const { command, npm } = mockNpm(t)
+  await npm.load()
+  npm.log.level = 'silent'
+  npm.localPrefix = prefix
+  const [scriptShell] = makeSpawnArgs({ path: prefix })
+  const script = spawk.spawn(scriptShell, (args) => {
+    t.ok(args.includes('node ./test-restart.js "foo"'), 'ran stop script with extra args')
+    return true
+  })
+  await command('restart', ['foo'])
+  t.ok(script.called, 'script ran')
 })

test/lib/prune.js

@@ -1,19 +1,13 @@
 const t = require('tap')
+const { real: mockNpm } = require('../fixtures/mock-npm')
 
-t.test('root', (t) => {
-  t.plan(3)
-  const dir = '/root/dir'
-
-  const Root = require('../../lib/root.js')
-  const root = new Root({
-    dir,
-    output: (output) => {
-      t.equal(output, dir, 'prints the correct directory')
-    },
-  })
-
-  root.exec([], (err) => {
-    t.error(err, 'npm root')
-    t.ok('should have printed directory')
-  })
+t.test('prefix', async (t) => {
+  const { joinedOutput, command, npm } = mockNpm(t)
+  await npm.load()
+  await command('root')
+  t.equal(
+    joinedOutput(),
+    npm.dir,
+    'outputs npm.dir'
+  )
 })

test/lib/restart.js

@@ -1,5 +1,32 @@
 const t = require('tap')
 
+// can't run this until npm set can save to npm.localPrefix
+t.skip('npm set', async t => {
+  const { real: mockNpm } = require('../fixtures/mock-npm')
+  const { joinedOutput, command, npm } = mockNpm(t)
+  await npm.load()
+
+  t.test('no args', async t => {
+    t.rejects(
+      command('set'),
+      /Usage:/,
+      'prints usage'
+    )
+  })
+
+  t.test('test-config-item', async t => {
+    npm.localPrefix = t.testdir({})
+    t.not(npm.config.get('test-config-item', 'project'), 'test config value', 'config is not already new value')
+    // This will write to ~/.npmrc!
+    // Don't unskip until we can write to project level
+    await command('set', ['test-config-item=test config value'])
+    t.equal(joinedOutput(), '', 'outputs nothing')
+    t.equal(npm.config.get('test-config-item', 'project'), 'test config value', 'config is set to new value')
+  })
+})
+
+// Everything after this can go away once the above test is unskipped
+
 let configArgs = null
 const npm = {
   commands: {