Skip to content

Commit cc2eff2

Browse files
devversionjohnjbarton
devversion
authored and
johnjbarton
committed
fix: restarted browsers not running tests (#3233)
* 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
1 parent 584dddc commit cc2eff2

File tree

7 files changed

+137
-29
lines changed

7 files changed

+137
-29
lines changed

‎client/karma.js‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,11 @@ function Karma (socket, iframe, opener, navigator, location) {
1414
var resultsBufferLimit = 50
1515
var resultsBuffer = []
1616

17+
// This variable will be set to "true" whenever the socket lost connection and was able to
18+
// reconnect to the Karma server. This will be passed to the Karma server then, so that
19+
// Karma can differentiate between a socket client reconnect and a full browser reconnect.
20+
var socketReconnect = false
21+
1722
this.VERSION = constant.VERSION
1823
this.config = {}
1924

@@ -227,20 +232,27 @@ function Karma (socket, iframe, opener, navigator, location) {
227232
this.complete()
228233
}.bind(this))
229234

230-
// report browser name, id
235+
// Report the browser name and Id. Note that this event can also fire if the connection has
236+
// been temporarily lost, but the socket reconnected automatically. Read more in the docs:
237+
// https://socket.io/docs/client-api/#Event-%E2%80%98connect%E2%80%99
231238
socket.on('connect', function () {
232239
socket.io.engine.on('upgrade', function () {
233240
resultsBufferLimit = 1
234241
})
235242
var info = {
236243
name: navigator.userAgent,
237-
id: browserId
244+
id: browserId,
245+
isSocketReconnect: socketReconnect
238246
}
239247
if (displayName) {
240248
info.displayName = displayName
241249
}
242250
socket.emit('register', info)
243251
})
252+
253+
socket.on('reconnect', function () {
254+
socketReconnect = true
255+
})
244256
}
245257

246258
module.exports = Karma

‎lib/browser.js‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ const logger = require('./logger')
77
const CONNECTED = 'CONNECTED' // The browser is connected but not yet been commanded to execute tests.
88
const CONFIGURING = 'CONFIGURING' // The browser has been told to execute tests; it is configuring before tests execution.
99
const EXECUTING = 'EXECUTING' // The browser is executing the tests.
10-
const EXECUTING_DISCONNECTED = 'EXECUTING_DISCONNECTED' // The browser is executing the tests, but temporarily disconnect (waiting for reconnecting).
11-
const DISCONNECTED = 'DISCONNECTED' // The browser got permanently disconnected (being removed from the collection and destroyed).
10+
const EXECUTING_DISCONNECTED = 'EXECUTING_DISCONNECTED' // The browser is executing the tests, but temporarily disconnect (waiting for socket reconnecting).
11+
const DISCONNECTED = 'DISCONNECTED' // The browser got completely disconnected (e.g. browser crash) and can be only restored with a restart of execution.
1212

1313
class Browser {
1414
constructor (id, fullName, collection, emitter, socket, timer, disconnectDelay, noActivityTimeout) {
@@ -121,15 +121,21 @@ class Browser {
121121

122122
reconnect (newSocket) {
123123
if (this.state === EXECUTING_DISCONNECTED) {
124-
this.log.debug(`Reconnected on ${newSocket.id}.`)
124+
this.log.debug(`Lost socket connection, but browser continued to execute. Reconnected ` +
125+
`on socket ${newSocket.id}.`)
125126
this.setState(EXECUTING)
126127
} else if ([CONNECTED, CONFIGURING, EXECUTING].includes(this.state)) {
127-
this.log.debug(`New connection ${newSocket.id} (already have ${this.getActiveSocketsIds()})`)
128+
this.log.debug(`Rebinding to new socket ${newSocket.id} (already have ` +
129+
`${this.getActiveSocketsIds()})`)
128130
} else if (this.state === DISCONNECTED) {
129-
this.log.info(`Connected on socket ${newSocket.id} with id ${this.id}`)
131+
this.log.info(`Disconnected browser returned on socket ${newSocket.id} with id ${this.id}.`)
130132
this.setState(CONNECTED)
131133

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

‎lib/server.js‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,10 +232,20 @@ class Server extends KarmaEventEmitter {
232232
let newBrowser = info.id ? (capturedBrowsers.getById(info.id) || singleRunBrowsers.getById(info.id)) : null
233233

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

237-
// We are restarting a previously disconnected browser.
238-
if (newBrowser.state === Browser.STATE_DISCONNECTED && config.singleRun) {
246+
// Since not every reconnected browser is able to continue with its previous execution,
247+
// we need to start a new execution in case a browser has restarted and is now idling.
248+
if (newBrowser.state === Browser.STATE_CONNECTED && config.singleRun) {
239249
newBrowser.execute(config.client)
240250
}
241251
} else {

‎test/client/karma.spec.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,15 @@ describe('Karma', function () {
147147
assert(spyInfo.called)
148148
})
149149

150+
it('should mark "register" event for reconnected socket', function () {
151+
socket.on('register', sinon.spy(function (info) {
152+
assert(info.isSocketReconnect === true)
153+
}))
154+
155+
socket.emit('reconnect')
156+
socket.emit('connect')
157+
})
158+
150159
it('should report browser id', function () {
151160
windowLocation.search = '?id=567'
152161
socket = new MockSocket()

‎test/unit/browser.spec.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,19 @@ describe('Browser', () => {
297297

298298
expect(browser.isConnected()).to.equal(true)
299299
})
300+
301+
it('should not add a disconnected browser to the collection multiple times', () => {
302+
browser = new Browser('id', 'Chrome 25.0', collection, emitter, socket, null, 10)
303+
browser.init()
304+
305+
expect(collection.length).to.equal(1)
306+
307+
browser.state = Browser.STATE_DISCONNECTED
308+
309+
browser.reconnect(mkSocket())
310+
311+
expect(collection.length).to.equal(1)
312+
})
300313
})
301314

302315
describe('onResult', () => {

‎test/unit/server.spec.js‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ const Server = require('../../lib/server')
22
const BundleUtils = require('../../lib/utils/bundle-utils')
33
const NetUtils = require('../../lib/utils/net-utils')
44
const BrowserCollection = require('../../lib/browser_collection')
5+
const Browser = require('../../lib/browser')
56

67
describe('server', () => {
78
let mockConfig
@@ -10,13 +11,15 @@ describe('server', () => {
1011
let fileListOnReject
1112
let mockLauncher
1213
let mockWebServer
14+
let mockServerSocket
1315
let mockSocketServer
1416
let mockBoundServer
1517
let mockExecutor
1618
let doneSpy
1719
let server = mockConfig = browserCollection = webServerOnError = null
1820
let fileListOnResolve = fileListOnReject = mockLauncher = null
1921
let mockFileList = mockWebServer = mockSocketServer = mockExecutor = doneSpy = null
22+
let mockSocketEventListeners = new Map()
2023

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

73+
mockServerSocket = {
74+
id: 'socket-id',
75+
on: (name, handler) => mockSocketEventListeners.set(name, handler),
76+
emit: () => {},
77+
removeListener: () => {}
78+
}
79+
7080
mockSocketServer = {
7181
close: () => {},
7282
flashPolicyServer: {
7383
close: () => {}
7484
},
7585
sockets: {
7686
sockets: {},
77-
on: () => {},
87+
on: (name, handler) => handler(mockServerSocket),
7888
emit: () => {},
7989
removeAllListeners: () => {}
8090
}
@@ -277,4 +287,44 @@ describe('server', () => {
277287
}
278288
})
279289
})
290+
291+
describe('reconnecting browser', () => {
292+
let mockBrowserSocket
293+
294+
beforeEach(() => {
295+
browserCollection = new BrowserCollection(server)
296+
server._start(mockConfig, mockLauncher, null, mockFileList, browserCollection, mockExecutor, doneSpy)
297+
298+
mockBrowserSocket = {
299+
id: 'browser-socket-id',
300+
on: () => {},
301+
emit: () => {}
302+
}
303+
})
304+
305+
it('should re-configure disconnected browser which has been restarted', () => {
306+
const testBrowserId = 'my-id'
307+
const browser = new Browser(testBrowserId, 'Chrome 19.0', browserCollection, server,
308+
mockBrowserSocket, null, 0)
309+
const registerFn = mockSocketEventListeners.get('register')
310+
311+
browser.init()
312+
browserCollection.add(browser)
313+
314+
// We assume that our browser was running when it disconnected randomly.
315+
browser.setState(Browser.STATE_EXECUTING_DISCONNECTED)
316+
317+
// We now simulate a "connect" event from the Karma client where it registers
318+
// a previous browser that disconnected while executing. Usually if it was just a
319+
// socket.io reconnect, we would not want to restart the execution, but since this is
320+
// a complete reconnect, we want to configure the browser and start a new execution.
321+
registerFn({
322+
name: 'fake-name',
323+
id: testBrowserId,
324+
isSocketReconnect: false
325+
})
326+
327+
expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
328+
})
329+
})
280330
})

‎yarn.lock‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2795,6 +2795,11 @@ flat-cache@^1.2.1:
27952795
graceful-fs "^4.1.2"
27962796
write "^0.2.1"
27972797

2798+
flatted@^2.0.0:
2799+
version "2.0.0"
2800+
resolved "https://registry.yarnpkg.com/flatted/-/flatted-2.0.0.tgz#55122b6536ea496b4b44893ee2608141d10d9916"
2801+
integrity sha512-R+H8IZclI8AAkSBRQJLVOsxwAoHd6WC40b4QTNWIjzAa6BXOBfQcM587MXDTVPeYaopFNWHUFLx7eNmHDSxMWg==
2802+
27982803
for-in@^1.0.1, for-in@^1.0.2:
27992804
version "1.0.2"
28002805
resolved "https://registry.yarnpkg.com/for-in/-/for-in-1.0.2.tgz#81068d295a8142ec0ac726c6e2200c30fb6d5e80"
@@ -4216,11 +4221,6 @@ json-stringify-safe@^5.0.1, json-stringify-safe@~5.0.1:
42164221
resolved "https://registry.yarnpkg.com/json-stringify-safe/-/json-stringify-safe-5.0.1.tgz#1296a2d58fd45f19a0f6ce01d65701e2c735b6eb"
42174222
integrity sha1-Epai1Y/UXxmg9s4B1lcB4sc1tus=
42184223

4219-
json3@^3.3.2:
4220-
version "3.3.2"
4221-
resolved "https://registry.yarnpkg.com/json3/-/json3-3.3.2.tgz#3c0434743df93e2f5c42aee7b19bcb483575f4e1"
4222-
integrity sha1-PAQ0dD35Pi9cQq7nsZvLSDV19OE=
4223-
42244224
jsonfile@^3.0.0:
42254225
version "3.0.1"
42264226
resolved "https://registry.yarnpkg.com/jsonfile/-/jsonfile-3.0.1.tgz#a5ecc6f65f53f662c4415c7675a0331d0992ec66"
@@ -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
45834583
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.4.tgz#78203a4d1c328ae1d86dca6460e369b57f4055ae"
45844584
integrity sha1-eCA6TRwyiuHYbcpkYONptX9AVa4=
45854585

4586+
lodash@^4.17.5:
4587+
version "4.17.11"
4588+
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.11.tgz#b39ea6229ef607ecd89e2c8df12536891cac9b8d"
4589+
integrity sha512-cQKh8igo5QUhZ7lg38DYWAxMvjSAKG0A8wGSVimP07SIUEK2UO+arSRKbRZWtelMtN5V0Hkwh5ryOto/SshYIg==
4590+
45864591
lodash@~4.3.0:
45874592
version "4.3.0"
45884593
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.3.0.tgz#efd9c4a6ec53f3b05412429915c3e4824e4d25a4"
@@ -4641,10 +4646,13 @@ lower-case@^1.1.1:
46414646
resolved "https://registry.yarnpkg.com/lower-case/-/lower-case-1.1.4.tgz#9a2cabd1b9e8e0ae993a4bf7d5875c39c42e8eac"
46424647
integrity sha1-miyr0bno4K6ZOkv31YdcOcQujqw=
46434648

4644-
lru-cache@2.2.x:
4645-
version "2.2.4"
4646-
resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-2.2.4.tgz#6c658619becf14031d0d0b594b16042ce4dc063d"
4647-
integrity sha1-bGWGGb7PFAMdDQtZSxYELOTcBj0=
4649+
lru-cache@4.1.x:
4650+
version "4.1.5"
4651+
resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-4.1.5.tgz#8bbe50ea85bed59bc9e33dcab8235ee9bcf443cd"
4652+
integrity sha512-sWZlbEP2OsHNkXrMl5GYk/jKk70MBng6UU4YI/qGDYbgf6YbP4EvmqISbXCoJiRKs+1bSpFHVgQxvJ17F2li5g==
4653+
dependencies:
4654+
pseudomap "^1.0.2"
4655+
yallist "^2.1.2"
46484656

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

6280-
sinon-chai@^2.7.0:
6281-
version "2.14.0"
6282-
resolved "https://registry.yarnpkg.com/sinon-chai/-/sinon-chai-2.14.0.tgz#da7dd4cc83cd6a260b67cca0f7a9fdae26a1205d"
6283-
integrity sha512-9stIF1utB0ywNHNT7RgiXbdmen8QDCRsrTjw+G9TgKt1Yexjiv8TOWZ6WHsTPz57Yky3DIswZvEqX8fpuHNDtQ==
6288+
sinon-chai@^3.0.0:
6289+
version "3.3.0"
6290+
resolved "https://registry.yarnpkg.com/sinon-chai/-/sinon-chai-3.3.0.tgz#8084ff99451064910fbe2c2cb8ab540c00b740ea"
6291+
integrity sha512-r2JhDY7gbbmh5z3Q62pNbrjxZdOAjpsqW/8yxAZRSqLZqowmfGZPGUZPFf3UX36NLis0cv8VEM5IJh9HgkSOAA==
62846292

62856293
sinon@^6.1.5:
62866294
version "6.3.5"
@@ -7149,12 +7157,12 @@ use@^3.1.0:
71497157
dependencies:
71507158
kind-of "^6.0.2"
71517159

7152-
useragent@2.2.1:
7153-
version "2.2.1"
7154-
resolved "https://registry.yarnpkg.com/useragent/-/useragent-2.2.1.tgz#cf593ef4f2d175875e8bb658ea92e18a4fd06d8e"
7155-
integrity sha1-z1k+9PLRdYdei7ZY6pLhik/QbY4=
7160+
useragent@2.3.0:
7161+
version "2.3.0"
7162+
resolved "https://registry.yarnpkg.com/useragent/-/useragent-2.3.0.tgz#217f943ad540cb2128658ab23fc960f6a88c9972"
7163+
integrity sha512-4AoH4pxuSvHCjqLO04sU6U/uE65BYza8l/KKBS0b0hnUPWi+cQ2BpeTEwejCSx9SPV5/U03nniDTrWx5NrmKdw==
71567164
dependencies:
7157-
lru-cache "2.2.x"
7165+
lru-cache "4.1.x"
71587166
tmp "0.0.x"
71597167

71607168
util-arity@^1.0.2:

0 commit comments

Comments
 (0)