From 9c29d45108523e778154797fde9fe717ad749b3c Mon Sep 17 00:00:00 2001
From: johnjbarton <johnjbarton@johnjbarton.com>
Date: Mon, 21 Dec 2020 16:52:37 -0800
Subject: [PATCH] chore(test): mark all second connections reconnects

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.
---
 client/karma.js                       | 11 +++--
 lib/browser.js                        | 64 ++++++++++++++++-----------
 lib/server.js                         | 28 +++---------
 static/karma.js                       | 11 +++--
 test/client/karma.spec.js             |  6 ++-
 test/e2e/reconnecting.feature         |  1 +
 test/e2e/support/reconnecting/test.js | 15 +++----
 test/unit/browser.spec.js             | 42 ++++++++++--------
 test/unit/server.spec.js              |  9 ++--
 9 files changed, 96 insertions(+), 91 deletions(-)

diff --git a/client/karma.js b/client/karma.js
index 6790a77ac..a0ba6cddf 100644
--- a/client/karma.js
+++ b/client/karma.js
@@ -41,9 +41,11 @@ function Karma (socket, iframe, opener, navigator, location, document) {
     }
   }
 
-  // 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.
+  // To start we will signal the server that we are not reconnecting. If the socket loses
+  // connection and was able to reconnect to the Karma server we will get a
+  // second 'connect' event. There we will pass 'true' and that will be passed to the
+  // Karma server then, so that Karma can differentiate between a socket client
+  // econnect and a full browser reconnect.
   var socketReconnect = false
 
   this.VERSION = constant.VERSION
@@ -306,9 +308,6 @@ function Karma (socket, iframe, opener, navigator, location, document) {
       info.displayName = displayName
     }
     socket.emit('register', info)
-  })
-
-  socket.on('reconnect', function () {
     socketReconnect = true
   })
 }
diff --git a/lib/browser.js b/lib/browser.js
index c3ab8cc21..5cc389817 100644
--- a/lib/browser.js
+++ b/lib/browser.js
@@ -11,7 +11,8 @@ const EXECUTING_DISCONNECTED = 'EXECUTING_DISCONNECTED' // The browser is execut
 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) {
+  constructor (id, fullName, collection, emitter, socket, timer, disconnectDelay,
+    noActivityTimeout, singleRun, clientConfig) {
     this.id = id
     this.fullName = fullName
     this.name = helper.browserFullNameToShort(fullName)
@@ -19,6 +20,8 @@ class Browser {
     this.disconnectsCount = 0
     this.activeSockets = [socket]
     this.noActivityTimeout = noActivityTimeout
+    this.singleRun = singleRun
+    this.clientConfig = clientConfig
     this.collection = collection
     this.emitter = emitter
     this.socket = socket
@@ -41,7 +44,10 @@ class Browser {
   }
 
   setState (toState) {
-    this.log.debug(`${this.state} -> ${toState}`)
+    this.log.info(`${this.state} -> ${toState}`)
+    if (`${this.state} -> ${toState}` === 'CONFIGURING -> CONNECTED') {
+      console.trace()
+    }
     this.state = toState
   }
 
@@ -94,9 +100,8 @@ class Browser {
     }
   }
 
-  onDisconnect (reason, disconnectedSocket) {
+  onSocketDisconnect (reason, disconnectedSocket) {
     helper.arrayRemove(this.activeSockets, disconnectedSocket)
-
     if (this.activeSockets.length) {
       this.log.debug(`Disconnected ${disconnectedSocket.id}, still have ${this.getActiveSocketsIds()}`)
       return
@@ -119,24 +124,27 @@ class Browser {
     }
   }
 
-  reconnect (newSocket) {
-    if (this.state === EXECUTING_DISCONNECTED) {
+  reconnect (newSocket, clientSaysReconnect) {
+    if (!clientSaysReconnect || this.state === DISCONNECTED) {
+      this.log.info(`Disconnected browser returned on socket ${newSocket.id} with id ${this.id}.`)
+      this.setState(CONNECTED)
+
+      // The disconnected browser is already part of the collection.
+      // Update the collection view in the UI (header on client.html)
+      this.emitter.emit('browsers_change', this.collection)
+      // Notify the launcher
+      this.emitter.emit('browser_register', this)
+      // Execute tests if configured to do so.
+      if (this.singleRun) {
+        this.execute()
+      }
+    } else if (this.state === EXECUTING_DISCONNECTED) {
       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(`Rebinding to new socket ${newSocket.id} (already have ` +
         `${this.getActiveSocketsIds()})`)
-    } else if (this.state === DISCONNECTED) {
-      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
-      // 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)
     }
 
     if (!this.activeSockets.some((s) => s.id === newSocket.id)) {
@@ -161,8 +169,8 @@ class Browser {
     this.refreshNoActivityTimeout()
   }
 
-  execute (config) {
-    this.activeSockets.forEach((socket) => socket.emit('execute', config))
+  execute () {
+    this.activeSockets.forEach((socket) => socket.emit('execute', this.clientConfig))
     this.setState(CONFIGURING)
     this.refreshNoActivityTimeout()
   }
@@ -172,10 +180,14 @@ class Browser {
   }
 
   disconnect (reason) {
-    this.log.warn(`Disconnected (${this.disconnectsCount} times)${reason || ''}`)
-    this.setState(DISCONNECTED)
+    this.log.warn(`Disconnected (${this.disconnectsCount} times) ${reason || ''}`)
     this.disconnectsCount++
-    this.emitter.emit('browser_error', this, `Disconnected${reason || ''}`)
+    this.emitter.emit('browser_error', this, `Disconnected ${reason || ''}`)
+    this.remove()
+  }
+
+  remove () {
+    this.setState(DISCONNECTED)
     this.collection.remove(this)
   }
 
@@ -201,7 +213,7 @@ class Browser {
 
   bindSocketEvents (socket) {
     // TODO: check which of these events are actually emitted by socket
-    socket.on('disconnect', (reason) => this.onDisconnect(reason, socket))
+    socket.on('disconnect', (reason) => this.onSocketDisconnect(reason, socket))
     socket.on('start', (info) => this.onStart(info))
     socket.on('karma_error', (error) => this.onKarmaError(error))
     socket.on('complete', (result) => this.onComplete(result))
@@ -246,9 +258,11 @@ class Browser {
 Browser.factory = function (
   id, fullName, /* capturedBrowsers */ collection, emitter, socket, timer,
   /* config.browserDisconnectTimeout */ disconnectDelay,
-  /* config.browserNoActivityTimeout */ noActivityTimeout
-) {
-  return new Browser(id, fullName, collection, emitter, socket, timer, disconnectDelay, noActivityTimeout)
+  /* config.browserNoActivityTimeout */ noActivityTimeout,
+  /* config.singleRun */ singleRun,
+  /* config.client */ clientConfig) {
+  return new Browser(id, fullName, collection, emitter, socket, timer,
+    disconnectDelay, noActivityTimeout, singleRun, clientConfig)
 }
 
 Browser.STATE_CONNECTED = CONNECTED
diff --git a/lib/server.js b/lib/server.js
index 4a3eecfca..ea4af3940 100644
--- a/lib/server.js
+++ b/lib/server.js
@@ -263,27 +263,12 @@ class Server extends KarmaEventEmitter {
       })
 
       socket.on('register', (info) => {
-        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)
+        const knownBrowser = info.id ? (capturedBrowsers.getById(info.id) || singleRunBrowsers.getById(info.id)) : null
 
-          // 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)
-          }
+        if (knownBrowser) {
+          knownBrowser.reconnect(socket, info.isSocketReconnect)
         } else {
-          newBrowser = this._injector.createChild([{
+          const newBrowser = this._injector.createChild([{
             id: ['value', info.id || null],
             fullName: ['value', (helper.isDefined(info.displayName) ? info.displayName : info.name)],
             socket: ['value', socket]
@@ -292,7 +277,7 @@ class Server extends KarmaEventEmitter {
           newBrowser.init()
 
           if (config.singleRun) {
-            newBrowser.execute(config.client)
+            newBrowser.execute()
             singleRunBrowsers.add(newBrowser)
           }
         }
@@ -336,8 +321,7 @@ class Server extends KarmaEventEmitter {
         singleRunDoneBrowsers[completedBrowser.id] = true
 
         if (launcher.kill(completedBrowser.id)) {
-          // workaround to supress "disconnect" warning
-          completedBrowser.state = Browser.STATE_DISCONNECTED
+          completedBrowser.remove()
         }
 
         emitRunCompleteIfAllBrowsersDone()
diff --git a/static/karma.js b/static/karma.js
index 0c289e0ca..5147441c2 100644
--- a/static/karma.js
+++ b/static/karma.js
@@ -51,9 +51,11 @@ function Karma (socket, iframe, opener, navigator, location, document) {
     }
   }
 
-  // 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.
+  // To start we will signal the server that we are not reconnecting. If the socket loses
+  // connection and was able to reconnect to the Karma server we will get a
+  // second 'connect' event. There we will pass 'true' and that will be passed to the
+  // Karma server then, so that Karma can differentiate between a socket client
+  // econnect and a full browser reconnect.
   var socketReconnect = false
 
   this.VERSION = constant.VERSION
@@ -316,9 +318,6 @@ function Karma (socket, iframe, opener, navigator, location, document) {
       info.displayName = displayName
     }
     socket.emit('register', info)
-  })
-
-  socket.on('reconnect', function () {
     socketReconnect = true
   })
 }
diff --git a/test/client/karma.spec.js b/test/client/karma.spec.js
index a309b174f..d7fe1d847 100644
--- a/test/client/karma.spec.js
+++ b/test/client/karma.spec.js
@@ -204,11 +204,13 @@ describe('Karma', function () {
   })
 
   it('should mark "register" event for reconnected socket', function () {
+    // First connect.
+    socket.emit('connect')
+
     socket.on('register', sinon.spy(function (info) {
       assert(info.isSocketReconnect === true)
     }))
-
-    socket.emit('reconnect')
+    // Reconnect
     socket.emit('connect')
   })
 
diff --git a/test/e2e/reconnecting.feature b/test/e2e/reconnecting.feature
index 829cbaec7..246d4e0da 100644
--- a/test/e2e/reconnecting.feature
+++ b/test/e2e/reconnecting.feature
@@ -21,6 +21,7 @@ Feature: Passing Options
     When I start Karma
     Then it passes with:
       """
+      LOG: '============== START TEST =============='
       .....
       Chrome Headless
       """
diff --git a/test/e2e/support/reconnecting/test.js b/test/e2e/support/reconnecting/test.js
index a3b682e1b..74216c7fe 100644
--- a/test/e2e/support/reconnecting/test.js
+++ b/test/e2e/support/reconnecting/test.js
@@ -6,27 +6,26 @@ describe('plus', function () {
   }
 
   it('should pass', function () {
+    // In flaky fails we probably get two starts.
+    console.log('============== START TEST ==============')
     expect(1).toBe(1)
   })
 
   it('should disconnect', function (done) {
     expect(2).toBe(2)
-    socket().disconnect()
-
-    done()
+    setTimeout(() => {
+      socket().disconnect()
+      done()
+    }, 500)
   })
 
   it('should work', function () {
     expect(plus(1, 2)).toBe(3)
   })
 
-  it('should re-connect', function (done) {
+  it('should re-connect', function () {
     expect(4).toBe(4)
-    // Emit reconnect, so Karma will not start new test run after reconnecting.
-    socket().emit('reconnect')
     socket().connect()
-
-    done()
   })
 
   it('should work', function () {
diff --git a/test/unit/browser.spec.js b/test/unit/browser.spec.js
index ec889b083..15b150a59 100644
--- a/test/unit/browser.spec.js
+++ b/test/unit/browser.spec.js
@@ -215,7 +215,7 @@ describe('Browser', () => {
     })
   })
 
-  describe('onDisconnect', () => {
+  describe('onSocketDisconnect', () => {
     let timer = null
 
     beforeEach(() => {
@@ -227,7 +227,7 @@ describe('Browser', () => {
     it('should remove from parent collection', () => {
       expect(collection.length).to.equal(1)
 
-      browser.onDisconnect('socket.io-reason', socket)
+      browser.onSocketDisconnect('socket.io-reason', socket)
       expect(collection.length).to.equal(0)
     })
 
@@ -236,7 +236,7 @@ describe('Browser', () => {
       emitter.on('browser_complete', spy)
       browser.state = Browser.STATE_EXECUTING
 
-      browser.onDisconnect('socket.io-reason', socket)
+      browser.onSocketDisconnect('socket.io-reason', socket)
       timer.wind(20)
 
       expect(browser.lastResult.disconnected).to.equal(true)
@@ -248,7 +248,7 @@ describe('Browser', () => {
       emitter.on('browser_complete', spy)
       browser.state = Browser.STATE_CONNECTED
 
-      browser.onDisconnect('socket.io-reason', socket)
+      browser.onSocketDisconnect('socket.io-reason', socket)
       expect(spy).not.to.have.been.called
     })
   })
@@ -261,8 +261,8 @@ describe('Browser', () => {
       browser.init()
       browser.state = Browser.STATE_EXECUTING
 
-      browser.onDisconnect('socket.io-reason', socket)
-      browser.reconnect(mkSocket())
+      browser.onSocketDisconnect('socket.io-reason', socket)
+      browser.reconnect(mkSocket(), true)
 
       timer.wind(10)
       expect(browser.state).to.equal(Browser.STATE_EXECUTING)
@@ -275,7 +275,7 @@ describe('Browser', () => {
       browser.init()
       browser.state = Browser.STATE_EXECUTING
 
-      browser.reconnect(mkSocket())
+      browser.reconnect(mkSocket(), true)
 
       // still accept results on the old socket
       socket.emit('result', { success: true })
@@ -293,7 +293,7 @@ describe('Browser', () => {
       browser = new Browser('id', 'Chrome 25.0', collection, emitter, socket, null, 10)
       browser.state = Browser.STATE_DISCONNECTED
 
-      browser.reconnect(mkSocket())
+      browser.reconnect(mkSocket(), true)
 
       expect(browser.isConnected()).to.equal(true)
     })
@@ -306,7 +306,7 @@ describe('Browser', () => {
 
       browser.state = Browser.STATE_DISCONNECTED
 
-      browser.reconnect(mkSocket())
+      browser.reconnect(mkSocket(), false)
 
       expect(collection.length).to.equal(1)
     })
@@ -387,13 +387,18 @@ describe('Browser', () => {
   describe('execute and start', () => {
     it('should emit execute and change state to CONFIGURING', () => {
       const spyExecute = sinon.spy()
-      const config = {}
-      browser = new Browser('fake-id', 'full name', collection, emitter, socket)
+      const timer = undefined
+      const disconnectDelay = 0
+      const noActivityTimeout = 0
+      const singleRun = false
+      const clientConfig = {}
+      browser = new Browser('fake-id', 'full name', collection, emitter, socket,
+        timer, disconnectDelay, noActivityTimeout, singleRun, clientConfig)
       socket.on('execute', spyExecute)
-      browser.execute(config)
+      browser.execute()
 
       expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
-      expect(spyExecute).to.have.been.calledWith(config)
+      expect(spyExecute).to.have.been.calledWith(clientConfig)
     })
 
     it('should emit start and change state to EXECUTING', () => {
@@ -417,7 +422,7 @@ describe('Browser', () => {
       expect(browser.isConnected()).to.equal(false)
 
       const newSocket = mkSocket()
-      browser.reconnect(newSocket)
+      browser.reconnect(newSocket, true)
       expect(browser.isConnected()).to.equal(false)
 
       newSocket.emit('result', { success: false, suite: [], log: [] })
@@ -466,7 +471,7 @@ describe('Browser', () => {
       emitter.on('browser_register', () => browser.execute())
 
       // reconnect on a new socket (which triggers re-execution)
-      browser.reconnect(newSocket)
+      browser.reconnect(newSocket, false)
       expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
       newSocket.emit('start', { total: 11 })
       expect(browser.state).to.equal(Browser.STATE_EXECUTING)
@@ -487,13 +492,14 @@ describe('Browser', () => {
       expect(browser.state).to.equal(Browser.STATE_CONNECTED)
 
       browser.execute()
+      expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
 
       // A second connection...
       const newSocket = mkSocket()
-      browser.reconnect(newSocket)
+      browser.reconnect(newSocket, true)
 
       // Disconnect the second connection...
-      browser.onDisconnect('socket.io-reason', newSocket)
+      browser.onSocketDisconnect('socket.io-reason', newSocket)
       expect(browser.state).to.equal(Browser.STATE_CONFIGURING)
       socket.emit('start', { total: 1 })
       expect(browser.state).to.equal(Browser.STATE_EXECUTING)
@@ -512,7 +518,7 @@ describe('Browser', () => {
       browser.execute()
 
       // A second connection...
-      browser.reconnect(socket)
+      browser.reconnect(socket, true)
 
       socket.emit('result', { success: true, suite: [], log: [] })
       socket.emit('complete')
diff --git a/test/unit/server.spec.js b/test/unit/server.spec.js
index 8bfd39200..01121213e 100644
--- a/test/unit/server.spec.js
+++ b/test/unit/server.spec.js
@@ -45,7 +45,8 @@ describe('server', () => {
       singleRun: true,
       logLevel: 'OFF',
       plugins: [],
-      browserDisconnectTolerance: 0
+      browserDisconnectTolerance: 0,
+      browserNoActivityTimeout: 0
     }
 
     server = new Server(mockConfig, doneSpy)
@@ -432,7 +433,7 @@ describe('server', () => {
           resolveExitCode(exitCode)
         })
 
-        server.emit('browser_complete_with_no_more_retries', { id: 'fake' })
+        server.emit('browser_complete_with_no_more_retries', { id: 'fake', remove: () => {} })
 
         function mockProcess (process) {
           sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
@@ -449,7 +450,7 @@ describe('server', () => {
           resolveExitCode(exitCode)
         })
 
-        server.emit('browser_complete_with_no_more_retries', { id: 'fake' })
+        server.emit('browser_complete_with_no_more_retries', { id: 'fake', remove: () => {} })
 
         function mockProcess (process) {
           sinon.stub(process, 'kill').callsFake((pid, ev) => process.emit(ev))
@@ -476,7 +477,7 @@ describe('server', () => {
     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)
+        mockBrowserSocket, undefined, 0, 0, true, {})
       const registerFn = mockSocketEventListeners.get('register')
 
       browser.init()