From b4831c190909960095e3e28234c1a207ccecb6be Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 14 Dec 2018 19:11:43 +0100 Subject: [PATCH 1/4] 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). --- client/karma.js | 12 +++++++++-- lib/browser.js | 19 +++++++++++++---- lib/server.js | 14 +++++++++++-- test/client/karma.spec.js | 9 ++++++++ test/unit/browser.spec.js | 13 ++++++++++++ yarn.lock | 44 +++++++++++++++++++++++---------------- 6 files changed, 85 insertions(+), 26 deletions(-) diff --git a/client/karma.js b/client/karma.js index ff4217c62..7febf20d8 100644 --- a/client/karma.js +++ b/client/karma.js @@ -5,6 +5,7 @@ var util = require('../common/util') function Karma (socket, iframe, opener, navigator, location) { var startEmitted = false var reloadingContext = false + var socketReconnect = false var self = this var queryParams = util.parseQueryParams(location.search) var browserId = queryParams.id || util.generateId('manual-') @@ -227,20 +228,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 diff --git a/lib/browser.js b/lib/browser.js index 81865395a..7ec026529 100644 --- a/lib/browser.js +++ b/lib/browser.js @@ -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) { @@ -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. 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) 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}`) 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) } diff --git a/lib/server.js b/lib/server.js index 9624d4c6b..209006145 100644 --- a/lib/server.js +++ b/lib/server.js @@ -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) { + // In case we are in single run and the reconnected browser was not able to restore + // or continue with its previous execution, we start the execution again. + if (newBrowser.state === Browser.STATE_CONNECTED && config.singleRun) { newBrowser.execute(config.client) } } else { diff --git a/test/client/karma.spec.js b/test/client/karma.spec.js index 78745962f..2152ed1c6 100644 --- a/test/client/karma.spec.js +++ b/test/client/karma.spec.js @@ -147,6 +147,15 @@ describe('Karma', function () { assert(spyInfo.called) }) + it.only('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() diff --git a/test/unit/browser.spec.js b/test/unit/browser.spec.js index 447bb02cc..33eca8013 100644 --- a/test/unit/browser.spec.js +++ b/test/unit/browser.spec.js @@ -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', () => { diff --git a/yarn.lock b/yarn.lock index 8833be7d0..75469fc1c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -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" @@ -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" @@ -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" @@ -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" @@ -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" @@ -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: From 83f2493c633ff5848bb24b06f83844adf5667360 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 14 Dec 2018 20:27:39 +0100 Subject: [PATCH 2/4] test: add unit test that covers disconnected restarted browsers --- test/client/karma.spec.js | 2 +- test/unit/server.spec.js | 52 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/test/client/karma.spec.js b/test/client/karma.spec.js index 2152ed1c6..d07995a99 100644 --- a/test/client/karma.spec.js +++ b/test/client/karma.spec.js @@ -147,7 +147,7 @@ describe('Karma', function () { assert(spyInfo.called) }) - it.only('should mark "register" event for reconnected socket', function () { + it('should mark "register" event for reconnected socket', function () { socket.on('register', sinon.spy(function (info) { assert(info.isSocketReconnect === true) })) diff --git a/test/unit/server.spec.js b/test/unit/server.spec.js index 276618b12..b24ef5f6e 100644 --- a/test/unit/server.spec.js +++ b/test/unit/server.spec.js @@ -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 @@ -10,6 +11,7 @@ describe('server', () => { let fileListOnReject let mockLauncher let mockWebServer + let mockServerSocket let mockSocketServer let mockBoundServer let mockExecutor @@ -17,6 +19,7 @@ describe('server', () => { 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 () { @@ -67,6 +70,13 @@ describe('server', () => { kill: () => true } + mockServerSocket = { + id: 'socket-id', + on: (name, handler) => mockSocketEventListeners.set(name, handler), + emit: () => {}, + removeListener: () => {} + } + mockSocketServer = { close: () => {}, flashPolicyServer: { @@ -74,7 +84,7 @@ describe('server', () => { }, sockets: { sockets: {}, - on: () => {}, + on: (name, handler) => handler(mockServerSocket), emit: () => {}, removeAllListeners: () => {} } @@ -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) + }) + }) }) From 4a1b33f153d8b3a7a2d8564ef83bb9830f82c7de Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 14 Dec 2018 20:48:20 +0100 Subject: [PATCH 3/4] fixup! test: add unit test that covers disconnected restarted browsers Address feedback --- client/karma.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/karma.js b/client/karma.js index 7febf20d8..97b3b62fe 100644 --- a/client/karma.js +++ b/client/karma.js @@ -5,7 +5,6 @@ var util = require('../common/util') function Karma (socket, iframe, opener, navigator, location) { var startEmitted = false var reloadingContext = false - var socketReconnect = false var self = this var queryParams = util.parseQueryParams(location.search) var browserId = queryParams.id || util.generateId('manual-') @@ -15,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 = {} From 9c69aa5f4fa8eb754b498c739a03e2a94a195014 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 15 Dec 2018 07:47:43 +0100 Subject: [PATCH 4/4] fixup! test: add unit test that covers disconnected restarted browsers Improve comments & log messages --- lib/browser.js | 15 +++++---------- lib/server.js | 4 ++-- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/lib/browser.js b/lib/browser.js index 7ec026529..f4389c172 100644 --- a/lib/browser.js +++ b/lib/browser.js @@ -121,19 +121,14 @@ 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. - 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)) { - // In case for some reason the socket has changed, we just bind the events of the - // new socket (see below) - 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) { - // 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}`) + this.log.info(`Disconnected browser returned on socket ${newSocket.id} with id ${this.id}.`) this.setState(CONNECTED) // Since the disconnected browser is already part of the collection and we want to diff --git a/lib/server.js b/lib/server.js index 209006145..235929a54 100644 --- a/lib/server.js +++ b/lib/server.js @@ -243,8 +243,8 @@ class Server extends KarmaEventEmitter { newBrowser.reconnect(socket) - // In case we are in single run and the reconnected browser was not able to restore - // or continue with its previous execution, we start the execution again. + // 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) }