fix(ci): remove node_modules post-validation (#4913)

The removal of node_modules was happening in a race with the loading of
the virtualTree, and before the validation of the package-lock against
the package.json.  This defers the removal till after all that
validation has happened.

It also makes the errors thrown usage errors, and refactors the tests to
be real.

1 files changed, 23 insertions, 26 deletions

diff --git a/lib/commands/ci.js b/lib/commands/ci.js
index eb1e02bcd..9d8946987 100644
--- a/lib/commands/ci.js
+++ b/lib/commands/ci.js
@@ -8,19 +8,10 @@ const readdir = util.promisify(fs.readdir)
 const log = require('../utils/log-shim.js')
 const validateLockfile = require('../utils/validate-lockfile.js')
 
-const removeNodeModules = async where => {
-  const rimrafOpts = { glob: false }
-  process.emit('time', 'npm-ci:rm')
-  const path = `${where}/node_modules`
-  // get the list of entries so we can skip the glob for performance
-  const entries = await readdir(path, null).catch(er => [])
-  await Promise.all(entries.map(f => rimraf(`${path}/${f}`, rimrafOpts)))
-  process.emit('timeEnd', 'npm-ci:rm')
-}
 const ArboristWorkspaceCmd = require('../arborist-cmd.js')
 
 class CI extends ArboristWorkspaceCmd {
-  static description = 'Install a project with a clean slate'
+  static description = 'Clean install a project'
   static name = 'ci'
   static params = [
     'audit',
@@ -31,9 +22,9 @@ class CI extends ArboristWorkspaceCmd {
 
   async exec () {
     if (this.npm.config.get('global')) {
-      const err = new Error('`npm ci` does not work for global packages')
-      err.code = 'ECIGLOBAL'
-      throw err
+      throw Object.assign(new Error('`npm ci` does not work for global packages'), {
+        code: 'ECIGLOBAL',
+      })
     }
 
     const where = this.npm.prefix
@@ -46,17 +37,14 @@ class CI extends ArboristWorkspaceCmd {
     }
 
     const arb = new Arborist(opts)
-    await Promise.all([
-      arb.loadVirtual().catch(er => {
-        log.verbose('loadVirtual', er.stack)
-        const msg =
-          'The `npm ci` command can only install with an existing package-lock.json or\n' +
-          'npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or\n' +
-          'later to generate a package-lock.json file, then try again.'
-        throw new Error(msg)
-      }),
-      removeNodeModules(where),
-    ])
+    await arb.loadVirtual().catch(er => {
+      log.verbose('loadVirtual', er.stack)
+      const msg =
+        'The `npm ci` command can only install with an existing package-lock.json or\n' +
+        'npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or\n' +
+        'later to generate a package-lock.json file, then try again.'
+      throw this.usageError(msg)
+    })
 
     // retrieves inventory of packages from loaded virtual tree (lock file)
     const virtualInventory = new Map(arb.virtualTree.inventory)
@@ -70,15 +58,24 @@ class CI extends ArboristWorkspaceCmd {
     // throws a validation error in case of mismatches
     const errors = validateLockfile(virtualInventory, arb.idealTree.inventory)
     if (errors.length) {
-      throw new Error(
+      throw this.usageError(
         '`npm ci` can only install packages when your package.json and ' +
         'package-lock.json or npm-shrinkwrap.json are in sync. Please ' +
         'update your lock file with `npm install` ' +
         'before continuing.\n\n' +
-        errors.join('\n') + '\n'
+        errors.join('\n')
       )
     }
 
+    // Only remove node_modules after we've successfully loaded the virtual
+    // tree and validated the lockfile
+    await this.npm.time('npm-ci:rm', async () => {
+      const path = `${where}/node_modules`
+      // get the list of entries so we can skip the glob for performance
+      const entries = await readdir(path, null).catch(er => [])
+      return Promise.all(entries.map(f => rimraf(`${path}/${f}`, { glob: false })))
+    })
+
     await arb.reify(opts)
 
     const ignoreScripts = this.npm.config.get('ignore-scripts')