fix: consolidate path delimiter logic

Author: wraithgar

lib/commands/bin.js

@@ -1,5 +1,9 @@
-const envPath = require('../utils/path.js')
 const BaseCommand = require('../base-command.js')
+// TODO this may not be needed at all. Ignoring because our tests normalize
+// process.env.path already
+/* istanbul ignore next */
+const path = process.env.path || process.env.Path || process.env.PATH
+const { delimiter } = require('path')
 
 class Bin extends BaseCommand {
   static description = 'Display npm bin folder'
@@ -10,7 +14,7 @@ class Bin extends BaseCommand {
   async exec (args) {
     const b = this.npm.bin
     this.npm.output(b)
-    if (this.npm.config.get('global') && !envPath.includes(b)) {
+    if (this.npm.config.get('global') && !path.split(delimiter).includes(b)) {
       // XXX: does this need to be console?
       console.error('(not in PATH env variable)')
     }

lib/utils/path.js

@@ -115,6 +115,7 @@ const LoadMockNpm = async (t, {
     Npm,
     npm,
     prefix,
+    globalPrefix,
     testdir: dir,
     cache,
     debugFile: async () => {

test/fixtures/mock-npm.js

@@ -1,76 +1,43 @@
 const t = require('tap')
-const { fake: mockNpm } = require('../../fixtures/mock-npm')
-
-t.test('bin', async t => {
-  t.plan(2)
-  const dir = '/bin/dir'
-
-  const Bin = require('../../../lib/commands/bin.js')
-
-  const npm = mockNpm({
-    bin: dir,
-    config: { global: false },
-    output: (output) => {
-      t.equal(output, dir, 'prints the correct directory')
-    },
+const { load: loadMockNpm } = require('../../fixtures/mock-npm')
+const mockGlobals = require('../../fixtures/mock-globals')
+
+t.test('bin not global', async t => {
+  let error
+  const { npm, joinedOutput } = await loadMockNpm(t)
+  mockGlobals(t, {
+    'console.error': (err) => error = err,
   })
-  const bin = new Bin(npm)
-  t.match(bin.usage, 'bin', 'usage has command name in it')
-
-  await bin.exec([])
+  await npm.exec('bin', [])
+  t.match(joinedOutput(), npm.localBin)
+  t.notOk(error, 'should not error')
 })
 
-t.test('bin -g', async t => {
-  t.plan(1)
-  const consoleError = console.error
-  t.teardown(() => {
-    console.error = consoleError
-  })
-
-  console.error = (output) => {
-    t.fail('should not have printed to console.error')
-  }
-  const dir = '/bin/dir'
-
-  const Bin = t.mock('../../../lib/commands/bin.js', {
-    '../../../lib/utils/path.js': [dir],
-  })
-
-  const npm = mockNpm({
-    bin: dir,
+t.test('bin global in env.path', async t => {
+  let error
+  const { npm, joinedOutput } = await loadMockNpm(t, {
     config: { global: true },
-    output: (output) => {
-      t.equal(output, dir, 'prints the correct directory')
-    },
   })
-  const bin = new Bin(npm)
-
-  await bin.exec([])
-})
 
-t.test('bin -g (not in path)', async t => {
-  t.plan(2)
-  const consoleError = console.error
-  t.teardown(() => {
-    console.error = consoleError
+  mockGlobals(t, {
+    'console.error': (err) => error = err,
+    'process.env': { path: npm.globalBin },
   })
+  await npm.exec('bin', [])
+  t.match(joinedOutput(), npm.globalBin)
+  t.notOk(error, 'should not error')
+})
 
-  console.error = (output) => {
-    t.equal(output, '(not in PATH env variable)', 'prints env warning')
-  }
-  const dir = '/bin/dir'
-
-  const Bin = t.mock('../../../lib/commands/bin.js', {
-    '../../../lib/utils/path.js': ['/not/my/dir'],
-  })
-  const npm = mockNpm({
-    bin: dir,
-    config: { global: true },
-    output: (output) => {
-      t.equal(output, dir, 'prints the correct directory')
+t.test('bin not in path', async t => {
+  let error
+  const { npm } = await loadMockNpm(t, {
+    config: {
+      global: true,
+    },
+    globals: {
+      'console.error': (err) => error = err,
     },
   })
-  const bin = new Bin(npm)
-
-  await bin.exec([])
+  await npm.exec('bin', [])
+  t.match(error, '(not in PATH env variable)')
 })

test/lib/commands/bin.js

@@ -72,8 +72,6 @@ const read = (options, cb) => {
   process.nextTick(() => cb(READ_ERROR, READ_RESULT))
 }
 
-const PATH = require('../../../lib/utils/path.js')
-
 let CI_NAME = 'travis-ci'
 
 const log = {
@@ -154,7 +152,7 @@ t.test('npx foo, bin already exists locally', async t => {
       stdioString: true,
       event: 'npx',
       env: {
-        PATH: [npm.localBin, ...PATH].join(delimiter),
+        PATH: [npm.localBin, process.env.PATH].join(delimiter),
       },
       stdio: 'inherit',
     },
@@ -183,7 +181,7 @@ t.test('npx foo, bin already exists globally', async t => {
       stdioString: true,
       event: 'npx',
       env: {
-        PATH: [npm.globalBin, ...PATH].join(delimiter),
+        PATH: [npm.globalBin, process.env.PATH].join(delimiter),
       },
       stdio: 'inherit',
     },
@@ -1175,7 +1173,7 @@ t.test('workspaces', t => {
         stdioString: true,
         event: 'npx',
         env: {
-          PATH: [npm.localBin, ...PATH].join(delimiter),
+          PATH: [npm.localBin, process.env.PATH].join(delimiter),
         },
         stdio: 'inherit',
       },