feat(app): Persist known robots to file-system when using new discove…

…ry (#2065)

app-shell/Makefile

@@ -50,7 +50,8 @@ electron := electron . \
 	--log.level.console="debug" \
 	--disable_ui.webPreferences.webSecurity \
 	--ui.url.protocol="http:" \
-	--ui.url.path="localhost:$(port)"
+	--ui.url.path="localhost:$(port)" \
+	--discovery.candidates=localhost
 
 # standard targets
 #####################################################################

app-shell/__mocks__/electron-store.js

@@ -0,0 +1,22 @@
+// mock electron-store
+'use strict'
+
+const Store = jest.fn()
+
+const mockStore = {
+  set: jest.fn(),
+  get: jest.fn(),
+  has: jest.fn(),
+  delete: jest.fn(),
+  clear: jest.fn(),
+  onDidChange: jest.fn(),
+  openInEditor: jest.fn()
+}
+
+module.exports = Store
+module.exports.__store = mockStore
+module.exports.__mockReset = () => {
+  Object.values(mockStore).forEach(m => m.mockReset())
+  Store.mockReset()
+  Store.mockImplementation(() => mockStore)
+}

app-shell/src/__tests__/discovery.test.js

@@ -1,9 +1,11 @@
 // tests for the app-shell's discovery module
 import EventEmitter from 'events'
+import Store from 'electron-store'
 import DiscoveryClient from '@opentrons/discovery-client'
 import {registerDiscovery} from '../discovery'
-import {getConfig} from '../config'
+import {getConfig, getOverrides} from '../config'
 
+jest.mock('electron-store')
 jest.mock('@opentrons/discovery-client')
 jest.mock('../log')
 jest.mock('../config')
@@ -23,12 +25,16 @@ describe('app-shell/discovery', () => {
       setPollInterval: jest.fn().mockReturnThis()
     })
 
-    getConfig.mockReturnValue({
-      candidates: []
-    })
+    getConfig.mockReturnValue({enabled: true, candidates: []})
+    getOverrides.mockReturnValue({})
 
     dispatch = jest.fn()
     DiscoveryClient.mockReturnValue(mockClient)
+    Store.__mockReset()
+    Store.__store.get.mockImplementation(key => {
+      if (key === 'services') return []
+      return null
+    })
   })
 
   afterEach(() => {
@@ -40,21 +46,61 @@ describe('app-shell/discovery', () => {
 
     expect(DiscoveryClient).toHaveBeenCalledWith(
       expect.objectContaining({
-        nameFilter: /^opentrons/i,
-        pollInterval: 5000,
-        candidates: []
+        pollInterval: expect.any(Number),
+        // support for legacy IPv6 wired robots
+        candidates: ['[fd00:0:cafe:fefe::1]'],
+        services: []
       })
     )
   })
 
+  test('calls client.start on discovery registration', () => {
+    registerDiscovery(dispatch)
+    expect(mockClient.start).toHaveBeenCalled()
+  })
+
   test('calls client.start on "discovery:START"', () => {
     registerDiscovery(dispatch)({type: 'discovery:START'})
-    expect(mockClient.start).toHaveBeenCalled()
+    expect(mockClient.start).toHaveBeenCalledTimes(2)
   })
 
-  test('sets client to slow poll on "discovery:FINISH"', () => {
-    registerDiscovery(dispatch)({type: 'discovery:FINISH'})
-    expect(mockClient.setPollInterval).toHaveBeenCalledWith(15000)
+  test('sets poll speed on "discovery:START" and "discovery:FINISH"', () => {
+    const handleAction = registerDiscovery(dispatch)
+
+    handleAction({type: 'discovery:START'})
+    expect(mockClient.setPollInterval).toHaveBeenLastCalledWith(
+      expect.any(Number)
+    )
+    handleAction({type: 'discovery:FINISH'})
+    expect(mockClient.setPollInterval).toHaveBeenLastCalledWith(
+      expect.any(Number)
+    )
+
+    expect(mockClient.setPollInterval).toHaveBeenCalledTimes(2)
+    const fastPoll = mockClient.setPollInterval.mock.calls[0][0]
+    const slowPoll = mockClient.setPollInterval.mock.calls[1][0]
+    expect(fastPoll).toBeLessThan(slowPoll)
+  })
+
+  test('always sends "discovery:UPDATE_LIST" on "discovery:START"', () => {
+    mockClient.services = [
+      {name: 'opentrons-dev', ip: '192.168.1.42', port: 31950, ok: true}
+    ]
+
+    const expected = [
+      {
+        name: 'opentrons-dev',
+        connections: [
+          {ip: '192.168.1.42', port: 31950, ok: true, local: false}
+        ]
+      }
+    ]
+
+    registerDiscovery(dispatch)({type: 'discovery:START'})
+    expect(dispatch).toHaveBeenCalledWith({
+      type: 'discovery:UPDATE_LIST',
+      payload: {robots: expected}
+    })
   })
 
   describe('"service" event handling', () => {
@@ -69,7 +115,9 @@ describe('app-shell/discovery', () => {
         expected: [
           {
             name: 'opentrons-dev',
-            connections: [{ip: '192.168.1.42', port: 31950, ok: true, local: false}]
+            connections: [
+              {ip: '192.168.1.42', port: 31950, ok: true, local: false}
+            ]
           }
         ]
       },
@@ -112,14 +160,100 @@ describe('app-shell/discovery', () => {
       }
     ]
 
-    SPECS.forEach(spec => test(spec.name, () => {
-      mockClient.services = spec.services
+    SPECS.forEach(spec =>
+      test(spec.name, () => {
+        mockClient.services = spec.services
+
+        mockClient.emit('service')
+        expect(dispatch).toHaveBeenCalledWith({
+          type: 'discovery:UPDATE_LIST',
+          payload: {robots: spec.expected}
+        })
+      })
+    )
+  })
+
+  test('stores services to file on service events', () => {
+    registerDiscovery(dispatch)
+    expect(Store).toHaveBeenCalledWith({
+      name: 'discovery',
+      defaults: {services: []}
+    })
+
+    mockClient.services = [{name: 'foo'}, {name: 'bar'}]
+    mockClient.emit('service')
+    expect(Store.__store.set).toHaveBeenCalledWith('services', [
+      {name: 'foo'},
+      {name: 'bar'}
+    ])
+
+    mockClient.services = [{name: 'foo'}]
+    mockClient.emit('serviceRemoved')
+    expect(Store.__store.set).toHaveBeenCalledWith('services', [{name: 'foo'}])
+  })
+
+  test('loads services from file on client initialization', () => {
+    Store.__store.get.mockImplementation(key => {
+      if (key === 'services') return [{name: 'foo'}]
+      return null
+    })
+
+    registerDiscovery(dispatch)
+    expect(DiscoveryClient).toHaveBeenCalledWith(
+      expect.objectContaining({
+        services: [{name: 'foo'}]
+      })
+    )
+  })
+
+  test('loads candidates from config on client initialization', () => {
+    getConfig.mockReturnValue({enabled: true, candidates: ['1.2.3.4']})
+    registerDiscovery(dispatch)
+
+    expect(DiscoveryClient).toHaveBeenCalledWith(
+      expect.objectContaining({
+        candidates: expect.arrayContaining(['1.2.3.4'])
+      })
+    )
+  })
+
+  // ensures config override works with only one candidate specified
+  test('canidates in config can be single value', () => {
+    getConfig.mockReturnValue({enabled: true, candidates: '1.2.3.4'})
+    registerDiscovery(dispatch)
 
-      mockClient.emit('service')
-      expect(dispatch).toHaveBeenCalledWith({
-        type: 'discovery:UPDATE_LIST',
-        payload: {robots: spec.expected}
+    expect(DiscoveryClient).toHaveBeenCalledWith(
+      expect.objectContaining({
+        candidates: expect.arrayContaining(['1.2.3.4'])
       })
-    }))
+    )
+  })
+
+  test('services from overridden canidates are not persisted', () => {
+    getConfig.mockReturnValue({enabled: true, candidates: 'localhost'})
+    getOverrides.mockImplementation(key => {
+      if (key === 'discovery.candidates') return ['1.2.3.4', '5.6.7.8']
+      return null
+    })
+
+    registerDiscovery(dispatch)
+
+    mockClient.services = [{name: 'foo', ip: '5.6.7.8'}, {name: 'bar'}]
+    mockClient.emit('service')
+    expect(Store.__store.set).toHaveBeenCalledWith('services', [{name: 'bar'}])
+  })
+
+  test('service from overridden single candidate is not persisted', () => {
+    getConfig.mockReturnValue({enabled: true, candidates: 'localhost'})
+    getOverrides.mockImplementation(key => {
+      if (key === 'discovery.candidates') return '1.2.3.4'
+      return null
+    })
+
+    registerDiscovery(dispatch)
+
+    mockClient.services = [{name: 'foo', ip: '1.2.3.4'}, {name: 'bar'}]
+    mockClient.emit('service')
+    expect(Store.__store.set).toHaveBeenCalledWith('services', [{name: 'bar'}])
   })
 })

app-shell/src/config.js

@@ -1,3 +1,4 @@
+// @flow
 // app configuration and settings
 import Store from 'electron-store'
 import mergeOptions from 'merge-options'
@@ -8,7 +9,12 @@ import yargsParser from 'yargs-parser'
 import pkg from '../package.json'
 import createLogger from './log'
 
+// TODO(mc, 2018-08-08): figure out type exports from app
+import type {Action} from '@opentrons/app/src/types'
+import type {Config} from '@opentrons/app/src/config'
+
 // make sure all arguments are included in production
+// $FlowFixMe: process.defaultApp exists in electron
 const argv = process.defaultApp
   ? process.argv.slice(2)
   : process.argv.slice(1)
@@ -21,7 +27,7 @@ const PARSE_ARGS_OPTS = {
 }
 
 // TODO(mc, 2018-05-25): future config changes may require migration strategy
-const DEFAULTS = {
+const DEFAULTS: Config = {
   devtools: false,
 
   modules: false,
@@ -84,11 +90,11 @@ const overrides = () => _over || (_over = yargsParser(argv, PARSE_ARGS_OPTS))
 const log = () => _log || (_log = createLogger(__filename))
 
 // initialize and register the config module with dispatches from the UI
-export function registerConfig (dispatch) {
-  return function handleIncomingAction (action) {
-    const {type, payload} = action
+export function registerConfig (dispatch: Action => void) {
+  return function handleIncomingAction (action: Action) {
+    if (action.type === 'config:UPDATE') {
+      const {payload} = action
 
-    if (type === 'config:UPDATE') {
       log().debug('Handling config:UPDATE', payload)
 
       if (getIn(overrides(), payload.path) != null) {
@@ -106,11 +112,11 @@ export function getStore () {
   return store().store
 }
 
-export function getOverrides () {
-  return overrides()
+export function getOverrides (path?: string) {
+  return getIn(overrides(), path)
 }
 
-export function getConfig (path) {
+export function getConfig (path?: string) {
   const result = store().get(path)
   const over = getIn(overrides(), path)