Skip to content

Commit 9a8c252

Browse files
mjhuffCarlos-fernandez
authored andcommitted
refactor(app-shell): Migrate desktop app notify connectivity to discovery-client (#14648)
Closes EXEC-304
1 parent 2382e83 commit 9a8c252

File tree

18 files changed

+1140
-477
lines changed

18 files changed

+1140
-477
lines changed

app-shell/src/__fixtures__/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
export * from './config'
2+
export * from './robots'

app-shell/src/__fixtures__/robots.ts

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
import { HEALTH_STATUS_NOT_OK, HEALTH_STATUS_OK } from '../constants'
2+
3+
export const mockLegacyHealthResponse = {
4+
name: 'opentrons-dev',
5+
api_version: '1.2.3',
6+
fw_version: '4.5.6',
7+
system_version: '7.8.9',
8+
robot_model: 'OT-2 Standard',
9+
}
10+
11+
export const mockLegacyServerHealthResponse = {
12+
name: 'opentrons-dev',
13+
apiServerVersion: '1.2.3',
14+
serialNumber: '12345',
15+
updateServerVersion: '1.2.3',
16+
smoothieVersion: '4.5.6',
17+
systemVersion: '7.8.9',
18+
}
19+
20+
export const MOCK_DISCOVERY_ROBOTS = [
21+
{
22+
name: 'opentrons-dev',
23+
health: mockLegacyHealthResponse,
24+
serverHealth: mockLegacyServerHealthResponse,
25+
addresses: [
26+
{
27+
ip: '10.14.19.50',
28+
port: 31950,
29+
seen: true,
30+
healthStatus: HEALTH_STATUS_OK,
31+
serverHealthStatus: HEALTH_STATUS_OK,
32+
healthError: null,
33+
serverHealthError: null,
34+
advertisedModel: null,
35+
},
36+
],
37+
},
38+
{
39+
name: 'opentrons-dev2',
40+
health: mockLegacyHealthResponse,
41+
serverHealth: mockLegacyServerHealthResponse,
42+
addresses: [
43+
{
44+
ip: '10.14.19.51',
45+
port: 31950,
46+
seen: true,
47+
healthStatus: HEALTH_STATUS_OK,
48+
serverHealthStatus: HEALTH_STATUS_OK,
49+
healthError: null,
50+
serverHealthError: null,
51+
advertisedModel: null,
52+
},
53+
],
54+
},
55+
{
56+
name: 'opentrons-dev3',
57+
health: mockLegacyHealthResponse,
58+
serverHealth: mockLegacyServerHealthResponse,
59+
addresses: [
60+
{
61+
ip: '10.14.19.52',
62+
port: 31950,
63+
seen: true,
64+
healthStatus: HEALTH_STATUS_NOT_OK,
65+
serverHealthStatus: HEALTH_STATUS_NOT_OK,
66+
healthError: null,
67+
serverHealthError: null,
68+
advertisedModel: null,
69+
},
70+
],
71+
},
72+
{
73+
name: 'opentrons-dev4',
74+
health: mockLegacyHealthResponse,
75+
serverHealth: mockLegacyServerHealthResponse,
76+
addresses: [
77+
{
78+
ip: '10.14.19.53',
79+
port: 31950,
80+
seen: true,
81+
healthStatus: HEALTH_STATUS_OK,
82+
serverHealthStatus: HEALTH_STATUS_OK,
83+
healthError: null,
84+
serverHealthError: null,
85+
advertisedModel: null,
86+
},
87+
],
88+
},
89+
]
90+
91+
export const MOCK_STORE_ROBOTS = [
92+
{
93+
robotName: 'opentrons-dev',
94+
ip: '10.14.19.50',
95+
},
96+
{
97+
robotName: 'opentrons-dev2',
98+
ip: '10.14.19.51',
99+
},
100+
{
101+
robotName: 'opentrons-dev3',
102+
ip: '10.14.19.52',
103+
},
104+
{
105+
robotName: 'opentrons-dev4',
106+
ip: '10.14.19.53',
107+
},
108+
]
109+
110+
export const MOCK_HEALTHY_ROBOTS = [
111+
{
112+
robotName: 'opentrons-dev',
113+
ip: '10.14.19.50',
114+
},
115+
{
116+
robotName: 'opentrons-dev2',
117+
ip: '10.14.19.51',
118+
},
119+
{
120+
robotName: 'opentrons-dev4',
121+
ip: '10.14.19.53',
122+
},
123+
]

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ vi.mock('../log', () => {
2727
},
2828
}
2929
})
30+
vi.mock('../notifications')
3031

3132
let mockGet = vi.fn(property => {
3233
return []

app-shell/src/config/actions.ts

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,6 @@ import {
7676
VALUE_UPDATED,
7777
VIEW_PROTOCOL_SOURCE_FOLDER,
7878
NOTIFY_SUBSCRIBE,
79-
NOTIFY_UNSUBSCRIBE,
8079
ROBOT_MASS_STORAGE_DEVICE_ADDED,
8180
ROBOT_MASS_STORAGE_DEVICE_ENUMERATED,
8281
ROBOT_MASS_STORAGE_DEVICE_REMOVED,
@@ -99,7 +98,6 @@ import type {
9998
AppRestartAction,
10099
NotifySubscribeAction,
101100
NotifyTopic,
102-
NotifyUnsubscribeAction,
103101
ReloadUiAction,
104102
RobotMassStorageDeviceAdded,
105103
RobotMassStorageDeviceEnumerated,
@@ -421,15 +419,3 @@ export const notifySubscribeAction = (
421419
},
422420
meta: { shell: true },
423421
})
424-
425-
export const notifyUnsubscribeAction = (
426-
hostname: string,
427-
topic: NotifyTopic
428-
): NotifyUnsubscribeAction => ({
429-
type: NOTIFY_UNSUBSCRIBE,
430-
payload: {
431-
hostname,
432-
topic,
433-
},
434-
meta: { shell: true },
435-
})

app-shell/src/constants.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -225,8 +225,6 @@ export const ROBOT_MASS_STORAGE_DEVICE_ENUMERATED: 'shell:ROBOT_MASS_STORAGE_DEV
225225
'shell:ROBOT_MASS_STORAGE_DEVICE_ENUMERATED'
226226
export const NOTIFY_SUBSCRIBE: 'shell:NOTIFY_SUBSCRIBE' =
227227
'shell:NOTIFY_SUBSCRIBE'
228-
export const NOTIFY_UNSUBSCRIBE: 'shell:NOTIFY_UNSUBSCRIBE' =
229-
'shell:NOTIFY_UNSUBSCRIBE'
230228

231229
// copy
232230
// TODO(mc, 2020-05-11): i18n
@@ -247,3 +245,9 @@ export const DISCOVERY_UPDATE_LIST: DISCOVERY_UPDATE_LIST_TYPE =
247245
export const DISCOVERY_REMOVE: DISCOVERY_REMOVE_TYPE = 'discovery:REMOVE'
248246

249247
export const CLEAR_CACHE: CLEAR_CACHE_TYPE = 'discovery:CLEAR_CACHE'
248+
export const HEALTH_STATUS_OK: 'ok' = 'ok'
249+
export const HEALTH_STATUS_NOT_OK: 'notOk' = 'notOk'
250+
export const FAILURE_STATUSES = {
251+
ECONNREFUSED: 'ECONNREFUSED',
252+
ECONNFAILED: 'ECONNFAILED',
253+
} as const

app-shell/src/discovery.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,27 +9,29 @@ import {
99
DEFAULT_PORT,
1010
} from '@opentrons/discovery-client'
1111
import {
12-
CLEAR_CACHE,
13-
DISCOVERY_FINISH,
14-
DISCOVERY_REMOVE,
15-
DISCOVERY_START,
16-
OPENTRONS_USB,
1712
UI_INITIALIZED,
1813
USB_HTTP_REQUESTS_START,
1914
USB_HTTP_REQUESTS_STOP,
20-
} from './constants'
15+
} from '@opentrons/app/src/redux/shell/actions'
16+
import {
17+
DISCOVERY_START,
18+
DISCOVERY_FINISH,
19+
DISCOVERY_REMOVE,
20+
CLEAR_CACHE,
21+
} from '@opentrons/app/src/redux/discovery/actions'
22+
import { OPENTRONS_USB } from '@opentrons/app/src/redux/discovery/constants'
2123

2224
import { getFullConfig, handleConfigChange } from './config'
2325
import { createLogger } from './log'
2426
import { getSerialPortHttpAgent } from './usb'
27+
import { handleNotificationConnectionsFor } from './notifications'
2528

2629
import type {
2730
Address,
2831
DiscoveryClientRobot,
2932
LegacyService,
3033
DiscoveryClient,
3134
} from '@opentrons/discovery-client'
32-
3335
import type { Action, Dispatch } from './types'
3436
import type { ConfigV1 } from '@opentrons/app/src/redux/config/schema-types'
3537

@@ -199,6 +201,7 @@ export function registerDiscovery(
199201

200202
function handleRobots(): void {
201203
const robots = client.getRobots()
204+
handleNotificationConnectionsFor(robots)
202205

203206
if (!disableCache) store.set('robots', robots)
204207

app-shell/src/main.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import { registerProtocolStorage } from './protocol-storage'
1919
import { getConfig, getStore, getOverrides, registerConfig } from './config'
2020
import { registerUsb } from './usb'
2121
import { createUsbDeviceMonitor } from './system-info/usb-devices'
22-
import { registerNotify, closeAllNotifyConnections } from './notify'
22+
import { registerNotify, closeAllNotifyConnections } from './notifications'
2323

2424
import type { BrowserWindow } from 'electron'
2525
import type { Dispatch, Logger } from './types'
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
import { vi, describe, expect, it } from 'vitest'
2+
3+
import {
4+
getHealthyRobotDataForNotifyConnections,
5+
cleanUpUnreachableRobots,
6+
establishConnections,
7+
closeConnectionsForcefullyFor,
8+
} from '../connect'
9+
import { connectionStore } from '../store'
10+
import { FAILURE_STATUSES } from '../../constants'
11+
import {
12+
MOCK_DISCOVERY_ROBOTS,
13+
MOCK_HEALTHY_ROBOTS,
14+
MOCK_STORE_ROBOTS,
15+
} from '../../__fixtures__'
16+
17+
vi.mock('electron-store')
18+
vi.mock('../notifyLog', () => {
19+
return {
20+
createLogger: () => {
21+
return { debug: () => null }
22+
},
23+
notifyLog: { debug: vi.fn(), warn: vi.fn() },
24+
}
25+
})
26+
27+
describe('getHealthyRobotDataForNotifyConnections', () => {
28+
it('should filter a list of discovery robots, only returning robots that have a health status of ok', () => {
29+
const healthyRobots = getHealthyRobotDataForNotifyConnections(
30+
MOCK_DISCOVERY_ROBOTS
31+
)
32+
expect(healthyRobots).toEqual(MOCK_HEALTHY_ROBOTS)
33+
})
34+
})
35+
36+
describe('cleanUpUnreachableRobots', () => {
37+
it('should close connections forcefully for unreachable robots and resolve them', async () => {
38+
MOCK_STORE_ROBOTS.forEach(robot => {
39+
void connectionStore
40+
.setPendingConnection(robot.robotName)
41+
.then(() =>
42+
connectionStore.setConnected(robot.robotName, vi.fn() as any)
43+
)
44+
})
45+
const unreachableRobots = await cleanUpUnreachableRobots(
46+
MOCK_HEALTHY_ROBOTS
47+
)
48+
expect(unreachableRobots).toEqual(['opentrons-dev3'])
49+
})
50+
})
51+
52+
describe('establishConnections', () => {
53+
it('should not resolve any new connections if all reported robots are already in the connection store and connected', async () => {
54+
connectionStore.clearStore()
55+
MOCK_STORE_ROBOTS.forEach(robot => {
56+
void connectionStore
57+
.setPendingConnection(robot.robotName)
58+
.then(() =>
59+
connectionStore.setConnected(robot.robotName, vi.fn() as any)
60+
)
61+
})
62+
63+
const newRobots = await establishConnections(MOCK_HEALTHY_ROBOTS)
64+
expect(newRobots).toEqual([])
65+
})
66+
67+
it('should not attempt to connect to a robot if it a known notification port blocked robot', async () => {
68+
await connectionStore.setErrorStatus(
69+
'10.14.19.51',
70+
FAILURE_STATUSES.ECONNREFUSED
71+
)
72+
connectionStore.clearStore()
73+
74+
const newRobots = await establishConnections(MOCK_HEALTHY_ROBOTS)
75+
expect(newRobots).toEqual([
76+
{ ip: '10.14.19.50', robotName: 'opentrons-dev' },
77+
{ ip: '10.14.19.53', robotName: 'opentrons-dev4' },
78+
])
79+
})
80+
81+
it('should not report a robot as new if it is connecting', async () => {
82+
connectionStore.clearStore()
83+
MOCK_STORE_ROBOTS.forEach(robot => {
84+
void connectionStore.setPendingConnection(robot.robotName)
85+
})
86+
87+
const newRobots = await establishConnections(MOCK_HEALTHY_ROBOTS)
88+
expect(newRobots).toEqual([])
89+
})
90+
91+
it('should create a new entry in the connection store for a new robot', async () => {
92+
connectionStore.clearStore()
93+
await establishConnections(MOCK_HEALTHY_ROBOTS)
94+
console.log(connectionStore)
95+
expect(connectionStore.getRobotNameByIP('10.14.19.50')).not.toBeNull()
96+
})
97+
})
98+
99+
describe('closeConnectionsForcefullyFor', () => {
100+
it('should return an array of promises for each closing connection and resolve after closing connections', async () => {
101+
connectionStore.clearStore()
102+
MOCK_STORE_ROBOTS.forEach(robot => {
103+
void connectionStore
104+
.setPendingConnection(robot.robotName)
105+
.then(() =>
106+
connectionStore.setConnected(robot.robotName, vi.fn() as any)
107+
)
108+
})
109+
const closingRobots = closeConnectionsForcefullyFor([
110+
'opentrons-dev',
111+
'opentrons-dev2',
112+
])
113+
closingRobots.forEach(robot => expect(robot).toBeInstanceOf(Promise))
114+
})
115+
})
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import { describe, expect, it } from 'vitest'
2+
3+
import { deserializeExpectedMessages } from '../deserialize'
4+
5+
import type { NotifyResponseData } from '@opentrons/app/src/redux/shell/types'
6+
7+
const MOCK_VALID_RESPONSE: NotifyResponseData = { refetchUsingHTTP: true }
8+
const MOCK_VALID_STRING_RESPONSE = JSON.stringify(MOCK_VALID_RESPONSE)
9+
const MOCK_INVALID_OBJECT = JSON.stringify({ test: 'MOCK_RESPONSE' })
10+
const MOCK_INVALID_STRING = 'MOCK_STRING'
11+
12+
describe('closeConnectionsForcefullyFor', () => {
13+
it('should resolve with the deserialized message if it is a valid notify response', async () => {
14+
const response = await deserializeExpectedMessages(
15+
MOCK_VALID_STRING_RESPONSE
16+
)
17+
expect(response).toEqual(MOCK_VALID_RESPONSE)
18+
})
19+
20+
it('should reject with an error if the deserialized message is not a valid notify response', async () => {
21+
const responsePromise = deserializeExpectedMessages(MOCK_INVALID_OBJECT)
22+
await expect(responsePromise).rejects.toThrowError(
23+
'Unexpected data received from notify broker: {"test":"MOCK_RESPONSE"}'
24+
)
25+
})
26+
27+
it('should reject with an error if the message cannot be deserialized', async () => {
28+
const responsePromise = deserializeExpectedMessages(MOCK_INVALID_STRING)
29+
await expect(responsePromise).rejects.toThrowError(
30+
'Unexpected data received from notify broker: MOCK_STRING'
31+
)
32+
})
33+
})

0 commit comments

Comments
 (0)