Skip to content

Commit 1c9c2de

Browse files
johnjbartonjohnjbarton
authoredDec 23, 2020
fix(test): mark all second connections reconnects (#3598)
The reconnecting test is flakey, sometimes running the tests twice. This is exactly the behavior we are trying to prevent. Hard to reproduce. Refactoring: * rename newBrowser to knownBrowser as appropriate. * move browser STATE constant set/check into browser module. Browser should be the only place the state is set. * pass singleRun and clientConfig into Browser * pass isSocketReconnect into browser.reconnect() * rename browser.onDisconnect to browser.onSocketDisconnect to distinguish the socket from the reload cases.
·
v6.4.4v6.0.0
1 parent 87f7e5e commit 1c9c2de

File tree

9 files changed

+92
-90
lines changed

9 files changed

+92
-90
lines changed
 

‎client/karma.js

+5-6
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,11 @@ function Karma (socket, iframe, opener, navigator, location, document) {
4141
}
4242
}
4343

44-
// This variable will be set to "true" whenever the socket lost connection and was able to
45-
// reconnect to the Karma server. This will be passed to the Karma server then, so that
46-
// Karma can differentiate between a socket client reconnect and a full browser reconnect.
44+
// To start we will signal the server that we are not reconnecting. If the socket loses
45+
// connection and was able to reconnect to the Karma server we will get a
46+
// second 'connect' event. There we will pass 'true' and that will be passed to the
47+
// Karma server then, so that Karma can differentiate between a socket client
48+
// econnect and a full browser reconnect.
4749
var socketReconnect = false
4850

4951
this.VERSION = constant.VERSION
@@ -299,9 +301,6 @@ function Karma (socket, iframe, opener, navigator, location, document) {
299301
info.displayName = displayName
300302
}
301303
socket.emit('register', info)
302-
})
303-
304-
socket.on('reconnect', function () {
305304
socketReconnect = true
306305
})
307306
}

‎lib/browser.js

+35-24
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,17 @@ const EXECUTING_DISCONNECTED = 'EXECUTING_DISCONNECTED' // The browser is execut
1111
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 {
14-
constructor (id, fullName, collection, emitter, socket, timer, disconnectDelay, noActivityTimeout) {
14+
constructor (id, fullName, collection, emitter, socket, timer, disconnectDelay,
15+
noActivityTimeout, singleRun, clientConfig) {
1516
this.id = id
1617
this.fullName = fullName
1718
this.name = helper.browserFullNameToShort(fullName)
1819
this.lastResult = new BrowserResult()
1920
this.disconnectsCount = 0
2021
this.activeSockets = [socket]
2122
this.noActivityTimeout = noActivityTimeout
23+
this.singleRun = singleRun
24+
this.clientConfig = clientConfig
2225
this.collection = collection
2326
this.emitter = emitter
2427
this.socket = socket
@@ -94,9 +97,8 @@ class Browser {
9497
}
9598
}
9699

97-
onDisconnect (reason, disconnectedSocket) {
100+
onSocketDisconnect (reason, disconnectedSocket) {
98101
helper.arrayRemove(this.activeSockets, disconnectedSocket)
99-
100102
if (this.activeSockets.length) {
101103
this.log.debug(`Disconnected ${disconnectedSocket.id}, still have ${this.getActiveSocketsIds()}`)
102104
return
@@ -119,24 +121,27 @@ class Browser {
119121
}
120122
}
121123

122-
reconnect (newSocket) {
123-
if (this.state === EXECUTING_DISCONNECTED) {
124+
reconnect (newSocket, clientSaysReconnect) {
125+
if (!clientSaysReconnect || this.state === DISCONNECTED) {
126+
this.log.info(`Disconnected browser returned on socket ${newSocket.id} with id ${this.id}.`)
127+
this.setState(CONNECTED)
128+
129+
// The disconnected browser is already part of the collection.
130+
// Update the collection view in the UI (header on client.html)
131+
this.emitter.emit('browsers_change', this.collection)
132+
// Notify the launcher
133+
this.emitter.emit('browser_register', this)
134+
// Execute tests if configured to do so.
135+
if (this.singleRun) {
136+
this.execute()
137+
}
138+
} else if (this.state === EXECUTING_DISCONNECTED) {
124139
this.log.debug('Lost socket connection, but browser continued to execute. Reconnected ' +
125140
`on socket ${newSocket.id}.`)
126141
this.setState(EXECUTING)
127142
} else if ([CONNECTED, CONFIGURING, EXECUTING].includes(this.state)) {
128143
this.log.debug(`Rebinding to new socket ${newSocket.id} (already have ` +
129144
`${this.getActiveSocketsIds()})`)
130-
} else if (this.state === DISCONNECTED) {
131-
this.log.info(`Disconnected browser returned on socket ${newSocket.id} with id ${this.id}.`)
132-
this.setState(CONNECTED)
133-
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)
139-
this.emitter.emit('browser_register', this)
140145
}
141146

142147
if (!this.activeSockets.some((s) => s.id === newSocket.id)) {
@@ -161,8 +166,8 @@ class Browser {
161166
this.refreshNoActivityTimeout()
162167
}
163168

164-
execute (config) {
165-
this.activeSockets.forEach((socket) => socket.emit('execute', config))
169+
execute () {
170+
this.activeSockets.forEach((socket) => socket.emit('execute', this.clientConfig))
166171
this.setState(CONFIGURING)
167172
this.refreshNoActivityTimeout()
168173
}
@@ -172,10 +177,14 @@ class Browser {
172177
}
173178

174179
disconnect (reason) {
175-
this.log.warn(`Disconnected (${this.disconnectsCount} times)${reason || ''}`)
176-
this.setState(DISCONNECTED)
180+
this.log.warn(`Disconnected (${this.disconnectsCount} times) ${reason || ''}`)
177181
this.disconnectsCount++
178-
this.emitter.emit('browser_error', this, `Disconnected${reason || ''}`)
182+
this.emitter.emit('browser_error', this, `Disconnected ${reason || ''}`)
183+
this.remove()
184+
}
185+
186+
remove () {
187+
this.setState(DISCONNECTED)
179188
this.collection.remove(this)
180189
}
181190

@@ -201,7 +210,7 @@ class Browser {
201210

202211
bindSocketEvents (socket) {
203212
// TODO: check which of these events are actually emitted by socket
204-
socket.on('disconnect', (reason) => this.onDisconnect(reason, socket))
213+
socket.on('disconnect', (reason) => this.onSocketDisconnect(reason, socket))
205214
socket.on('start', (info) => this.onStart(info))
206215
socket.on('karma_error', (error) => this.onKarmaError(error))
207216
socket.on('complete', (result) => this.onComplete(result))
@@ -246,9 +255,11 @@ class Browser {
246255
Browser.factory = function (
247256
id, fullName, /* capturedBrowsers */ collection, emitter, socket, timer,
248257
/* config.browserDisconnectTimeout */ disconnectDelay,
249-
/* config.browserNoActivityTimeout */ noActivityTimeout
250-
) {
251-
return new Browser(id, fullName, collection, emitter, socket, timer, disconnectDelay, noActivityTimeout)
258+
/* config.browserNoActivityTimeout */ noActivityTimeout,
259+
/* config.singleRun */ singleRun,
260+
/* config.client */ clientConfig) {
261+
return new Browser(id, fullName, collection, emitter, socket, timer,
262+
disconnectDelay, noActivityTimeout, singleRun, clientConfig)
252263
}
253264

254265
Browser.STATE_CONNECTED = CONNECTED

‎lib/server.js

+6-22
Original file line numberDiff line numberDiff line change
@@ -261,27 +261,12 @@ class Server extends KarmaEventEmitter {
261261
})
262262

263263
socket.on('register', (info) => {
264-
let newBrowser = info.id ? (capturedBrowsers.getById(info.id) || singleRunBrowsers.getById(info.id)) : null
265-
266-
if (newBrowser) {
267-
// By default if a browser disconnects while still executing, we assume that the test
268-
// execution still continues because just the socket connection has been terminated. Now
269-
// since we know whether this is just a socket reconnect or full client reconnect, we
270-
// need to update the browser state accordingly. This is necessary because in case a
271-
// browser crashed and has been restarted, we need to start with a fresh execution.
272-
if (!info.isSocketReconnect) {
273-
newBrowser.setState(Browser.STATE_DISCONNECTED)
274-
}
275-
276-
newBrowser.reconnect(socket)
264+
const knownBrowser = info.id ? (capturedBrowsers.getById(info.id) || singleRunBrowsers.getById(info.id)) : null
277265

278-
// Since not every reconnected browser is able to continue with its previous execution,
279-
// we need to start a new execution in case a browser has restarted and is now idling.
280-
if (newBrowser.state === Browser.STATE_CONNECTED && config.singleRun) {
281-
newBrowser.execute(config.client)
282-
}
266+
if (knownBrowser) {
267+
knownBrowser.reconnect(socket, info.isSocketReconnect)
283268
} else {
284-
newBrowser = this._injector.createChild([{
269+
const newBrowser = this._injector.createChild([{
285270
id: ['value', info.id || null],
286271
fullName: ['value', (helper.isDefined(info.displayName) ? info.displayName : info.name)],
287272
socket: ['value', socket]
@@ -290,7 +275,7 @@ class Server extends KarmaEventEmitter {
290275
newBrowser.init()
291276

292277
if (config.singleRun) {
293-
newBrowser.execute(config.client)
278+
newBrowser.execute()
294279
singleRunBrowsers.add(newBrowser)
295280
}
296281
}
@@ -334,8 +319,7 @@ class Server extends KarmaEventEmitter {
334319
singleRunDoneBrowsers[completedBrowser.id] = true
335320

336321
if (launcher.kill(completedBrowser.id)) {
337-
// workaround to supress "disconnect" warning
338-
completedBrowser.state = Browser.STATE_DISCONNECTED
322+
completedBrowser.remove()
339323
}
340324

341325
emitRunCompleteIfAllBrowsersDone()

‎static/karma.js

+5-6
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,11 @@ function Karma (socket, iframe, opener, navigator, location, document) {
5151
}
5252
}
5353

54-
// This variable will be set to "true" whenever the socket lost connection and was able to
55-
// reconnect to the Karma server. This will be passed to the Karma server then, so that
56-
// Karma can differentiate between a socket client reconnect and a full browser reconnect.
54+
// To start we will signal the server that we are not reconnecting. If the socket loses
55+
// connection and was able to reconnect to the Karma server we will get a
56+
// second 'connect' event. There we will pass 'true' and that will be passed to the
57+
// Karma server then, so that Karma can differentiate between a socket client
58+
// econnect and a full browser reconnect.
5759
var socketReconnect = false
5860

5961
this.VERSION = constant.VERSION
@@ -309,9 +311,6 @@ function Karma (socket, iframe, opener, navigator, location, document) {
309311
info.displayName = displayName
310312
}
311313
socket.emit('register', info)
312-
})
313-
314-
socket.on('reconnect', function () {
315314
socketReconnect = true
316315
})
317316
}

‎test/client/karma.spec.js

+4-2
Original file line numberDiff line numberDiff line change
@@ -204,11 +204,13 @@ describe('Karma', function () {
204204
})
205205

206206
it('should mark "register" event for reconnected socket', function () {
207+
// First connect.
208+
socket.emit('connect')
209+
207210
socket.on('register', sinon.spy(function (info) {
208211
assert(info.isSocketReconnect === true)
209212
}))
210-
211-
socket.emit('reconnect')
213+
// Reconnect
212214
socket.emit('connect')
213215
})
214216

‎test/e2e/reconnecting.feature

+1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ Feature: Passing Options
2121
When I start Karma
2222
Then it passes with:
2323
"""
24+
LOG: '============== START TEST =============='
2425
.....
2526
Chrome Headless
2627
"""

‎test/e2e/support/reconnecting/test.js

+7-8
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,26 @@ describe('plus', function () {
66
}
77

88
it('should pass', function () {
9+
// In flaky fails we probably get two starts.
10+
console.log('============== START TEST ==============')
911
expect(1).toBe(1)
1012
})
1113

1214
it('should disconnect', function (done) {
1315
expect(2).toBe(2)
14-
socket().disconnect()
15-
16-
done()
16+
setTimeout(() => {
17+
socket().disconnect()
18+
done()
19+
}, 500)
1720
})
1821

1922
it('should work', function () {
2023
expect(plus(1, 2)).toBe(3)
2124
})
2225

23-
it('should re-connect', function (done) {
26+
it('should re-connect', function () {
2427
expect(4).toBe(4)
25-
// Emit reconnect, so Karma will not start new test run after reconnecting.
26-
socket().emit('reconnect')
2728
socket().connect()
28-
29-
done()
3029
})
3130

3231
it('should work', function () {

‎test/unit/browser.spec.js

+24-18
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ describe('Browser', () => {
215215
})
216216
})
217217

218-
describe('onDisconnect', () => {
218+
describe('onSocketDisconnect', () => {
219219
let timer = null
220220

221221
beforeEach(() => {
@@ -227,7 +227,7 @@ describe('Browser', () => {
227227
it('should remove from parent collection', () => {
228228
expect(collection.length).to.equal(1)
229229

230-
browser.onDisconnect('socket.io-reason', socket)
230+
browser.onSocketDisconnect('socket.io-reason', socket)
231231
expect(collection.length).to.equal(0)
232232
})
233233

@@ -236,7 +236,7 @@ describe('Browser', () => {
236236
emitter.on('browser_complete', spy)
237237
browser.state = Browser.STATE_EXECUTING
238238

239-
browser.onDisconnect('socket.io-reason', socket)
239+
browser.onSocketDisconnect('socket.io-reason', socket)
240240
timer.wind(20)
241241

242242
expect(browser.lastResult.disconnected).to.equal(true)
@@ -248,7 +248,7 @@ describe('Browser', () => {
248248
emitter.on('browser_complete', spy)
249249
browser.state = Browser.STATE_CONNECTED
250250

251-
browser.onDisconnect('socket.io-reason', socket)
251+
browser.onSocketDisconnect('socket.io-reason', socket)
252252
expect(spy).not.to.have.been.called
253253
})
254254
})
@@ -261,8 +261,8 @@ describe('Browser', () => {
261261
browser.init()
262262
browser.state = Browser.STATE_EXECUTING
263263

264-
browser.onDisconnect('socket.io-reason', socket)
265-
browser.reconnect(mkSocket())
264+
browser.onSocketDisconnect('socket.io-reason', socket)
265+
browser.reconnect(mkSocket(), true)
266266

267267
timer.wind(10)
268268
expect(browser.state).to.equal(Browser.STATE_EXECUTING)
@@ -275,7 +275,7 @@ describe('Browser', () => {
275275
browser.init()
276276
browser.state = Browser.STATE_EXECUTING
277277

278-
browser.reconnect(mkSocket())
278+
browser.reconnect(mkSocket(), true)
279279

280280
// still accept results on the old socket
281281
socket.emit('result', { success: true })
@@ -293,7 +293,7 @@ describe('Browser', () => {
293293
browser = new Browser('id', 'Chrome 25.0', collection, emitter, socket, null, 10)
294294
browser.state = Browser.STATE_DISCONNECTED
295295

296-
browser.reconnect(mkSocket())
296+
browser.reconnect(mkSocket(), true)
297297

298298
expect(browser.isConnected()).to.equal(true)
299299
})
@@ -306,7 +306,7 @@ describe('Browser', () => {
306306

307307
browser.state = Browser.STATE_DISCONNECTED
308308

309-
browser.reconnect(mkSocket())
309+
browser.reconnect(mkSocket(), false)
310310

311311
expect(collection.length).to.equal(1)
312312
})
@@ -387,13 +387,18 @@ describe('Browser', () => {
387387
describe('execute and start', () => {
388388
it('should emit execute and change state to CONFIGURING', () => {
389389
const spyExecute = sinon.spy()
390-
const config = {}
391-
browser = new Browser('fake-id', 'full name', collection, emitter, socket)
390+
const timer = undefined
391+
const disconnectDelay = 0
392+
const noActivityTimeout = 0
393+
const singleRun = false
394+
const clientConfig = {}
395+
browser = new Browser('fake-id', 'full name', collection, emitter, socket,
396+
timer, disconnectDelay, noActivityTimeout, singleRun, clientConfig)
392397
socket.on('execute', spyExecute)
393-
browser.execute(config)
398+
browser.execute()
394399

395400
expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
396-
expect(spyExecute).to.have.been.calledWith(config)
401+
expect(spyExecute).to.have.been.calledWith(clientConfig)
397402
})
398403

399404
it('should emit start and change state to EXECUTING', () => {
@@ -417,7 +422,7 @@ describe('Browser', () => {
417422
expect(browser.isConnected()).to.equal(false)
418423

419424
const newSocket = mkSocket()
420-
browser.reconnect(newSocket)
425+
browser.reconnect(newSocket, true)
421426
expect(browser.isConnected()).to.equal(false)
422427

423428
newSocket.emit('result', { success: false, suite: [], log: [] })
@@ -466,7 +471,7 @@ describe('Browser', () => {
466471
emitter.on('browser_register', () => browser.execute())
467472

468473
// reconnect on a new socket (which triggers re-execution)
469-
browser.reconnect(newSocket)
474+
browser.reconnect(newSocket, false)
470475
expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
471476
newSocket.emit('start', { total: 11 })
472477
expect(browser.state).to.equal(Browser.STATE_EXECUTING)
@@ -487,13 +492,14 @@ describe('Browser', () => {
487492
expect(browser.state).to.equal(Browser.STATE_CONNECTED)
488493

489494
browser.execute()
495+
expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
490496

491497
// A second connection...
492498
const newSocket = mkSocket()
493-
browser.reconnect(newSocket)
499+
browser.reconnect(newSocket, true)
494500

495501
// Disconnect the second connection...
496-
browser.onDisconnect('socket.io-reason', newSocket)
502+
browser.onSocketDisconnect('socket.io-reason', newSocket)
497503
expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
498504
socket.emit('start', { total: 1 })
499505
expect(browser.state).to.equal(Browser.STATE_EXECUTING)
@@ -512,7 +518,7 @@ describe('Browser', () => {
512518
browser.execute()
513519

514520
// A second connection...
515-
browser.reconnect(socket)
521+
browser.reconnect(socket, true)
516522

517523
socket.emit('result', { success: true, suite: [], log: [] })
518524
socket.emit('complete')

‎test/unit/server.spec.js

+5-4
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ describe('server', () => {
4545
singleRun: true,
4646
logLevel: 'OFF',
4747
plugins: [],
48-
browserDisconnectTolerance: 0
48+
browserDisconnectTolerance: 0,
49+
browserNoActivityTimeout: 0
4950
}
5051

5152
server = new Server(mockConfig, doneSpy)
@@ -432,7 +433,7 @@ describe('server', () => {
432433
resolveExitCode(exitCode)
433434
})
434435

435-
server.emit('browser_complete_with_no_more_retries', { id: 'fake' })
436+
server.emit('browser_complete_with_no_more_retries', { id: 'fake', remove: () => {} })
436437

437438
function mockProcess (process) {
438439
sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
@@ -449,7 +450,7 @@ describe('server', () => {
449450
resolveExitCode(exitCode)
450451
})
451452

452-
server.emit('browser_complete_with_no_more_retries', { id: 'fake' })
453+
server.emit('browser_complete_with_no_more_retries', { id: 'fake', remove: () => {} })
453454

454455
function mockProcess (process) {
455456
sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
@@ -476,7 +477,7 @@ describe('server', () => {
476477
it('should re-configure disconnected browser which has been restarted', () => {
477478
const testBrowserId = 'my-id'
478479
const browser = new Browser(testBrowserId, 'Chrome 19.0', browserCollection, server,
479-
mockBrowserSocket, null, 0)
480+
mockBrowserSocket, undefined, 0, 0, true, {})
480481
const registerFn = mockSocketEventListeners.get('register')
481482

482483
browser.init()

0 commit comments

Comments
 (0)
Please sign in to comment.