fix(ci): remove node_modules post-validation

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.

docs/content/commands/npm-ci.md

@@ -1,7 +1,7 @@
 ---
 title: npm-ci
 section: 1
-description: Install a project with a clean slate
+description: Clean install a project
 ---
 
 ### Synopsis
@@ -28,12 +28,7 @@ it's meant to be used in automated environments such as test platforms,
 continuous integration, and deployment -- or any situation where you want
 to make sure you're doing a clean install of your dependencies.
 
-`npm ci` will be significantly faster when:
-
-- There is a `package-lock.json` or `npm-shrinkwrap.json` file.
-- The `node_modules` folder is missing or empty.
-
-In short, the main differences between using `npm install` and `npm ci` are:
+The main differences between using `npm install` and `npm ci` are:
 
 * The project **must** have an existing `package-lock.json` or
   `npm-shrinkwrap.json`.

lib/commands/ci.js

@@ -8,15 +8,6 @@ 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 {
@@ -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')

test/fixtures/mock-registry.js

@@ -202,16 +202,21 @@ class MockRegistry {
     nock = nock.reply(200, manifest)
     if (tarballs) {
       for (const version in tarballs) {
-      // for (const version in manifest.versions) {
-        const packument = manifest.versions[version]
-        const dist = new URL(packument.dist.tarball)
-        const tarball = await pacote.tarball(tarballs[version])
-        nock.get(dist.pathname).reply(200, tarball)
+        const m = manifest.versions[version]
+        nock = await this.tarball({ manifest: m, tarball: tarballs[version] })
       }
     }
     this.nock = nock
   }
 
+  async tarball ({ manifest, tarball }) {
+    const nock = this.nock
+    const dist = new URL(manifest.dist.tarball)
+    const tar = await pacote.tarball(tarball)
+    nock.get(dist.pathname).reply(200, tar)
+    return nock
+  }
+
   // 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