Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(browser): ensure browser state is EXECUTING when tests start #3074

Merged
merged 3 commits into from
Jul 23, 2018
Merged

fix(browser): ensure browser state is EXECUTING when tests start #3074

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69ab0d7
fix(browser): ensure browser state is EXECUTING when tests start
Jul 5, 2018
88b6a74
fix(browser): introduced an additional browser state CONFIGURING
Jul 23, 2018
65799d3
fix(browser): refactored browser state names
Jul 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions lib/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,20 @@ const logger = require('./logger')
// The browser is ready to execute tests.
const READY = 1

// The browser is configuring before tests execution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a slightly better description would be

// The browser has been told to execute tests; it is configuring before tests execution.

const CONFIGURING = 2

// The browser is executing the tests.
const EXECUTING = 2
const EXECUTING = 3

// The browser is not executing, but temporarily disconnected (waiting for reconnecting).
const READY_DISCONNECTED = 3
const READY_DISCONNECTED = 4

// The browser is executing the tests, but temporarily disconnect (waiting for reconnecting).
const EXECUTING_DISCONNECTED = 4
const EXECUTING_DISCONNECTED = 5

// The browser got permanently disconnected (being removed from the collection and destroyed).
const DISCONNECTED = 5
const DISCONNECTED = 6

class Browser {
constructor (id, fullName, collection, emitter, socket, timer, disconnectDelay, noActivityTimeout) {
Expand Down Expand Up @@ -101,6 +104,8 @@ class Browser {
this.lastResult = new Result()
this.lastResult.total = info.total

this.state = EXECUTING

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this.state in client callbacks responding to events makes sense: we should to this uniformly unless there is some oddity which prevents us. Therefore please try to remove the this.state = EXECUTING in the client command execute(). The execute() command should not set the state; it should be set as you do here, in the start callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that execute() command shouldn't set state to EXECUTING, as actually tests are not executing at that moment and before onStart(). But we should make a difference between READY state of the browser and the state which is after execute() and before onStart(). We want at least the browser's methods onKarmaError() and onInfo()during this state.

I've added additional state called CONFIGURING, which explains the actual state between execute() and onStart(). Is that OK?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! This is better. How about one more improvement? The READY state is IMO misnamed. After this change the states are:

Browser.STATE_READY = READY
Browser.STATE_CONFIGURING = CONFIGURING
Browser.STATE_EXECUTING = EXECUTING
Browser.STATE_READY_DISCONNECTED = READY_DISCONNECTED
Browser.STATE_EXECUTING_DISCONNECTED = EXECUTING_DISCONNECTED
Browser.STATE_DISCONNECTED = DISCONNECTED

But I think the code would be clearer if the initial state was CONNECTED rather than 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

That clarifies the relationship with others states like EXECUTING_DISCONNECTED and avoids ambiguity about what the browser is "ready-for".

This version of the new state would be documented eg:

// The browser is connected but not yet been commanded to execute tests.
  const CONNECTED = 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any strong opinion here about naming.
State READY_DISCONNECTED is actually never used (and sounds confusing with the new states). I'll remove it.

if (info.total === null) {
this.log.warn('Adapter did not report total number of specs.')
}
Expand Down Expand Up @@ -137,7 +142,7 @@ class Browser {

if (this.state === READY) {
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 @@ -156,7 +161,7 @@ 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 === READY || 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
Expand Down Expand Up @@ -209,7 +214,8 @@ class Browser {
execute (config) {
this.activeSockets.forEach((socket) => socket.emit('execute', config))

this.state = EXECUTING
this.state = CONFIGURING

this.refreshNoActivityTimeout()
}

Expand Down Expand Up @@ -267,6 +273,7 @@ Browser.factory = function (
}

Browser.STATE_READY = READY
Browser.STATE_CONFIGURING = CONFIGURING
Browser.STATE_EXECUTING = EXECUTING
Browser.STATE_READY_DISCONNECTED = READY_DISCONNECTED
Browser.STATE_EXECUTING_DISCONNECTED = EXECUTING_DISCONNECTED
Expand Down
9 changes: 0 additions & 9 deletions lib/browser_collection.js
View file
Open in desktop
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,14 +30,6 @@ 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 || []

Expand Down
1 change: 0 additions & 1 deletion lib/executor.js
View file
Open in desktop
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
31 changes: 25 additions & 6 deletions test/unit/browser.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,13 @@ describe('Browser', () => {
browser = new Browser('fake-id', 'full name', collection, emitter, socket)
})

it('should change state to EXECUTING', () => {
browser.state = Browser.STATE_READY
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 @@ -146,7 +151,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 Down Expand Up @@ -362,17 +366,26 @@ describe('Browser', () => {
})
})

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_READY)
socket.emit('start', {total: 1})
expect(browser.state).to.equal(Browser.STATE_EXECUTING)
})
})

describe('scenario:', () => {
Expand Down Expand Up @@ -419,6 +432,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_READY)

browser.execute()
socket.emit('start', {total: 10})
Expand All @@ -435,8 +449,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 @@ -451,6 +466,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_READY)

browser.execute()

// A second connection...
Expand All @@ -459,6 +476,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
26 changes: 0 additions & 26 deletions test/unit/browser_collection.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,32 +71,6 @@ 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

Expand Down