Skip to content

Commit

Permalink
fix: restarted browsers not running tests (#3233)
Browse files Browse the repository at this point in the history
* fix: restarted browsers not running tests

Currently whenever a browser disconnects completely (no socket.io connection loss), the launcher is instructed to "restart" the browser. Whenever the restarted browser now tries to "register" again, Karma considers the browser instance to be still executing and doesn't do anything about it (except setting the state to `EXECUTING` again).

This means that the browser is in the state of executing, but
practically it does nothing just waits. Resulting another disconnect
(repeat here).

* test: add unit test that covers disconnected restarted browsers

* fixup! test: add unit test that covers disconnected restarted browsers

Address feedback

* fixup! test: add unit test that covers disconnected restarted browsers

Improve comments & log messages
  • Loading branch information
devversion authored and johnjbarton committed Dec 17, 2018
1 parent 584dddc commit cc2eff2
Show file tree
Hide file tree
Showing 7 changed files with 137 additions and 29 deletions.
16 changes: 14 additions & 2 deletions client/karma.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ function Karma (socket, iframe, opener, navigator, location) {
var resultsBufferLimit = 50
var resultsBuffer = []

// This variable will be set to "true" whenever the socket lost connection and was able to
// reconnect to the Karma server. This will be passed to the Karma server then, so that
// Karma can differentiate between a socket client reconnect and a full browser reconnect.
var socketReconnect = false

this.VERSION = constant.VERSION
this.config = {}

Expand Down Expand Up @@ -227,20 +232,27 @@ function Karma (socket, iframe, opener, navigator, location) {
this.complete()
}.bind(this))

// report browser name, id
// Report the browser name and Id. Note that this event can also fire if the connection has
// been temporarily lost, but the socket reconnected automatically. Read more in the docs:
// https://socket.io/docs/client-api/#Event-%E2%80%98connect%E2%80%99
socket.on('connect', function () {
socket.io.engine.on('upgrade', function () {
resultsBufferLimit = 1
})
var info = {
name: navigator.userAgent,
id: browserId
id: browserId,
isSocketReconnect: socketReconnect
}
if (displayName) {
info.displayName = displayName
}
socket.emit('register', info)
})

socket.on('reconnect', function () {
socketReconnect = true
})
}

module.exports = Karma
18 changes: 12 additions & 6 deletions lib/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ const logger = require('./logger')
const CONNECTED = 'CONNECTED' // The browser is connected but not yet been commanded to execute tests.
const CONFIGURING = 'CONFIGURING' // The browser has been told to execute tests; it is configuring before tests execution.
const EXECUTING = 'EXECUTING' // The browser is executing the tests.
const EXECUTING_DISCONNECTED = 'EXECUTING_DISCONNECTED' // The browser is executing the tests, but temporarily disconnect (waiting for reconnecting).
const DISCONNECTED = 'DISCONNECTED' // The browser got permanently disconnected (being removed from the collection and destroyed).
const EXECUTING_DISCONNECTED = 'EXECUTING_DISCONNECTED' // The browser is executing the tests, but temporarily disconnect (waiting for socket reconnecting).
const DISCONNECTED = 'DISCONNECTED' // The browser got completely disconnected (e.g. browser crash) and can be only restored with a restart of execution.

class Browser {
constructor (id, fullName, collection, emitter, socket, timer, disconnectDelay, noActivityTimeout) {
Expand Down Expand Up @@ -121,15 +121,21 @@ class Browser {

reconnect (newSocket) {
if (this.state === EXECUTING_DISCONNECTED) {
this.log.debug(`Reconnected on ${newSocket.id}.`)
this.log.debug(`Lost socket connection, but browser continued to execute. Reconnected ` +
`on socket ${newSocket.id}.`)
this.setState(EXECUTING)
} else if ([CONNECTED, CONFIGURING, EXECUTING].includes(this.state)) {
this.log.debug(`New connection ${newSocket.id} (already have ${this.getActiveSocketsIds()})`)
this.log.debug(`Rebinding to new socket ${newSocket.id} (already have ` +
`${this.getActiveSocketsIds()})`)
} else if (this.state === DISCONNECTED) {
this.log.info(`Connected on socket ${newSocket.id} with id ${this.id}`)
this.log.info(`Disconnected browser returned on socket ${newSocket.id} with id ${this.id}.`)
this.setState(CONNECTED)

this.collection.add(this)
// Since the disconnected browser is already part of the collection and we want to
// make sure that the server can properly handle the browser like it's the first time
// connecting this browser (as we want a complete new execution), we need to emit the
// following events:
this.emitter.emit('browsers_change', this.collection)
this.emitter.emit('browser_register', this)
}

Expand Down
14 changes: 12 additions & 2 deletions lib/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,20 @@ class Server extends KarmaEventEmitter {
let newBrowser = info.id ? (capturedBrowsers.getById(info.id) || singleRunBrowsers.getById(info.id)) : null

if (newBrowser) {
// By default if a browser disconnects while still executing, we assume that the test
// execution still continues because just the socket connection has been terminated. Now
// since we know whether this is just a socket reconnect or full client reconnect, we
// need to update the browser state accordingly. This is necessary because in case a
// browser crashed and has been restarted, we need to start with a fresh execution.
if (!info.isSocketReconnect) {
newBrowser.setState(Browser.STATE_DISCONNECTED)
}

newBrowser.reconnect(socket)

// We are restarting a previously disconnected browser.
if (newBrowser.state === Browser.STATE_DISCONNECTED && config.singleRun) {
// Since not every reconnected browser is able to continue with its previous execution,
// we need to start a new execution in case a browser has restarted and is now idling.
if (newBrowser.state === Browser.STATE_CONNECTED && config.singleRun) {
newBrowser.execute(config.client)
}
} else {
Expand Down
9 changes: 9 additions & 0 deletions test/client/karma.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,15 @@ describe('Karma', function () {
assert(spyInfo.called)
})

it('should mark "register" event for reconnected socket', function () {
socket.on('register', sinon.spy(function (info) {
assert(info.isSocketReconnect === true)
}))

socket.emit('reconnect')
socket.emit('connect')
})

it('should report browser id', function () {
windowLocation.search = '?id=567'
socket = new MockSocket()
Expand Down
13 changes: 13 additions & 0 deletions test/unit/browser.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,19 @@ describe('Browser', () => {

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

it('should not add a disconnected browser to the collection multiple times', () => {
browser = new Browser('id', 'Chrome 25.0', collection, emitter, socket, null, 10)
browser.init()

expect(collection.length).to.equal(1)

browser.state = Browser.STATE_DISCONNECTED

browser.reconnect(mkSocket())

expect(collection.length).to.equal(1)
})
})

describe('onResult', () => {
Expand Down
52 changes: 51 additions & 1 deletion test/unit/server.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const Server = require('../../lib/server')
const BundleUtils = require('../../lib/utils/bundle-utils')
const NetUtils = require('../../lib/utils/net-utils')
const BrowserCollection = require('../../lib/browser_collection')
const Browser = require('../../lib/browser')

describe('server', () => {
let mockConfig
Expand All @@ -10,13 +11,15 @@ describe('server', () => {
let fileListOnReject
let mockLauncher
let mockWebServer
let mockServerSocket
let mockSocketServer
let mockBoundServer
let mockExecutor
let doneSpy
let server = mockConfig = browserCollection = webServerOnError = null
let fileListOnResolve = fileListOnReject = mockLauncher = null
let mockFileList = mockWebServer = mockSocketServer = mockExecutor = doneSpy = null
let mockSocketEventListeners = new Map()

// Use regular function not arrow so 'this' is mocha 'this'.
beforeEach(function () {
Expand Down Expand Up @@ -67,14 +70,21 @@ describe('server', () => {
kill: () => true
}

mockServerSocket = {
id: 'socket-id',
on: (name, handler) => mockSocketEventListeners.set(name, handler),
emit: () => {},
removeListener: () => {}
}

mockSocketServer = {
close: () => {},
flashPolicyServer: {
close: () => {}
},
sockets: {
sockets: {},
on: () => {},
on: (name, handler) => handler(mockServerSocket),
emit: () => {},
removeAllListeners: () => {}
}
Expand Down Expand Up @@ -277,4 +287,44 @@ describe('server', () => {
}
})
})

describe('reconnecting browser', () => {
let mockBrowserSocket

beforeEach(() => {
browserCollection = new BrowserCollection(server)
server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)

mockBrowserSocket = {
id: 'browser-socket-id',
on: () => {},
emit: () => {}
}
})

it('should re-configure disconnected browser which has been restarted', () => {
const testBrowserId = 'my-id'
const browser = new Browser(testBrowserId, 'Chrome 19.0', browserCollection, server,
mockBrowserSocket, null, 0)
const registerFn = mockSocketEventListeners.get('register')

browser.init()
browserCollection.add(browser)

// We assume that our browser was running when it disconnected randomly.
browser.setState(Browser.STATE_EXECUTING_DISCONNECTED)

// We now simulate a "connect" event from the Karma client where it registers
// a previous browser that disconnected while executing. Usually if it was just a
// socket.io reconnect, we would not want to restart the execution, but since this is
// a complete reconnect, we want to configure the browser and start a new execution.
registerFn({
name: 'fake-name',
id: testBrowserId,
isSocketReconnect: false
})

expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
})
})
})
44 changes: 26 additions & 18 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2795,6 +2795,11 @@ flat-cache@^1.2.1:
graceful-fs "^4.1.2"
write "^0.2.1"

flatted@^2.0.0:
version "2.0.0"
resolved "https://registry.yarnpkg.com/flatted/-/flatted-2.0.0.tgz#55122b6536ea496b4b44893ee2608141d10d9916"
integrity sha512-R+H8IZclI8AAkSBRQJLVOsxwAoHd6WC40b4QTNWIjzAa6BXOBfQcM587MXDTVPeYaopFNWHUFLx7eNmHDSxMWg==

for-in@^1.0.1, for-in@^1.0.2:
version "1.0.2"
resolved "https://registry.yarnpkg.com/for-in/-/for-in-1.0.2.tgz#81068d295a8142ec0ac726c6e2200c30fb6d5e80"
Expand Down Expand Up @@ -4216,11 +4221,6 @@ json-stringify-safe@^5.0.1, json-stringify-safe@~5.0.1:
resolved "https://registry.yarnpkg.com/json-stringify-safe/-/json-stringify-safe-5.0.1.tgz#1296a2d58fd45f19a0f6ce01d65701e2c735b6eb"
integrity sha1-Epai1Y/UXxmg9s4B1lcB4sc1tus=

json3@^3.3.2:
version "3.3.2"
resolved "https://registry.yarnpkg.com/json3/-/json3-3.3.2.tgz#3c0434743df93e2f5c42aee7b19bcb483575f4e1"
integrity sha1-PAQ0dD35Pi9cQq7nsZvLSDV19OE=

jsonfile@^3.0.0:
version "3.0.1"
resolved "https://registry.yarnpkg.com/jsonfile/-/jsonfile-3.0.1.tgz#a5ecc6f65f53f662c4415c7675a0331d0992ec66"
Expand Down Expand Up @@ -4583,6 +4583,11 @@ lodash@^4.0.0, lodash@^4.14.0, lodash@^4.16.6, lodash@^4.17.2, lodash@^4.17.4, l
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.4.tgz#78203a4d1c328ae1d86dca6460e369b57f4055ae"
integrity sha1-eCA6TRwyiuHYbcpkYONptX9AVa4=

lodash@^4.17.5:
version "4.17.11"
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.11.tgz#b39ea6229ef607ecd89e2c8df12536891cac9b8d"
integrity sha512-cQKh8igo5QUhZ7lg38DYWAxMvjSAKG0A8wGSVimP07SIUEK2UO+arSRKbRZWtelMtN5V0Hkwh5ryOto/SshYIg==

lodash@~4.3.0:
version "4.3.0"
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.3.0.tgz#efd9c4a6ec53f3b05412429915c3e4824e4d25a4"
Expand Down Expand Up @@ -4641,10 +4646,13 @@ lower-case@^1.1.1:
resolved "https://registry.yarnpkg.com/lower-case/-/lower-case-1.1.4.tgz#9a2cabd1b9e8e0ae993a4bf7d5875c39c42e8eac"
integrity sha1-miyr0bno4K6ZOkv31YdcOcQujqw=

[email protected]:
version "2.2.4"
resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-2.2.4.tgz#6c658619becf14031d0d0b594b16042ce4dc063d"
integrity sha1-bGWGGb7PFAMdDQtZSxYELOTcBj0=
[email protected]:
version "4.1.5"
resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-4.1.5.tgz#8bbe50ea85bed59bc9e33dcab8235ee9bcf443cd"
integrity sha512-sWZlbEP2OsHNkXrMl5GYk/jKk70MBng6UU4YI/qGDYbgf6YbP4EvmqISbXCoJiRKs+1bSpFHVgQxvJ17F2li5g==
dependencies:
pseudomap "^1.0.2"
yallist "^2.1.2"

lru-cache@^4.0.1:
version "4.1.1"
Expand Down Expand Up @@ -6277,10 +6285,10 @@ signal-exit@^3.0.0, signal-exit@^3.0.2:
resolved "https://registry.yarnpkg.com/signal-exit/-/signal-exit-3.0.2.tgz#b5fdc08f1287ea1178628e415e25132b73646c6d"
integrity sha1-tf3AjxKH6hF4Yo5BXiUTK3NkbG0=

sinon-chai@^2.7.0:
version "2.14.0"
resolved "https://registry.yarnpkg.com/sinon-chai/-/sinon-chai-2.14.0.tgz#da7dd4cc83cd6a260b67cca0f7a9fdae26a1205d"
integrity sha512-9stIF1utB0ywNHNT7RgiXbdmen8QDCRsrTjw+G9TgKt1Yexjiv8TOWZ6WHsTPz57Yky3DIswZvEqX8fpuHNDtQ==
sinon-chai@^3.0.0:
version "3.3.0"
resolved "https://registry.yarnpkg.com/sinon-chai/-/sinon-chai-3.3.0.tgz#8084ff99451064910fbe2c2cb8ab540c00b740ea"
integrity sha512-r2JhDY7gbbmh5z3Q62pNbrjxZdOAjpsqW/8yxAZRSqLZqowmfGZPGUZPFf3UX36NLis0cv8VEM5IJh9HgkSOAA==

sinon@^6.1.5:
version "6.3.5"
Expand Down Expand Up @@ -7149,12 +7157,12 @@ use@^3.1.0:
dependencies:
kind-of "^6.0.2"

useragent@2.2.1:
version "2.2.1"
resolved "https://registry.yarnpkg.com/useragent/-/useragent-2.2.1.tgz#cf593ef4f2d175875e8bb658ea92e18a4fd06d8e"
integrity sha1-z1k+9PLRdYdei7ZY6pLhik/QbY4=
useragent@2.3.0:
version "2.3.0"
resolved "https://registry.yarnpkg.com/useragent/-/useragent-2.3.0.tgz#217f943ad540cb2128658ab23fc960f6a88c9972"
integrity sha512-4AoH4pxuSvHCjqLO04sU6U/uE65BYza8l/KKBS0b0hnUPWi+cQ2BpeTEwejCSx9SPV5/U03nniDTrWx5NrmKdw==
dependencies:
lru-cache "2.2.x"
lru-cache "4.1.x"
tmp "0.0.x"

util-arity@^1.0.2:
Expand Down

0 comments on commit cc2eff2

Please sign in to comment.