From 05791db24a2c13e9dab8ec78415c1d41527a56cd Mon Sep 17 00:00:00 2001 From: Gar Date: Tue, 29 Mar 2022 10:12:42 -0700 Subject: [PATCH] fix: consolidate split-package-names It was only ever used by `npm edit` so it's inline now. Rewrote `npm edit` tests to be real --- lib/commands/edit.js | 27 +++- lib/utils/split-package-names.js | 25 ---- test/fixtures/tspawk.js | 16 ++ test/lib/commands/config.js | 12 +- test/lib/commands/edit.js | 201 +++++++++++++------------- test/lib/commands/restart.js | 7 +- test/lib/commands/start.js | 7 +- test/lib/commands/stop.js | 7 +- test/lib/commands/test.js | 7 +- test/lib/utils/split-package-names.js | 18 --- 10 files changed, 150 insertions(+), 177 deletions(-) delete mode 100644 lib/utils/split-package-names.js create mode 100644 test/fixtures/tspawk.js delete mode 100644 test/lib/utils/split-package-names.js diff --git a/lib/commands/edit.js b/lib/commands/edit.js index ce74ff79b2b7e..0256f4f3a6f01 100644 --- a/lib/commands/edit.js +++ b/lib/commands/edit.js @@ -3,11 +3,32 @@ const { resolve } = require('path') const fs = require('graceful-fs') -const { spawn } = require('child_process') -const splitPackageNames = require('../utils/split-package-names.js') +const cp = require('child_process') const completion = require('../utils/completion/installed-shallow.js') const BaseCommand = require('../base-command.js') +const splitPackageNames = (path) => { + return path.split('/') + // combine scoped parts + .reduce((parts, part) => { + if (parts.length === 0) { + return [part] + } + + const lastPart = parts[parts.length - 1] + // check if previous part is the first part of a scoped package + if (lastPart[0] === '@' && !lastPart.includes('/')) { + parts[parts.length - 1] += '/' + part + } else { + parts.push(part) + } + + return parts + }, []) + .join('/node_modules/') + .replace(/(\/node_modules)+/, '/node_modules') +} + class Edit extends BaseCommand { static description = 'Edit an installed package' static name = 'edit' @@ -36,7 +57,7 @@ class Edit extends BaseCommand { return reject(err) } const [bin, ...args] = this.npm.config.get('editor').split(/\s+/) - const editor = spawn(bin, [...args, dir], { stdio: 'inherit' }) + const editor = cp.spawn(bin, [...args, dir], { stdio: 'inherit' }) editor.on('exit', (code) => { if (code) { return reject(new Error(`editor process exited with code: ${code}`)) diff --git a/lib/utils/split-package-names.js b/lib/utils/split-package-names.js deleted file mode 100644 index 395c2517d5934..0000000000000 --- a/lib/utils/split-package-names.js +++ /dev/null @@ -1,25 +0,0 @@ -'use strict' - -const splitPackageNames = (path) => { - return path.split('/') - // combine scoped parts - .reduce((parts, part) => { - if (parts.length === 0) { - return [part] - } - - const lastPart = parts[parts.length - 1] - // check if previous part is the first part of a scoped package - if (lastPart[0] === '@' && !lastPart.includes('/')) { - parts[parts.length - 1] += '/' + part - } else { - parts.push(part) - } - - return parts - }, []) - .join('/node_modules/') - .replace(/(\/node_modules)+/, '/node_modules') -} - -module.exports = splitPackageNames diff --git a/test/fixtures/tspawk.js b/test/fixtures/tspawk.js new file mode 100644 index 0000000000000..554a9d402c32f --- /dev/null +++ b/test/fixtures/tspawk.js @@ -0,0 +1,16 @@ +'use strict' + +const spawk = require('spawk') + +module.exports = tspawk + +function tspawk (t) { + spawk.preventUnmatched() + t.teardown(function () { + spawk.unload() + }) + t.afterEach(function () { + spawk.done() + }) + return spawk +} diff --git a/test/lib/commands/config.js b/test/lib/commands/config.js index 8217131479fe4..42df8b52d8e57 100644 --- a/test/lib/commands/config.js +++ b/test/lib/commands/config.js @@ -1,10 +1,10 @@ const { join } = require('path') const { promisify } = require('util') const fs = require('fs') -const spawk = require('spawk') +const tspawk = require('../../fixtures/tspawk') const t = require('tap') -spawk.preventUnmatched() +const spawk = tspawk(t) const readFile = promisify(fs.readFile) @@ -369,10 +369,6 @@ t.test('config edit', async t => { '.npmrc': 'foo=bar\nbar=baz', }) - t.teardown(() => { - spawk.clean() - }) - const EDITOR = 'vim' const editor = spawk.spawn(EDITOR).exit(0) @@ -394,10 +390,6 @@ t.test('config edit', async t => { }) t.test('config edit - editor exits non-0', async t => { - t.teardown(() => { - spawk.clean() - }) - const EDITOR = 'vim' const editor = spawk.spawn(EDITOR).exit(1) diff --git a/test/lib/commands/edit.js b/test/lib/commands/edit.js index 92754f2823256..1943e8c5fb4ae 100644 --- a/test/lib/commands/edit.js +++ b/test/lib/commands/edit.js @@ -1,138 +1,137 @@ const t = require('tap') -const { resolve } = require('path') -const { EventEmitter } = require('events') +const path = require('path') +const tspawk = require('../../fixtures/tspawk') +const { load: loadMockNpm } = require('../../fixtures/mock-npm') -let editorBin = null -let editorArgs = null -let editorOpts = null -let EDITOR_CODE = 0 -const childProcess = { - spawn: (bin, args, opts) => { - // save for assertions - editorBin = bin - editorArgs = args - editorOpts = opts +const spawk = tspawk(t) - const editorEvents = new EventEmitter() - process.nextTick(() => { - editorEvents.emit('exit', EDITOR_CODE) - }) - return editorEvents - }, -} +// 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') -let rebuildArgs = null -let rebuildFail = null -let EDITOR = 'vim' -const npm = { +const npmConfig = { config: { - get: () => EDITOR, + 'ignore-scripts': false, + editor: 'testeditor', }, - dir: resolve(__dirname, '../../../node_modules'), - exec: async (cmd, args) => { - rebuildArgs = args - if (rebuildFail) { - throw rebuildFail - } + prefixDir: { + node_modules: { + semver: { + 'package.json': JSON.stringify({ + scripts: { + install: 'testinstall', + }, + }), + node_modules: { + abbrev: {}, + }, + }, + '@npmcli': { + 'scoped-package': {}, + }, + }, }, } -const gracefulFs = require('graceful-fs') -const Edit = t.mock('../../../lib/commands/edit.js', { - child_process: childProcess, - 'graceful-fs': gracefulFs, -}) -const edit = new Edit(npm) - t.test('npm edit', async t => { - t.teardown(() => { - rebuildArgs = null - editorBin = null - editorArgs = null - editorOpts = null - }) + const { npm, joinedOutput } = await loadMockNpm(t, npmConfig) - await edit.exec(['semver']) - const path = resolve(__dirname, '../../../node_modules/semver') - t.strictSame(editorBin, EDITOR, 'used the correct editor') - t.strictSame(editorArgs, [path], 'edited the correct directory') - t.strictSame(editorOpts, { stdio: 'inherit' }, 'passed the correct opts') - t.strictSame(rebuildArgs, [path], 'passed the correct path to rebuild') + const semverPath = path.resolve(npm.prefix, 'node_modules', 'semver') + const [scriptShell] = makeSpawnArgs({ + event: 'install', + path: npm.prefix, + }) + spawk.spawn('testeditor', [semverPath]) + spawk.spawn( + scriptShell, + args => args.includes('testinstall'), + { cwd: semverPath } + ) + await npm.exec('edit', ['semver']) + t.match(joinedOutput(), 'rebuilt dependencies successfully') }) -t.test('rebuild fails', async t => { - t.teardown(() => { - rebuildFail = null - rebuildArgs = null - editorBin = null - editorArgs = null - editorOpts = null +t.test('rebuild failure', async t => { + const { npm } = await loadMockNpm(t, npmConfig) + const semverPath = path.resolve(npm.prefix, 'node_modules', 'semver') + const [scriptShell] = makeSpawnArgs({ + event: 'install', + path: npm.prefix, }) + spawk.spawn('testeditor', [semverPath]) + spawk.spawn( + scriptShell, + args => args.includes('testinstall'), + { cwd: semverPath } + ).exit(1).stdout('test error') + await t.rejects( + npm.exec('edit', ['semver']), + { message: 'command failed' } + ) +}) - rebuildFail = new Error('test error') +t.test('editor failure', async t => { + const { npm } = await loadMockNpm(t, npmConfig) + const semverPath = path.resolve(npm.prefix, 'node_modules', 'semver') + spawk.spawn('testeditor', [semverPath]).exit(1).stdout('test editor failure') await t.rejects( - edit.exec(['semver']), - { message: 'test error' } + npm.exec('edit', ['semver']), + { message: 'editor process exited with code: 1' } ) - const path = resolve(__dirname, '../../../node_modules/semver') - t.strictSame(editorBin, EDITOR, 'used the correct editor') - t.strictSame(editorArgs, [path], 'edited the correct directory') - t.strictSame(editorOpts, { stdio: 'inherit' }, 'passed the correct opts') - t.strictSame(rebuildArgs, [path], 'passed the correct path to rebuild') }) t.test('npm edit editor has flags', async t => { - EDITOR = 'code -w' - t.teardown(() => { - rebuildArgs = null - editorBin = null - editorArgs = null - editorOpts = null - EDITOR = 'vim' + const { npm } = await loadMockNpm(t, { + ...npmConfig, + config: { + ...npmConfig.config, + editor: 'testeditor --flag', + }, }) - await edit.exec(['semver']) - - const path = resolve(__dirname, '../../../node_modules/semver') - t.strictSame(editorBin, 'code', 'used the correct editor') - t.strictSame(editorArgs, ['-w', path], 'edited the correct directory, keeping flags') - t.strictSame(editorOpts, { stdio: 'inherit' }, 'passed the correct opts') - t.strictSame(rebuildArgs, [path], 'passed the correct path to rebuild') + const semverPath = path.resolve(npm.prefix, 'node_modules', 'semver') + const [scriptShell] = makeSpawnArgs({ + event: 'install', + path: npm.prefix, + }) + spawk.spawn('testeditor', ['--flag', semverPath]) + spawk.spawn( + scriptShell, + args => args.includes('testinstall'), + { cwd: semverPath } + ) + await npm.exec('edit', ['semver']) }) t.test('npm edit no args', async t => { + const { npm } = await loadMockNpm(t) await t.rejects( - edit.exec([]), - /npm edit/, + npm.exec('edit', []), + { code: 'EUSAGE' }, 'throws usage error' ) }) -t.test('npm edit lstat error propagates', async t => { - const _lstat = gracefulFs.lstat - gracefulFs.lstat = (dir, cb) => { - return cb(new Error('lstat failed')) - } - t.teardown(() => { - gracefulFs.lstat = _lstat - }) +t.test('npm edit nonexistent package', async t => { + const { npm } = await loadMockNpm(t, npmConfig) await t.rejects( - edit.exec(['semver']), - /lstat failed/, - 'user received correct error' + npm.exec('edit', ['abbrev']), + /lstat/ ) }) -t.test('npm edit editor exit code error propagates', async t => { - EDITOR_CODE = 137 - t.teardown(() => { - EDITOR_CODE = 0 - }) +t.test('scoped package', async t => { + const { npm } = await loadMockNpm(t, npmConfig) + const scopedPath = path.resolve(npm.prefix, 'node_modules', '@npmcli', 'scoped-package') + spawk.spawn('testeditor', [scopedPath]) + await npm.exec('edit', ['@npmcli/scoped-package']) +}) - await t.rejects( - edit.exec(['semver']), - /exited with code: 137/, - 'user received correct error' - ) +t.test('subdependency', async t => { + const { npm } = await loadMockNpm(t, npmConfig) + const subdepPath = path.resolve(npm.prefix, 'node_modules', 'semver', 'node_modules', 'abbrev') + spawk.spawn('testeditor', [subdepPath]) + await npm.exec('edit', ['semver/abbrev']) }) diff --git a/test/lib/commands/restart.js b/test/lib/commands/restart.js index 83773eae9543b..84bd93d8c99ca 100644 --- a/test/lib/commands/restart.js +++ b/test/lib/commands/restart.js @@ -1,11 +1,8 @@ const t = require('tap') -const spawk = require('spawk') +const tspawk = require('../../fixtures/tspawk') const { load: loadMockNpm } = require('../../fixtures/mock-npm') -spawk.preventUnmatched() -t.teardown(() => { - spawk.unload() -}) +const spawk = tspawk(t) // 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 diff --git a/test/lib/commands/start.js b/test/lib/commands/start.js index c9312c8e2adc7..8fc73493d20a9 100644 --- a/test/lib/commands/start.js +++ b/test/lib/commands/start.js @@ -1,11 +1,8 @@ const t = require('tap') -const spawk = require('spawk') +const tspawk = require('../../fixtures/tspawk') const { load: loadMockNpm } = require('../../fixtures/mock-npm') -spawk.preventUnmatched() -t.teardown(() => { - spawk.unload() -}) +const spawk = tspawk(t) // 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 diff --git a/test/lib/commands/stop.js b/test/lib/commands/stop.js index f5db4a047d3f7..f2aef21899f6c 100644 --- a/test/lib/commands/stop.js +++ b/test/lib/commands/stop.js @@ -1,11 +1,8 @@ const t = require('tap') -const spawk = require('spawk') +const tspawk = require('../../fixtures/tspawk') const { load: loadMockNpm } = require('../../fixtures/mock-npm') -spawk.preventUnmatched() -t.teardown(() => { - spawk.unload() -}) +const spawk = tspawk(t) // 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 diff --git a/test/lib/commands/test.js b/test/lib/commands/test.js index 665df7148a0e5..e9ea0a3c834aa 100644 --- a/test/lib/commands/test.js +++ b/test/lib/commands/test.js @@ -1,11 +1,8 @@ const t = require('tap') -const spawk = require('spawk') +const tspawk = require('../../fixtures/tspawk') const { load: loadMockNpm } = require('../../fixtures/mock-npm') -spawk.preventUnmatched() -t.teardown(() => { - spawk.unload() -}) +const spawk = tspawk(t) // 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 diff --git a/test/lib/utils/split-package-names.js b/test/lib/utils/split-package-names.js deleted file mode 100644 index 5fe1e6cd8dde3..0000000000000 --- a/test/lib/utils/split-package-names.js +++ /dev/null @@ -1,18 +0,0 @@ -'use strict' - -const t = require('tap') -const splitPackageNames = require('../../../lib/utils/split-package-names.js') - -t.test('splitPackageNames', t => { - const assertions = [ - ['semver', 'semver'], - ['read-pkg/semver', 'read-pkg/node_modules/semver'], - ['@npmcli/one/@npmcli/two', '@npmcli/one/node_modules/@npmcli/two'], - ['@npmcli/one/semver', '@npmcli/one/node_modules/semver'], - ] - - for (const [input, expected] of assertions) { - t.equal(splitPackageNames(input), expected, `split ${input} correctly`) - } - t.end() -})