Repository: karma-runner/karma
Pull request #3647 (2021-02-03 14:32:25)

Bug fix: Report launcher process error when the process exit event is not emitted
This PR is to ensure the launcher process error is reported in the logs when the process exit event is not emitted which can happen in some cases.

As per described on this page https://nodejs.org/api/child_process.html#child_process_event_error, "The 'exit' event may or may not fire after an error has occurred. When listening to both the 'exit' and 'error' events, guard against accidentally invoking handler functions multiple times."

**Without this fix**, if the browser binary file is missing, Karma will report the following **if only in Debug mode**:

```
DEBUG [launcher]: Process {{browser_name}} exited with code -1
```
This is not really helpful to the users and if debug log level is not set, they won't see this message.

**With this fix**, if the browser binary file is missing, Karma will **always** report the following:

```
ERROR [launcher]: Cannot start {{browser_name}}
        Can not find the binary {{path_to_binary}}
        Please set env variable {{env_cmd}}
```
This error is clear to the users and always visible

google-cla[bot]: 
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: **Please visit <https://cla.developers.google.com/> to sign.**

Once you've signed (or fixed any issues), please reply here with `@googlebot I signed it!` and we'll verify it.

----

#### What to do if you already signed the CLA

##### Individual signers

*   It's possible we don't have your GitHub username or you're using a different email address on your commit. Check [your existing CLA data](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).

##### Corporate signers

*   Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to [go/cla#troubleshoot](http://go/cla#troubleshoot) ([Public version](https://opensource.google/docs/cla/#troubleshoot)).
*   The email used to register you as an authorized contributor must be the email used for the Git commit. Check [your existing CLA data](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).
*   The email used to register you as an authorized contributor must also be [attached to your GitHub account](https://github.com/settings/emails).
		

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fkarma-runner%2Fkarma%2Fpull%2F3647) for more info**.

<!-- need_sender_cla -->
AppVeyorBot: :x: [Build karma 2918 failed](https://ci.appveyor.com/project/dignifiedquire/karma/builds/37590358) (commit https://github.com/karma-runner/karma/commit/f5b78c5937 by @)
karmarunnerbot: :x: [Build karma 521 failed](https://ci.appveyor.com/project/karmarunnerbot/karma/builds/37590357) (commit https://github.com/karma-runner/karma/commit/f5b78c5937 by @)
karmarunnerbot: :x: [Build karma 520 failed](https://ci.appveyor.com/project/karmarunnerbot/karma-8n04t/builds/37590359) (commit https://github.com/karma-runner/karma/commit/f5b78c5937 by @)
chrisbottin: @googlebot I signed it!
karmarunnerbot: :white_check_mark: [Build karma 522 completed](https://ci.appveyor.com/project/karmarunnerbot/karma/builds/37590645) (commit https://github.com/karma-runner/karma/commit/cdc8f51f3c by @)
AppVeyorBot: :white_check_mark: [Build karma 2919 completed](https://ci.appveyor.com/project/dignifiedquire/karma/builds/37590646) (commit https://github.com/karma-runner/karma/commit/cdc8f51f3c by @)
karmarunnerbot: :white_check_mark: [Build karma 521 completed](https://ci.appveyor.com/project/karmarunnerbot/karma-8n04t/builds/37590648) (commit https://github.com/karma-runner/karma/commit/cdc8f51f3c by @)
AppVeyorBot: :white_check_mark: [Build karma 2920 completed](https://ci.appveyor.com/project/dignifiedquire/karma/builds/37591918) (commit https://github.com/karma-runner/karma/commit/a5d35acf91 by @)
karmarunnerbot: :white_check_mark: [Build karma 523 completed](https://ci.appveyor.com/project/karmarunnerbot/karma/builds/37591917) (commit https://github.com/karma-runner/karma/commit/a5d35acf91 by @)
karmarunnerbot: :white_check_mark: [Build karma 522 completed](https://ci.appveyor.com/project/karmarunnerbot/karma-8n04t/builds/37591919) (commit https://github.com/karma-runner/karma/commit/a5d35acf91 by @)
AppVeyorBot: :white_check_mark: [Build karma 2921 completed](https://ci.appveyor.com/project/dignifiedquire/karma/builds/37595408) (commit https://github.com/karma-runner/karma/commit/c4bdbd38a5 by @)
karmarunnerbot: :white_check_mark: [Build karma 524 completed](https://ci.appveyor.com/project/karmarunnerbot/karma/builds/37595409) (commit https://github.com/karma-runner/karma/commit/c4bdbd38a5 by @)
karmarunnerbot: :white_check_mark: [Build karma 523 completed](https://ci.appveyor.com/project/karmarunnerbot/karma-8n04t/builds/37595411) (commit https://github.com/karma-runner/karma/commit/c4bdbd38a5 by @)

Patch:
diff --git a/lib/launchers/process.js b/lib/launchers/process.js
index 072c15b8..a7aa101c 100644
--- a/lib/launchers/process.js
+++ b/lib/launchers/process.js
@@ -93,6 +93,7 @@ function ProcessLauncher (spawn, tempDir, timer, processKillTimeout) {
       } else {
         errorOutput += err.toString()
       }
+      self._onProcessExit(-1, null, errorOutput)
     })
 
     self._process.stderr.on('data', function (errBuff) {
@@ -101,6 +102,10 @@ function ProcessLauncher (spawn, tempDir, timer, processKillTimeout) {
   }
 
   this._onProcessExit = function (code, signal, errorOutput) {
+    if (!self._process) {
+      // Both exit and error events trigger _onProcessExit(), but we only need one cleanup.
+      return
+    }
     log.debug(`Process ${self.name} exited with code ${code} and signal ${signal}`)
 
     let error = null


Test patch:
diff --git a/test/unit/launchers/process.spec.js b/test/unit/launchers/process.spec.js
index 6dfc29b4..489d7ecd 100644
--- a/test/unit/launchers/process.spec.js
+++ b/test/unit/launchers/process.spec.js
@@ -7,18 +7,25 @@ const CaptureTimeoutLauncher = require('../../../lib/launchers/capture_timeout')
 const ProcessLauncher = require('../../../lib/launchers/process')
 const EventEmitter = require('../../../lib/events').EventEmitter
 const createMockTimer = require('../mocks/timer')
+const logger = require('../../../lib/logger')
 
 describe('launchers/process.js', () => {
   let emitter
   let mockSpawn
   let mockTempDir
   let launcher
+  let logErrorSpy
+  let logDebugSpy
 
   const BROWSER_PATH = path.normalize('/usr/bin/browser')
 
   beforeEach(() => {
     emitter = new EventEmitter()
     launcher = new BaseLauncher('fake-id', emitter)
+    launcher.name = 'fake-name'
+    launcher.ENV_CMD = 'fake-ENV-CMD'
+    logErrorSpy = sinon.spy(logger.create('launcher'), 'error')
+    logDebugSpy = sinon.spy(logger.create('launcher'), 'debug')
 
     mockSpawn = sinon.spy(function (cmd, args) {
       const process = new EventEmitter()
@@ -74,7 +81,7 @@ describe('launchers/process.js', () => {
   })
 
   describe('with RetryLauncher', () => {
-    it('should handle spawn ENOENT error and not even retry', (done) => {
+    function assertSpawnError ({ errorCode, emitExit, expectedError }, done) {
       ProcessLauncher.call(launcher, mockSpawn, mockTempDir)
       RetryLauncher.call(launcher, 2)
       launcher._getCommand = () => BROWSER_PATH
@@ -83,35 +90,56 @@ describe('launchers/process.js', () => {
       emitter.on('browser_process_failure', failureSpy)
 
       launcher.start('http://host:9876/')
-      mockSpawn._processes[0].emit('error', { code: 'ENOENT' })
-      mockSpawn._processes[0].emit('exit', 1)
+      mockSpawn._processes[0].emit('error', { code: errorCode })
+      if (emitExit) {
+        mockSpawn._processes[0].emit('exit', 1)
+      }
       mockTempDir.remove.callArg(1)
 
       _.defer(() => {
         expect(launcher.state).to.equal(launcher.STATE_FINISHED)
         expect(failureSpy).to.have.been.called
+        expect(logDebugSpy).to.have.been.callCount(5)
+        expect(logDebugSpy.getCall(0)).to.have.been.calledWithExactly('null -> BEING_CAPTURED')
+        expect(logDebugSpy.getCall(1)).to.have.been.calledWithExactly(`${BROWSER_PATH} http://host:9876/?id=fake-id`)
+        expect(logDebugSpy.getCall(2)).to.have.been.calledWithExactly('Process fake-name exited with code -1 and signal null')
+        expect(logDebugSpy.getCall(3)).to.have.been.calledWithExactly('fake-name failed (cannot start). Not restarting.')
+        expect(logDebugSpy.getCall(4)).to.have.been.calledWithExactly('BEING_CAPTURED -> FINISHED')
+        expect(logErrorSpy).to.have.been.calledWith(expectedError)
         done()
       })
+    }
+
+    it('should handle spawn ENOENT error and not even retry', (done) => {
+      assertSpawnError({
+        errorCode: 'ENOENT',
+        emitExit: true,
+        expectedError: `Cannot start fake-name\n\tCan not find the binary ${BROWSER_PATH}\n\tPlease set env variable fake-ENV-CMD`
+      }, done)
     })
 
     it('should handle spawn EACCES error and not even retry', (done) => {
-      ProcessLauncher.call(launcher, mockSpawn, mockTempDir)
-      RetryLauncher.call(launcher, 2)
-      launcher._getCommand = () => BROWSER_PATH
-
-      const failureSpy = sinon.spy()
-      emitter.on('browser_process_failure', failureSpy)
+      assertSpawnError({
+        errorCode: 'EACCES',
+        emitExit: true,
+        expectedError: `Cannot start fake-name\n\tPermission denied accessing the binary ${BROWSER_PATH}\n\tMaybe it's a directory?`
+      }, done)
+    })
 
-      launcher.start('http://host:9876/')
-      mockSpawn._processes[0].emit('error', { code: 'EACCES' })
-      mockSpawn._processes[0].emit('exit', 1)
-      mockTempDir.remove.callArg(1)
+    it('should handle spawn ENOENT error and report the error when exit event is not emitted', (done) => {
+      assertSpawnError({
+        errorCode: 'ENOENT',
+        emitExit: false,
+        expectedError: `Cannot start fake-name\n\tCan not find the binary ${BROWSER_PATH}\n\tPlease set env variable fake-ENV-CMD`
+      }, done)
+    })
 
-      _.defer(() => {
-        expect(launcher.state).to.equal(launcher.STATE_FINISHED)
-        expect(failureSpy).to.have.been.called
-        done()
-      })
+    it('should handle spawn EACCES error and report the error when exit event is not emitted', (done) => {
+      assertSpawnError({
+        errorCode: 'EACCES',
+        emitExit: false,
+        expectedError: `Cannot start fake-name\n\tPermission denied accessing the binary ${BROWSER_PATH}\n\tMaybe it's a directory?`
+      }, done)
     })
   })