-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(app): Collect system USB information #5482
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
4f27b0f
wip: start work on collecting usb device info
mcous a6b4036
wip: get usb detection working end-to-end
mcous d2a8357
wip: get electron-rebuild task properly configured
mcous 1c6c639
wip: ensure hex codes are padded in PowerShell driver version call
mcous c958061
wip: add U2E information to analytics and support profiles
mcous b8cb7f2
wip: install libusb-dev on linux to keep make install happy
mcous b66ab00
Merge branch 'edge' into app-shell_system-info-collection
mcous a9cc27e
fixup: upgrade usb-detection for prebuilt binaries
mcous 821cc64
fixup: add build dep to js e2e test
mcous c2b4ca7
fixup: add build dep to js e2e test (part 2)
mcous 3e827d3
fixup: clean up usb-detection event listeners on stop monitoring
mcous c2cbee6
fixup: teak support profile TODO for pipette + robot info
mcous 9bc408c
fixup: remove unnecessary comment
mcous 37f2997
fixup: move analytics out from behind FF
mcous File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
// @flow | ||
// os helpers | ||
|
||
export const isWindows = () => process.platform === 'win32' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,156 @@ | ||
// @flow | ||
import noop from 'lodash/noop' | ||
import { app } from 'electron' | ||
import * as Fixtures from '@opentrons/app/src/system-info/__fixtures__' | ||
import * as SystemInfo from '@opentrons/app/src/system-info' | ||
import { uiInitialized } from '@opentrons/app/src/shell' | ||
import * as OS from '../../os' | ||
import * as UsbDevices from '../usb-devices' | ||
import { registerSystemInfo } from '..' | ||
|
||
import type { | ||
Device, | ||
UsbDeviceMonitor, | ||
UsbDeviceMonitorOptions, | ||
} from '../usb-devices' | ||
|
||
jest.mock('../../os') | ||
jest.mock('../usb-devices') | ||
|
||
const createUsbDeviceMonitor: JestMockFn< | ||
[UsbDeviceMonitorOptions | void], | ||
UsbDeviceMonitor | ||
> = UsbDevices.createUsbDeviceMonitor | ||
|
||
const getWindowsDriverVersion: JestMockFn<[Device], any> = | ||
UsbDevices.getWindowsDriverVersion | ||
|
||
const isWindows: JestMockFn<[], boolean> = OS.isWindows | ||
|
||
const flush = () => new Promise(resolve => setTimeout(resolve, 0)) | ||
|
||
describe('app-shell::system-info module action tests', () => { | ||
const dispatch = jest.fn() | ||
const getAllDevices: JestMockFn<[], any> = jest.fn() | ||
const stop = jest.fn() | ||
const monitor: $Shape<UsbDeviceMonitor> = { getAllDevices, stop } | ||
const { windowsDriverVersion: _, ...notRealtek } = Fixtures.mockUsbDevice | ||
const realtek0 = { ...notRealtek, manufacturer: 'Realtek' } | ||
const realtek1 = { ...notRealtek, manufacturer: 'realtek' } | ||
let handler | ||
|
||
beforeEach(() => { | ||
handler = registerSystemInfo(dispatch) | ||
isWindows.mockReturnValue(false) | ||
createUsbDeviceMonitor.mockReturnValue(monitor) | ||
getAllDevices.mockResolvedValue([realtek0]) | ||
}) | ||
|
||
afterEach(() => { | ||
jest.resetAllMocks() | ||
}) | ||
|
||
it('sends initial USB device list on shell:UI_INITIALIZED', () => { | ||
handler(uiInitialized()) | ||
|
||
return flush().then(() => { | ||
expect(dispatch).toHaveBeenCalledWith(SystemInfo.initialized([realtek0])) | ||
expect(getWindowsDriverVersion).toHaveBeenCalledTimes(0) | ||
}) | ||
}) | ||
|
||
it('will not initialize multiple monitors', () => { | ||
handler(uiInitialized()) | ||
handler(uiInitialized()) | ||
|
||
return flush().then(() => { | ||
expect(createUsbDeviceMonitor).toHaveBeenCalledTimes(1) | ||
expect(dispatch).toHaveBeenCalledTimes(1) | ||
}) | ||
}) | ||
|
||
it('sends systemInfo:USB_DEVICE_ADDED when device added', () => { | ||
handler(uiInitialized()) | ||
const monitorOptions = createUsbDeviceMonitor.mock.calls[0][0] | ||
|
||
expect(monitorOptions?.onDeviceAdd).toEqual(expect.any(Function)) | ||
const onDeviceAdd = monitorOptions?.onDeviceAdd ?? noop | ||
onDeviceAdd(realtek0) | ||
|
||
return flush().then(() => { | ||
expect(dispatch).toHaveBeenCalledWith(SystemInfo.usbDeviceAdded(realtek0)) | ||
expect(getWindowsDriverVersion).toHaveBeenCalledTimes(0) | ||
}) | ||
}) | ||
|
||
it('sends systemInfo:USB_DEVICE_REMOVED when device removed', () => { | ||
handler(uiInitialized()) | ||
const monitorOptions = createUsbDeviceMonitor.mock.calls[0][0] | ||
|
||
expect(monitorOptions?.onDeviceRemove).toEqual(expect.any(Function)) | ||
const onDeviceRemove = monitorOptions?.onDeviceRemove ?? noop | ||
onDeviceRemove(realtek0) | ||
|
||
return flush().then(() => { | ||
expect(dispatch).toHaveBeenCalledWith( | ||
SystemInfo.usbDeviceRemoved(realtek0) | ||
) | ||
}) | ||
}) | ||
|
||
it('stops monitoring on app quit', () => { | ||
handler(uiInitialized()) | ||
|
||
const appQuitHandler = app.once.mock.calls.find( | ||
([event, handler]) => event === 'will-quit' | ||
)?.[1] | ||
|
||
expect(typeof appQuitHandler).toBe('function') | ||
appQuitHandler() | ||
expect(monitor.stop).toHaveBeenCalled() | ||
}) | ||
|
||
describe('on windows', () => { | ||
beforeEach(() => { | ||
isWindows.mockReturnValue(true) | ||
getWindowsDriverVersion.mockResolvedValue('1.2.3') | ||
}) | ||
|
||
it('should add Windows driver versions to Realtek devices on initialization', () => { | ||
getAllDevices.mockResolvedValue([realtek0, notRealtek, realtek1]) | ||
handler(uiInitialized()) | ||
|
||
return flush().then(() => { | ||
expect(getWindowsDriverVersion).toHaveBeenCalledWith(realtek0) | ||
expect(getWindowsDriverVersion).toHaveBeenCalledWith(realtek1) | ||
|
||
expect(dispatch).toHaveBeenCalledWith( | ||
SystemInfo.initialized([ | ||
{ ...realtek0, windowsDriverVersion: '1.2.3' }, | ||
notRealtek, | ||
{ ...realtek1, windowsDriverVersion: '1.2.3' }, | ||
]) | ||
) | ||
}) | ||
}) | ||
|
||
it('should add Windows driver versions to Realtek devices on add', () => { | ||
getAllDevices.mockResolvedValue([]) | ||
handler(uiInitialized()) | ||
const monitorOptions = createUsbDeviceMonitor.mock.calls[0][0] | ||
const onDeviceAdd = monitorOptions?.onDeviceAdd ?? noop | ||
onDeviceAdd(realtek0) | ||
|
||
return flush().then(() => { | ||
expect(getWindowsDriverVersion).toHaveBeenCalledWith(realtek0) | ||
|
||
expect(dispatch).toHaveBeenCalledWith( | ||
SystemInfo.usbDeviceAdded({ | ||
...realtek0, | ||
windowsDriverVersion: '1.2.3', | ||
}) | ||
) | ||
}) | ||
}) | ||
}) | ||
}) |
101 changes: 101 additions & 0 deletions
101
app-shell/src/system-info/__tests__/usb-devices.test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
// @flow | ||
|
||
import execa from 'execa' | ||
import usbDetection from 'usb-detection' | ||
|
||
import * as Fixtures from '@opentrons/app/src/system-info/__fixtures__' | ||
import { createUsbDeviceMonitor, getWindowsDriverVersion } from '../usb-devices' | ||
|
||
jest.mock('execa') | ||
jest.mock('usb-detection', () => { | ||
const EventEmitter = require('events') | ||
const detector = new EventEmitter() | ||
detector.startMonitoring = jest.fn() | ||
detector.stopMonitoring = jest.fn() | ||
detector.find = jest.fn() | ||
return detector | ||
}) | ||
|
||
const usbDetectionFind: JestMockFn<[], any> = (usbDetection.find: any) | ||
|
||
describe('app-shell::system-info::usb-devices', () => { | ||
const { windowsDriverVersion: _, ...mockDevice } = Fixtures.mockUsbDevice | ||
afterEach(() => { | ||
jest.resetAllMocks() | ||
}) | ||
|
||
it('can create a usb device monitor', () => { | ||
expect(usbDetection.startMonitoring).toHaveBeenCalledTimes(0) | ||
createUsbDeviceMonitor() | ||
expect(usbDetection.startMonitoring).toHaveBeenCalledTimes(1) | ||
}) | ||
|
||
it('usb device monitor can be stopped', () => { | ||
const monitor = createUsbDeviceMonitor() | ||
monitor.stop() | ||
expect(usbDetection.stopMonitoring).toHaveBeenCalledTimes(1) | ||
}) | ||
|
||
it('can return the list of all devices', () => { | ||
const mockDevices = [ | ||
{ ...mockDevice, deviceName: 'foo' }, | ||
{ ...mockDevice, deviceName: 'bar' }, | ||
{ ...mockDevice, deviceName: 'baz' }, | ||
] | ||
|
||
usbDetectionFind.mockResolvedValueOnce(mockDevices) | ||
|
||
const monitor = createUsbDeviceMonitor() | ||
const result = monitor.getAllDevices() | ||
|
||
return expect(result).resolves.toEqual(mockDevices) | ||
}) | ||
|
||
it('can notify when devices are added', () => { | ||
const onDeviceAdd = jest.fn() | ||
createUsbDeviceMonitor({ onDeviceAdd }) | ||
|
||
usbDetection.emit('add', mockDevice) | ||
|
||
expect(onDeviceAdd).toHaveBeenCalledWith(mockDevice) | ||
}) | ||
|
||
it('can notify when devices are removed', () => { | ||
const onDeviceRemove = jest.fn() | ||
createUsbDeviceMonitor({ onDeviceRemove }) | ||
|
||
usbDetection.emit('remove', mockDevice) | ||
|
||
expect(onDeviceRemove).toHaveBeenCalledWith(mockDevice) | ||
}) | ||
|
||
it('can get the Windows driver version of a device', () => { | ||
execa.command.mockResolvedValue({ stdout: '1.2.3' }) | ||
|
||
const device = { | ||
...mockDevice, | ||
// 291 == 0x0123 | ||
vendorId: 291, | ||
// 43981 == 0xABCD | ||
productId: 43981, | ||
// plain string for serial | ||
serialNumber: 'abcdefg', | ||
} | ||
|
||
return getWindowsDriverVersion(device).then(version => { | ||
expect(execa.command).toHaveBeenCalledWith( | ||
'Get-PnpDeviceProperty -InstanceID "USB\\VID_0123&PID_ABCD\\abcdefg" -KeyName "DEVPKEY_Device_DriverVersion" | % { $_.Data }', | ||
{ shell: 'PowerShell.exe' } | ||
) | ||
expect(version).toBe('1.2.3') | ||
}) | ||
}) | ||
|
||
it('returns null for unknown if command errors out', () => { | ||
execa.command.mockRejectedValue('AH!') | ||
|
||
return getWindowsDriverVersion(mockDevice).then(version => { | ||
expect(version).toBe(null) | ||
}) | ||
}) | ||
}) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only a build dep, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but hopefully it's unnecessary now that
usb-detection
is publishing prebuilt binaries for the correct node versions. Still evaluating but will likely updaate the travis config once moreThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's some continuing native build shenanigans. For now, it appears we need both the prebuilt binaries and these build dependencies in place for everything to work. My hunch is that there's a problem with the Linux prebuilt binaries.
The
usb-detection
author appears to be in the middle of anode-gyp
upgrade, so I'll take a look at removing these deps when that update is released. Keeping in place right now so we can have successful builds.