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

Author: wraithgar

lib/commands/edit.js

@@ -4,10 +4,31 @@
 const { resolve } = require('path')
 const fs = require('graceful-fs')
 const { spawn } = require('child_process')
-const splitPackageNames = require('../utils/split-package-names.js')
 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'

lib/utils/split-package-names.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
+}

test/fixtures/tspawk.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)
 

test/lib/commands/config.js

@@ -1,138 +1,135 @@
 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,
+    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: {
+      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'])
 })

test/lib/commands/edit.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

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

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