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.

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,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')

smoke-tests/tap-snapshots/test/index.js.test.cjs

@@ -43,10 +43,24 @@ npm {CWD}
 `
 
 exports[`test/index.js TAP npm ci > should throw mismatch deps in lock file error 1`] = `
+npm ERR! code EUSAGE
+npm ERR! 
 npm ERR! \`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.
 npm ERR! 
 npm ERR! Invalid: lock file's abbrev@1.0.4 does not satisfy abbrev@1.1.1
 npm ERR! 
+npm ERR! Clean install a project
+npm ERR! 
+npm ERR! Usage:
+npm ERR! npm ci
+npm ERR! 
+npm ERR! Options:
+npm ERR! [--no-audit] [--foreground-scripts] [--ignore-scripts]
+npm ERR! [--script-shell <script-shell>]
+npm ERR! 
+npm ERR! aliases: clean-install, ic, install-clean, isntall-clean
+npm ERR! 
+npm ERR! Run "npm help ci" for more info
 
 npm ERR! A complete log of this run can be found in:
 

tap-snapshots/test/lib/load-all-commands.js.test.cjs

@@ -113,7 +113,7 @@ Run "npm help cache" for more info
 `
 
 exports[`test/lib/load-all-commands.js TAP load each command ci > must match snapshot 1`] = `
-Install a project with a clean slate
+Clean install a project
 
 Usage:
 npm ci

tap-snapshots/test/lib/utils/npm-usage.js.test.cjs

@@ -251,7 +251,7 @@ All commands:
                     
                     Run "npm help cache" for more info
 
-    ci              Install a project with a clean slate
+    ci              Clean install a project
                     
                     Usage:
                     npm ci

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