Skip to content

Commit

Permalink
fix(browser): ensure browser state is EXECUTING when tests start (#3074)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
Alexei authored and johnjbarton committed Jul 23, 2018
1 parent 7617279 commit dc7265b
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 84 deletions.
2 changes: 1 addition & 1 deletion client/updater.js
Original file line number Diff line number Diff line change
Expand Up @@ -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('<li class="' + status + '">' + browsers[i].name + ' is ' + status + '</li>')
}
browsersElement.innerHTML = items.join('\n')
Expand Down
47 changes: 25 additions & 22 deletions lib/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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]
Expand Down Expand Up @@ -54,8 +54,8 @@ class Browser {
this.emitter.emit('browser_register', this)
}

isReady () {
return this.state === READY
isConnected () {
return this.state === CONNECTED
}

toString () {
Expand All @@ -76,7 +76,7 @@ class Browser {
}

onKarmaError (error) {
if (this.isReady()) {
if (this.isConnected()) {
return
}

Expand All @@ -87,7 +87,7 @@ class Browser {
}

onInfo (info) {
if (this.isReady()) {
if (this.isConnected()) {
return
}

Expand All @@ -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.')
}
Expand All @@ -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) {
Expand All @@ -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

Expand All @@ -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)

Expand Down Expand Up @@ -201,7 +203,7 @@ class Browser {
}

// ignore - probably results from last run (after server disconnecting)
if (this.isReady()) {
if (this.isConnected()) {
return
}

Expand All @@ -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()
}

Expand Down Expand Up @@ -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

Expand Down
11 changes: 1 addition & 10 deletions lib/browser_collection.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict'

const EXECUTING = require('./browser').STATE_EXECUTING
const Result = require('./browser_result')

class BrowserCollection {
Expand Down Expand Up @@ -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)
}
})
Expand Down
1 change: 0 additions & 1 deletion lib/executor.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion lib/reporters/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
59 changes: 39 additions & 20 deletions test/unit/browser.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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})

Expand All @@ -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)
})
Expand All @@ -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})
Expand All @@ -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', () => {
Expand All @@ -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
})
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -294,7 +298,7 @@ describe('Browser', () => {

browser.reconnect(mkSocket())

expect(browser.isReady()).to.equal(true)
expect(browser.isConnected()).to.equal(true)
})
})

Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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:', () => {
Expand All @@ -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)
})
Expand Down Expand Up @@ -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})
Expand All @@ -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)
Expand All @@ -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...
Expand All @@ -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.
Expand Down
Loading

0 comments on commit dc7265b

Please sign in to comment.