fix(taskkill): make killing of tasks more relient on windows

stryker-mutator · Jul 6, 2018 · a820577 · a820577
1 parent 10bed57
commit a820577
Show file tree

Hide file tree

Showing 8 changed files with 113 additions and 90 deletions.
diff --git a/packages/stryker/src/isolated-runner/IsolatedTestRunnerAdapter.ts b/packages/stryker/src/isolated-runner/IsolatedTestRunnerAdapter.ts
@@ -7,6 +7,7 @@ import { serialize, kill } from '../utils/objectUtils';
 import { AdapterMessage, WorkerMessage } from './MessageProtocol';
 import IsolatedRunnerOptions from './IsolatedRunnerOptions';
 import Task from '../utils/Task';
+import StrykerError from '../utils/StrykerError';
 
 const MAX_WAIT_FOR_DISPOSE = 2000;
 
@@ -109,7 +110,7 @@ export default class TestRunnerChildProcessAdapter extends EventEmitter implemen
       if (code !== 0 && code !== null) {
         this.log.error(`Child process exited with non-zero exit code ${code}. Last 10 message from the child process were: \r\n${this.lastMessagesQueue.map(msg => `\t${msg}`).join('\r\n')}`);
         if (this.currentTask) {
-          this.currentTask.reject(`Test runner child process exited with non-zero exit code ${code}`);
+          this.currentTask.reject(new StrykerError(`Test runner child process exited with non-zero exit code ${code}`));
         }
       }
     });

diff --git a/packages/stryker/src/isolated-runner/IsolatedTestRunnerAdapterWorker.ts b/packages/stryker/src/isolated-runner/IsolatedTestRunnerAdapterWorker.ts
@@ -29,7 +29,8 @@ class IsolatedTestRunnerAdapterWorker {
           this.init();
           break;
         case 'dispose':
-          this.dispose();
+          const sendDisposeDone = this.sendDisposeDone.bind(this);
+          this.dispose().then(sendDisposeDone, sendDisposeDone);
           break;
         default:
           this.logReceivedMessageWarning(message);
@@ -86,11 +87,13 @@ class IsolatedTestRunnerAdapterWorker {
   }
 
   async dispose() {
-    if (this.underlyingTestRunner.dispose) {
-      await this.underlyingTestRunner.dispose();
+    try {
+      if (this.underlyingTestRunner.dispose) {
+        await this.underlyingTestRunner.dispose();
+      }
+    } finally {
+      await LogConfigurator.shutdown();
     }
-    await LogConfigurator.shutdown();
-    this.sendDisposeDone();
   }
 
   sendDisposeDone() {

diff --git a/packages/stryker/src/isolated-runner/TestRunnerDecorator.ts b/packages/stryker/src/isolated-runner/TestRunnerDecorator.ts
@@ -30,6 +30,4 @@ export default class TestRunnerDecorator implements TestRunner {
       return Promise.resolve();
     }
   }
-
-
 }
diff --git a/packages/stryker/src/utils/objectUtils.ts b/packages/stryker/src/utils/objectUtils.ts
@@ -86,12 +86,18 @@ export function normalizeWhiteSpaces(str: string) {
 
 export function kill(pid: number): Promise<void> {
   return new Promise((res, rej) => {
-    treeKill(pid, 'SIGKILL', err => {
-      if (err) {
+    treeKill(pid, 'SIGKILL', (err: { code?: number } & Error) => {
+      if (err && !canIgnore(err.code)) {
         rej(err);
       } else {
         res();
       }
     });
+
+    function canIgnore(code: number | undefined) {
+      // https://docs.microsoft.com/en-us/windows/desktop/Debug/system-error-codes--0-499-
+      // these error codes mean the program is _already_ closed.
+      return code === 255 || code === 128;
+    }
   });
 }
diff --git a/packages/stryker/test/helpers/LoggingServer.ts b/packages/stryker/test/helpers/LoggingServer.ts
@@ -0,0 +1,44 @@
+import * as net from 'net';
+import * as log4js from 'log4js';
+import { Subscriber, Observable } from 'rxjs';
+
+export default class LoggingServer {
+
+  private readonly server: net.Server;
+  private subscribers: Subscriber<log4js.LoggingEvent>[] = [];
+  public readonly event$: Observable<log4js.LoggingEvent>;
+
+  constructor(public readonly port: number) {
+    this.server = net.createServer(socket => {
+      socket.on('data', data => {
+        const str = data.toString();
+        try {
+          const json = JSON.parse(str);
+          this.subscribers.map(sub => sub.next(json));
+        } catch {
+          // IDLE. Log4js also sends "__LOG4JS__" to signal an event end. Ignore those.
+        }
+      });
+    });
+    this.server.listen(this.port);
+
+    this.event$ = new Observable<log4js.LoggingEvent>(subscriber => {
+      this.subscribers.push(subscriber);
+      this.server.on('close', () => {
+        subscriber.complete();
+      });
+    });
+  }
+
+  dispose(): Promise<void> {
+    return new Promise((res, rej) => {
+      this.server.close((err: Error) => {
+        if (err) {
+          rej(err);
+        } else {
+          res();
+        }
+      });
+    });
+  }
+}
diff --git a/packages/stryker/test/integration/child-proxy/ChildProcessProxy.it.ts b/packages/stryker/test/integration/child-proxy/ChildProcessProxy.it.ts
@@ -2,11 +2,10 @@ import { expect } from 'chai';
 import Echo from './Echo';
 import ChildProcessProxy from '../../../src/child-proxy/ChildProcessProxy';
 import { File, LogLevel } from 'stryker-api/core';
-import * as net from 'net';
 import * as log4js from 'log4js';
-import { Observable, Subscriber } from 'rxjs';
 import * as getPort from 'get-port';
 import Task from '../../../src/utils/Task';
+import LoggingServer from '../../helpers/LoggingServer';
 
 describe('ChildProcessProxy', () => {
 
@@ -74,44 +73,3 @@ function toLogLevel(level: log4js.Level) {
   const levelName = (level as any).levelStr.toLowerCase();
   return [LogLevel.Debug, LogLevel.Error, LogLevel.Fatal, LogLevel.Information, LogLevel.Off, LogLevel.Trace, LogLevel.Warning].find(level => level === levelName);
 }
-
-class LoggingServer {
-
-  private readonly server: net.Server;
-  private subscribers: Subscriber<log4js.LoggingEvent>[] = [];
-  public readonly event$: Observable<log4js.LoggingEvent>;
-
-  constructor(private port: number) {
-    this.server = net.createServer(socket => {
-      socket.on('data', data => {
-        const str = data.toString();
-        try {
-          const json = JSON.parse(str);
-          this.subscribers.map(sub => sub.next(json));
-        } catch {
-          // IDLE. Log4js also sends "__LOG4JS__" to signal an event end. Ignore those.
-        }
-      });
-    });
-    this.server.listen(this.port);
-
-    this.event$ = new Observable<log4js.LoggingEvent>(subscriber => {
-      this.subscribers.push(subscriber);
-      this.server.on('close', () => {
-        subscriber.complete();
-      });
-    });
-  }
-
-  dispose(): Promise<void> {
-    return new Promise((res, rej) => {
-      this.server.close((err: Error) => {
-        if (err) {
-          rej(err);
-        } else {
-          res();
-        }
-      });
-    });
-  }
-}
diff --git a/packages/stryker/test/integration/isolated-runner/AdditionalTestRunners.ts b/packages/stryker/test/integration/isolated-runner/AdditionalTestRunners.ts
@@ -1,39 +1,36 @@
-import { EventEmitter } from 'events';
-import { RunResult, RunStatus, RunOptions, RunnerOptions, TestRunner, TestRunnerFactory } from 'stryker-api/test_runner';
+import { RunResult, RunStatus, RunnerOptions, TestRunner, TestRunnerFactory } from 'stryker-api/test_runner';
 import { isRegExp } from 'util';
 
-class CoverageReportingTestRunner extends EventEmitter implements TestRunner {
-  run(options: RunOptions) {
+class CoverageReportingTestRunner implements TestRunner {
+  run() {
     (global as any).__coverage__ = 'overridden';
     return Promise.resolve({ status: RunStatus.Complete, tests: [], coverage: <any>'realCoverage' });
   }
 }
 
-class TimeBombTestRunner extends EventEmitter implements TestRunner {
+class TimeBombTestRunner implements TestRunner {
   constructor() {
-    super();
     // Setting a time bomb after 100 ms
     setTimeout(() => process.exit(), 100);
   }
-  run(options: RunOptions) {
+  run() {
     return Promise.resolve({ status: RunStatus.Complete, tests: [] });
   }
 }
 
-class DirectResolvedTestRunner extends EventEmitter implements TestRunner {
-  run(options: RunOptions) {
+class DirectResolvedTestRunner implements TestRunner {
+  run() {
     (global as any).__coverage__ = 'coverageObject';
     return Promise.resolve({ status: RunStatus.Complete, tests: [] });
   }
 }
 
-class DiscoverRegexTestRunner extends EventEmitter implements TestRunner {
+class DiscoverRegexTestRunner implements TestRunner {
 
   constructor(private runnerOptions: RunnerOptions) {
-    super();
   }
 
-  run(options: RunOptions): Promise<RunResult> {
+  run(): Promise<RunResult> {
     if (isRegExp(this.runnerOptions.strykerOptions['someRegex'])) {
       return Promise.resolve({ status: RunStatus.Complete, tests: [] });
     } else {
@@ -43,9 +40,9 @@ class DiscoverRegexTestRunner extends EventEmitter implements TestRunner {
 }
 
 
-class ErroredTestRunner extends EventEmitter implements TestRunner {
+class ErroredTestRunner implements TestRunner {
 
-  run(options: RunOptions) {
+  run() {
     let expectedError: any = null;
     try {
       throw new SyntaxError('This is invalid syntax!');
@@ -62,18 +59,18 @@ class RejectInitRunner implements TestRunner {
     return Promise.reject(new Error('Init was rejected'));
   }
 
-  run(options: RunOptions): Promise<RunResult> {
+  run(): Promise<RunResult> {
     throw new Error();
   }
 }
 
-class NeverResolvedTestRunner extends EventEmitter implements TestRunner {
-  run(options: RunOptions) {
-    return new Promise<RunResult>(res => { });
+class NeverResolvedTestRunner implements TestRunner {
+  run() {
+    return new Promise<RunResult>(() => { });
   }
 }
 
-class SlowInitAndDisposeTestRunner extends EventEmitter implements TestRunner {
+class SlowInitAndDisposeTestRunner implements TestRunner {
 
   inInit: boolean;
 
@@ -87,7 +84,7 @@ class SlowInitAndDisposeTestRunner extends EventEmitter implements TestRunner {
     });
   }
 
-  run(options: RunOptions) {
+  run() {
     if (this.inInit) {
       throw new Error('Test should fail! Not yet initialized!');
     }
@@ -98,11 +95,11 @@ class SlowInitAndDisposeTestRunner extends EventEmitter implements TestRunner {
     return this.init();
   }
 }
-class VerifyWorkingFolderTestRunner extends EventEmitter implements TestRunner {
+class VerifyWorkingFolderTestRunner implements TestRunner {
 
   runResult: RunResult = { status: RunStatus.Complete, tests: [] };
 
-  run(options: RunOptions) {
+  run() {
     if (process.cwd().toLowerCase() === __dirname.toLowerCase()) {
       return Promise.resolve(this.runResult);
     } else {
@@ -111,14 +108,14 @@ class VerifyWorkingFolderTestRunner extends EventEmitter implements TestRunner {
   }
 }
 
-class AsyncronousPromiseRejectionHandlerTestRunner extends EventEmitter implements TestRunner {
+class AsyncronousPromiseRejectionHandlerTestRunner implements TestRunner {
   promise: Promise<void>;
 
   init() {
     this.promise = Promise.reject('Reject for now, but will be caught asynchronously');
   }
 
-  run(options: RunOptions) {
+  run() {
     this.promise.catch(() => { });
     return Promise.resolve({ status: RunStatus.Complete, tests: [] });
   }

diff --git a/packages/stryker/test/integration/isolated-runner/ResilientTestRunnerFactorySpec.ts b/packages/stryker/test/integration/isolated-runner/ResilientTestRunnerFactorySpec.ts
@@ -1,13 +1,15 @@
 import * as path from 'path';
 import { Logger } from 'stryker-api/logging';
 import { expect } from 'chai';
+import * as getPort from 'get-port';
 import { RunResult, RunStatus } from 'stryker-api/test_runner';
 import ResilientTestRunnerFactory from '../../../src/isolated-runner/ResilientTestRunnerFactory';
 import IsolatedRunnerOptions from '../../../src/isolated-runner/IsolatedRunnerOptions';
 import TestRunnerDecorator from '../../../src/isolated-runner/TestRunnerDecorator';
 import currentLogMock from '../../helpers/logMock';
 import { Mock } from '../../helpers/producers';
 import { LogLevel } from 'stryker-api/core';
+import LoggingServer from '../../helpers/LoggingServer';
 
 function sleep(ms: number) {
   return new Promise(res => {
@@ -20,24 +22,38 @@ describe('ResilientTestRunnerFactory', function () {
   this.timeout(15000);
   let log: Mock<Logger>;
   let sut: TestRunnerDecorator;
-  let options: IsolatedRunnerOptions = {
-    strykerOptions: {
-      plugins: ['../../test/integration/isolated-runner/AdditionalTestRunners'],
-      testRunner: 'karma',
-      testFramework: 'jasmine',
-      port: 0,
-      'someRegex': /someRegex/
-    },
-    port: 0,
-    fileNames: [],
-    sandboxWorkingFolder: path.resolve('./test/integration/isolated-runner'),
-    loggingContext: { port: 4200, level: LogLevel.Fatal }
-  };
+  let options: IsolatedRunnerOptions;
 
   beforeEach(() => {
     log = currentLogMock();
   });
 
+  let loggingServer: LoggingServer;
+
+  before(async () => {
+    // Make sure there is a logging server listening
+    const port = await getPort();
+    loggingServer = new LoggingServer(port);
+
+    options = {
+      strykerOptions: {
+        plugins: ['../../test/integration/isolated-runner/AdditionalTestRunners'],
+        testRunner: 'karma',
+        testFramework: 'jasmine',
+        port: 0,
+        'someRegex': /someRegex/
+      },
+      port: 0,
+      fileNames: [],
+      sandboxWorkingFolder: path.resolve('./test/integration/isolated-runner'),
+      loggingContext: { port, level: LogLevel.Fatal }
+    };
+  });
+
+  after(() => {
+    return loggingServer.dispose();
+  });
+
   describe('when sending a regex in the options', () => {
     before(() => sut = ResilientTestRunnerFactory.create('discover-regex', options));
-Original file line number
+Diff line change
@@ Expand Up @@
           return Promise.resolve();
         }
       }
     }