fix: start detached and stop cleanup (#403)

* fix: 🐛 healthcheck default port when provided port is undefined

Also use the HEALTHCHECK_PATH constant in this helper

* fix: 🐛 do not pass "undefined" into detached start process

* fix: 🐛 cleanup PID file on stop rather than kill the process

The process has handlers to finalize on SIGHUP, but the stop request already
finalizes the build before sending SIGHUP. Sometimes this caused duplicate,
empty, builds due to the finalize method creating a build when the current build
has already been finalized.

* ✅ Add separate test for no flags passed to start

* fix: 🐛 Use oclif exit method

Testing with `process.exit` causes the test process to exit

* fix: 🐛 await on subprocess and exit gracefully

This command was hard to test due to the subprocess being spawn async. In
practice the parent process remains open as well, but for our tests this method
returned before the command was finished.

* ✅ Update helpers to not throw oclif exit errors

* ✅ Fix snapshot command tests

These tests previously caused the entire suite to exit early. Once that was
fixed, the two tests written with the oclif helper failed.

One was updated to not need the helper, and the other test was removed since the
snapshot command now has a default directory argument.

* ✅ Fix agent tests and remove global mutation

This agent test caused many other tests to fail since it mutated the
DEFAULT_CONFIGURATION object.

* ✅ Unskip exec tests

With recent bug fixes, these tests can be unskipped and actually run.

The 'echo' test was replaced with 'sleep' to avoid printing to the inherited
stdio of the test process.

An unfinished test was fleshed out, and the other unfinished test removed since
exec does not support that behavior.

* 🔧 Remove silent junit reporter

When the tests exited early or threw warnings and logs, it went unnoticed in CI
since junit reports to a file. Circle only shows tests when they fail, so passed
tests or missed tests were never shown.

.circleci/config.yml

@@ -29,7 +29,7 @@ commands:
           name: Unit tests
           command: |
             mkdir -p reports
-            $NYC yarn test --reporter mocha-junit-reporter
+            $NYC yarn test
       - run:
           name: Client tests
           command: $NYC yarn test-client --singleRun

src/commands/exec.ts

@@ -58,11 +58,19 @@ export default class Exec extends PercyCommand {
     }
 
     // Even if Percy will not run, continue to run the subprocess
-    const spawnedProcess = spawn(command, argv, { stdio: 'inherit' })
-    spawnedProcess.on('exit', (code) => this.stop(code))
-    spawnedProcess.on('error', (error) => {
+    await new Promise((resolve, reject) => {
+      const spawnedProcess = spawn(command, argv, { stdio: 'inherit' })
+      spawnedProcess.on('exit', resolve)
+      spawnedProcess.on('error', reject)
+    }).catch((error) => {
+      // catch subprocess errors
       this.logger.error(error)
-      this.stop(1)
+      return 1
+    }).then((code: any) => {
+      // oclif exit raises an error to stop script execution, so the following
+      // stop method cannot be used inside of the subprocess event handlers
+      // which would warn about an uncaught promise
+      return this.stop(code)
     })
   }
 }

src/commands/percy-command.ts

@@ -53,20 +53,24 @@ export default class PercyCommand extends Command {
       this.logStart()
 
       // Receiving any of these events should stop the agent and exit
-      process.on('SIGHUP', () => this.stop())
-      process.on('SIGINT', () => this.stop())
-      process.on('SIGTERM', () => this.stop())
+      process.on('SIGHUP', () => this.stop(0, true))
+      process.on('SIGINT', () => this.stop(0, true))
+      process.on('SIGTERM', () => this.stop(0, true))
     }
   }
 
-  async stop(exitCode?: number | null) {
+  async stop(exitCode?: number | null, stopProcess?: boolean) {
     if (this.exiting) { return }
     this.exiting = true
 
     if (this.percyWillRun()) {
       await this.agentService.stop()
     }
 
-    process.exit(exitCode || 0)
+    if (stopProcess) {
+      process.exit(exitCode || 0)
+    } else {
+      this.exit(exitCode || 0)
+    }
   }
 }

src/commands/start.ts

@@ -67,11 +67,20 @@ export default class Start extends PercyCommand {
   }
 
   private runDetached(flags: any) {
+    let args: string[] = []
+
+    if (flags.port) {
+      args = args.concat('-p', flags.port)
+    }
+
+    if (flags['network-idle-timeout']) {
+      args = args.concat('-t', flags['network-idle-timeout'])
+    }
+
     const pid = this.processService.runDetached([
       path.resolve(`${__dirname}/../../bin/run`),
       'start',
-      '-p', String(flags.port!),
-      '-t', String(flags['network-idle-timeout']),
+      ...args,
     ])
 
     if (pid) {

src/services/agent-service.ts

@@ -113,7 +113,7 @@ export class AgentService {
 
     if (domSnapshot.length > Constants.MAX_FILE_SIZE_BYTES) {
       logger.info(`snapshot skipped[max_file_size_exceeded]: '${request.body.name}'`)
-      return response.json({success: true})
+      return response.json({ success: true })
     }
 
     let resources = await this.snapshotService.buildResources(
@@ -157,7 +157,7 @@ export class AgentService {
 
   private async handleStop(_: express.Request, response: express.Response) {
     await this.stop()
-    new ProcessService().kill()
+    new ProcessService().cleanup()
     return response.json({ success: true })
   }
 

src/utils/health-checker.ts

@@ -1,9 +1,10 @@
 import Axios from 'axios'
 import * as retryAxios from 'retry-axios'
+import { DEFAULT_PORT, HEALTHCHECK_PATH } from '../services/agent-service-constants'
 import logger from './logger'
 
-async function healthCheck(port: number, retryOptions?: object) {
-  const healthcheckUrl = `http://localhost:${port}/percy/healthcheck`
+async function healthCheck(port: number = DEFAULT_PORT, retryOptions?: object) {
+  const healthcheckUrl = `http://localhost:${port}${HEALTHCHECK_PATH}`
 
   const retryConfig = {
     retry: 5, // with exponential back off

test/commands/exec.test.ts

@@ -1,17 +1,18 @@
-import * as chai from 'chai'
-import {describe} from 'mocha'
+import { describe } from 'mocha'
 import * as nock from 'nock'
 import * as sinon from 'sinon'
 import Exec from '../../src/commands/exec'
-import {AgentService} from '../../src/services/agent-service'
-import {DEFAULT_PORT} from '../../src/services/agent-service-constants'
-import {captureStdOut} from '../helpers/stdout'
+import { DEFAULT_CONFIGURATION } from '../../src/configuration/configuration'
+import { AgentService } from '../../src/services/agent-service'
+import { captureStdOut } from '../helpers/stdout'
+import chai from '../support/chai'
 
 const expect = chai.expect
 
 describe('Exec', () => {
-  xdescribe('#run', () => {
+  describe('#run', () => {
     const sandbox = sinon.createSandbox()
+    let agentServiceStub: AgentService
 
     function AgentServiceStub(): AgentService {
       const agentService = AgentService.prototype as AgentService
@@ -24,6 +25,7 @@ describe('Exec', () => {
     }
 
     beforeEach(() => {
+      agentServiceStub = AgentServiceStub()
       nock(/localhost/).get('/percy/healthcheck').reply(200)
     })
 
@@ -33,17 +35,23 @@ describe('Exec', () => {
     })
 
     it('starts and stops percy', async () => {
-      const agentServiceStub = AgentServiceStub()
-
       const stdout = await captureStdOut(async () => {
-        await Exec.run(['--', 'echo', 'hello'])
+        await Exec.run(['--', 'sleep', '0'])
       })
 
-      expect(agentServiceStub.start).to.calledWithMatch(DEFAULT_PORT)
-      expect(stdout).to.match(/\[percy\] percy has started on port \d+./)
+      expect(agentServiceStub.start).to.calledWithMatch(DEFAULT_CONFIGURATION)
+      expect(stdout).to.match(/\[percy\] percy has started./)
     })
 
-    xit('starts percy on a specific port')
-    xit('warns when percy is already running')
+    it('starts percy on a specific port', async () => {
+      await captureStdOut(async () => {
+        await Exec.run(['--port', '55000', '--', 'sleep', '0'])
+      })
+
+      expect(agentServiceStub.start).to.calledWithMatch({
+        ...DEFAULT_CONFIGURATION,
+        agent: { ...DEFAULT_CONFIGURATION.agent, port: 55000 },
+      })
+    })
   })
 })

test/commands/snapshot.test.ts

@@ -1,19 +1,21 @@
-import {expect, test} from '@oclif/test'
-import * as chai from 'chai'
-import {describe} from 'mocha'
+import { expect, test } from '@oclif/test'
+import { describe } from 'mocha'
 import * as sinon from 'sinon'
 import Snapshot from '../../src/commands/snapshot'
 import { DEFAULT_CONFIGURATION } from '../../src/configuration/configuration'
-import {AgentService} from '../../src/services/agent-service'
+import { AgentService } from '../../src/services/agent-service'
 import StaticSnapshotService from '../../src/services/static-snapshot-service'
-import {captureStdOut} from '../helpers/stdout'
+import { captureStdErr, captureStdOut } from '../helpers/stdout'
+import chai from '../support/chai'
 
 describe('snapshot', () => {
   describe('#run', () => {
     const sandbox = sinon.createSandbox()
 
     afterEach(() => {
       sandbox.restore()
+      // restore token to fake value
+      process.env.PERCY_TOKEN = 'abc'
     })
 
     function AgentServiceStub(): AgentService {
@@ -47,23 +49,15 @@ describe('snapshot', () => {
       chai.expect(staticSnapshotServiceStub.snapshotAll).to.have.callCount(1)
       chai.expect(stdout).to.match(/\[percy\] percy has started./)
     })
-  })
 
-  describe('snapshot command', () => {
-    test
-      .stub(process, 'env', {PERCY_TOKEN: ''})
-      .stderr()
-      .command(['snapshot', './test_dir'])
-      .exit(0)
-      .do((output) => expect(output.stderr).to.contain(
-        'Warning: Skipping visual tests. PERCY_TOKEN was not provided.',
-      ))
-      .it('warns about PERCY_TOKEN not being set and exits gracefully')
+    it('warns about PERCY_TOKEN not being set and exits gracefully', async () => {
+      process.env.PERCY_TOKEN = ''
 
-    test
-      .env({PERCY_TOKEN: 'abc'})
-      .command(['snapshot'])
-      .exit(2)
-      .it('exits when the asset directory arg is missing')
+      const stderr = await captureStdErr(async () => {
+        await Snapshot.run(['.'])
+      })
+
+      chai.expect(stderr).to.match(/Warning: Skipping visual tests\. PERCY_TOKEN was not provided\./)
+    })
   })
 })

test/commands/start.test.ts

@@ -1,4 +1,3 @@
-import * as chai from 'chai'
 import {describe} from 'mocha'
 import * as nock from 'nock'
 import * as path from 'path'
@@ -8,6 +7,7 @@ import { DEFAULT_CONFIGURATION } from '../../src/configuration/configuration'
 import {AgentService} from '../../src/services/agent-service'
 import ProcessService from '../../src/services/process-service'
 import {captureStdOut} from '../helpers/stdout'
+import chai from '../support/chai'
 
 const expect = chai.expect
 
@@ -66,26 +66,39 @@ describe('Start', () => {
         [
           path.resolve(`${__dirname}/../../bin/run`),
           'start',
-          '-p', String(DEFAULT_CONFIGURATION.agent.port),
-          '-t', '50',
+        ],
+      )
+    })
+
+    it('starts percy agent in detached mode with flags', async () => {
+      const processService = ProcessServiceStub()
+
+      await captureStdOut(async () => {
+        await Start.run(['--detached', '-p', '55000', '-t', '100'])
+      })
+
+      expect(processService.runDetached).to.calledWithMatch(
+        [
+          path.resolve(`${__dirname}/../../bin/run`),
+          'start',
+          '-p', 55000,
+          '-t', 100,
         ],
       )
     })
 
     it('starts percy agent on a specific port', async () => {
       const agentServiceStub = AgentServiceStub()
 
-      const port = '55000'
-      const options = ['--port', port]
-
       const stdout = await captureStdOut(async () => {
-        await Start.run(options)
+        await Start.run(['--port', '55000'])
       })
 
-      const expectedConfiguration = DEFAULT_CONFIGURATION
-      expectedConfiguration.agent.port = +port
+      expect(agentServiceStub.start).to.calledWithMatch({
+        ...DEFAULT_CONFIGURATION,
+        agent: { ...DEFAULT_CONFIGURATION.agent, port: 55000 },
+      })
 
-      expect(agentServiceStub.start).to.calledWithMatch(expectedConfiguration)
       expect(stdout).to.contain('[percy] percy has started.')
     })
 

test/helpers/stdout.ts

@@ -2,16 +2,28 @@ import * as std from 'stdout-stderr'
 
 export async function captureStdOut(callback: any): Promise<string> {
   std.stdout.start()
-  await callback()
-  std.stdout.stop()
+
+  try {
+    await callback()
+  } catch (err) {
+    if (!err.oclif || err.oclif.exit !== 0) { throw err }
+  } finally {
+    std.stdout.stop()
+  }
 
   return std.stdout.output
 }
 
 export async function captureStdErr(callback: any): Promise<string> {
   std.stderr.start()
-  await callback()
-  std.stderr.stop()
+
+  try {
+    await callback()
+  } catch (err) {
+    if (!err.oclif || err.oclif.exit !== 0) { throw err }
+  } finally {
+    std.stderr.stop()
+  }
 
   return std.stderr.output
 }