refactor(server): refactoring of lib/browser (#3171)

* refactor(config): small refactoring of lib/config * refactor(server): refactoring of lib/browser
karma-runner · Oct 11, 2018 · 8efb28d · 8efb28d
1 parent dab8a82
commit 8efb28d
Show file tree

Hide file tree

Showing 6 changed files with 133 additions and 188 deletions.
diff --git a/lib/browser.js b/lib/browser.js
@@ -1,31 +1,22 @@
 'use strict'
 
-const Result = require('./browser_result')
+const BrowserResult = require('./browser_result')
 const helper = require('./helper')
 const logger = require('./logger')
 
-// The browser is connected but not yet been commanded to execute tests.
-const CONNECTED = 1
-
-// The browser has been told to execute tests; it is configuring before tests execution.
-const CONFIGURING = 2
-
-// The browser is executing the tests.
-const EXECUTING = 3
-
-// The browser is executing the tests, but temporarily disconnect (waiting for reconnecting).
-const EXECUTING_DISCONNECTED = 4
-
-// The browser got permanently disconnected (being removed from the collection and destroyed).
-const DISCONNECTED = 5
+const CONNECTED = 1 // The browser is connected but not yet been commanded to execute tests.
+const CONFIGURING = 2 // The browser has been told to execute tests; it is configuring before tests execution.
+const EXECUTING = 3 // The browser is executing the tests.
+const EXECUTING_DISCONNECTED = 4 // The browser is executing the tests, but temporarily disconnect (waiting for reconnecting).
+const DISCONNECTED = 5 // The browser got permanently disconnected (being removed from the collection and destroyed).
 
 class Browser {
   constructor (id, fullName, collection, emitter, socket, timer, disconnectDelay, noActivityTimeout) {
     this.id = id
     this.fullName = fullName
     this.name = helper.browserFullNameToShort(fullName)
     this.state = CONNECTED
-    this.lastResult = new Result()
+    this.lastResult = new BrowserResult()
     this.disconnectsCount = 0
     this.activeSockets = [socket]
     this.noActivityTimeout = noActivityTimeout
@@ -42,118 +33,77 @@ class Browser {
   }
 
   init () {
-    this.collection.add(this)
+    this.log.info(`Connected on socket ${this.socket.id} with id ${this.id}`)
 
     this.bindSocketEvents(this.socket)
-
-    this.log.info('Connected on socket %s with id %s', this.socket.id, this.id)
-
-    // TODO(vojta): move to collection
-    this.emitter.emit('browsers_change', this.collection)
-
+    this.collection.add(this)
+    this.emitter.emit('browsers_change', this.collection) // TODO(vojta): move to collection
     this.emitter.emit('browser_register', this)
   }
 
-  isConnected () {
-    return this.state === CONNECTED
-  }
-
-  toString () {
-    return this.name
-  }
-
-  toJSON () {
-    return {
-      id: this.id,
-      fullName: this.fullName,
-      name: this.name,
-      state: this.state,
-      lastResult: this.lastResult,
-      disconnectsCount: this.disconnectsCount,
-      noActivityTimeout: this.noActivityTimeout,
-      disconnectDelay: this.disconnectDelay
-    }
-  }
-
   onKarmaError (error) {
-    if (this.isConnected()) {
-      return
+    if (this.isNotConnected()) {
+      this.lastResult.error = true
+      this.emitter.emit('browser_error', this, error)
+      this.refreshNoActivityTimeout()
     }
-
-    this.lastResult.error = true
-    this.emitter.emit('browser_error', this, error)
-
-    this.refreshNoActivityTimeout()
   }
 
   onInfo (info) {
-    if (this.isConnected()) {
-      return
-    }
-
-    // TODO(vojta): remove
-    if (helper.isDefined(info.dump)) {
-      this.emitter.emit('browser_log', this, info.dump, 'dump')
-    }
+    if (this.isNotConnected()) {
+      if (helper.isDefined(info.dump)) {
+        this.emitter.emit('browser_log', this, info.dump, 'dump')
+      }
 
-    if (helper.isDefined(info.log)) {
-      this.emitter.emit('browser_log', this, info.log, info.type)
-    }
+      if (helper.isDefined(info.log)) {
+        this.emitter.emit('browser_log', this, info.log, info.type)
+      } else if (!helper.isDefined(info.dump)) {
+        this.emitter.emit('browser_info', this, info)
+      }
 
-    if (
-      !helper.isDefined(info.log) &&
-      !helper.isDefined(info.dump)
-    ) {
-      this.emitter.emit('browser_info', this, info)
+      this.refreshNoActivityTimeout()
     }
-
-    this.refreshNoActivityTimeout()
   }
 
   onStart (info) {
-    this.lastResult = new Result()
-    this.lastResult.total = info.total
-
-    this.state = EXECUTING
-
     if (info.total === null) {
       this.log.warn('Adapter did not report total number of specs.')
     }
 
+    this.lastResult = new BrowserResult(info.total)
+    this.state = EXECUTING
     this.emitter.emit('browser_start', this, info)
     this.refreshNoActivityTimeout()
   }
 
   onComplete (result) {
-    if (this.isConnected()) {
-      return
-    }
-
-    this.state = CONNECTED
-    this.lastResult.totalTimeEnd()
+    if (this.isNotConnected()) {
+      this.state = CONNECTED
+      this.lastResult.totalTimeEnd()
 
-    if (!this.lastResult.success) {
-      this.lastResult.error = true
-    }
+      if (!this.lastResult.success) {
+        this.lastResult.error = true
+      }
 
-    this.emitter.emit('browsers_change', this.collection)
-    this.emitter.emit('browser_complete', this, result)
+      this.emitter.emit('browsers_change', this.collection)
+      this.emitter.emit('browser_complete', this, result)
 
-    this.clearNoActivityTimeout()
+      this.clearNoActivityTimeout()
+    }
   }
 
   onDisconnect (reason, disconnectedSocket) {
     helper.arrayRemove(this.activeSockets, disconnectedSocket)
 
     if (this.activeSockets.length) {
-      this.log.debug('Disconnected %s, still have %s', disconnectedSocket.id, this.getActiveSocketsIds())
+      this.log.debug(`Disconnected ${disconnectedSocket.id}, still have ${this.getActiveSocketsIds()}`)
       return
     }
 
-    if (this.state === CONNECTED) {
-      this.disconnect(`client disconnected from CONNECTED state (${reason})`)
-    } else if (this.state === CONFIGURING || this.state === EXECUTING) {
-      this.log.debug('Disconnected during run, waiting %sms for reconnecting.', this.disconnectDelay)
+    if (this.isConnected()) {
+      this.disconnect(`Client disconnected from CONNECTED state (${reason})`)
+    } else if ([CONFIGURING, EXECUTING].includes(this.state)) {
+      this.log.debug(`Disconnected during run, waiting ${this.disconnectDelay}ms for reconnecting.`)
       this.state = EXECUTING_DISCONNECTED
 
       this.pendingDisconnect = this.timer.setTimeout(() => {
@@ -169,23 +119,20 @@ class Browser {
 
   reconnect (newSocket) {
     if (this.state === EXECUTING_DISCONNECTED) {
+      this.log.debug(`Reconnected on ${newSocket.id}.`)
       this.state = EXECUTING
-      this.log.debug('Reconnected on %s.', newSocket.id)
-    } else if (this.state === CONNECTED || this.state === CONFIGURING || this.state === EXECUTING) {
-      this.log.debug('New connection %s (already have %s)', newSocket.id, this.getActiveSocketsIds())
+    } else if ([CONNECTED, CONFIGURING, EXECUTING].includes(this.state)) {
+      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}`)
       this.state = CONNECTED
-      this.log.info('Connected on socket %s with id %s', newSocket.id, this.id)
-      this.collection.add(this)
-
-      // TODO(vojta): move to collection
-      this.emitter.emit('browsers_change', this.collection)
 
+      this.collection.add(this)
+      this.emitter.emit('browsers_change', this.collection) // TODO(vojta): move to collection
       this.emitter.emit('browser_register', this)
     }
 
-    const exists = this.activeSockets.some((s) => s.id === newSocket.id)
-    if (!exists) {
+    if (!this.activeSockets.some((s) => s.id === newSocket.id)) {
       this.activeSockets.push(newSocket)
       this.bindSocketEvents(newSocket)
     }
@@ -199,33 +146,18 @@ class Browser {
 
   onResult (result) {
     if (result.length) {
-      return result.forEach(this.onResult, this)
-    }
-
-    // ignore - probably results from last run (after server disconnecting)
-    if (this.isConnected()) {
+      result.forEach(this.onResult, this)
       return
+    } else if (this.isNotConnected()) {
+      this.lastResult.add(result)
+      this.emitter.emit('spec_complete', this, result)
     }
-
-    this.lastResult.add(result)
-
-    this.emitter.emit('spec_complete', this, result)
     this.refreshNoActivityTimeout()
   }
 
-  serialize () {
-    return {
-      id: this.id,
-      name: this.name,
-      isConnected: this.state === CONNECTED
-    }
-  }
-
   execute (config) {
     this.activeSockets.forEach((socket) => socket.emit('execute', config))
-
     this.state = CONFIGURING
-
     this.refreshNoActivityTimeout()
   }
 
@@ -234,10 +166,10 @@ class Browser {
   }
 
   disconnect (reason) {
+    this.log.warn(`Disconnected (${this.disconnectsCount} times)${reason || ''}`)
     this.state = DISCONNECTED
     this.disconnectsCount++
-    this.log.warn('Disconnected (%d times)' + (reason || ''), this.disconnectsCount)
-    this.emitter.emit('browser_error', this, 'Disconnected' + (reason || ''))
+    this.emitter.emit('browser_error', this, `Disconnected${reason || ''}`)
     this.collection.remove(this)
   }
 
@@ -248,18 +180,16 @@ class Browser {
       this.noActivityTimeoutId = this.timer.setTimeout(() => {
         this.lastResult.totalTimeEnd()
         this.lastResult.disconnected = true
-        this.disconnect(', because no message in ' + this.noActivityTimeout + ' ms.')
+        this.disconnect(`, because no message in ${this.noActivityTimeout} ms.`)
         this.emitter.emit('browser_complete', this)
       }, this.noActivityTimeout)
     }
   }
 
   clearNoActivityTimeout () {
-    if (this.noActivityTimeout) {
-      if (this.noActivityTimeoutId) {
-        this.timer.clearTimeout(this.noActivityTimeoutId)
-        this.noActivityTimeoutId = null
-      }
+    if (this.noActivityTimeout && this.noActivityTimeoutId) {
+      this.timer.clearTimeout(this.noActivityTimeoutId)
+      this.noActivityTimeoutId = null
     }
   }
 
@@ -272,6 +202,39 @@ class Browser {
     socket.on('info', (info) => this.onInfo(info))
     socket.on('result', (result) => this.onResult(result))
   }
+
+  isConnected () {
+    return this.state === CONNECTED
+  }
+
+  isNotConnected () {
+    return !this.isConnected()
+  }
+
+  serialize () {
+    return {
+      id: this.id,
+      name: this.name,
+      isConnected: this.state === CONNECTED
+    }
+  }
+
+  toString () {
+    return this.name
+  }
+
+  toJSON () {
+    return {
+      id: this.id,
+      fullName: this.fullName,
+      name: this.name,
+      state: this.state,
+      lastResult: this.lastResult,
+      disconnectsCount: this.disconnectsCount,
+      noActivityTimeout: this.noActivityTimeout,
+      disconnectDelay: this.disconnectDelay
+    }
+  }
 }
 
 Browser.factory = function (

diff --git a/lib/browser_collection.js b/lib/browser_collection.js
@@ -1,6 +1,6 @@
 'use strict'
 
-const Result = require('./browser_result')
+const BrowserResult = require('./browser_result')
 const helper = require('./helper')
 
 class BrowserCollection {
@@ -67,7 +67,7 @@ class BrowserCollection {
 
   clearResults () {
     this.browsers.forEach((browser) => {
-      browser.lastResult = new Result()
+      browser.lastResult = new BrowserResult()
     })
   }
 

diff --git a/lib/browser_result.js b/lib/browser_result.js
@@ -1,10 +1,11 @@
 'use strict'
 
 class BrowserResult {
-  constructor () {
+  constructor (total = 0) {
     this.startTime = Date.now()
 
-    this.total = this.skipped = this.failed = this.success = 0
+    this.total = total
+    this.skipped = this.failed = this.success = 0
     this.netTime = this.totalTime = 0
     this.disconnected = this.error = false
   }

diff --git a/lib/helper.js b/lib/helper.js
@@ -9,8 +9,8 @@ const mm = require('minimatch')
 
 exports.browserFullNameToShort = (fullName) => {
   const agent = useragent.parse(fullName)
-  const isKnown = agent.family !== 'Other' && agent.os.family !== 'Other'
-  return isKnown ? agent.toAgent() + ' (' + agent.os + ')' : fullName
+  const isUnknown = agent.family === 'Other' || agent.os.family === 'Other'
+  return isUnknown ? fullName : `${agent.toAgent()} (${agent.os})`
 }
 
 exports.isDefined = (value) => {