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

fix: restarted browsers not running tests #3233

Merged
merged 4 commits into from
Dec 17, 2018
Merged

fix: restarted browsers not running tests #3233

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b4831c1
fix: restarted browsers not running tests
devversion Dec 14, 2018
83f2493
test: add unit test that covers disconnected restarted browsers
devversion Dec 14, 2018
4a1b33f
fixup! test: add unit test that covers disconnected restarted browsers
devversion Dec 14, 2018
9c69aa5
fixup! test: add unit test that covers disconnected restarted browsers
devversion Dec 15, 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
16 changes: 14 additions & 2 deletions client/karma.js
View file
Open in desktop
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
19 changes: 15 additions & 4 deletions lib/browser.js
View file
Open in desktop
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,26 @@ class Browser {

reconnect (newSocket) {
if (this.state === EXECUTING_DISCONNECTED) {
// In case the socket just lost connection and the browser continued the execution,
// we just update the state back to "EXECUTING" so that it matches the current state.
devversion marked this conversation as resolved.
Show resolved Hide resolved
this.log.debug(`Reconnected on ${newSocket.id}.`)
this.setState(EXECUTING)
} else if ([CONNECTED, CONFIGURING, EXECUTING].includes(this.state)) {
// In case for some reason the socket has changed, we just bind the events of the
// new socket (see below)
devversion marked this conversation as resolved.
Show resolved Hide resolved
this.log.debug(`New connection ${newSocket.id} (already have ${this.getActiveSocketsIds()})`)
} else if (this.state === DISCONNECTED) {
this.log.info(`Connected on socket ${newSocket.id} with id ${this.id}`)
// In case the browser disconnected completely, we want to ensure that the browser will be
// registered and starts execution again. This can happen for example if the browser
// instance crashed and cannot continue from it's own execution state.
this.log.info(`Reconnected on socket ${newSocket.id} with id ${this.id}`)
devversion marked this conversation as resolved.
Show resolved Hide resolved
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
View file
Open in desktop
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments are important but seem long. How about placing the comment inside the if and something like // Browser crashed and restarted.

(I really dislike the state-setting from outside design).

Copy link
Contributor Author

@devversion devversion Dec 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I honestly don't think that it's a concern that this comment is long. Since this is basically a side-effect and we want to make clear why it happens, I think it's good to have this explained as detailed as possible. Especially since this seems to be a long-term and critical issue that can cause flaky test runs on CI's.

// Browser crashed and restarted is basically a comment like below where it says // We are restarting a previously disconnected browser. but basically misses crucial information.

Also I agree with the state setting from outside being a bit weird, but I'd rather set it here than passing stuff back into the Browser. As a matter of the event flow and the data we need from the client socket, it kind of makes sense to me to handle it here. I'm up other ideas on how we can do it.

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

Choose a reason for hiding this comment

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

What it a bug that this was using DISCONNECTED before, or is this a change in behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was a bug. The reconnect function always sets the state to CONNECTED if the state was STATE_DISCONNECTED. So it would never pass this check.

See the reconnect function source

// In case we are in single run and the reconnected browser was not able to restore
devversion marked this conversation as resolved.
Show resolved Hide resolved
// or continue with its previous execution, we start the execution again.
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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=

lru-cache@2.2.x:
version "2.2.4"
resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-2.2.4.tgz#6c658619becf14031d0d0b594b16042ce4dc063d"
integrity sha1-bGWGGb7PFAMdDQtZSxYELOTcBj0=
lru-cache@4.1.x:
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