fix: correct exit on 'SIGHUP', 'SIGINT' or 'SIGTERM'

gemini-testing · Nov 16, 2016 · 1f0ffbe · 1f0ffbe
1 parent 2bf95bb
commit 1f0ffbe
Show file tree

Hide file tree

Showing 12 changed files with 79 additions and 76 deletions.
diff --git a/lib/browser-pool/basic-pool.js b/lib/browser-pool/basic-pool.js
@@ -2,9 +2,9 @@
 var util = require('util'),
     Browser = require('../browser'),
     Pool = require('./pool'),
-    signalHandler = require('../signal-handler'),
     _ = require('lodash'),
-    Promise = require('bluebird');
+    Promise = require('bluebird'),
+    CancelledError = require('../errors/cancelled-error');
 
 var activeSessions = {};
 
@@ -23,15 +23,16 @@ util.inherits(BasicPool, Pool);
 
 Pool.prototype.getBrowser = function(id) {
     var _this = this,
-        browser = Browser.create(this._config.forBrowser(id)),
-        launchPromise = browser.launch(this._calibrator);
+        browser = Browser.create(this._config.forBrowser(id));
 
-    activeSessions[browser.id] = {
-        browser: browser,
-        launchPromise: launchPromise
-    };
+    return browser.launch(this._calibrator)
+        .then(() => {
+            if (this._cancelled) {
+                return Promise.reject(new CancelledError());
+            }
 
-    return launchPromise
+            activeSessions[browser.sessionId] = browser;
+        })
         .then(browser.reset.bind(browser)).thenReturn(browser)
         .catch(function(e) {
             return _this.freeBrowser(browser)
@@ -42,19 +43,16 @@ Pool.prototype.getBrowser = function(id) {
 };
 
 Pool.prototype.freeBrowser = function(browser) {
-    delete activeSessions[browser.id];
+    delete activeSessions[browser.sessionId];
     return browser.quit();
 };
 
-signalHandler.on('exit', function() {
-    console.log('Killing browsers...');
-    return _(activeSessions)
-        .map(function(session) {
-            var quit_ = session.browser.quit.bind(session.browser);
-            return session.launchPromise.then(quit_);
-        })
-        .thru(Promise.all)
-        .value();
-});
+Pool.prototype.cancel = function() {
+    this._cancelled = true;
+
+    _.forEach(activeSessions, (browser) => browser.quit());
+
+    activeSessions = {};
+};
 
 module.exports = BasicPool;
diff --git a/lib/browser/new-browser.js b/lib/browser/new-browser.js
@@ -341,7 +341,7 @@ module.exports = class NewBrowser extends Browser {
             .then(() => this._wd.quit())
             .then(() => this.log('kill browser %o', this))
             .then(() => this._setHttpTimeout())
-            .catch((err) => console.warn(err));
+            .catch((err) => this.log(err));
     }
 
     inspect() {

diff --git a/lib/cli/index.js b/lib/cli/index.js
@@ -5,12 +5,15 @@ const program = require('commander');
 const Promise = require('bluebird');
 
 const Config = require('../config');
+const Events = require('../constants/events');
 const Gemini = require('../gemini');
 const pkg = require('../../package.json');
 const handleErrors = require('./errors').handleErrors;
 const checkForDeprecations = require('./deprecations').checkForDeprecations;
 const handleUncaughtExceptions = require('./errors').handleUncaughtExceptions;
 
+let exitCode;
+
 exports.run = () => {
     program
         .version(pkg.version)
@@ -86,7 +89,11 @@ function runGemini(method, paths, options) {
 
     return Promise.try(() => {
         checkForDeprecations();
-        return new Gemini(program.config, {cli: true, env: true});
+
+        const gemini = new Gemini(program.config, {cli: true, env: true});
+        gemini.on(Events.INTERRUPT, (data) => exitCode = data.exitCode);
+
+        return gemini;
     })
         .then((gemini) => {
             return gemini[method](paths, {
@@ -109,5 +116,5 @@ function runGemini(method, paths, options) {
 }
 
 function exit(code) {
-    process.on('exit', () => process.exit(code));
+    process.on('exit', () => process.exit(exitCode || code));
 }
diff --git a/lib/constants/events.js b/lib/constants/events.js
@@ -33,5 +33,7 @@ module.exports = {
     UPDATE_RESULT: 'updateResult',
 
     BEFORE_FILE_READ: 'beforeFileRead',
-    AFTER_FILE_READ: 'afterFileRead'
+    AFTER_FILE_READ: 'afterFileRead',
+
+    INTERRUPT: 'interrupt'
 };
diff --git a/lib/gemini.js b/lib/gemini.js
@@ -65,6 +65,13 @@ module.exports = class Gemini extends PassthroughEmitter {
 
         this._passThroughEvents(runner);
 
+        // it is important to require signal handler here in order to guarantee subscribing to "INTERRUPT" event
+        require('./signal-handler').on(Events.INTERRUPT, (data) => {
+            this.emit(Events.INTERRUPT, data);
+
+            runner.cancel();
+        });
+
         if (browsers) {
             this.checkUnknownBrowsers(browsers);
 

diff --git a/lib/reporters/html/view-model.js b/lib/reporters/html/view-model.js
@@ -49,28 +49,36 @@ module.exports = class ViewModel {
      * @param {TestStateResult} result
      */
     addFail(result) {
+        this._addFailResult(result);
+
+        this._counter.onFailed(result);
+    }
+
+    _addFailResult(result) {
         this._addTestResult(result, {
             fail: true,
             actualPath: lib.currentPath(result),
             expectedPath: lib.referencePath(result),
             diffPath: lib.diffPath(result)
         });
-
-        this._counter.onFailed(result);
     }
 
     /**
      * @param {ErrorStateResult} result
      */
     addError(result) {
+        this._addErrorResult(result);
+
+        this._counter.onFailed(result);
+    }
+
+    _addErrorResult(result) {
         this._addTestResult(result, {
             actualPath: result.state ? lib.currentPath(result) : '',
             error: true,
             image: !!result.imagePath || !!result.currentPath,
             reason: (result.stack || result.message || result || '')
         });
-
-        this._counter.onFailed(result);
     }
 
     /**
@@ -91,9 +99,9 @@ module.exports = class ViewModel {
      */
     addRetry(result) {
         if (result.hasOwnProperty('equal')) {
-            this.addFail(result);
+            this._addFailResult(result);
         } else {
-            this.addError(result);
+            this._addErrorResult(result);
         }
 
         this._counter.onRetry(result);

diff --git a/lib/runner/browser-runner/index.js b/lib/runner/browser-runner/index.js
@@ -1,7 +1,6 @@
 'use strict';
 
 const _ = require('lodash');
-const Promise = require('bluebird');
 const promiseUtils = require('q-promise-utils');
 const BrowserAgent = require('./browser-agent');
 const Runner = require('../runner');
@@ -35,14 +34,9 @@ module.exports = class BrowserRunner extends Runner {
     }
 
     cancel() {
-        this._runSuite = this._doNothing;
         this._suiteRunners.forEach((runner) => runner.cancel());
     }
 
-    _doNothing() {
-        return Promise.resolve();
-    }
-
     _runSuites(suiteCollection, stateProcessor) {
         const suites = suiteCollection.clone().allSuites();
 

diff --git a/lib/runner/index.js b/lib/runner/index.js
@@ -40,7 +40,7 @@ module.exports = class TestsRunner extends Runner {
         return Promise.resolve(this.emitAndWait(Events.START_RUNNER, this))
             .then(() => this.emit(Events.BEGIN, this._formatBeginEventData(suiteCollection)))
             .then(() => this._stateProcessor.prepare(this))
-            .then(() => this._runTests(suiteCollection))
+            .then(() => !this._cancelled && this._runTests(suiteCollection))
             .then(() => this.coverage && this.coverage.processStats())
             .finally(() => {
                 this.emit(Events.END);
@@ -76,8 +76,6 @@ module.exports = class TestsRunner extends Runner {
     _runTestsInBrowser(suiteCollection, browserId) {
         const runner = BrowserRunner.create(browserId, this.config, this._browserPool);
 
-        this._browserRunners.push(runner);
-
         this.passthroughEvent(runner, [
             Events.RETRY,
             Events.START_BROWSER,
@@ -97,11 +95,8 @@ module.exports = class TestsRunner extends Runner {
         runner.on(Events.TEST_RESULT, (result) => this._handleResult(result, [Events.END_TEST, Events.TEST_RESULT]));
         runner.on(Events.UPDATE_RESULT, (result) => this._handleResult(result, Events.UPDATE_RESULT));
 
-        return runner.run(suiteCollection, this._stateProcessor)
-            .catch((e) => {
-                this._cancel();
-                return Promise.reject(e);
-            });
+        this._browserRunners.push(runner);
+        return runner.run(suiteCollection, this._stateProcessor);
     }
 
     _handleResult(result, events) {
@@ -116,8 +111,11 @@ module.exports = class TestsRunner extends Runner {
         }
     }
 
-    _cancel() {
+    cancel() {
+        this._cancelled = true;
+
         this._browserRunners.forEach((runner) => runner.cancel());
+
         this._browserPool.cancel();
     }
 };
diff --git a/lib/runner/suite-runner/insistent-suite-runner.js b/lib/runner/suite-runner/insistent-suite-runner.js
@@ -20,10 +20,6 @@ module.exports = class InsistentSuiteRunner extends SuiteRunner {
         this._retriesPerformed = 0;
     }
 
-    cancel() {
-        this._suiteRunner.cancel();
-    }
-
     _doRun(stateProcessor) {
         this._suiteRunner = this._initSuiteRunner();
         this._statesToRetry = [];
@@ -61,7 +57,7 @@ module.exports = class InsistentSuiteRunner extends SuiteRunner {
     }
 
     _retry(stateProcessor) {
-        if (_.isEmpty(this._statesToRetry)) {
+        if (_.isEmpty(this._statesToRetry) || this._cancelled) {
             return;
         }
 

diff --git a/lib/runner/suite-runner/regular-suite-runner.js b/lib/runner/suite-runner/regular-suite-runner.js
@@ -53,14 +53,6 @@ module.exports = class RegularSuiteRunner extends SuiteRunner {
         });
     }
 
-    cancel() {
-        this._runStateInSession = this._doNothing;
-    }
-
-    _doNothing() {
-        return Promise.resolve();
-    }
-
     _processStates(stateProcessor) {
         const browser = this._session.browser;
 

diff --git a/lib/runner/suite-runner/suite-runner.js b/lib/runner/suite-runner/suite-runner.js
@@ -33,7 +33,11 @@ var SuiteRunner = inherit(Runner, {
         throw new Error('Not implemented');
     },
 
-    cancel: _.noop
+    cancel: function() {
+        this._cancelled = true;
+
+        this.emit = _.noop;
+    }
 });
 
 module.exports = SuiteRunner;
diff --git a/lib/signal-handler.js b/lib/signal-handler.js
@@ -1,30 +1,27 @@
 'use strict';
 
-var QEmitter = require('qemitter'),
-    signalHandler = new QEmitter();
+const EventEmitter = require('events').EventEmitter;
+const Events = require('./constants/events');
+const logger = require('./utils').logger;
 
-module.exports = signalHandler;
+const signalHandler = module.exports = new EventEmitter();
 
-process.on('SIGHUP', notifyAndExit(1));
-process.on('SIGINT', notifyAndExit(2));
-process.on('SIGTERM', notifyAndExit(15));
+process.on('SIGHUP', handleSignal(1));
+process.on('SIGINT', handleSignal(2));
+process.on('SIGTERM', handleSignal(15));
 
-var callCount = 0;
+let callCount = 0;
 
-function notifyAndExit(signalNo) {
-    var exitCode = 128 + signalNo;
+function handleSignal(signalNo) {
+    const exitCode = 128 + signalNo;
 
-    return function() {
+    return () => {
         if (callCount++ > 0) {
-            console.log('Force quit.');
+            logger.warn('Force quit.');
             process.exit(exitCode);
         }
 
-        signalHandler.emitAndWait('exit')
-            .then(function() {
-                console.log('Done.');
-                process.exit(exitCode);
-            })
-            .done();
+        logger.warn('Cancelling...');
+        signalHandler.emit(Events.INTERRUPT, {exitCode});
     };
 }