From a2f4119ce7ed20cb7f5c50fff4fc52dc39e81b9d Mon Sep 17 00:00:00 2001 From: Gar Date: Wed, 18 May 2022 11:05:45 -0700 Subject: [PATCH] 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 | 9 +- lib/commands/ci.js | 49 +- .../tap-snapshots/test/index.js.test.cjs | 14 + .../test/lib/commands/ci.js.test.cjs | 13 - .../test/lib/load-all-commands.js.test.cjs | 2 +- .../test/lib/utils/npm-usage.js.test.cjs | 2 +- test/fixtures/mock-registry.js | 15 +- test/lib/commands/ci.js | 451 +++++++----------- 8 files changed, 219 insertions(+), 336 deletions(-) delete mode 100644 tap-snapshots/test/lib/commands/ci.js.test.cjs diff --git a/docs/content/commands/npm-ci.md b/docs/content/commands/npm-ci.md index 2bb542a725b5d..3374bf1e25136 100644 --- a/docs/content/commands/npm-ci.md +++ b/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`. diff --git a/lib/commands/ci.js b/lib/commands/ci.js index eb1e02bcdc724..9d8946987ecc6 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') diff --git a/smoke-tests/tap-snapshots/test/index.js.test.cjs b/smoke-tests/tap-snapshots/test/index.js.test.cjs index 5a13b4e9a5c20..6a803160a4076 100644 --- a/smoke-tests/tap-snapshots/test/index.js.test.cjs +++ b/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 ] +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: diff --git a/tap-snapshots/test/lib/commands/ci.js.test.cjs b/tap-snapshots/test/lib/commands/ci.js.test.cjs deleted file mode 100644 index d6a7471778aeb..0000000000000 --- a/tap-snapshots/test/lib/commands/ci.js.test.cjs +++ /dev/null @@ -1,13 +0,0 @@ -/* IMPORTANT - * This snapshot file is auto-generated, but designed for humans. - * It should be checked into source control and tracked carefully. - * Re-generate by setting TAP_SNAPSHOT=1 and running tests. - * Make sure to inspect the output below. Do not ignore changes! - */ -'use strict' -exports[`test/lib/commands/ci.js TAP should throw error when ideal inventory mismatches virtual > must match snapshot 1`] = ` -\`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. - -Invalid: lock file's foo@1.0.0 does not satisfy foo@2.0.0 - -` diff --git a/tap-snapshots/test/lib/load-all-commands.js.test.cjs b/tap-snapshots/test/lib/load-all-commands.js.test.cjs index 20915cdd87131..ceb10a8b352ef 100644 --- a/tap-snapshots/test/lib/load-all-commands.js.test.cjs +++ b/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 diff --git a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs b/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs index 3487378a2119d..8e178b1078280 100644 --- a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs +++ b/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 diff --git a/test/fixtures/mock-registry.js b/test/fixtures/mock-registry.js index a62890b72e2f6..61263da216efa 100644 --- a/test/fixtures/mock-registry.js +++ b/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 diff --git a/test/lib/commands/ci.js b/test/lib/commands/ci.js index 978cd03b877e6..179cee6c9b7e7 100644 --- a/test/lib/commands/ci.js +++ b/test/lib/commands/ci.js @@ -1,316 +1,201 @@ -const fs = require('fs') -const util = require('util') -const readdir = util.promisify(fs.readdir) - const t = require('tap') - -const { fake: mockNpm } = require('../../fixtures/mock-npm') - -t.test('should ignore scripts with --ignore-scripts', async t => { - const SCRIPTS = [] - let REIFY_CALLED = false - const CI = t.mock('../../../lib/commands/ci.js', { - '../../../lib/utils/reify-finish.js': async () => {}, - '@npmcli/run-script': ({ event }) => { - SCRIPTS.push(event) - }, - '@npmcli/arborist': function () { - this.loadVirtual = async () => {} - this.reify = () => { - REIFY_CALLED = true - } - this.buildIdealTree = () => {} - this.virtualTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '1.0.0' }], - ]), - } - this.idealTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '1.0.0' }], - ]), - } +const { load: _loadMockNpm } = require('../../fixtures/mock-npm') +const MockRegistry = require('../../fixtures/mock-registry.js') + +const path = require('path') +const fs = require('@npmcli/fs') + +// t.cleanSnapshot = str => str.replace(/ in [0-9ms]+/g, ' in {TIME}') + +const loadMockNpm = async (t, opts) => { + const mock = await _loadMockNpm(t, opts) + const registry = new MockRegistry({ + tap: t, + registry: mock.npm.config.get('registry'), + }) + return { registry, ...mock } +} + +const packageJson = { + name: 'test-package', + version: '1.0.0', + dependencies: { + abbrev: '^1.0.0', + }, +} +const packageLock = { + name: 'test-package', + version: '1.0.0', + lockfileVersion: 2, + requires: true, + packages: { + '': { + name: 'test-package', + version: '1.0.0', + dependencies: { + abbrev: '^1.0.0', + }, }, - }) + 'node_modules/abbrev': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.0.0.tgz', + // integrity changes w/ each test cause the path is different? + }, + }, + dependencies: { + abbrev: { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.0.0.tgz', + // integrity changes w/ each test cause the path is different? + }, + }, +} + +const abbrev = { + 'package.json': '{"name": "abbrev", "version": "1.0.0"}', + test: 'test file', +} + +t.test('reifies, audits, removes node_modules', async t => { + const { npm, joinedOutput, registry } = await loadMockNpm(t, { + prefixDir: { + abbrev: abbrev, + 'package.json': JSON.stringify(packageJson), + 'package-lock.json': JSON.stringify(packageLock), + node_modules: { test: 'test file that will be removed' }, + }, + }) + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), + }) + registry.nock.post('/-/npm/v1/security/advisories/bulk').reply(200, {}) + await npm.exec('ci', []) + t.match(joinedOutput(), 'added 1 package, and audited 2 packages in') + await t.resolveMatch( + fs.exists(path.join(npm.prefix, 'node_modules', 'test')), + false, + 'existing node_modules is removed' + ) + await t.resolveMatch( + fs.exists(path.join(npm.prefix, 'node_modules', 'abbrev')), + true, + 'installs abbrev' + ) +}) - const npm = mockNpm({ - globalDir: 'path/to/node_modules/', - prefix: 'foo', +t.test('--no-audit and --ignore-scripts', async t => { + const { npm, joinedOutput, registry } = await loadMockNpm(t, { config: { - global: false, 'ignore-scripts': true, + audit: false, }, - }) - const ci = new CI(npm) - - await ci.exec([]) - t.equal(REIFY_CALLED, true, 'called reify') - t.strictSame(SCRIPTS, [], 'no scripts when running ci') -}) - -t.test('should use Arborist and run-script', async t => { - const scripts = [ - 'preinstall', - 'install', - 'postinstall', - 'prepublish', // XXX should we remove this finally?? - 'preprepare', - 'prepare', - 'postprepare', - ] - - // set to true when timer starts, false when it ends - // when the test is done, we assert that all timers ended - const timers = {} - const onTime = msg => { - if (timers[msg]) { - throw new Error(`saw duplicate timer: ${msg}`) - } - timers[msg] = true - } - const onTimeEnd = msg => { - if (!timers[msg]) { - throw new Error(`ended timer that was not started: ${msg}`) - } - timers[msg] = false - } - process.on('time', onTime) - process.on('timeEnd', onTimeEnd) - t.teardown(() => { - process.removeListener('time', onTime) - process.removeListener('timeEnd', onTimeEnd) - }) - - const path = t.testdir({ - node_modules: { - foo: { - 'package.json': JSON.stringify({ - name: 'foo', - version: '1.2.3', - }), + prefixDir: { + abbrev: { + 'package.json': '{"name": "abbrev", "version": "1.0.0"}', + test: 'test-file', }, - '.dotdir': {}, - '.dotfile': 'a file with a dot', + 'package.json': JSON.stringify({ + ...packageJson, + // Would make install fail + scripts: { install: 'echo "SHOULD NOT RUN" && exit 1' }, + }), + 'package-lock.json': JSON.stringify(packageLock), }, }) - const expectRimrafs = 3 - let actualRimrafs = 0 - - const CI = t.mock('../../../lib/commands/ci.js', { - '../../../lib/utils/reify-finish.js': async () => {}, - '@npmcli/run-script': opts => { - t.match(opts, { event: scripts.shift() }) - }, - '@npmcli/arborist': function (args) { - t.ok(args, 'gets options object') - this.loadVirtual = () => { - t.ok(true, 'loadVirtual is called') - return Promise.resolve(true) - } - this.reify = () => { - t.ok(true, 'reify is called') - } - this.buildIdealTree = () => {} - this.virtualTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '1.0.0' }], - ]), - } - this.idealTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '1.0.0' }], - ]), - } - }, - rimraf: (path, ...args) => { - actualRimrafs++ - t.ok(path, 'rimraf called with path') - // callback is always last arg - args.pop()() - }, - '../../../lib/utils/reify-output.js': function (arb) { - t.ok(arb, 'gets arborist tree') - }, + require('nock').emitter.on('no match', req => { + t.fail('Should not audit') }) - - const npm = mockNpm({ - prefix: path, - config: { - global: false, - }, + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), }) - const ci = new CI(npm) - - await ci.exec(null) - for (const [msg, result] of Object.entries(timers)) { - t.notOk(result, `properly resolved ${msg} timer`) - } - t.match(timers, { 'npm-ci:rm': false }, 'saw the rimraf timer') - t.equal(actualRimrafs, expectRimrafs, 'removed the right number of things') - t.strictSame(scripts, [], 'called all scripts') + await npm.exec('ci', []) + t.match(joinedOutput(), 'added 1 package in', 'would fail if install script ran') }) -t.test('should pass flatOptions to Arborist.reify', async t => { - t.plan(1) - const CI = t.mock('../../../lib/commands/ci.js', { - '../../../lib/utils/reify-finish.js': async () => {}, - '@npmcli/run-script': opts => {}, - '@npmcli/arborist': function () { - this.loadVirtual = () => Promise.resolve(true) - this.reify = async (options) => { - t.equal(options.production, true, 'should pass flatOptions to Arborist.reify') - } - this.buildIdealTree = () => {} - this.virtualTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '1.0.0' }], - ]), - } - this.idealTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '1.0.0' }], - ]), - } +t.test('lifecycle scripts', async t => { + const scripts = [] + const { npm, registry } = await loadMockNpm(t, { + prefixDir: { + abbrev: abbrev, + 'package.json': JSON.stringify(packageJson), + 'package-lock.json': JSON.stringify(packageLock), + }, + mocks: { + '@npmcli/run-script': (opts) => { + t.ok(opts.banner) + scripts.push(opts.event) + }, }, }) - const npm = mockNpm({ - prefix: 'foo', - flatOptions: { - production: true, - }, + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), }) - const ci = new CI(npm) - await ci.exec(null) + registry.nock.post('/-/npm/v1/security/advisories/bulk').reply(200, {}) + await npm.exec('ci', []) + t.same(scripts, [ + 'preinstall', + 'install', + 'postinstall', + 'prepublish', + 'preprepare', + 'prepare', + 'postprepare', + ], 'runs appropriate scripts, in order') }) t.test('should throw if package-lock.json or npm-shrinkwrap missing', async t => { - const testDir = t.testdir({ - 'index.js': 'some contents', - 'package.json': 'some info', - }) - - const CI = t.mock('../../../lib/commands/ci.js', { - '@npmcli/run-script': opts => {}, - '../../../lib/utils/reify-finish.js': async () => {}, - 'proc-log': { - verbose: () => { - t.ok(true, 'log fn called') + const { npm } = await loadMockNpm(t, { + prefixDir: { + 'package.json': JSON.stringify(packageJson), + node_modules: { + 'test-file': 'should not be removed', }, }, }) - const npm = mockNpm({ - prefix: testDir, - config: { - global: false, - }, - }) - const ci = new CI(npm) - await t.rejects( - ci.exec(null), - /package-lock.json/, - 'throws error when there is no package-lock' + await t.rejects(npm.exec('ci', []), { code: 'EUSAGE', message: /package-lock.json/ }) + await t.resolveMatch( + fs.exists(path.join(npm.prefix, 'node_modules', 'test-file')), + true, + 'does not remove node_modules' ) }) t.test('should throw ECIGLOBAL', async t => { - const CI = t.mock('../../../lib/commands/ci.js', { - '@npmcli/run-script': opts => {}, - '../../../lib/utils/reify-finish.js': async () => {}, - }) - const npm = mockNpm({ - prefix: 'foo', - config: { - global: true, - }, - }) - const ci = new CI(npm) - await t.rejects( - ci.exec(null), - { code: 'ECIGLOBAL' }, - 'throws error with global packages' - ) -}) - -t.test('should remove existing node_modules before installing', async t => { - t.plan(3) - const testDir = t.testdir({ - node_modules: { - 'some-file': 'some contents', - }, + const { npm } = await loadMockNpm(t, { + config: { global: true }, }) - - const CI = t.mock('../../../lib/commands/ci.js', { - '@npmcli/run-script': opts => {}, - '../../../lib/utils/reify-finish.js': async () => {}, - '@npmcli/arborist': function () { - this.loadVirtual = () => Promise.resolve(true) - this.reify = async (options) => { - t.equal(options.packageLock, true, 'npm ci should never ignore lock') - t.equal(options.save, false, 'npm ci should never save') - // check if node_modules was removed before reifying - const contents = await readdir(testDir) - const nodeModules = contents.filter((path) => path.startsWith('node_modules')) - t.same(nodeModules, ['node_modules'], 'should only have the node_modules directory') - } - this.buildIdealTree = () => {} - this.virtualTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '1.0.0' }], - ]), - } - this.idealTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '1.0.0' }], - ]), - } - }, - }) - - const npm = mockNpm({ - prefix: testDir, - config: { - global: false, - }, - }) - const ci = new CI(npm) - - await ci.exec(null) + await t.rejects(npm.exec('ci', []), { code: 'ECIGLOBAL' }) }) t.test('should throw error when ideal inventory mismatches virtual', async t => { - const CI = t.mock('../../../lib/commands/ci.js', { - '../../../lib/utils/reify-finish.js': async () => {}, - '@npmcli/run-script': ({ event }) => {}, - '@npmcli/arborist': function () { - this.loadVirtual = async () => {} - this.reify = () => {} - this.buildIdealTree = () => {} - this.virtualTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '1.0.0' }], - ]), - } - this.idealTree = { - inventory: new Map([ - ['foo', { name: 'foo', version: '2.0.0' }], - ]), - } - }, - }) - - const npm = mockNpm({ - globalDir: 'path/to/node_modules/', - prefix: 'foo', - config: { - global: false, - 'ignore-scripts': true, + const { npm, registry } = await loadMockNpm(t, { + prefixDir: { + abbrev: abbrev, + 'package.json': JSON.stringify({ + ...packageJson, + dependencies: { notabbrev: '^1.0.0' }, + }), + 'package-lock.json': JSON.stringify(packageLock), + node_modules: { + 'test-file': 'should not be removed', + }, }, }) - const ci = new CI(npm) - - try { - await ci.exec([]) - } catch (err) { - t.matchSnapshot(err.message) - } + const manifest = registry.manifest({ name: 'notabbrev' }) + await registry.package({ manifest }) + await t.rejects( + npm.exec('ci', []), + { code: 'EUSAGE', message: /in sync/ } + ) + await t.resolveMatch( + fs.exists(path.join(npm.prefix, 'node_modules', 'test-file')), + true, + 'does not remove node_modules' + ) })