Skip to content
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.

Sign up for GitHub

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

fix(app): properly enable/disable robot update from file button #6483

Merged
merged 8 commits into from
Sep 14, 2020
Merged

fix(app): properly enable/disable robot update from file button #6483

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
011a35b
fix(app): properly enable/disable robot update from file button
mcous Sep 3, 2020
b421743
fixup: add some missing changes
mcous Sep 3, 2020
544a8ff
Merge branch 'edge' into app_update-from-file-tooltip
mcous Sep 8, 2020
65b3d19
fixup: revert some selector changes and tweak copy
mcous Sep 8, 2020
18c7fe7
Merge branch 'edge' into app_update-from-file-tooltip
mcous Sep 9, 2020
95ad791
fixup: remove console.log
mcous Sep 10, 2020
3fed4cd
fixup: refactor buildroot selector tests for correctness and readability
mcous Sep 14, 2020
9492dff
fixup: tweak update type selector test names
mcous Sep 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 137 additions & 10 deletions app/src/buildroot/__tests__/selectors.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import * as selectors from '../selectors'
import { mockReachableRobot } from '../../discovery/__fixtures__'
import {
HEALTH_STATUS_NOT_OK,
getViewableRobots,
getRobotApiVersion,
} from '../../discovery/selectors'
getRobotByName,
} from '../../discovery'

jest.mock('../../discovery/selectors')

describe('buildroot selectors', () => {
beforeEach(() => {
getViewableRobots.mockReturnValue([])
getRobotApiVersion.mockReturnValue(null)
getRobotByName.mockReturnValue(null)
})

afterEach(() => {
jest.clearAllMocks()
jest.resetAllMocks()
})

const SPECS = [
Expand Down Expand Up @@ -84,7 +93,10 @@ describe('buildroot selectors', () => {
args: [{ name: 'robot-name' }],
expected: 'upgrade',
setup: () => {
getRobotApiVersion.mockReturnValueOnce('0.9.9')
getRobotApiVersion.mockImplementation(robot => {
expect(robot).toEqual({ name: 'robot-name' })
return '0.9.9'
})
},
},
{
Expand All @@ -98,7 +110,10 @@ describe('buildroot selectors', () => {
args: [{ name: 'robot-name' }],
expected: 'downgrade',
setup: () => {
getRobotApiVersion.mockReturnValueOnce('1.0.1')
getRobotApiVersion.mockImplementation(robot => {
expect(robot).toEqual({ name: 'robot-name' })
return '1.0.1'
})
},
},
{
Expand All @@ -112,7 +127,10 @@ describe('buildroot selectors', () => {
args: [{ name: 'robot-name' }],
expected: 'reinstall',
setup: () => {
getRobotApiVersion.mockReturnValueOnce('1.0.0')
getRobotApiVersion.mockImplementation(robot => {
expect(robot).toEqual({ name: 'robot-name' })
return '1.0.0'
})
},
},
{
Expand All @@ -126,8 +144,22 @@ describe('buildroot selectors', () => {
args: [{ name: 'robot-name' }],
expected: null,
setup: () => {
getRobotApiVersion.mockReturnValueOnce('1.0.0')
getRobotApiVersion.mockImplementation(robot => {
expect(robot).toEqual({ name: 'robot-name' })
return '1.0.0'
})
},
},
{
name: 'getBuildrootUpdateAvailable with no robot version available',
selector: selectors.getBuildrootUpdateAvailable,
state: {
buildroot: {
version: '1.0.0',
},
},
args: [{ name: 'robot-name' }],
expected: null,
},
{
name: 'getBuildrootUpdateSession',
Expand Down Expand Up @@ -159,7 +191,7 @@ describe('buildroot selectors', () => {
},
expected: { name: 'robot-name', host: '10.10.0.0', port: 31950 },
setup: () =>
getViewableRobots.mockReturnValueOnce([
getViewableRobots.mockReturnValue([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come you switched mockReturnValueOnce to mockReturnValue?

Copy link
Contributor Author

@mcous mcous Sep 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mockReturnValue is more accurate to how the selectors would actually behave; a selector (if it chose to) might call some selector it is dependent on multiple times with the same input, and you would expect the same output.

These tests weren't very well written (by me) with respect to mocks, so I've refactored to be more clear

{ name: 'other-robot-name', host: '10.10.0.1', port: 31950 },
{ name: 'robot-name', host: '10.10.0.0', port: 31950 },
{ name: 'another-robot-name', host: '10.10.0.2', port: 31950 },
Expand All @@ -180,7 +212,7 @@ describe('buildroot selectors', () => {
serverHealth: { capabilities: { buildrootUpdate: '/' } },
},
setup: () =>
getViewableRobots.mockReturnValueOnce([
getViewableRobots.mockReturnValue([
{ name: 'other-robot-name', host: '10.10.0.1', port: 31950 },
{
name: 'robot-name',
Expand All @@ -191,12 +223,107 @@ describe('buildroot selectors', () => {
{ name: 'another-robot-name', host: '10.10.0.2', port: 31950 },
]),
},
{
name: 'getBuildrootUpdateDisplayInfo returns not responding if no robot',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: {} },
setup: () => {
getRobotByName.mockReturnValue(null)
},
expected: expect.objectContaining({
autoUpdateDisabledReason: expect.stringMatching(
/update server is not responding/
),
updateFromFileDisabledReason: expect.stringMatching(
/update server is not responding/
),
}),
},
{
name:
'getBuildrootUpdateDisplayInfo returns not responding if robot has unhealthy update server',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: {} },
setup: () => {
getRobotByName.mockReturnValue({
...mockReachableRobot,
serverHealthStatus: HEALTH_STATUS_NOT_OK,
})
},
expected: expect.objectContaining({
autoUpdateDisabledReason: expect.stringMatching(
/update server is not responding/
),
updateFromFileDisabledReason: expect.stringMatching(
/update server is not responding/
),
}),
},
{
name:
'getBuildrootUpdateDisplayInfo returns not allowed if another robot is updating',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: { session: { robotName: 'other-robot-name' } } },
setup: () => {
getRobotByName.mockImplementation((state, name) => {
return { ...mockReachableRobot, name }
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we overwrite the name of the robot here? When I console.log the name that gets passed in to the mock, it's undefined.

Wouldn't we want to always just return the name of mockReachableRobot, which is different than other-robot-name? I.e.

getRobotByName.mockReturnValueOnce(mockReachableRobot)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, test was bad. Good catch!

getViewableRobots.mockReturnValue([
{ name: 'other-robot-name', host: '10.10.0.1', port: 31950 },
])
},
expected: expect.objectContaining({
autoUpdateDisabledReason: expect.stringMatching(
/updating a different robot/
),
updateFromFileDisabledReason: expect.stringMatching(
/updating a different robot/
),
}),
},
{
name:
'getBuildrootUpdateDisplayInfo returns allowed only from file if no auto files',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: {} },
setup: () => {
getRobotByName.mockReturnValue(mockReachableRobot)
},
expected: {
autoUpdateAction: expect.stringMatching(/unavailable/i),
autoUpdateDisabledReason: expect.stringMatching(
/unable to retrieve update/i
),
updateFromFileDisabledReason: null,
},
},
{
name:
'getBuildrootUpdateDisplayInfo returns allowed with action if all good',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: { version: '1.0.0' } },
setup: () => {
getRobotByName.mockReturnValue(mockReachableRobot)
getRobotApiVersion.mockImplementation(robot => {
expect(robot).toEqual(mockReachableRobot)
return '0.9.9'
})
},
expected: {
autoUpdateAction: expect.stringMatching(/upgrade/i),
autoUpdateDisabledReason: null,
updateFromFileDisabledReason: null,
},
},
]

SPECS.forEach(spec => {
const { name, selector, state, expected, setup } = spec
const args = spec.args || []
if (typeof setup === 'function') setup()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun fact: mock setup wasn't working at all in these tests...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃

it(name, () => expect(selector(state, ...args)).toEqual(expected))

it(name, () => {
if (typeof setup === 'function') setup()
expect(selector(state, ...args)).toEqual(expected)
})
})
})
82 changes: 72 additions & 10 deletions app/src/buildroot/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
import semver from 'semver'
import { createSelector } from 'reselect'

import { getViewableRobots, getRobotApiVersion } from '../discovery/selectors'
import {
HEALTH_STATUS_OK,
getViewableRobots,
getRobotApiVersion,
getRobotByName,
} from '../discovery'
import * as Constants from './constants'

import type { State } from '../types'
Expand All @@ -14,6 +19,15 @@ import type {
RobotSystemType,
} from './types'

// TODO(mc, 2020-08-02): i18n
const UPDATE_SERVER_UNAVAILABLE =
"Unable to update because your robot's update server is not responding."
const OTHER_ROBOT_UPDATING =
'Unable to update because the app is currently updating a different robot.'
const NO_UPDATE_FILES =
'Unable to retrieve update for this robot. Ensure your computer is connected to the internet and try again later.'
Copy link
Member

@sanni-t sanni-t Sep 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this happen? (Not having update files)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly this will happen in two cases:

  • The user's computer is not connected to the internet
    • It's not able to check the releases.json manifest from S3 for the robot updates
    • fix(app): cache robot releases manifest for offline usage #6494 makes this happen less often, so that it will only show up if the computer is not connected to the internet and it has never been connected long enough to download the latest update
  • The user has updated the latest version of the app but the robot update hasn't finished building / hasn't been uploaded to the release bucket
    • So in the 30 to 60 min window right when the app release is cut

const UNAVAILABLE = 'Update unavailable'

export function getBuildrootUpdateVersion(state: State): string | null {
return state.buildroot.version || null
}
Expand Down Expand Up @@ -87,19 +101,16 @@ export const getBuildrootRobot: State => ViewableRobot | null = createSelector(
}
)

export function getBuildrootUpdateAvailable(
state: State,
robot: ViewableRobot
): BuildrootUpdateType | null {
const updateVersion = getBuildrootUpdateVersion(state)
const currentVersion = getRobotApiVersion(robot)

const getBuildrootUpdateType = (
currentVersion: string | null,
updateVersion: string | null
): BuildrootUpdateType | null => {
const validCurrent: string | null = semver.valid(currentVersion)
const validUpdate: string | null = semver.valid(updateVersion)
let type = null

if (validUpdate) {
if (!validCurrent || semver.gt(validUpdate, validCurrent)) {
if (validUpdate && validCurrent) {
if (semver.gt(validUpdate, validCurrent)) {
type = Constants.UPGRADE
} else if (semver.lt(validUpdate, validCurrent)) {
type = Constants.DOWNGRADE
Expand All @@ -108,9 +119,60 @@ export function getBuildrootUpdateAvailable(
}
}

console.log(currentVersion, updateVersion, type)
mcous marked this conversation as resolved.
Show resolved Hide resolved

return type
}

export function getBuildrootUpdateAvailable(
state: State,
robot: ViewableRobot
): BuildrootUpdateType | null {
const currentVersion = getRobotApiVersion(robot)
const updateVersion = getBuildrootUpdateVersion(state)

return getBuildrootUpdateType(currentVersion, updateVersion)
}

export const getBuildrootUpdateDisplayInfo: (
state: State,
robotName: string
) => {|
autoUpdateAction: string,
autoUpdateDisabledReason: string | null,
updateFromFileDisabledReason: string | null,
|} = createSelector(
getRobotByName,
state => getBuildrootRobot(state),
state => getBuildrootUpdateVersion(state),
(robot, currentUpdatingRobot, updateVersion) => {
const robotVersion = robot ? getRobotApiVersion(robot) : null
const autoUpdateType = getBuildrootUpdateType(robotVersion, updateVersion)
const autoUpdateAction = autoUpdateType ?? UNAVAILABLE
let autoUpdateDisabledReason = null
let updateFromFileDisabledReason = null

if (robot?.serverHealthStatus !== HEALTH_STATUS_OK) {
autoUpdateDisabledReason = UPDATE_SERVER_UNAVAILABLE
updateFromFileDisabledReason = UPDATE_SERVER_UNAVAILABLE
} else if (
currentUpdatingRobot !== null &&
currentUpdatingRobot.name !== robot?.name
) {
autoUpdateDisabledReason = OTHER_ROBOT_UPDATING
updateFromFileDisabledReason = OTHER_ROBOT_UPDATING
} else if (autoUpdateType === null) {
autoUpdateDisabledReason = NO_UPDATE_FILES
}

return {
autoUpdateAction,
autoUpdateDisabledReason,
updateFromFileDisabledReason,
}
}
)

export function getRobotSystemType(
robot: ViewableRobot
): RobotSystemType | null {
Expand Down
3 changes: 1 addition & 2 deletions app/src/components/ConnectPanel/RobotItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ export function RobotItemComponent(props: RobotItemProps): React.Node {
return getBuildrootUpdateAvailable(state, robot) === UPGRADE
})
const isConnectable = status === CONNECTABLE
// NOTE(mc, 2020-03-30): redundant && true to satisfy Flow
const isConnected = Boolean(robot.connected && true)
const isConnected = robot.connected
const isSelected = robot.name === match.params.name
const connectInProgress = useSelector(
(state: State) => RobotSelectors.getConnectRequest(state).inProgress
Expand Down
Loading