fix kill and make it work better on more platforms

cli.mjs

@@ -27,15 +27,20 @@ function PrintHelp() {
   process.exit(1)
 }
 
-function showErrorAndExit(message = '', err = null, code = 1) {
-  console.log('')
+function showErrorAndExit(message = '', err = null, code = 1, clean = false) {
+  if (!clean) {
+    console.log('')
+  }
 
   if (message) {
     console.error(`\x1b[31m${message}\x1b[0m`)
   }
 
   if (err) {
-    printError(err)
+    printError(err, '', clean)
+    if (err.inner) {
+      return showErrorAndExit(null, err.inner, code, true)
+    }
   } else {
     PrintHelp()
   }
@@ -54,7 +59,7 @@ cli.parseOptions(args)
 })
 .catch(function(err) { showErrorAndExit('Unknown error while processing arguments', err) })
 .then(function() {
-  if (!cli.files.length) {
+  if (!cli.files.length && cli.run === 'test') {
     showErrorAndExit('No files were found with pattern', cli.targets.join(','))
   }
 

lib/cli.mjs

@@ -3,6 +3,7 @@ import fs from 'fs'
 import fsPromise from 'fs/promises'
 import cluster from 'cluster'
 import child_process from 'child_process'
+import kill from './kill.mjs'
 import Watcher, { EVENT_REMOVE, EVENT_UPDATE } from './watch/index.mjs'
 
 export const MESSAGE_FILES_REQUEST = 'message_files_request'
@@ -41,8 +42,12 @@ export function CLI(e, overrides = {}) {
   this.logger = overrides.logger || console
   this.child_process = overrides.child_process || child_process
   this.process = overrides.process || process
+  this.kill = overrides.kill || kill
   this.importer = overrides.importer
+  this.loadDefaults()
+}
 
+CLI.prototype.loadDefaults = function() {
   // Eltro specific options
   this.reporter = 'list'
   this.ignoreOnly = false
@@ -110,7 +115,7 @@ CLI.prototype.parseOptions = function(args) {
     }
   }
 
-  if (!this.targets.length) {
+  if (!this.targets.length && this.run === 'test') {
     this.targets.push('test/**')
   }
 
@@ -284,7 +289,7 @@ CLI.prototype.runProgram = function() {
     if (runningTest) {
       return
     } else {
-      this.worker.kill()
+      this.kill(this.worker.pid)
     }
   }
 
@@ -435,9 +440,9 @@ export function getFilesFromTarget(files, match, insidePath, grabAll, insideStar
   })
 }
 
-export function printError(err, msg) {
+export function printError(err, msg, clean = false) {
   let before = msg || ''
-  console.error('')
+  if (!clean) console.error('')
   console.error('\x1b[31m    '
         + before + err.toString()
         + '\x1b[0m\n    \x1b[90m'

lib/kill.mjs

@@ -0,0 +1,85 @@
+import { promisify } from 'util'
+import { spawn, exec } from 'child_process'
+
+const execPromise = promisify(exec)
+
+export default function kill(pid, signal) {
+  let pids = new Set([pid])
+  let getSpawn = null
+  let getPids = null
+
+  switch (process.platform) {
+    case 'win32':
+      return execPromise('taskkill /pid ' + pid + ' /T /F').then(() => pids)
+    case 'darwin':
+      getSpawn = function(parentPid) {
+        return spawn('pgrep', ['-P', parentPid])
+      }
+      getPids = function(data) {
+        return data.match(/\d+/g).map(Number)
+      }
+      break
+    default:
+      getSpawn = function (parentPid) {
+        return exec('ps -opid="" -oppid="" | grep ' + parentPid)
+      }
+      getPids = function(data, parentPid) {
+        let output = data.trim().split('\n')
+        return output.map(line => {
+          let [child, parent] = line.trim().split(/ +/)
+
+          if (Number(parent) === parentPid) {
+            return Number(child)
+          }
+
+          return 0
+        }).filter(x => x)
+      }
+      break
+  }
+
+  return buildTree(pids, getSpawn, getPids, pid)
+    .then(function() {
+      for (let pid of pids) {
+        try {
+          process.kill(pid, signal)
+        } catch (err) {
+          if (err.code !== 'ESRCH') throw err;
+        }
+      }
+      return pids
+    })
+}
+
+function buildTree(allPids, spawnGetChildren, spawnGetPids, parentPid) {
+  allPids.add(parentPid)
+
+  let ps = spawnGetChildren(parentPid)
+  let data = ''
+  let err = ''
+  ps.stdout.on('data', function(buf) {
+    data += buf.toString('ascii')
+  })
+  ps.stderr.on('data', function(buf) {
+    err += buf.toString('ascii')
+  })
+
+  return new Promise(function(res, rej) {
+    ps.on('close', function(code) {
+      // Check if ps errored out
+      if (code !== 0 && err.trim()) {
+        return rej(new Error('Error running ps to kill processes:\n\t' + err))
+      }
+
+      // Check if we otherwise got an error code (usually means empty results)
+      if (code !== 0 || !data.trim()) return res()
+
+      let pids = spawnGetPids(data, parentPid)
+
+      res(Promise.all(
+        pids.filter(pid => pid && !allPids.has(pid))
+          .map(buildTree.bind(this, allPids, spawnGetChildren, spawnGetPids))
+      ))
+    })
+  })
+}

package.json

@@ -1,6 +1,6 @@
 {
   "name": "eltro",
-  "version": "1.4.0",
+  "version": "1.4.5",
   "description": "Eltro is a tiny no-dependancy test framework for node",
   "main": "index.mjs",
   "scripts": {

test/cli.test.mjs

@@ -26,8 +26,9 @@ t.describe('CLI', function() {
       const cluster = { c: 3 }
       const process = { d: 4 }
       const importer = { e: 5 }
+      const kill = { f: 6 }
       
-      let cliTest = createSafeCli(eltro, { logger, cluster, process, importer })
+      let cliTest = createSafeCli(eltro, { logger, cluster, process, importer, kill })
       assert.strictEqual(cliTest.reporter, 'list')
       assert.strictEqual(cliTest.ignoreOnly, false)
       assert.strictEqual(cliTest.timeout, 2000)
@@ -43,6 +44,7 @@ t.describe('CLI', function() {
       assert.strictEqual(cliTest.cluster, cluster)
       assert.strictEqual(cliTest.process, process)
       assert.strictEqual(cliTest.importer, importer)
+      assert.strictEqual(cliTest.kill, kill)
     })
   
     t.test('should detect isSlave from cluster', function() {
@@ -59,32 +61,31 @@ t.describe('CLI', function() {
    *****************************************/
 
   t.describe('#parseOptions()', function() {
+    t.beforeEach(function() {
+      cli.loadDefaults()
+    })
+
     t.test('should not do anything if no options', async function() {
-      cli.reporter = 'list'
       await cli.parseOptions([])
       assert.strictEqual(cli.reporter, 'list')
     })
 
     t.test('should support overriding reporter with shorthand option', async function() {
-      cli.reporter = 'list'
       await cli.parseOptions(['-r', 'dot'])
       assert.strictEqual(cli.reporter, 'dot')
     })
 
     t.test('should support overriding reporter with long option', async function() {
-      cli.reporter = 'list'
       await cli.parseOptions(['--reporter', 'dot'])
       assert.strictEqual(cli.reporter, 'dot')
     })
     
     t.test('should support enabling ignore-only long option', async function() {
-      cli.ignoreOnly = false
       await cli.parseOptions(['--ignore-only', '-r', 'dot'])
       assert.strictEqual(cli.ignoreOnly, true)
     })
 
     t.test('should support reporter list', async function() {
-      cli.reporter = 'list'
       await cli.parseOptions(['-r', 'list'])
       assert.strictEqual(cli.reporter, 'list')
     })
@@ -100,13 +101,11 @@ t.describe('CLI', function() {
     })
 
     t.test('should support overriding timeout with shorthand option', async function() {
-      cli.timeout = 2000
       await cli.parseOptions(['-t', '1000'])
       assert.strictEqual(cli.timeout, 1000)
     })
 
     t.test('should support overriding timeout with long option', async function() {
-      cli.timeout = 2000
       await cli.parseOptions(['--timeout', '250'])
       assert.strictEqual(cli.timeout, 250)
     })
@@ -122,57 +121,54 @@ t.describe('CLI', function() {
     })
 
     t.test('should support overriding watch', async function() {
-      cli.watch = null
       await cli.parseOptions(['-w', 'unittest_test1'])
       assert.strictEqual(cli.watch, 'unittest_test1')
     })
 
     t.test('should support overriding watch with long option', async function() {
-      cli.watch = null
       await cli.parseOptions(['--watch', 'unittest_test1'])
       assert.strictEqual(cli.watch, 'unittest_test1')
     })
 
-    t.test('should fail overriding if next parameter is missing', async function() {
-      cli.watch = null
+    t.test('should fail setting watch if value is missing', async function() {
       let err = await assert.isRejected(cli.parseOptions(['--watch']))
       assert.strictEqual(cli.watch, null)
       assert.match(err.message, /watch/i)
     })
 
-    t.test('should fail overriding if next parameter is a parameter', async function() {
-      cli.watch = null
+    t.test('should fail setting watch if value is parameter', async function() {
       let err = await assert.isRejected(cli.parseOptions(['-w', '--reporter', 'list']))
       assert.strictEqual(cli.watch, null)
       assert.match(err.message, /watch/i)
     })
 
     t.test('should support overriding run', async function() {
-      cli.run = null
       await cli.parseOptions(['-n', 'unittest_run1'])
       assert.strictEqual(cli.run, 'unittest_run1')
     })
 
     t.test('should support overriding run with long option', async function() {
-      cli.run = null
       await cli.parseOptions(['--npm', 'unittest_run1'])
       assert.strictEqual(cli.run, 'unittest_run1')
     })
 
-    t.test('should fail overriding if next parameter is missing', async function() {
-      cli.run = null
+    t.test('should fail setting npm if value is missing', async function() {
       let err = await assert.isRejected(cli.parseOptions(['--npm']))
-      assert.strictEqual(cli.run, null)
+      assert.strictEqual(cli.run, 'test')
       assert.match(err.message, /npm/i)
     })
 
-    t.test('should fail overriding if next parameter is a parameter', async function() {
-      cli.run = null
+    t.test('should fail setting npm if value is parameter', async function() {
       let err = await assert.isRejected(cli.parseOptions(['-n', '--reporter', 'list']))
-      assert.strictEqual(cli.run, null)
+      assert.strictEqual(cli.run, 'test')
       assert.match(err.message, /npm/i)
     })
 
+    t.test('when using run and no target, leave target empty', async function() {
+      await cli.parseOptions(['--npm', 'unittest_run1'])
+      assert.strictEqual(cli.targets.length, 0)
+    })
+
     t.test('should add file to targets', async function() {
       await cli.parseOptions(['test'])
       assert.deepEqual(cli.targets, ['test'])
@@ -756,10 +752,10 @@ t.describe('CLI', function() {
     
           assert.ok(eltro.starting)
           assert.strictEqual(loaded.length, 2)
-          assert.match(loaded[0], path.join('root', testFiles[1]))
-          assert.match(loaded[1], path.join('root', testFiles[2]))
-          assert.match(filenames[0], testFiles[1])
-          assert.match(filenames[1], testFiles[2])
+          assert.match(loaded[0], new RegExp(path.join('root', testFiles[1]).replace(/\\/g, '\\\\')))
+          assert.match(loaded[1], new RegExp(path.join('root', testFiles[2]).replace(/\\/g, '\\\\')))
+          assert.match(filenames[0], new RegExp(testFiles[1].replace(/\\/g, '\\\\')))
+          assert.match(filenames[1], new RegExp(testFiles[2].replace(/\\/g, '\\\\')))
         })
     
         t.test('on error should throw and wrap in inner error', async function() {
@@ -919,14 +915,16 @@ t.describe('CLI', function() {
     let testCluster
     let testChildProcess
     let testWorker
+    let testKill
     let cli
 
     t.beforeEach(function() {
+      testKill = stub()
       testProcess = { stdout: { write: stub() }, stderr: { write: stub() } }
-      testWorker = { on: stub(), once: stub(), kill: stub(), send: stub(), stderr: { on: stub() }, stdout: { on: stub() } }
+      testWorker = { on: stub(), once: stub(),  send: stub(), stderr: { on: stub() }, stdout: { on: stub() } }
       testCluster = { fork: stub().returns(testWorker) }
       testChildProcess = { spawn: stub().returns(testWorker) }
-      cli = createSafeCli(null, { cluster: testCluster, child_process: testChildProcess, process: testProcess })
+      cli = createSafeCli(null, { cluster: testCluster, child_process: testChildProcess, process: testProcess, kill: testKill })
     })
 
     t.describe('in test mode', function() {
@@ -990,14 +988,14 @@ t.describe('CLI', function() {
       t.test('multiple calls should cancel', function() {
         cli.runProgram()
   
-        assert.notOk(testWorker.kill.called)
+        assert.notOk(testKill.called)
         assert.ok(testCluster.fork.called)
         testCluster.fork.reset()
   
         cli.runProgram()
   
         assert.notOk(testCluster.fork.called)
-        assert.notOk(testWorker.kill.called)
+        assert.notOk(testKill.called)
       })
     })
 
@@ -1069,16 +1067,19 @@ t.describe('CLI', function() {
       })
   
       t.test('multiple calls should kill', function() {
+        const assertPid = 12345235
+        testWorker.pid = assertPid
         cli.runProgram()
   
-        assert.notOk(testWorker.kill.called)
+        assert.notOk(testKill.called)
         assert.ok(testChildProcess.spawn.called)
         testChildProcess.spawn.reset().returns(testWorker)
   
         cli.runProgram()
   
         assert.ok(testChildProcess.spawn.called)
-        assert.ok(testWorker.kill.called)
+        assert.ok(testKill.called)
+        assert.strictEqual(testKill.firstCall[0], assertPid)
       })
     })
   })

test/eltro.test.mjs

@@ -234,7 +234,6 @@ e.test('Eltro should support capturing unknown errors outside scope', async func
   t.describe('', function() {
     t.test('', function(cb) {
       process.nextTick(function() {
-        console.log('throw')
         throw assertError
       })
     })

test/kill/kill.test.mjs

@@ -0,0 +1,41 @@
+import { spawn } from 'child_process'
+import t from '../../lib/eltro.mjs'
+import assert from '../../lib/assert.mjs'
+import kill from '../../lib/kill.mjs'
+
+t.describe('kill', function() {
+  let worker
+
+  t.afterEach(function() {
+    if (worker?.pid && !worker.killed) {
+      worker.kill()
+    }
+  })
+
+  t.test('should kill process correctly', function(done) {
+    worker = spawn('node', ['./test/kill/runner.mjs'])
+    assert.ok(worker.pid)
+
+    worker.on('exit', done.finish(function(code, signal) {
+      assert.ok(code || signal)
+    }))
+
+    kill(worker.pid)
+  })
+
+  t.test('should succeed in killing tree', async function() {
+    worker = spawn('node', ['./test/kill/runner.mjs'])
+    assert.ok(worker.pid)
+
+    // Give it some time to start
+    await new Promise(res => {
+      worker.stdout.on('data', function(data) {
+        if (data.toString().indexOf('secondary') >= 0) res()
+      })
+    })
+
+    return kill(worker.pid).then(function(pids) {
+      assert.strictEqual(pids.size, 2)
+    })
+  })
+})

test/kill/runner.mjs

@@ -0,0 +1,11 @@
+import { spawn } from 'child_process'
+
+console.log('primary', process.pid)
+
+let secondary = spawn('node', ['./test/kill/second_runner.mjs'])
+
+secondary.stdout.on('data', function(data) {
+  process.stdout.write(data)
+})
+
+setInterval(function() { console.log('primary', process.pid) }, 100)

test/kill/second_runner.mjs

@@ -0,0 +1,2 @@
+console.log('secondary', process.pid)
+setInterval(function() { console.log('secondary', process.pid) }, 100)

test/watch.test.mjs

@@ -28,7 +28,7 @@ t.afterEach(function(done) {
 
 t.after(function() {
   if (builder) {
-    builder.cleanup()
+    return builder.cleanup()
   }
 })
 

test/watch/builder.mjs

@@ -173,7 +173,7 @@ Builder.prototype.newRandomFiles = function(fpath, count) {
 }
 
 Builder.prototype.cleanup = function() {
-  return fs.rmdir(this.root)
+  return fs.rm(this.root, { recursive: true, force: true })
 }
 
 Builder.prototype.getAllDirectories = function() {