refactor(test): add common method to start server in background (#3495)

The existing code had pretty confusing logic (old lines 33-36)`: when background server start fails it is actually considered a success, but child process is not started. Because of this `lastRun` contains result output of the `karma start`. This is no longer the case, hence two test cases had to be updated as they never executed `karma run` despite the statement and were asserting against `karma start` output. This should be more clear now.

As background server process now has its own variable to store output, there is no need for a dedicated runOut command and it can be removed.

test/e2e/browser_console.feature

@@ -183,7 +183,7 @@ Feature: Browser Console Configuration
       ];
       singleRun = false;
       """
-    When I runOut Karma
+    When I run Karma
     Then it passes with like:
       """
       LOG: 'foo'

test/e2e/error.feature

@@ -29,7 +29,7 @@ Feature: Error Display
       ];
       singleRun = false;
       """
-    When I runOut Karma
+    When I run Karma
     Then it fails with like:
       """
       SyntaxError: Unexpected token '}'

test/e2e/launcher-error.feature

@@ -13,7 +13,7 @@ Feature: Launcher error
         'karma-script-launcher'
       ];
       """
-    When I run Karma
+    When I start Karma
     Then it fails with like:
       """
       Missing fake dependency

test/e2e/pass-opts.feature

@@ -15,7 +15,7 @@ Feature: Passing Options
       singleRun = false;
       """
     And command line arguments of: "-- arg1 arg2"
-    When I runOut Karma
+    When I run Karma
     Then it passes with no debug:
       """
       .

test/e2e/step_definitions/core_steps.js

@@ -1,7 +1,7 @@
 const { defineParameterType, Given, Then, When } = require('cucumber')
 const fs = require('fs')
 const path = require('path')
-const { exec, spawn } = require('child_process')
+const { exec } = require('child_process')
 const stopper = require('../../../lib/stopper')
 
 let additionalArgs = []
@@ -21,29 +21,10 @@ function execKarma (command, level, callback) {
     }, done)
   }
 
-  const runOut = command === 'runOut'
-  if (command === 'run' || command === 'runOut') {
-    let isRun = false
-    this.child = spawn('' + runtimePath, ['start', '--log-level', 'warn', configFile])
-    const done = () => {
-      this.child && this.child.kill()
-      callback()
-    }
-
-    this.child.on('error', (error) => {
-      this.lastRun.error = error
-      done()
-    })
-
-    this.child.stderr.on('data', (chunk) => {
-      this.lastRun.stderr += chunk.toString()
-    })
-
-    this.child.stdout.on('data', (chunk) => {
-      this.lastRun.stdout += chunk.toString()
-      const cmd = runtimePath + ' run ' + configFile + ' ' + additionalArgs
-      if (!isRun) {
-        isRun = true
+  if (command === 'run') {
+    this.runBackgroundProcess(['start', '--log-level', 'warn', configFile])
+      .then(() => {
+        const cmd = runtimePath + ' run ' + configFile + ' ' + additionalArgs
 
         setTimeout(() => {
           exec(cmd, {
@@ -52,15 +33,13 @@ function execKarma (command, level, callback) {
             if (error) {
               this.lastRun.error = error
             }
-            if (runOut) {
-              this.lastRun.stdout = stdout
-              this.lastRun.stderr = stderr
-            }
-            done()
+            this.lastRun.stdout = stdout
+            this.lastRun.stderr = stderr
+            callback()
           })
         }, 1000)
-      }
-    })
+      })
+      .catch((error) => callback(error))
   } else {
     executor((error, stdout, stderr) => {
       if (error) {
@@ -100,24 +79,13 @@ When('I stop a server programmatically', function (callback) {
   }, 1000)
 })
 
-When('I start a server in background', function (callback) {
-  this.writeConfigFile()
-
-  const configFile = this.configFile
-  const runtimePath = this.karmaExecutable
-  this.child = spawn(runtimePath, ['start', '--log-level', 'debug', configFile])
-  this.child.stdout.on('data', () => {
-    callback()
-    callback = () => null
-  })
-  this.child.on('exit', (exitCode) => {
-    this.childExitCode = exitCode
-  })
+When('I start a server in background', async function () {
+  await this.runBackgroundProcess(['start', '--log-level', 'debug', this.configFile])
 })
 
 defineParameterType({
   name: 'command',
-  regexp: /run|runOut|start|init|stop/
+  regexp: /run|start|init|stop/
 })
 
 defineParameterType({
@@ -190,17 +158,15 @@ Then('it fails with like:', function (expectedOutput, callback) {
   }
 })
 
-Then(/^The (server|stopper) is dead(:? with exit code (\d+))?$/,
-  function (stopperOrServer, code, callback) {
-    const server = stopperOrServer === 'server'
-    const _this = this
-    setTimeout(function () {
-      const actualExitCode = server ? _this.childExitCode : _this.stopperExitCode
-      if (actualExitCode === undefined) return callback(new Error('Server has not exited.'))
-      if (code === undefined || parseInt(code, 10) === actualExitCode) return callback()
-      callback(new Error('Exit-code mismatch'))
-    }, 4000)
-  })
+Then(/^The (server|stopper) is dead(:? with exit code (\d+))?$/, function (stopperOrServer, code, callback) {
+  const server = stopperOrServer === 'server'
+  setTimeout(() => {
+    const actualExitCode = server ? this.backgroundProcess.handle.exitCode : this.stopperExitCode
+    if (actualExitCode === undefined) return callback(new Error('Server has not exited.'))
+    if (code === undefined || parseInt(code, 10) === actualExitCode) return callback()
+    callback(new Error('Exit-code mismatch'))
+  }, 4000)
+})
 
 Then(/^the file at ([a-zA-Z0-9/\\_.]+) contains:$/, function (filePath, expectedOutput) {
   const data = fs.readFileSync(path.join(this.workDir, filePath), 'utf8')

test/e2e/step_definitions/hooks.js

@@ -6,10 +6,5 @@ Before(function () {
 
 After(async function () {
   await this.proxy.stopIfRunning()
-
-  const running = this.child != null && typeof this.child.kill === 'function'
-  if (running) {
-    this.child.kill()
-    this.child = null
-  }
+  await this.stopBackgroundProcessIfRunning()
 })

test/e2e/support/world.js

@@ -1,3 +1,4 @@
+const { spawn } = require('child_process')
 const fs = require('fs')
 const vm = require('vm')
 const path = require('path')
@@ -56,6 +57,12 @@ class World {
       stdout: '',
       stderr: ''
     }
+
+    this.backgroundProcess = {
+      handle: null,
+      stdout: '',
+      stderr: ''
+    }
   }
 
   updateConfig (configOverrides) {
@@ -85,6 +92,59 @@ module.exports = (config) => {
     rimraf.sync(this.sandboxDir)
     mkdirp.sync(this.sandboxDir)
   }
+
+  async runBackgroundProcess (args, readyOutput = null) {
+    return new Promise((resolve, reject) => {
+      const handle = this.backgroundProcess.handle = spawn(this.karmaExecutable, args, { cwd: this.workDir })
+
+      let isSettled = false
+
+      // The errorHandler only handles spawn errors (e.g. invalid executable
+      // path). It is removed once process has spawned. Kill errors
+      // handling is done in {@link stopBackgroundProcessIfRunning}.
+      // See https://nodejs.org/api/child_process.html#child_process_event_error
+      //
+      // This is because the Cucumber step to start the background process is
+      // considered successful once first output from the spawned process has
+      // been received. Then Karma server is running in the background (while
+      // subsequent Cucumber steps are executed) and there is no way to mark
+      // Cucumber step that started a server as failed since it has already
+      // completed.
+      const errorHandler = (error) => {
+        isSettled = true
+        this.backgroundProcess.handle = null
+        reject(error)
+      }
+      handle.once('error', errorHandler)
+
+      handle.stderr.on('data', (chunk) => {
+        this.backgroundProcess.stderr += chunk.toString()
+      })
+
+      handle.stdout.on('data', (chunk) => {
+        this.backgroundProcess.stdout += chunk.toString()
+
+        if (!isSettled) {
+          if (readyOutput == null || this.backgroundProcess.stdout.includes(readyOutput)) {
+            isSettled = true
+            handle.off('error', errorHandler)
+            resolve()
+          }
+        }
+      })
+    })
+  }
+
+  async stopBackgroundProcessIfRunning () {
+    if (this.backgroundProcess.handle != null && this.backgroundProcess.handle.exitCode == null) {
+      return new Promise((resolve, reject) => {
+        this.backgroundProcess.handle
+          .once('exit', () => resolve())
+          .once('error', (error) => reject(error))
+          .kill()
+      })
+    }
+  }
 }
 
 setWorldConstructor(World)

test/e2e/timeout.feature

@@ -14,7 +14,7 @@ Feature: Timeout
       ];
       captureTimeout = 100
       """
-    When I run Karma
+    When I start Karma
     Then it fails with like:
       """
       have not captured in 100 ms