From 3bd0b337ea4fc957aebcb9e689af32c4ae8e2e64 Mon Sep 17 00:00:00 2001 From: Wil Wilsman Date: Mon, 19 Aug 2019 15:30:33 -0500 Subject: [PATCH 1/5] 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. --- src/commands/percy-command.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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.') } } From b128007e0e8238b9430922e992a90a64eee9d251 Mon Sep 17 00:00:00 2001 From: Wil Wilsman Date: Mon, 19 Aug 2019 15:43:07 -0500 Subject: [PATCH 2/5] Stop the agent service when the exec process is terminated --- src/commands/exec.ts | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/commands/exec.ts b/src/commands/exec.ts index 21f52e4a..6335e174 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' @@ -35,9 +35,7 @@ export default class Exec extends PercyCommand { 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 +52,20 @@ 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)) + + // Recieving 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() - } + async stop(exitCode?: number | null) { + if (this.percyWillRun()) { + await this.agentService.stop() + } - process.exit(code) - }) + process.exit(exitCode || 0) } } From fbc11cdd9b7cb1b29ec3a7ce8d8c5021134b7a9a Mon Sep 17 00:00:00 2001 From: Wil Wilsman Date: Mon, 19 Aug 2019 16:06:54 -0500 Subject: [PATCH 3/5] 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. --- src/commands/exec.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/commands/exec.ts b/src/commands/exec.ts index 6335e174..d6b32d9b 100644 --- a/src/commands/exec.ts +++ b/src/commands/exec.ts @@ -32,6 +32,9 @@ export default class Exec extends PercyCommand { }), } + // helps prevent exiting before the agent service has stopped + private exiting = false + async run() { await super.run() @@ -61,7 +64,10 @@ export default class Exec extends PercyCommand { process.on('SIGTERM', () => this.stop()) } - async stop(exitCode?: number | null) { + private async stop(exitCode?: number | null) { + if (this.exiting) { return } + this.exiting = true + if (this.percyWillRun()) { await this.agentService.stop() } From 7acb91403c850559ddd34c32dff0a6f2648b1180 Mon Sep 17 00:00:00 2001 From: Wil Wilsman Date: Mon, 19 Aug 2019 16:22:22 -0500 Subject: [PATCH 4/5] 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. --- test/commands/percy-command.test.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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') }) From 7715f6bfdc027dfaab2e68cc3f5ff99e8565117c Mon Sep 17 00:00:00 2001 From: Wil Wilsman Date: Thu, 22 Aug 2019 10:30:07 -0500 Subject: [PATCH 5/5] Fix comment typo --- src/commands/exec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/exec.ts b/src/commands/exec.ts index d6b32d9b..795d66c1 100644 --- a/src/commands/exec.ts +++ b/src/commands/exec.ts @@ -58,7 +58,7 @@ export default class Exec extends PercyCommand { const spawnedProcess = spawn(command, argv, { stdio: 'inherit' }) spawnedProcess.on('exit', (code) => this.stop(code)) - // Recieving any of these events should stop the agent and exit + // 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())