fix(karma runner): restart a browser on disconnect error (#3020)

* Restart a browser when it disconnects. This allows us to recover from an infinite loop (as long as the timeout allows for it).
* Refactor the karma runner to allow restarting of the browser
* Refactor to no longer [pass raw options to `karma.runner.run`](http://karma-runner.github.io/6.3/dev/public-api.html#karmarunner)
* Implement `dispose` in the karma runner. It now closes the browser and karma server. Useful during integration testing.

e2e/test/angular-project/stryker.conf.json

@@ -24,7 +24,7 @@
     "event-recorder"
   ],
   "coverageAnalysis": "perTest",
-  "concurrency": 2,
+  "concurrency": 1,
   "concurrency_comment": "Recommended to use about half of your available cores when running stryker with angular",
   "timeoutMS": 60000
 }

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

@@ -1,31 +1,42 @@
 import { TestRunner } from '@stryker-mutator/api/test-runner';
 import { StrykerOptions } from '@stryker-mutator/api/core';
 import { tokens, commonTokens } from '@stryker-mutator/api/plugin';
+import { LoggerFactoryMethod } from '@stryker-mutator/api/logging';
 
 import { LoggingClientContext } from '../logging';
 import { coreTokens } from '../di';
 import { Sandbox } from '../sandbox/sandbox';
 
-import { RetryDecorator } from './retry-decorator';
+import { RetryRejectedDecorator } from './retry-rejected-decorator';
 import { TimeoutDecorator } from './timeout-decorator';
 import { ChildProcessTestRunnerDecorator } from './child-process-test-runner-decorator';
 import { CommandTestRunner } from './command-test-runner';
 import { MaxTestRunnerReuseDecorator } from './max-test-runner-reuse-decorator';
 
-createTestRunnerFactory.inject = tokens(commonTokens.options, coreTokens.sandbox, coreTokens.loggingContext);
+createTestRunnerFactory.inject = tokens(commonTokens.options, coreTokens.sandbox, coreTokens.loggingContext, commonTokens.getLogger);
 export function createTestRunnerFactory(
   options: StrykerOptions,
   sandbox: Pick<Sandbox, 'workingDirectory'>,
-  loggingContext: LoggingClientContext
+  loggingContext: LoggingClientContext,
+  getLogger: LoggerFactoryMethod
 ): () => Required<TestRunner> {
   if (CommandTestRunner.is(options.testRunner)) {
-    return () => new RetryDecorator(() => new TimeoutDecorator(() => new CommandTestRunner(sandbox.workingDirectory, options)));
+    return () => new RetryRejectedDecorator(() => new TimeoutDecorator(() => new CommandTestRunner(sandbox.workingDirectory, options)));
   } else {
     return () =>
-      new RetryDecorator(
+      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/src/test-runner/max-test-runner-reuse-decorator.ts

@@ -27,12 +27,6 @@ export class MaxTestRunnerReuseDecorator extends TestRunnerDecorator {
     return super.mutantRun(options);
   }
 
-  private async recover(): Promise<void> {
-    await this.dispose();
-    this.createInnerRunner();
-    return this.init();
-  }
-
   public dispose(): Promise<any> {
     this.runs = 0;
     return super.dispose();

packages/core/src/test-runner/retry-decorator.ts → packages/core/src/test-runner/retry-rejected-decorator.ts

@@ -7,12 +7,13 @@ import { OutOfMemoryError } from '../child-proxy/out-of-memory-error';
 import { TestRunnerDecorator } from './test-runner-decorator';
 
 const ERROR_MESSAGE = 'Test runner crashed. Tried twice to restart it without any luck. Last time the error message was: ';
+export const MAX_RETRIES = 2;
 
 /**
- * Wraps a test runner and implements the retry functionality.
+ * Implements the retry functionality whenever an internal test runner rejects a promise.
  */
-export class RetryDecorator extends TestRunnerDecorator {
-  private readonly log = getLogger(RetryDecorator.name);
+export class RetryRejectedDecorator extends TestRunnerDecorator {
+  private readonly log = getLogger(RetryRejectedDecorator.name);
 
   public async dryRun(options: DryRunOptions): Promise<DryRunResult> {
     const result = await this.run(() => super.dryRun(options));
@@ -38,7 +39,11 @@ export class RetryDecorator extends TestRunnerDecorator {
     }
   }
 
-  private async run<T extends DryRunResult | MutantRunResult>(actRun: () => Promise<T>, attemptsLeft = 2, lastError?: unknown): Promise<T | string> {
+  private async run<T extends DryRunResult | MutantRunResult>(
+    actRun: () => Promise<T>,
+    attemptsLeft = MAX_RETRIES,
+    lastError?: unknown
+  ): Promise<T | string> {
     if (attemptsLeft > 0) {
       try {
         return await actRun();
@@ -57,10 +62,4 @@ export class RetryDecorator extends TestRunnerDecorator {
       return `${ERROR_MESSAGE}${errorToString(lastError)}`;
     }
   }
-
-  private async recover(): Promise<void> {
-    await this.dispose();
-    this.createInnerRunner();
-    return this.init();
-  }
 }

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

@@ -33,4 +33,14 @@ export class TestRunnerDecorator implements Required<TestRunner>, Disposable {
       return Promise.resolve();
     }
   }
+
+  /**
+   * Disposes the current test runner and creates a new one
+   * To be used in decorators that need recreation.
+   */
+  protected async recover(): Promise<void> {
+    await this.dispose();
+    this.createInnerRunner();
+    return this.init();
+  }
 }

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

@@ -41,7 +41,7 @@ class ProximityMineTestRunner implements TestRunner {
 
 export class CounterTestRunner implements TestRunner {
   private count = 0;
-  public static COUNTER_FILE = `${os.tmpdir()}/counter-file`;
+  public static COUNTER_FILE = `${os.tmpdir()}/stryker-js-test-counter-file`;
 
   public async dryRun(): Promise<DryRunResult> {
     return factory.completeDryRunResult();
@@ -93,6 +93,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,15 @@ 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);
   });
 
   afterEach(async () => {
     if (!alreadyDisposed) {
       await sut.dispose();
     }
     await loggingServer.dispose();
-
-    if (fs.existsSync(CounterTestRunner.COUNTER_FILE)) {
-      await fs.unlinkSync(CounterTestRunner.COUNTER_FILE);
-    }
+    rmSync(CounterTestRunner.COUNTER_FILE);
   });
 
   async function arrangeSut(name: string): Promise<void> {
@@ -65,8 +66,8 @@ describe(`${createTestRunnerFactory.name} integration`, () => {
     return sut.dryRun({ timeout, coverageAnalysis: 'all' });
   }
 
-  function actMutantRun() {
-    return sut.mutantRun(factory.mutantRunOptions());
+  function actMutantRun(options = factory.mutantRunOptions()) {
+    return sut.mutantRun(options);
   }
 
   it('should pass along the coverage result from the test runner behind', async () => {
@@ -125,6 +126,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/core/test/unit/test-runner/retry-decorator.spec.ts → packages/core/test/unit/test-runner/retry-rejected-decorator.spec.ts

@@ -6,12 +6,12 @@ import { factory, assertions } from '@stryker-mutator/test-helpers';
 
 import { ChildProcessCrashedError } from '../../../src/child-proxy/child-process-crashed-error';
 import { OutOfMemoryError } from '../../../src/child-proxy/out-of-memory-error';
-import { RetryDecorator } from '../../../src/test-runner/retry-decorator';
+import { RetryRejectedDecorator } from '../../../src/test-runner/retry-rejected-decorator';
 import { TestRunnerDecorator } from '../../../src/test-runner/test-runner-decorator';
 import { currentLogMock } from '../../helpers/log-mock';
 
-describe(RetryDecorator.name, () => {
-  let sut: RetryDecorator;
+describe(RetryRejectedDecorator.name, () => {
+  let sut: RetryRejectedDecorator;
   let testRunner1: sinon.SinonStubbedInstance<Required<TestRunner>>;
   let testRunner2: sinon.SinonStubbedInstance<Required<TestRunner>>;
   let availableTestRunners: Array<sinon.SinonStubbedInstance<Required<TestRunner>>>;
@@ -23,7 +23,7 @@ describe(RetryDecorator.name, () => {
     testRunner2 = factory.testRunner();
     logMock = currentLogMock();
     availableTestRunners = [testRunner1, testRunner2];
-    sut = new RetryDecorator(() => availableTestRunners.shift() ?? factory.testRunner());
+    sut = new RetryRejectedDecorator(() => availableTestRunners.shift() ?? factory.testRunner());
   });
 
   it('should not override `init`', () => {
@@ -58,7 +58,7 @@ describe(RetryDecorator.name, () => {
 
   function describeRun<T extends keyof RunOptionsByMethod>(
     runMethod: T,
-    act: (sut: RetryDecorator, options: RunOptionsByMethod[T]) => Promise<RunResultByMethod[T]>,
+    act: (sut: RetryRejectedDecorator, options: RunOptionsByMethod[T]) => Promise<RunResultByMethod[T]>,
     optionsFactory: () => RunOptionsByMethod[T],
     resultFactory: () => ReturnType<TestRunner[T]> extends Promise<infer R> ? R : never
   ) {