correctly handle dispose errors in test runners

packages/core/src/test-runner/child-process-test-runner-decorator.ts

@@ -1,4 +1,5 @@
 import { StrykerOptions } from '@stryker-mutator/api/core';
+import { Logger } from '@stryker-mutator/api/logging';
 import { TestRunner, DryRunOptions, MutantRunOptions, MutantRunResult, DryRunResult } from '@stryker-mutator/api/test-runner';
 import { ExpirableTask } from '@stryker-mutator/util';
 
@@ -16,7 +17,7 @@ const MAX_WAIT_FOR_DISPOSE = 2000;
 export class ChildProcessTestRunnerDecorator implements TestRunner {
   private readonly worker: ChildProcessProxy<ChildProcessTestRunnerWorker>;
 
-  constructor(options: StrykerOptions, sandboxWorkingDirectory: string, loggingContext: LoggingClientContext) {
+  constructor(options: StrykerOptions, sandboxWorkingDirectory: string, loggingContext: LoggingClientContext, private readonly log: Logger) {
     this.worker = ChildProcessProxy.create(
       require.resolve('./child-process-test-runner-worker'),
       loggingContext,
@@ -45,7 +46,11 @@ export class ChildProcessTestRunnerDecorator implements TestRunner {
       this.worker.proxy.dispose().catch((error) => {
         // It's OK if the child process is already down.
         if (!(error instanceof ChildProcessCrashedError)) {
-          throw error;
+          // Handle error by logging it. We still want to kill the child process.
+          this.log.warn(
+            'An unexpected error occurred during test runner disposal. This might be worth looking into. Stryker will ignore this error.',
+            error
+          );
         }
       }),
 

packages/core/src/test-runner/index.ts

@@ -30,7 +30,16 @@ export function createTestRunnerFactory(
           new RetryRejectedDecorator(
             () =>
               new MaxTestRunnerReuseDecorator(
-                () => new TimeoutDecorator(() => new ChildProcessTestRunnerDecorator(options, sandbox.workingDirectory, loggingContext)),
+                () =>
+                  new TimeoutDecorator(
+                    () =>
+                      new ChildProcessTestRunnerDecorator(
+                        options,
+                        sandbox.workingDirectory,
+                        loggingContext,
+                        getLogger(ChildProcessTestRunnerDecorator.name)
+                      )
+                  ),
                 options
               )
           ),

packages/core/test/integration/test-runner/additional-test-runners.ts

@@ -112,6 +112,10 @@ class ErroredTestRunner implements TestRunner {
   public async mutantRun(): Promise<MutantRunResult> {
     throw new Error('Method not implemented.');
   }
+
+  public dispose(): Promise<void> {
+    throw new Error('Test runner exited with exit code 1');
+  }
 }
 
 class RejectInitRunner implements TestRunner {

packages/core/test/integration/test-runner/create-test-runner-factory.it.spec.ts

@@ -24,6 +24,12 @@ describe(`${createTestRunnerFactory.name} integration`, () => {
   let loggingServer: LoggingServer;
   let alreadyDisposed: boolean;
 
+  function rmSync(fileName: string) {
+    if (fs.existsSync(fileName)) {
+      fs.unlinkSync(fileName);
+    }
+  }
+
   beforeEach(async () => {
     // Make sure there is a logging server listening
     loggingServer = new LoggingServer();
@@ -39,20 +45,17 @@ describe(`${createTestRunnerFactory.name} integration`, () => {
       .provideValue(coreTokens.loggingContext, loggingContext)
       .injectFunction(createTestRunnerFactory);
 
-    if (fs.existsSync(CounterTestRunner.COUNTER_FILE)) {
-      await fs.unlinkSync(CounterTestRunner.COUNTER_FILE);
-    }
+    rmSync(CounterTestRunner.COUNTER_FILE);
+    rmSync(SecondTimeIsTheCharm.COUNTER_FILE);
   });
 
   afterEach(async () => {
     if (!alreadyDisposed) {
       await sut.dispose();
     }
     await loggingServer.dispose();
-
-    if (fs.existsSync(CounterTestRunner.COUNTER_FILE)) {
-      fs.unlinkSync(CounterTestRunner.COUNTER_FILE);
-    }
+    rmSync(CounterTestRunner.COUNTER_FILE);
+    rmSync(SecondTimeIsTheCharm.COUNTER_FILE);
   });
 
   async function arrangeSut(name: string): Promise<void> {
@@ -125,6 +128,11 @@ describe(`${createTestRunnerFactory.name} integration`, () => {
     await expect(arrangeSut('reject-init')).rejectedWith('Init was rejected');
   });
 
+  it('should still shutdown the child process, even when test runner dispose rejects', async () => {
+    arrangeSut('errored');
+    await sut.dispose();
+  });
+
   it('should change the current working directory to the sandbox directory', async () => {
     await arrangeSut('verify-working-folder');
     const result = await actDryRun();

packages/core/test/unit/test-runner/child-process-test-runner-decorator.spec.ts

@@ -3,7 +3,7 @@ import { expect } from 'chai';
 import sinon from 'sinon';
 import { Task } from '@stryker-mutator/util';
 import { TestRunner } from '@stryker-mutator/api/test-runner';
-import { factory } from '@stryker-mutator/test-helpers';
+import { factory, testInjector } from '@stryker-mutator/test-helpers';
 
 import { ChildProcessCrashedError } from '../../../src/child-proxy/child-process-crashed-error';
 import { ChildProcessProxy } from '../../../src/child-proxy/child-process-proxy';
@@ -36,7 +36,7 @@ describe(ChildProcessTestRunnerDecorator.name, () => {
   });
 
   function createSut(): ChildProcessTestRunnerDecorator {
-    return new ChildProcessTestRunnerDecorator(options, 'a working directory', loggingContext);
+    return new ChildProcessTestRunnerDecorator(options, 'a working directory', loggingContext, testInjector.logger);
   }
 
   it('should create the child process proxy', () => {
@@ -97,6 +97,19 @@ describe(ChildProcessTestRunnerDecorator.name, () => {
       childProcessProxyMock.proxy.dispose.rejects(new ChildProcessCrashedError(1, '1'));
       await sut.dispose();
       expect(childProcessProxyMock.dispose).called;
+      expect(testInjector.logger.warn).not.called;
+    });
+
+    it('should log, but not reject, when the child process rejects', async () => {
+      const sut = createSut();
+      const expectedError = new Error('Could not divide by zero 🤷‍♀️');
+      childProcessProxyMock.proxy.dispose.rejects(expectedError);
+      await sut.dispose();
+      expect(childProcessProxyMock.dispose).called;
+      expect(testInjector.logger.warn).calledWithExactly(
+        'An unexpected error occurred during test runner disposal. This might be worth looking into. Stryker will ignore this error.',
+        expectedError
+      );
     });
 
     it('should only wait 2 seconds for the test runner to be disposed', async () => {

packages/karma-runner/src/karma-test-runner.ts

@@ -1,3 +1,5 @@
+import { promisify } from 'util';
+
 import karma from 'karma';
 import { StrykerOptions, MutantCoverage } from '@stryker-mutator/api/core';
 import { Logger, LoggerFactoryMethod } from '@stryker-mutator/api/logging';
@@ -11,6 +13,7 @@ import {
   DryRunResult,
   MutantRunResult,
   toMutantRunResult,
+  MutantRunStatus,
 } from '@stryker-mutator/api/test-runner';
 
 import { StrykerKarmaSetup } from '../src-generated/karma-runner-options';
@@ -32,6 +35,7 @@ export class KarmaTestRunner implements TestRunner {
   private currentCoverageReport?: MutantCoverage;
   private readonly starter: ProjectStarter;
   public port: number | undefined;
+  private exitPromise: Promise<number> | undefined;
 
   public static inject = tokens(commonTokens.logger, commonTokens.getLogger, commonTokens.options);
   constructor(private readonly log: Logger, getLogger: LoggerFactoryMethod, options: StrykerOptions) {
@@ -47,8 +51,19 @@ export class KarmaTestRunner implements TestRunner {
 
   public async init(): Promise<void> {
     return new Promise((res, rej) => {
-      StrykerReporter.instance.once('browsers_ready', () => res());
-      this.starter.start().catch(rej);
+      let promiseResolved = false;
+      StrykerReporter.instance.once('browsers_ready', () => {
+        promiseResolved = true;
+        res();
+      });
+      this.exitPromise = this.starter.start().catch((err) => {
+        if (promiseResolved) {
+          throw err;
+        } else {
+          rej(err);
+          return 0;
+        }
+      });
     });
   }
 
@@ -60,6 +75,13 @@ export class KarmaTestRunner implements TestRunner {
   public async mutantRun(options: MutantRunOptions): Promise<MutantRunResult> {
     TestHooksMiddleware.instance.configureActiveMutant(options);
     const dryRunResult = await this.run();
+    if (
+      dryRunResult.status === DryRunStatus.Error &&
+      /Disconnected reconnect failed before timeout of \d+ms \(ping timeout\)/.exec(dryRunResult.errorMessage)
+    ) {
+      // Browser is stuck in an infinite loop, see https://github.com/stryker-mutator/stryker-js/issues/2989#issuecomment-885461482
+      return { status: MutantRunStatus.Timeout };
+    }
     return toMutantRunResult(dryRunResult);
   }
 
@@ -74,6 +96,11 @@ export class KarmaTestRunner implements TestRunner {
     return runResult;
   }
 
+  public async dispose(): Promise<void> {
+    await promisify(karma.stopper.stop).bind(karma.stopper)({ port: this.port });
+    await this.exitPromise;
+  }
+
   private loadSetup(options: StrykerOptions): StrykerKarmaSetup {
     const defaultKarmaConfig: StrykerKarmaSetup = {
       projectType: 'custom',

packages/karma-runner/src/starters/angular-starter.ts

@@ -10,7 +10,7 @@ import { NgConfigOptions, NgTestArguments } from '../../src-generated/karma-runn
 
 const MIN_ANGULAR_CLI_VERSION = '6.1.0';
 
-export async function start(getLogger: LoggerFactoryMethod, ngConfig?: NgConfigOptions): Promise<void> {
+export async function start(getLogger: LoggerFactoryMethod, ngConfig?: NgConfigOptions): Promise<number> {
   const logger: Logger = getLogger(path.basename(__filename));
   verifyAngularCliVersion();
 
@@ -44,6 +44,7 @@ export async function start(getLogger: LoggerFactoryMethod, ngConfig?: NgConfigO
         `\`ng test\` command failed with exit code ${exitCode}. Please run with logLevel 'trace' to see the angular-cli console output (actual command was ${actualCommand})`
       );
     }
+    return exitCode;
   });
 }
 

packages/karma-runner/src/starters/karma-starter.ts

@@ -1,9 +1,16 @@
 import { requireResolve } from '@stryker-mutator/util';
 
-export async function start(): Promise<void> {
+export function start(): Promise<number> {
   // Make sure require karma from inside this function, that way it won't break if karma isn't installed and this file is required.
-  const karma: any = requireResolve('karma');
-  await new karma.Server({
-    configFile: require.resolve('./stryker-karma.conf'),
-  }).start();
+  const karma = requireResolve('karma') as typeof import('karma');
+  return new Promise((res, rej) => {
+    new karma.Server(
+      {
+        configFile: require.resolve('./stryker-karma.conf'),
+      },
+      res
+    )
+      .start()
+      .catch(rej);
+  });
 }

packages/karma-runner/src/starters/project-starter.ts

@@ -7,7 +7,7 @@ import * as karmaStarter from './karma-starter';
 
 export class ProjectStarter {
   constructor(private readonly getLogger: LoggerFactoryMethod, private readonly setup: StrykerKarmaSetup) {}
-  public start(): Promise<void> {
+  public start(): Promise<number> {
     if (this.setup.projectType === 'angular-cli') {
       return angularStarter.start(this.getLogger, this.setup.ngConfig);
     } else {

packages/karma-runner/test/integration/angular.it.spec.ts

@@ -0,0 +1,24 @@
+import { testInjector } from '@stryker-mutator/test-helpers';
+import { expect } from 'chai';
+
+import { KarmaTestRunner } from '../../src/karma-test-runner';
+import { KarmaRunnerOptionsWithStrykerOptions } from '../../src/karma-runner-options-with-stryker-options';
+import { StrykerReporter } from '../../src/karma-plugins/stryker-reporter';
+import { resolveTestResource } from '../helpers/resolve-test-resource';
+
+describe.only(`${KarmaTestRunner.name} on using projectType angular`, () => {
+  afterEach(() => {
+    StrykerReporter.instance.removeAllListeners();
+  });
+
+  it('should reject when no angular cli is available', async () => {
+    (testInjector.options as KarmaRunnerOptionsWithStrykerOptions).karma = {
+      projectType: 'angular-cli',
+      configFile: resolveTestResource('sampleProject', 'karma-jasmine.conf.js'),
+    };
+    const sut = testInjector.injector.injectClass(KarmaTestRunner);
+    await expect(sut.init()).rejectedWith("Cannot find module '@angular/cli");
+  });
+
+  // Other tests are done in e2e testing 🤷‍♀️
+});

packages/karma-runner/test/integration/instrumented.it.spec.ts

@@ -24,8 +24,9 @@ describe(`${KarmaTestRunner.name} running on instrumented code`, () => {
       await sut.init();
     });
 
-    after(() => {
+    after(async () => {
       StrykerReporter.instance.removeAllListeners();
+      await sut.dispose();
     });
 
     describe('dryRun', () => {
@@ -158,8 +159,9 @@ describe(`${KarmaTestRunner.name} running on instrumented code`, () => {
       sut = createSut();
       await sut.init();
     });
-    after(() => {
+    after(async () => {
       StrykerReporter.instance.removeAllListeners();
+      await sut.dispose();
     });
 
     describe('dryRun', () => {

packages/karma-runner/test/integration/karma-test-runner.it.spec.ts

@@ -49,8 +49,9 @@ describe(`${KarmaTestRunner.name} integration`, () => {
       sut = createSut();
       return sut.init();
     });
-    after(() => {
+    after(async () => {
       StrykerReporter.instance.removeAllListeners();
+      await sut.dispose();
     });
 
     describe('dryRun()', () => {
@@ -85,8 +86,9 @@ describe(`${KarmaTestRunner.name} integration`, () => {
       sut = createSut();
       return sut.init();
     });
-    after(() => {
+    after(async () => {
       StrykerReporter.instance.removeAllListeners();
+      await sut.dispose();
     });
     describe('dryRun', () => {
       it('should report the first failed test (bail)', async () => {
@@ -117,8 +119,9 @@ describe(`${KarmaTestRunner.name} integration`, () => {
       sut = createSut();
       return sut.init();
     });
-    after(() => {
+    after(async () => {
       StrykerReporter.instance.removeAllListeners();
+      await sut.dispose();
     });
     describe('dryRun', () => {
       it('should report Error with the error message', async () => {
@@ -143,8 +146,9 @@ describe(`${KarmaTestRunner.name} integration`, () => {
       sut = createSut();
       return sut.init();
     });
-    after(() => {
+    after(async () => {
       StrykerReporter.instance.removeAllListeners();
+      await sut.dispose();
     });
     it('should report Complete without errors', async () => {
       const runResult = await sut.dryRun(factory.dryRunOptions());
@@ -164,8 +168,9 @@ describe(`${KarmaTestRunner.name} integration`, () => {
       sut = createSut();
       return sut.init();
     });
-    after(() => {
+    after(async () => {
       StrykerReporter.instance.removeAllListeners();
+      await sut.dispose();
     });
     it('should report Complete without errors', async () => {
       const runResult = await sut.dryRun(factory.dryRunOptions());
@@ -188,6 +193,7 @@ describe(`${KarmaTestRunner.name} integration`, () => {
         await dummyServer.dispose();
       }
       StrykerReporter.instance.removeAllListeners();
+      await sut.dispose();
     });
 
     it('should choose different port automatically and report Complete without errors', async () => {