fix(test): mark all second connections reconnects (#3598)

johnjbarton · web-flow · commit 1c9c2de54fa1 · 2020-12-23T14:06:36.000-08:00
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.
diff --git 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
@@ -299,9 +301,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
@@ -11,14 +11,17 @@ 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)
     this.lastResult = new BrowserResult()
     this.disconnectsCount = 0
     this.activeSockets = [socket]
     this.noActivityTimeout = noActivityTimeout
+    this.singleRun = singleRun
+    this.clientConfig = clientConfig
     this.collection = collection
     this.emitter = emitter
     this.socket = socket
@@ -94,9 +97,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 +121,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 +166,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 +177,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 +210,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 +255,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
@@ -261,27 +261,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]
@@ -290,7 +275,7 @@ class Server extends KarmaEventEmitter {
           newBrowser.init()
 
           if (config.singleRun) {
-            newBrowser.execute(config.client)
+            newBrowser.execute()
             singleRunBrowsers.add(newBrowser)
           }
         }
@@ -334,8 +319,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
@@ -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
@@ -309,9 +311,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
@@ -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
@@ -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
@@ -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
diff --git a/test/unit/server.spec.js b/test/unit/server.spec.js

Original file line number	Diff line number	Diff line change
`@@ -41,9 +41,11 @@ function Karma (socket, iframe, opener, navigator, location, document) {`
`41`	`41`	`}`
`42`	`42`	`}`
`43`	`43`
`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.`
`47`	`49`	`var socketReconnect = false`
`48`	`50`
`49`	`51`	`this.VERSION = constant.VERSION`
`@@ -299,9 +301,6 @@ function Karma (socket, iframe, opener, navigator, location, document) {`
`299`	`301`	`info.displayName = displayName`
`300`	`302`	`}`
`301`	`303`	`socket.emit('register', info)`
`302`		`- })`
`303`		`-`
`304`		`- socket.on('reconnect', function () {`
`305`	`304`	`socketReconnect = true`
`306`	`305`	`})`
`307`	`306`	`}`
Original file line number	Diff line number	Diff line change
`@@ -51,9 +51,11 @@ function Karma (socket, iframe, opener, navigator, location, document) {`
`51`	`51`	`}`
`52`	`52`	`}`
`53`	`53`
`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.`
`57`	`59`	`var socketReconnect = false`
`58`	`60`
`59`	`61`	`this.VERSION = constant.VERSION`
`@@ -309,9 +311,6 @@ function Karma (socket, iframe, opener, navigator, location, document) {`
`309`	`311`	`info.displayName = displayName`
`310`	`312`	`}`
`311`	`313`	`socket.emit('register', info)`
`312`		`- })`
`313`		`-`
`314`		`- socket.on('reconnect', function () {`
`315`	`314`	`socketReconnect = true`
`316`	`315`	`})`
`317`	`316`	`}`