From 2fd90b9bc96841f4f676f7325a4471d91fbc6b1b Mon Sep 17 00:00:00 2001
From: Wil Wilsman <wil@percy.io>
Date: Thu, 22 Aug 2019 10:45:18 -0500
Subject: [PATCH] fix: Stop and finalize build on exec termination (#332)

* Fix incorrect run check

This property is a function and not a getter. Without invoking it as a function,
it's value is always truthy.

* Stop the agent service when the exec process is terminated

* Do not handle stopping exec multiple times

When the process was terminated, all of the signal handlers were triggered
causing multiple logs to be outputted. Skipping stopping the agent once another
event has stopped it causes subsequent events to skip straight to exiting, and
never finalizes builds.

Exiting is tracked so that if it is already being handled, there is no need to
handle exiting again.

* Correct test for PERCY_ENABLED

The previous test tested that PERCY_ENABLED _did not_ work. That is, it tested
that a warning was still thrown even though it was disabled.

That test has been corrected to assert that _nothing_ is thrown when disabled.

* Fix comment typo
---
 src/commands/exec.ts                | 32 +++++++++++++++++++----------
 src/commands/percy-command.ts       |  2 +-
 test/commands/percy-command.test.ts | 10 ++++-----
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/src/commands/exec.ts b/src/commands/exec.ts
index 21f52e4a..795d66c1 100644
--- a/src/commands/exec.ts
+++ b/src/commands/exec.ts
@@ -1,4 +1,4 @@
-import {flags} from '@oclif/command'
+import { flags } from '@oclif/command'
 import * as spawn from 'cross-spawn'
 import { DEFAULT_CONFIGURATION } from '../configuration/configuration'
 import ConfigurationService from '../services/configuration-service'
@@ -32,12 +32,13 @@ export default class Exec extends PercyCommand {
     }),
   }
 
+  // helps prevent exiting before the agent service has stopped
+  private exiting = false
+
   async run() {
     await super.run()
 
-    const {argv} = this.parse(Exec)
-    const {flags} = this.parse(Exec)
-
+    const { argv, flags } = this.parse(Exec)
     const command = argv.shift()
 
     if (!command) {
@@ -54,14 +55,23 @@ export default class Exec extends PercyCommand {
     }
 
     // Even if Percy will not run, continue to run the subprocess
-    const spawnedProcess = spawn(command, argv, {stdio: 'inherit'})
+    const spawnedProcess = spawn(command, argv, { stdio: 'inherit' })
+    spawnedProcess.on('exit', (code) => this.stop(code))
+
+    // 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())
+  }
 
-    spawnedProcess.on('exit', async (code: any) => {
-      if (this.percyWillRun()) {
-        await this.agentService.stop()
-      }
+  private async stop(exitCode?: number | null) {
+    if (this.exiting) { return }
+    this.exiting = true
+
+    if (this.percyWillRun()) {
+      await this.agentService.stop()
+    }
 
-      process.exit(code)
-    })
+    process.exit(exitCode || 0)
   }
 }
diff --git a/src/commands/percy-command.ts b/src/commands/percy-command.ts
index 941ac277..ed6d1f9a 100644
--- a/src/commands/percy-command.ts
+++ b/src/commands/percy-command.ts
@@ -22,7 +22,7 @@ export default class PercyCommand extends Command {
   }
 
   async run() {
-    if (this.percyEnabled && !this.percyTokenPresent()) {
+    if (this.percyEnabled() && !this.percyTokenPresent()) {
       this.warn('Skipping visual tests. PERCY_TOKEN was not provided.')
     }
   }
diff --git a/test/commands/percy-command.test.ts b/test/commands/percy-command.test.ts
index 34a3d0de..83b256f4 100644
--- a/test/commands/percy-command.test.ts
+++ b/test/commands/percy-command.test.ts
@@ -14,15 +14,13 @@ describe('percy-command', () => {
     .stub(process, 'env', {PERCY_ENABLE: '0', PERCY_TOKEN: ''})
     .stderr()
     .command(['percy-command'])
-    .do((output) => expect(output.stderr).to.contain(
-      'Warning: Skipping visual tests. PERCY_TOKEN was not provided.',
-    ))
-    .it('warns about PERCY_TOKEN to be set')
+    .do((output) => expect(output.stderr).to.eql(''))
+    .it('outputs no warnings when PERCY_ENABLED is 0')
 
   test
-    .stub(process, 'env', {PERCY_ENABLE: '0', PERCY_TOKEN: 'ABC'})
+    .stub(process, 'env', {PERCY_TOKEN: 'ABC'})
     .stderr()
     .command(['percy-command'])
     .do((output) => expect(output.stderr).to.eql(''))
-    .it('outputs no errors')
+    .it('outputs no errors when PERCY_TOKEN is set')
 })