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

@@ -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')

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
 

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)
       }
     })

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)

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) {

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.