fix: emit browser start/stop events in correct places

Author: j0tunn

doc/events.md

@@ -12,3 +12,7 @@
 
 * `INTERRUPT` - emitted on signal events `SIGHUP`, `SIGINT` or `SIGTERM`. The event is emitted with 1 argument `data`:
     * `data.exitCode` - exit code with which gemini will be interrupted
+
+* `START_BROWSER` - emitted on browser session start. Emitted with [browser instance](../lib/browser/new-browser.js). If handler returns a promise tests will be executed in this session only after the promise is resolved.
+
+* `STOP_BROWSER` - emitted right before browser session end. Emitted with [browser instance](../lib/browser/new-browser.js). If handler returns a promise quit will be performed only after the promise is resolved.

lib/browser-pool/basic-pool.js

@@ -4,20 +4,27 @@ const Promise = require('bluebird');
 const _ = require('lodash');
 const Browser = require('../browser');
 const CancelledError = require('../errors/cancelled-error');
+const Events = require('../constants/events');
 const Pool = require('./pool');
+const log = require('debug')('gemini:pool:basic');
 
 module.exports = class BasicPool extends Pool {
+    static create(config, calibrator, emitter) {
+        return new BasicPool(config, calibrator, emitter);
+    }
+
     /**
      * @constructor
      * @extends Pool
      * @param {Config} config
      * @param {Calibrator} calibrator
      */
-    constructor(config, calibrator) {
+    constructor(config, calibrator, emitter) {
         super();
 
         this._config = config;
         this._calibrator = calibrator;
+        this._emitter = emitter;
 
         this._activeSessions = {};
     }
@@ -26,6 +33,10 @@ module.exports = class BasicPool extends Pool {
         const browser = Browser.create(this._config.forBrowser(id));
 
         return browser.launch(this._calibrator)
+            .then(() => {
+                log(`browser ${id} started`);
+                return this._emitter.emitAndWait(Events.START_BROWSER, browser);
+            })
             .then(() => {
                 if (this._cancelled) {
                     return Promise.reject(new CancelledError());
@@ -40,7 +51,11 @@ module.exports = class BasicPool extends Pool {
 
     freeBrowser(browser) {
         delete this._activeSessions[browser.sessionId];
-        return browser.quit();
+        log(`stop browser ${browser.id}`);
+
+        return this._emitter.emitAndWait(Events.STOP_BROWSER, browser)
+            .catch((err) => console.warn(err && err.stack || err))
+            .then(() => browser.quit());
     }
 
     cancel() {

lib/browser-pool/index.js

@@ -10,9 +10,9 @@ var _ = require('lodash'),
  * @param {Config} config
  * @returns {BasicPool}
  */
-exports.create = function(config) {
+exports.create = function(config, emitter) {
     var calibrator = new Calibrator(),
-        pool = new BasicPool(config, calibrator);
+        pool = BasicPool.create(config, calibrator, emitter);
 
     pool = new CachingPool(config, pool);
     pool = new PerBrowserLimitedPool(config, pool);

lib/runner/browser-runner/index.js

@@ -7,8 +7,6 @@ const Runner = require('../runner');
 const SuiteRunner = require('../suite-runner');
 const Events = require('../../constants/events');
 
-const log = require('debug')('gemini:runner');
-
 module.exports = class BrowserRunner extends Runner {
     constructor(browserId, config, browserPool) {
         super();
@@ -24,20 +22,6 @@ module.exports = class BrowserRunner extends Runner {
     }
 
     run(suiteCollection, stateProcessor) {
-        log('start browser %s', this._browserId);
-        this.emit(Events.START_BROWSER, {browserId: this._browserId});
-        return this._runSuites(suiteCollection, stateProcessor)
-            .finally(() => {
-                log('stop browser %s', this._browserId);
-                this.emit(Events.STOP_BROWSER, {browserId: this._browserId});
-            });
-    }
-
-    cancel() {
-        this._suiteRunners.forEach((runner) => runner.cancel());
-    }
-
-    _runSuites(suiteCollection, stateProcessor) {
         const suites = suiteCollection.clone().allSuites();
 
         return _(suites)
@@ -47,6 +31,10 @@ module.exports = class BrowserRunner extends Runner {
             .value();
     }
 
+    cancel() {
+        this._suiteRunners.forEach((runner) => runner.cancel());
+    }
+
     _runSuite(suite, stateProcessor) {
         const browserAgent = BrowserAgent.create(this._browserId, this._browserPool);
         const runner = SuiteRunner.create(suite, browserAgent, this._config);

lib/runner/index.js

@@ -24,7 +24,7 @@ module.exports = class TestsRunner extends Runner {
 
         this._stateProcessor = stateProcessor;
 
-        this._browserPool = pool.create(this.config);
+        this._browserPool = pool.create(this.config, this);
 
         this._suiteMonitor = SuiteMonitor.create(this);
         this.passthroughEvent(this._suiteMonitor, Events.END_SUITE);

test/unit/browser-pool/basic-pool.js

@@ -1,15 +1,19 @@
 'use strict';
 
 const Promise = require('bluebird');
+const QEmitter = require('qemitter');
+const bluebirdQ = require('bluebird-q');
 const Browser = require('lib/browser');
 const BasicPool = require('lib/browser-pool/basic-pool');
 const CancelledError = require('lib/errors/cancelled-error');
 const browserWithId = require('test/util').browserWithId;
+const Events = require('lib/constants/events');
+const _ = require('lodash');
 
 describe('BasicPool', () => {
     const sandbox = sinon.sandbox.create();
 
-    const stubBrowser = (id) => {
+    const stubBrowser_ = (id) => {
         id = id || 'id';
         const browser = browserWithId(id);
         sandbox.stub(browser, 'launch', () => {
@@ -26,66 +30,164 @@ describe('BasicPool', () => {
 
     const stubConfig = () => ({forBrowser: (id) => ({id})});
 
-    let pool;
-
-    beforeEach(() => {
-        pool = new BasicPool(stubConfig());
+    const mkPool_ = (opts) => {
+        opts = _.defaults(opts || {}, {
+            config: stubConfig(),
+            calibrator: 'default-calibrator',
+            emitter: new QEmitter()
+        });
 
-        sandbox.stub(Browser, 'create');
-    });
-    afterEach(() => sandbox.restore());
+        return new BasicPool(opts.config, opts.calibrator, opts.emitter);
+    };
 
-    const requestBrowser = (browser) => {
-        browser = browser || stubBrowser();
+    const requestBrowser_ = (browser, pool) => {
+        browser = browser || stubBrowser_();
+        pool = pool || mkPool_();
 
         return pool.getBrowser(browser.id);
     };
 
+    beforeEach(() => {
+        sandbox.stub(Browser, 'create');
+    });
+
+    afterEach(() => sandbox.restore());
+
     it('should create new browser when requested', () => {
-        const browser = stubBrowser('foo');
+        const browser = stubBrowser_('foo');
 
-        return requestBrowser(browser)
+        return requestBrowser_(browser)
             .then(() => assert.calledWith(Browser.create, {id: 'foo'}));
     });
 
     it('should launch a browser', () => {
-        const browser = stubBrowser();
+        const browser = stubBrowser_();
 
-        return requestBrowser(browser)
+        return requestBrowser_(browser)
             .then(() => assert.calledOnce(browser.launch));
     });
 
     it('should launch a browser with calibrator', () => {
-        pool = new BasicPool(stubConfig(), 'calibrator');
+        const pool = mkPool_({calibrator: 'calibrator'});
 
-        const browser = stubBrowser();
+        const browser = stubBrowser_();
 
-        return requestBrowser(browser)
+        return requestBrowser_(browser, pool)
             .then(() => assert.calledWith(browser.launch, 'calibrator'));
     });
 
     it('should finalize browser if failed to create it', () => {
-        const browser = stubBrowser();
+        const browser = stubBrowser_();
+        const pool = mkPool_();
         const freeBrowser = sinon.spy(pool, 'freeBrowser');
         const assertCalled = () => assert.called(freeBrowser);
 
-        browser.reset.returns(Promise.reject());
+        browser.reset.returns(bluebirdQ.reject());
 
-        return requestBrowser(browser)
+        return requestBrowser_(browser, pool)
             .then(assertCalled, assertCalled);
     });
 
+    describe('START_BROWSER event', () => {
+        it('should be emitted on browser start', () => {
+            const emitter = new QEmitter();
+            const onSessionStart = sinon.spy().named('onSessionStart');
+            emitter.on(Events.START_BROWSER, onSessionStart);
+
+            const pool = mkPool_({emitter});
+            const browser = stubBrowser_();
+
+            return requestBrowser_(browser, pool)
+                .then(() => {
+                    assert.calledOnce(onSessionStart);
+                    assert.calledWith(onSessionStart, browser);
+                });
+        });
+
+        it('handler should be waited by pool', () => {
+            const emitter = new QEmitter();
+            const afterSessionStart = sinon.spy().named('afterSessionStart');
+            emitter.on(Events.START_BROWSER, () => bluebirdQ.delay(100).then(afterSessionStart));
+
+            const pool = mkPool_({emitter});
+            const browser = stubBrowser_();
+
+            return requestBrowser_(browser, pool)
+                .then(() => assert.callOrder(afterSessionStart, browser.reset));
+        });
+
+        it('handler fail should fail browser request', () => {
+            const emitter = new QEmitter();
+            emitter.on(Events.START_BROWSER, () => bluebirdQ.reject('some-error'));
+
+            const pool = mkPool_({emitter});
+            const browser = stubBrowser_();
+
+            return assert.isRejected(requestBrowser_(browser, pool), 'some-error');
+        });
+    });
+
     it('should quit a browser when freed', () => {
-        const browser = stubBrowser();
+        const pool = mkPool_();
+        const browser = stubBrowser_();
 
-        return requestBrowser(browser)
+        return requestBrowser_(browser, pool)
             .then((browser) => pool.freeBrowser(browser))
             .then(() => assert.calledOnce(browser.quit));
     });
 
+    describe('STOP_BROWSER event', () => {
+        it('should be emitted on browser quit', () => {
+            const emitter = new QEmitter();
+            const onSessionEnd = sinon.spy().named('onSessionEnd');
+            emitter.on(Events.STOP_BROWSER, onSessionEnd);
+
+            const pool = mkPool_({emitter});
+            const browser = stubBrowser_();
+
+            return requestBrowser_(browser, pool)
+                .then((browser) => pool.freeBrowser(browser))
+                .then(() => {
+                    assert.calledOnce(onSessionEnd);
+                    assert.calledWith(onSessionEnd, browser);
+                });
+        });
+
+        it('handler should be waited before actual quit', () => {
+            const emitter = new QEmitter();
+            const afterSessionEnd = sinon.spy().named('afterSessionEnd');
+            emitter.on(Events.STOP_BROWSER, () => bluebirdQ.delay(100).then(afterSessionEnd));
+
+            const pool = mkPool_({emitter});
+            const browser = stubBrowser_();
+
+            return requestBrowser_(browser, pool)
+                .then((browser) => pool.freeBrowser(browser))
+                .then(() => assert.callOrder(afterSessionEnd, browser.quit));
+        });
+
+        it('handler fail should not prevent browser from quit', () => {
+            const emitter = new QEmitter();
+            emitter.on(Events.STOP_BROWSER, () => bluebirdQ.reject());
+
+            const pool = mkPool_({emitter});
+            const browser = stubBrowser_();
+
+            return requestBrowser_(browser, pool)
+                .then((browser) => pool.freeBrowser(browser))
+                .then(() => assert.calledOnce(browser.quit));
+        });
+    });
+
     describe('cancel', () => {
         it('should quit all browsers on cancel', () => {
-            return Promise.all([requestBrowser(stubBrowser('id1')), requestBrowser(stubBrowser('id2'))])
+            const pool = mkPool_();
+
+            return Promise
+                .all([
+                    requestBrowser_(stubBrowser_('id1'), pool),
+                    requestBrowser_(stubBrowser_('id2'), pool)
+                ])
                 .spread((firstBrowser, secondBrowser) => {
                     pool.cancel();
 
@@ -95,7 +197,13 @@ describe('BasicPool', () => {
         });
 
         it('should quit all browser with the same id on cancel', () => {
-            return Promise.all([requestBrowser(stubBrowser('id')), requestBrowser(stubBrowser('id'))])
+            const pool = mkPool_();
+
+            return Promise
+                .all([
+                    requestBrowser_(stubBrowser_('id'), pool),
+                    requestBrowser_(stubBrowser_('id'), pool)
+                ])
                 .spread((firstBrowser, secondBrowser) => {
                     pool.cancel();
 
@@ -105,24 +213,28 @@ describe('BasicPool', () => {
         });
 
         it('should be rejected if getting of a browser was called after cancel', () => {
+            const pool = mkPool_();
+
             pool.cancel();
 
-            return assert.isRejected(requestBrowser(stubBrowser()), CancelledError);
+            return assert.isRejected(requestBrowser_(stubBrowser_(), pool), CancelledError);
         });
 
         it('should quit a browser once if it was launched after cancel', () => {
-            const browser = stubBrowser();
+            const pool = mkPool_();
+            const browser = stubBrowser_();
 
             pool.cancel();
 
-            return requestBrowser(browser)
+            return requestBrowser_(browser, pool)
                 .catch(() => assert.calledOnce(browser.quit));
         });
 
         it('should clean active sessions after cancel', () => {
-            const browser = stubBrowser();
+            const pool = mkPool_();
+            const browser = stubBrowser_();
 
-            return requestBrowser(browser)
+            return requestBrowser_(browser, pool)
                 .then(() => pool.cancel())
                 .then(() => pool.cancel())
                 .then(() => assert.calledOnce(browser.quit));