From dc7265bbb5e92b3b9fd48ce85dbdc0d776772cf5 Mon Sep 17 00:00:00 2001 From: Alexei Date: Tue, 24 Jul 2018 01:35:02 +0200 Subject: [PATCH] fix(browser): ensure browser state is EXECUTING when tests start (#3074) * fix(browser): ensure browser state is EXECUTING when tests start Browser state is EXECUTING when it actually started to execute tests. This state change is triggered by client on actual tests execution start. Introduced an additional browser state CONFIGURING The CONFIGURING state means that the browser is not just CONNECTED for tests, but someone has requested tests execution (and provided a config file). But the provided config file is not yet processed, configuration is not applied or the tests execution is not yet started and we have not received the first event from the remote browser, so the browser object is not yet at EXECUTING state. Refactored browser state names: renamed READY -> CONNECTED Fixes #1640 --- client/updater.js | 2 +- lib/browser.js | 47 +++++++++++----------- lib/browser_collection.js | 11 +----- lib/executor.js | 1 - lib/reporters/base.js | 2 +- test/unit/browser.spec.js | 59 ++++++++++++++++++---------- test/unit/browser_collection.spec.js | 32 ++------------- 7 files changed, 70 insertions(+), 84 deletions(-) diff --git a/client/updater.js b/client/updater.js index b1562e1cf..6c6be933a 100644 --- a/client/updater.js +++ b/client/updater.js @@ -8,7 +8,7 @@ function StatusUpdater (socket, titleElement, bannerElement, browsersElement) { var items = [] var status for (var i = 0; i < browsers.length; i++) { - status = browsers[i].isReady ? 'idle' : 'executing' + status = browsers[i].isConnected ? 'idle' : 'executing' items.push('
  • ' + browsers[i].name + ' is ' + status + '
    • ') } browsersElement.innerHTML = items.join('\n') diff --git a/lib/browser.js b/lib/browser.js index 0d764dfec..1ff636555 100644 --- a/lib/browser.js +++ b/lib/browser.js @@ -4,14 +4,14 @@ const Result = require('./browser_result') const helper = require('./helper') const logger = require('./logger') -// The browser is ready to execute tests. -const READY = 1 +// The browser is connected but not yet been commanded to execute tests. +const CONNECTED = 1 -// The browser is executing the tests. -const EXECUTING = 2 +// The browser has been told to execute tests; it is configuring before tests execution. +const CONFIGURING = 2 -// The browser is not executing, but temporarily disconnected (waiting for reconnecting). -const READY_DISCONNECTED = 3 +// The browser is executing the tests. +const EXECUTING = 3 // The browser is executing the tests, but temporarily disconnect (waiting for reconnecting). const EXECUTING_DISCONNECTED = 4 @@ -24,7 +24,7 @@ class Browser { this.id = id this.fullName = fullName this.name = helper.browserFullNameToShort(fullName) - this.state = READY + this.state = CONNECTED this.lastResult = new Result() this.disconnectsCount = 0 this.activeSockets = [socket] @@ -54,8 +54,8 @@ class Browser { this.emitter.emit('browser_register', this) } - isReady () { - return this.state === READY + isConnected () { + return this.state === CONNECTED } toString () { @@ -76,7 +76,7 @@ class Browser { } onKarmaError (error) { - if (this.isReady()) { + if (this.isConnected()) { return } @@ -87,7 +87,7 @@ class Browser { } onInfo (info) { - if (this.isReady()) { + if (this.isConnected()) { return } @@ -114,6 +114,8 @@ class Browser { this.lastResult = new Result() this.lastResult.total = info.total + this.state = EXECUTING + if (info.total === null) { this.log.warn('Adapter did not report total number of specs.') } @@ -123,11 +125,11 @@ class Browser { } onComplete (result) { - if (this.isReady()) { + if (this.isConnected()) { return } - this.state = READY + this.state = CONNECTED this.lastResult.totalTimeEnd() if (!this.lastResult.success) { @@ -148,9 +150,9 @@ class Browser { return } - if (this.state === READY) { + if (this.state === CONNECTED) { this.disconnect() - } else if (this.state === EXECUTING) { + } else if (this.state === CONFIGURING || this.state === EXECUTING) { this.log.debug('Disconnected during run, waiting %sms for reconnecting.', this.disconnectDelay) this.state = EXECUTING_DISCONNECTED @@ -169,10 +171,10 @@ class Browser { if (this.state === EXECUTING_DISCONNECTED) { this.state = EXECUTING this.log.debug('Reconnected on %s.', newSocket.id) - } else if (this.state === EXECUTING || this.state === READY) { + } else if (this.state === CONNECTED || this.state === CONFIGURING || this.state === EXECUTING) { this.log.debug('New connection %s (already have %s)', newSocket.id, this.getActiveSocketsIds()) } else if (this.state === DISCONNECTED) { - this.state = READY + this.state = CONNECTED this.log.info('Connected on socket %s with id %s', newSocket.id, this.id) this.collection.add(this) @@ -201,7 +203,7 @@ class Browser { } // ignore - probably results from last run (after server disconnecting) - if (this.isReady()) { + if (this.isConnected()) { return } @@ -215,14 +217,15 @@ class Browser { return { id: this.id, name: this.name, - isReady: this.state === READY + isConnected: this.state === CONNECTED } } execute (config) { this.activeSockets.forEach((socket) => socket.emit('execute', config)) - this.state = EXECUTING + this.state = CONFIGURING + this.refreshNoActivityTimeout() } @@ -279,9 +282,9 @@ Browser.factory = function ( return new Browser(id, fullName, collection, emitter, socket, timer, disconnectDelay, noActivityTimeout) } -Browser.STATE_READY = READY +Browser.STATE_CONNECTED = CONNECTED +Browser.STATE_CONFIGURING = CONFIGURING Browser.STATE_EXECUTING = EXECUTING -Browser.STATE_READY_DISCONNECTED = READY_DISCONNECTED Browser.STATE_EXECUTING_DISCONNECTED = EXECUTING_DISCONNECTED Browser.STATE_DISCONNECTED = DISCONNECTED diff --git a/lib/browser_collection.js b/lib/browser_collection.js index fd77fd815..0c624d762 100644 --- a/lib/browser_collection.js +++ b/lib/browser_collection.js @@ -1,6 +1,5 @@ 'use strict' -const EXECUTING = require('./browser').STATE_EXECUTING const Result = require('./browser_result') class BrowserCollection { @@ -31,19 +30,11 @@ class BrowserCollection { return this.browsers.find((browser) => browser.id === browserId) || null } - setAllToExecuting () { - this.browsers.forEach((browser) => { - browser.state = EXECUTING - }) - - this.emitter.emit('browsers_change', this) - } - areAllReady (nonReadyList) { nonReadyList = nonReadyList || [] this.browsers.forEach((browser) => { - if (!browser.isReady()) { + if (!browser.isConnected()) { nonReadyList.push(browser) } }) diff --git a/lib/executor.js b/lib/executor.js index 55694c4b8..189c575fc 100644 --- a/lib/executor.js +++ b/lib/executor.js @@ -31,7 +31,6 @@ class Executor { log.debug('Captured %s browsers', this.capturedBrowsers.length) this.executionScheduled = false this.capturedBrowsers.clearResults() - this.capturedBrowsers.setAllToExecuting() this.pendingCount = this.capturedBrowsers.length this.runningBrowsers = this.capturedBrowsers.clone() this.emitter.emit('run_start', this.runningBrowsers) diff --git a/lib/reporters/base.js b/lib/reporters/base.js index d5227ef4a..e55a8819d 100644 --- a/lib/reporters/base.js +++ b/lib/reporters/base.js @@ -47,7 +47,7 @@ const BaseReporter = function (formatError, reportSlow, useColors, browserConsol msg += util.format(' (skipped %d)', results.skipped) } - if (browser.isReady) { + if (browser.isConnected) { if (results.disconnected) { msg += this.FINISHED_DISCONNECTED } else if (results.error) { diff --git a/test/unit/browser.spec.js b/test/unit/browser.spec.js index f375f67e0..8c670eb97 100644 --- a/test/unit/browser.spec.js +++ b/test/unit/browser.spec.js @@ -94,7 +94,7 @@ describe('Browser', () => { it('should ignore if browser not executing', () => { const spy = sinon.spy() emitter.on('browser_error', spy) - browser.state = Browser.STATE_READY + browser.state = Browser.STATE_CONNECTED browser.onKarmaError() expect(browser.lastResult.error).to.equal(false) @@ -130,7 +130,7 @@ describe('Browser', () => { const spy = sinon.spy() emitter.on('browser_dump', spy) - browser.state = Browser.STATE_READY + browser.state = Browser.STATE_CONNECTED browser.onInfo({dump: 'something'}) browser.onInfo({total: 20}) @@ -144,8 +144,13 @@ describe('Browser', () => { browser = new Browser('fake-id', 'full name', collection, emitter, socket) }) + it('should change state to EXECUTING', () => { + browser.state = Browser.STATE_CONNECTED + browser.onStart({total: 20}) + expect(browser.state).to.equal(Browser.STATE_EXECUTING) + }) + it('should set total count of specs', () => { - browser.state = Browser.STATE_EXECUTING browser.onStart({total: 20}) expect(browser.lastResult.total).to.equal(20) }) @@ -154,7 +159,6 @@ describe('Browser', () => { const spy = sinon.spy() emitter.on('browser_start', spy) - browser.state = Browser.STATE_EXECUTING browser.onStart({total: 20}) expect(spy).to.have.been.calledWith(browser, {total: 20}) @@ -172,10 +176,10 @@ describe('Browser', () => { Date.now.restore() }) - it('should set isReady to true', () => { + it('should set isConnected to true', () => { browser.state = Browser.STATE_EXECUTING browser.onComplete() - expect(browser.isReady()).to.equal(true) + expect(browser.isConnected()).to.equal(true) }) it('should fire "browsers_change" event', () => { @@ -192,7 +196,7 @@ describe('Browser', () => { emitter.on('browsers_change', spy) emitter.on('browser_complete', spy) - browser.state = Browser.STATE_READY + browser.state = Browser.STATE_CONNECTED browser.onComplete() expect(spy).not.to.have.been.called }) @@ -245,7 +249,7 @@ describe('Browser', () => { it('should not complete if browser not executing', () => { const spy = sinon.spy() emitter.on('browser_complete', spy) - browser.state = Browser.STATE_READY + browser.state = Browser.STATE_CONNECTED browser.onDisconnect('socket.io-reason', socket) expect(spy).not.to.have.been.called @@ -294,7 +298,7 @@ describe('Browser', () => { browser.reconnect(mkSocket()) - expect(browser.isReady()).to.equal(true) + expect(browser.isConnected()).to.equal(true) }) }) @@ -328,7 +332,7 @@ describe('Browser', () => { }) it('should ignore if not running', () => { - browser.state = Browser.STATE_READY + browser.state = Browser.STATE_CONNECTED browser.onResult(createSuccessResult()) browser.onResult(createSuccessResult()) browser.onResult(createFailedResult()) @@ -360,27 +364,36 @@ describe('Browser', () => { }) describe('serialize', () => { - it('should return plain object with only name, id, isReady properties', () => { + it('should return plain object with only name, id, isConnected properties', () => { browser = new Browser('fake-id', 'full name', collection, emitter, socket) - browser.state = Browser.STATE_READY + browser.state = Browser.STATE_CONNECTED browser.name = 'Browser 1.0' browser.id = '12345' - expect(browser.serialize()).to.deep.equal({id: '12345', name: 'Browser 1.0', isReady: true}) + expect(browser.serialize()).to.deep.equal({id: '12345', name: 'Browser 1.0', isConnected: true}) }) }) - describe('execute', () => { - it('should emit execute and change state to EXECUTING', () => { + describe('execute and start', () => { + it('should emit execute and change state to CONFIGURING', () => { const spyExecute = sinon.spy() const config = {} browser = new Browser('fake-id', 'full name', collection, emitter, socket) socket.on('execute', spyExecute) browser.execute(config) - expect(browser.isReady()).to.equal(false) + expect(browser.state).to.equal(Browser.STATE_CONFIGURING) expect(spyExecute).to.have.been.calledWith(config) }) + + it('should emit start and change state to EXECUTING', () => { + browser = new Browser('fake-id', 'full name', collection, emitter, socket) + browser.init() // init socket listeners + + expect(browser.state).to.equal(Browser.STATE_CONNECTED) + socket.emit('start', {total: 1}) + expect(browser.state).to.equal(Browser.STATE_EXECUTING) + }) }) describe('scenario:', () => { @@ -391,15 +404,15 @@ describe('Browser', () => { browser.state = Browser.STATE_EXECUTING socket.emit('result', {success: true, suite: [], log: []}) socket.emit('disconnect', 'socket.io reason') - expect(browser.isReady()).to.equal(false) + expect(browser.isConnected()).to.equal(false) const newSocket = mkSocket() browser.reconnect(newSocket) - expect(browser.isReady()).to.equal(false) + expect(browser.isConnected()).to.equal(false) newSocket.emit('result', {success: false, suite: [], log: []}) newSocket.emit('complete') - expect(browser.isReady()).to.equal(true) + expect(browser.isConnected()).to.equal(true) expect(browser.lastResult.success).to.equal(1) expect(browser.lastResult.failed).to.equal(1) }) @@ -427,6 +440,7 @@ describe('Browser', () => { const timer = createMockTimer() browser = new Browser('fake-id', 'Chrome 31.0', collection, emitter, socket, timer, 10) browser.init() + expect(browser.state).to.equal(Browser.STATE_CONNECTED) browser.execute() socket.emit('start', {total: 10}) @@ -443,8 +457,9 @@ describe('Browser', () => { // reconnect on a new socket (which triggers re-execution) browser.reconnect(newSocket) - expect(browser.state).to.equal(Browser.STATE_EXECUTING) + expect(browser.state).to.equal(Browser.STATE_CONFIGURING) newSocket.emit('start', {total: 11}) + expect(browser.state).to.equal(Browser.STATE_EXECUTING) socket.emit('result', {success: true, suite: [], log: []}) // expected cleared last result (should not include the results from previous run) @@ -459,6 +474,8 @@ describe('Browser', () => { // we need to keep the old socket, in the case that the new socket will disconnect. browser = new Browser('fake-id', 'Chrome 31.0', collection, emitter, socket, null, 10) browser.init() + expect(browser.state).to.equal(Browser.STATE_CONNECTED) + browser.execute() // A second connection... @@ -467,6 +484,8 @@ describe('Browser', () => { // Disconnect the second connection... browser.onDisconnect('socket.io-reason', newSocket) + expect(browser.state).to.equal(Browser.STATE_CONFIGURING) + socket.emit('start', {total: 1}) expect(browser.state).to.equal(Browser.STATE_EXECUTING) // It should still be listening on the old socket. diff --git a/test/unit/browser_collection.spec.js b/test/unit/browser_collection.spec.js index 7fb36a45f..417d2ad79 100644 --- a/test/unit/browser_collection.spec.js +++ b/test/unit/browser_collection.spec.js @@ -71,39 +71,13 @@ describe('BrowserCollection', () => { }) }) - describe('setAllToExecuting', () => { - let browsers = null - - beforeEach(() => { - browsers = [new Browser(), new Browser(), new Browser()] - browsers.forEach((browser) => { - collection.add(browser) - }) - }) - - it('should set all browsers state to executing', () => { - collection.setAllToExecuting() - browsers.forEach((browser) => { - expect(browser.isReady()).to.equal(false) - expect(browser.state).to.equal(Browser.STATE_EXECUTING) - }) - }) - - it('should fire "browsers_change" event', () => { - const spy = sinon.spy() - emitter.on('browsers_change', spy) - collection.setAllToExecuting() - expect(spy).to.have.been.called - }) - }) - describe('areAllReady', () => { let browsers = null beforeEach(() => { browsers = [new Browser(), new Browser(), new Browser()] browsers.forEach((browser) => { - browser.state = Browser.STATE_READY + browser.state = Browser.STATE_CONNECTED collection.add(browser) }) }) @@ -135,8 +109,8 @@ describe('BrowserCollection', () => { collection.add(browsers[1]) expect(collection.serialize()).to.deep.equal([ - {id: '1', name: 'B 1.0', isReady: true}, - {id: '2', name: 'B 2.0', isReady: true} + {id: '1', name: 'B 1.0', isConnected: true}, + {id: '2', name: 'B 2.0', isConnected: true} ]) }) })