From 31ee085128d1c22813253374188bf7ffd0be30e6 Mon Sep 17 00:00:00 2001 From: Nico Jansen Date: Wed, 24 Apr 2019 10:45:36 +0200 Subject: [PATCH] fix(dispose): clean up child processes in alternative flows (#1520) Make sure Stryker doesn't _hang indefinitely_ when it supposed to _exit prematurely_. Before, there was a code path that didn't dispose all child processes. Now using typed-injects [dispose functionality](https://github.com/nicojs/typed-inject/#-disposing-provided-stuff). --- e2e/test/exit-prematurely/.babelrc | 5 ++++ e2e/test/exit-prematurely/package-lock.json | 5 ++++ e2e/test/exit-prematurely/package.json | 15 +++++++++++ e2e/test/exit-prematurely/stryker.conf.js | 19 ++++++++++++++ e2e/test/exit-prematurely/test/test.js | 1 + e2e/test/exit-prematurely/tsconfig.json | 9 +++++++ e2e/test/exit-prematurely/verify/verify.ts | 19 ++++++++++++++ packages/core/src/SandboxPool.ts | 9 ++++--- packages/core/src/Stryker.ts | 25 ++++++++++++------- .../core/src/process/MutationTestExecutor.ts | 3 --- .../transpiler/MutantTranspileScheduler.ts | 6 ++--- packages/core/test/unit/SandboxPool.spec.ts | 8 +++--- packages/core/test/unit/Stryker.spec.ts | 12 +++++++++ .../unit/process/MutationTestExecutor.spec.ts | 12 +-------- 14 files changed, 113 insertions(+), 35 deletions(-) create mode 100644 e2e/test/exit-prematurely/.babelrc create mode 100644 e2e/test/exit-prematurely/package-lock.json create mode 100644 e2e/test/exit-prematurely/package.json create mode 100644 e2e/test/exit-prematurely/stryker.conf.js create mode 100644 e2e/test/exit-prematurely/test/test.js create mode 100644 e2e/test/exit-prematurely/tsconfig.json create mode 100644 e2e/test/exit-prematurely/verify/verify.ts diff --git a/e2e/test/exit-prematurely/.babelrc b/e2e/test/exit-prematurely/.babelrc new file mode 100644 index 0000000000..55fb4d4242 --- /dev/null +++ b/e2e/test/exit-prematurely/.babelrc @@ -0,0 +1,5 @@ +{ + "presets": [ + "@babel/env" + ] +} \ No newline at end of file diff --git a/e2e/test/exit-prematurely/package-lock.json b/e2e/test/exit-prematurely/package-lock.json new file mode 100644 index 0000000000..d38dc6e58f --- /dev/null +++ b/e2e/test/exit-prematurely/package-lock.json @@ -0,0 +1,5 @@ +{ + "name": "babel-transpiling", + "version": "0.0.0", + "lockfileVersion": 1 +} \ No newline at end of file diff --git a/e2e/test/exit-prematurely/package.json b/e2e/test/exit-prematurely/package.json new file mode 100644 index 0000000000..90630bb91e --- /dev/null +++ b/e2e/test/exit-prematurely/package.json @@ -0,0 +1,15 @@ +{ + "name": "exit-prematurely", + "version": "1.0.0", + "private": true, + "description": "A module to test the alternative flow when Stryker should exit prematurely, see https://github.com/stryker-mutator/stryker/issues/1519", + "main": "index.js", + "scripts": { + "pretest": "rimraf \"reports\" stryker.log .stryker-tmp ", + "test": "stryker run", + "posttest": "mocha --require ts-node/register verify/*.ts" + }, + "keywords": [], + "author": "", + "license": "ISC" +} diff --git a/e2e/test/exit-prematurely/stryker.conf.js b/e2e/test/exit-prematurely/stryker.conf.js new file mode 100644 index 0000000000..d47663ee3b --- /dev/null +++ b/e2e/test/exit-prematurely/stryker.conf.js @@ -0,0 +1,19 @@ +module.exports = function (config) { + config.set({ + mutate: [ + 'src/*.js' + ], + testFramework: 'mocha', + testRunner: 'mocha', + coverageAnalysis: 'off', + mutator: 'javascript', + transpilers: [ + 'babel' + ], + timeoutMS: 60000, + reporters: ['clear-text', 'html', 'event-recorder'], + maxConcurrentTestRunners: 2, + logLevel: 'info', + fileLogLevel: 'info' + }); +}; diff --git a/e2e/test/exit-prematurely/test/test.js b/e2e/test/exit-prematurely/test/test.js new file mode 100644 index 0000000000..bf0dffa6ce --- /dev/null +++ b/e2e/test/exit-prematurely/test/test.js @@ -0,0 +1 @@ +// Idle \ No newline at end of file diff --git a/e2e/test/exit-prematurely/tsconfig.json b/e2e/test/exit-prematurely/tsconfig.json new file mode 100644 index 0000000000..16b8566c82 --- /dev/null +++ b/e2e/test/exit-prematurely/tsconfig.json @@ -0,0 +1,9 @@ +{ + "extends": "../../tsconfig.json", + "compilerOptions": { + "declaration": false + }, + "files": [ + "verify/verify.ts" + ] +} \ No newline at end of file diff --git a/e2e/test/exit-prematurely/verify/verify.ts b/e2e/test/exit-prematurely/verify/verify.ts new file mode 100644 index 0000000000..b9ebe2c5ae --- /dev/null +++ b/e2e/test/exit-prematurely/verify/verify.ts @@ -0,0 +1,19 @@ +import * as fs from 'fs'; +import { expect } from 'chai'; + +describe('Verify stryker has ran correctly', () => { + + const strykerLog = fs.readFileSync('./stryker.log', 'utf8'); + + it('exit prematurely', async () => { + expect(strykerLog).contains('No tests were executed. Stryker will exit prematurely.'); + }); + + it('should log about a mutant free world', async () => { + expect(strykerLog).contains('It\'s a mutant-free world, nothing to test'); + }); + + it('should warn about the globbing expression resulting in no files', () => { + expect(strykerLog).contains('Globbing expression "src/*.js" did not result in any files.'); + }); +}); diff --git a/packages/core/src/SandboxPool.ts b/packages/core/src/SandboxPool.ts index 010b523577..fa1877e8cd 100644 --- a/packages/core/src/SandboxPool.ts +++ b/packages/core/src/SandboxPool.ts @@ -11,10 +11,11 @@ import { InitialTestRunResult } from './process/InitialTestExecutor'; import { Logger } from '@stryker-mutator/api/logging'; import TranspiledMutant from './TranspiledMutant'; import { MutantResult } from '@stryker-mutator/api/report'; +import { Disposable } from 'typed-inject'; const MAX_CONCURRENT_INITIALIZING_SANDBOXES = 2; -export class SandboxPool { +export class SandboxPool implements Disposable { private readonly allSandboxes: Promise[] = []; private readonly overheadTimeMS: number; @@ -84,11 +85,11 @@ export class SandboxPool { } private readonly registerSandbox = async (promisedSandbox: Promise): Promise => { - this.allSandboxes.push(promisedSandbox); - return promisedSandbox; + this.allSandboxes.push(promisedSandbox); + return promisedSandbox; } - public async disposeAll() { + public async dispose() { const sandboxes = await Promise.all(this.allSandboxes); return Promise.all(sandboxes.map(sandbox => sandbox.dispose())); } diff --git a/packages/core/src/Stryker.ts b/packages/core/src/Stryker.ts index c09b86b93a..a3e43137a5 100644 --- a/packages/core/src/Stryker.ts +++ b/packages/core/src/Stryker.ts @@ -79,16 +79,23 @@ export default class Stryker { const testableMutants = await mutationTestProcessInjector .injectClass(MutantTestMatcher) .matchWithMutants(mutator.mutate(inputFiles.filesToMutate)); - if (initialRunResult.runResult.tests.length && testableMutants.length) { - const mutationTestExecutor = mutationTestProcessInjector.injectClass(MutationTestExecutor); - const mutantResults = await mutationTestExecutor.run(testableMutants); - await this.reportScore(mutantResults, inputFileInjector); - await TempFolder.instance().clean(); - await this.logDone(); + try { + if (initialRunResult.runResult.tests.length && testableMutants.length) { + const mutationTestExecutor = mutationTestProcessInjector + .injectClass(MutationTestExecutor); + const mutantResults = await mutationTestExecutor.run(testableMutants); + await this.reportScore(mutantResults, inputFileInjector); + await TempFolder.instance().clean(); + await this.logDone(); + return mutantResults; + } else { + this.logRemark(); + } + } finally { + // `injector.dispose` calls `dispose` on all created instances + // Namely the `SandboxPool` and the `ChildProcessProxy` instances + mutationTestProcessInjector.dispose(); await LogConfigurator.shutdown(); - return mutantResults; - } else { - this.logRemark(); } } return Promise.resolve([]); diff --git a/packages/core/src/process/MutationTestExecutor.ts b/packages/core/src/process/MutationTestExecutor.ts index 7397d50da9..faee328b29 100644 --- a/packages/core/src/process/MutationTestExecutor.ts +++ b/packages/core/src/process/MutationTestExecutor.ts @@ -28,9 +28,6 @@ export class MutationTestExecutor { tap(this.reportAll) ).toPromise(); - // TODO: Let typed inject dispose of sandbox pool - await this.sandboxPool.disposeAll(); - await this.mutantTranspileScheduler.dispose(); return results; } diff --git a/packages/core/src/transpiler/MutantTranspileScheduler.ts b/packages/core/src/transpiler/MutantTranspileScheduler.ts index acd54b191e..3d286b0c08 100644 --- a/packages/core/src/transpiler/MutantTranspileScheduler.ts +++ b/packages/core/src/transpiler/MutantTranspileScheduler.ts @@ -15,7 +15,7 @@ import { BehaviorSubject } from 'rxjs'; const INITIAL_CONCURRENCY = 100; -export class MutantTranspileScheduler { +export class MutantTranspileScheduler implements Disposable { private currentMutatedFile: SourceFile; private readonly concurrencyTicket$ = new BehaviorSubject(INITIAL_CONCURRENCY); @@ -44,12 +44,10 @@ export class MutantTranspileScheduler { } /** - * Dispose all + * Dispose */ public dispose() { this.concurrencyTicket$.complete(); - // TODO: Let typed-inject dispose this one - this.transpiler.dispose(); } private createTranspiledMutant(mutant: TestableMutant, transpileResult: TranspileResult) { diff --git a/packages/core/test/unit/SandboxPool.spec.ts b/packages/core/test/unit/SandboxPool.spec.ts index 82142156ac..ca75355da4 100644 --- a/packages/core/test/unit/SandboxPool.spec.ts +++ b/packages/core/test/unit/SandboxPool.spec.ts @@ -178,18 +178,18 @@ describe(SandboxPool.name, () => { await expect(actRunMutants()).rejectedWith(expectedError); }); }); - describe('disposeAll', () => { + describe('dispose', () => { it('should have disposed all sandboxes', async () => { sut = createSut(); await actRunMutants(); - await sut.disposeAll(); + await sut.dispose(); expect(firstSandbox.dispose).called; expect(secondSandbox.dispose).called; }); it('should not do anything if no sandboxes were created', async () => { sut = createSut(); - await sut.disposeAll(); + await sut.dispose(); expect(firstSandbox.dispose).not.called; expect(secondSandbox.dispose).not.called; }); @@ -211,7 +211,7 @@ describe(SandboxPool.name, () => { const runPromise = sut.runMutants(from(inputMutants)).toPromise(); task.resolve(firstSandbox as unknown as Sandbox); await runPromise; - const disposePromise = sut.disposeAll(); + const disposePromise = sut.dispose(); task2.resolve(secondSandbox as unknown as Sandbox); await disposePromise; expect(secondSandbox.dispose).called; diff --git a/packages/core/test/unit/Stryker.spec.ts b/packages/core/test/unit/Stryker.spec.ts index b306f53777..0f8fc8041a 100644 --- a/packages/core/test/unit/Stryker.spec.ts +++ b/packages/core/test/unit/Stryker.spec.ts @@ -191,6 +191,11 @@ describe(Stryker.name, () => { await sut.runMutationTest(); expect(logMock.info).to.have.been.calledWith('Trouble figuring out what went wrong? Try `npx stryker run --fileLogLevel trace --logLevel debug` to get some more info.'); }); + + it('should dispose the injector', async () => { + await sut.runMutationTest(); + expect(injectorMock.dispose).called; + }); }); describe('happy flow', () => { @@ -263,10 +268,17 @@ describe(Stryker.name, () => { expect(reporterMock.wrapUp).to.have.been.called; }); + it('should dispose the injector', async () => { + sut = new Stryker({}); + await sut.runMutationTest(); + expect(injectorMock.dispose).called; + }); + it('should shutdown the log4js server', async () => { sut = new Stryker({}); await sut.runMutationTest(); expect(shutdownLoggingStub).called; + expect(shutdownLoggingStub).calledAfter(injectorMock.dispose); }); it('should create the transpiler with produceSourceMaps = true when coverage analysis is enabled', async () => { diff --git a/packages/core/test/unit/process/MutationTestExecutor.spec.ts b/packages/core/test/unit/process/MutationTestExecutor.spec.ts index dd34ca40b5..73d12acde5 100644 --- a/packages/core/test/unit/process/MutationTestExecutor.spec.ts +++ b/packages/core/test/unit/process/MutationTestExecutor.spec.ts @@ -31,7 +31,7 @@ describe(MutationTestExecutor.name, () => { sandboxPoolMock = mock(SandboxPool); mutantTranspileSchedulerMock = mock(MutantTranspileScheduler); mutantTranspileSchedulerMock.scheduleNext = sinon.stub(); - sandboxPoolMock.disposeAll.resolves(); + sandboxPoolMock.dispose.resolves(); reporter = mock(BroadcastReporter); inputFiles = new InputFileCollection([new File('input.ts', '')], []); mutants = [testableMutant()]; @@ -56,16 +56,6 @@ describe(MutationTestExecutor.name, () => { describe('run', () => { - it('should dispose all sandboxes afterwards', async () => { - await sut.run(mutants); - expect(sandboxPoolMock.disposeAll).called; - }); - - it('should dispose the mutantTranspiler', async () => { - await sut.run(mutants); - expect(mutantTranspileSchedulerMock.dispose).called; - }); - it('should have ran the mutants in the sandbox pool', async () => { await sut.run(mutants); expect(mutantTranspileSchedulerMock.scheduleTranspileMutants).calledWith(mutants);