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

Jump to bottom

feat(frontend): Support showing Omitted node phase with new Argo version #5339

Merged
merged 5 commits into from
Mar 23, 2021
Merged

feat(frontend): Support showing Omitted node phase with new Argo version #5339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9d2b36e
Support showing Omitted node phase with new Argo version
zijianjoy Mar 19, 2021
f0967dd
obsolete snapshot
zijianjoy Mar 19, 2021
68b63a1
Address comments
zijianjoy Mar 22, 2021
1c56637
smaller test
zijianjoy Mar 22, 2021
9b8dc68
Add future tests
zijianjoy Mar 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions frontend/src/lib/StatusUtils.test.tsx
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@ describe('StatusUtils', () => {
NodePhase.CACHED,
NodePhase.SKIPPED,
NodePhase.TERMINATED,
NodePhase.OMITTED,
].forEach(status => {
it(`returns \'true\' if status is: ${status}`, () => {
expect(hasFinished(status)).toBe(true);
Expand Down Expand Up @@ -75,6 +76,10 @@ describe('StatusUtils', () => {
expect(consoleSpy).toHaveBeenLastCalledWith('Unknown node phase:', undefined);
});

it("returns color 'not started' if status is 'Omitted'", () => {
expect(statusToBgColor(NodePhase.OMITTED)).toEqual(statusBgColors.notStarted);
});

it("returns color 'not started' if status is 'Pending'", () => {
expect(statusToBgColor(NodePhase.PENDING)).toEqual(statusBgColors.notStarted);
});
Expand Down Expand Up @@ -116,6 +121,7 @@ describe('StatusUtils', () => {
NodePhase.PENDING,
NodePhase.RUNNING,
NodePhase.TERMINATING,
NodePhase.OMITTED,
NodePhase.UNKNOWN,
].forEach(status => {
it(`returns the original status, even if message is 'terminated', if status is: ${status}`, () => {
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/lib/StatusUtils.ts
View file
Open in desktop
Expand Up @@ -38,6 +38,7 @@ export enum NodePhase {
TERMINATING = 'Terminating',
TERMINATED = 'Terminated',
UNKNOWN = 'Unknown',
OMITTED = 'Omitted',
}

export function hasFinished(status?: NodePhase): boolean {
Expand All @@ -48,6 +49,7 @@ export function hasFinished(status?: NodePhase): boolean {
case NodePhase.ERROR: // Fall through
case NodePhase.SKIPPED: // Fall through
case NodePhase.TERMINATED:
case NodePhase.OMITTED:
return true;
case NodePhase.PENDING: // Fall through
case NodePhase.RUNNING: // Fall through
Expand Down
250 changes: 250 additions & 0 deletions frontend/src/pages/Status.test.tsx
View file
Open in desktop
Expand Up @@ -48,6 +48,256 @@ describe('Status', () => {
expect(consoleSpy).toHaveBeenLastCalledWith('Unknown node phase:', undefined);
});

// TODO: Enable this test after react-scripts is upgraded to v4.0.0
// it('react testing for ERROR phase', async () => {
// const { findByText, getByTestId } = render(
// statusToIcon(NodePhase.ERROR),
// );

// fireEvent.mouseOver(getByTestId('node-status-sign'));
// findByText('Error while running this resource');
// });

it('handles ERROR phase', () => {
const tree = shallow(statusToIcon(NodePhase.ERROR));
expect(tree.find('span')).toMatchInlineSnapshot(`
<span
style={
Object {
"height": 18,
}
}
>
<pure(ErrorIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#d50000",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('handles FAILED phase', () => {
const tree = shallow(statusToIcon(NodePhase.FAILED));
expect(tree.find('span')).toMatchInlineSnapshot(`
<span
style={
Object {
"height": 18,
}
}
>
<pure(ErrorIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#d50000",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('handles PENDING phase', () => {
const tree = shallow(statusToIcon(NodePhase.PENDING));
expect(tree.find('span')).toMatchInlineSnapshot(`
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an unstable finder, if any other span element shows up, it'll break.

I'd suggest this blogpost https://kentcdodds.com/blog/making-your-ui-tests-resilient-to-change.
There are some general advice for choosing a selector, it should be

  1. reflect how users find elements
  2. if not feasible, add a unique data-testid

This is not a blocker for this PR, your improvements have been pretty good!

Copy link
Collaborator Author

@zijianjoy zijianjoy Mar 25, 2021 •
edited

Choose a reason for hiding this comment

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

Thank you for the suggestion! I agree with the testing approach which is based on user visible element. The current implementation is temporary, and we should use the react testing library (commented test) after version upgrade, so we can abandon this test.

<span
style={
Object {
"height": 18,
}
}
>
<pure(ScheduleIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#9aa0a6",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('handles RUNNING phase', () => {
const tree = shallow(statusToIcon(NodePhase.RUNNING));
expect(tree.find('span')).toMatchInlineSnapshot(`
<span
style={
Object {
"height": 18,
}
}
>
<StatusRunning
data-testid="node-status-sign"
style={
Object {
"color": "#4285f4",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('handles TERMINATING phase', () => {
const tree = shallow(statusToIcon(NodePhase.TERMINATING));
expect(tree.find('span')).toMatchInlineSnapshot(`
<span
style={
Object {
"height": 18,
}
}
>
<StatusRunning
data-testid="node-status-sign"
style={
Object {
"color": "#4285f4",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('handles SKIPPED phase', () => {
const tree = shallow(statusToIcon(NodePhase.SKIPPED));
expect(tree.find('span')).toMatchInlineSnapshot(`
<span
style={
Object {
"height": 18,
}
}
>
<pure(SkipNextIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#5f6368",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('handles SUCCEEDED phase', () => {
const tree = shallow(statusToIcon(NodePhase.SUCCEEDED));
expect(tree.find('span')).toMatchInlineSnapshot(`
<span
style={
Object {
"height": 18,
}
}
>
<pure(CheckCircleIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#34a853",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('handles CACHED phase', () => {
const tree = shallow(statusToIcon(NodePhase.CACHED));
expect(tree.find('span')).toMatchInlineSnapshot(`
<span
style={
Object {
"height": 18,
}
}
>
<StatusCached
data-testid="node-status-sign"
style={
Object {
"color": "#34a853",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('handles TERMINATED phase', () => {
const tree = shallow(statusToIcon(NodePhase.TERMINATED));
expect(tree.find('span')).toMatchInlineSnapshot(`
<span
style={
Object {
"height": 18,
}
}
>
<StatusRunning
data-testid="node-status-sign"
style={
Object {
"color": "#80868b",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('handles OMITTED phase', () => {
const tree = shallow(statusToIcon(NodePhase.OMITTED));
expect(tree.find('span')).toMatchInlineSnapshot(`
<span
style={
Object {
"height": 18,
}
}
>
<pure(BlockIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#5f6368",
"height": 18,
"width": 18,
}
}
/>
</span>
`);
});

it('displays start and end dates if both are provided', () => {
const tree = shallow(statusToIcon(NodePhase.SUCCEEDED, startDate, endDate));
expect(tree).toMatchSnapshot();
Expand Down
10 changes: 9 additions & 1 deletion frontend/src/pages/Status.tsx
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@ import PendingIcon from '@material-ui/icons/Schedule';
import RunningIcon from '../icons/statusRunning';
import SkippedIcon from '@material-ui/icons/SkipNext';
import SuccessIcon from '@material-ui/icons/CheckCircle';
import BlockIcon from '@material-ui/icons/Block';
import CachedIcon from '../icons/statusCached';
import TerminatedIcon from '../icons/statusTerminated';
import Tooltip from '@material-ui/core/Tooltip';
Expand Down Expand Up @@ -84,6 +85,10 @@ export function statusToIcon(
iconColor = color.terminated;
title = 'Run was manually terminated';
break;
case NodePhase.OMITTED:
IconComponent = BlockIcon;
title = 'Run was omitted because the previous step failed.';
break;
case NodePhase.UNKNOWN:
break;
default:
Expand All @@ -101,7 +106,10 @@ export function statusToIcon(
}
>
<span style={{ height: 18 }}>
<IconComponent style={{ color: iconColor, height: 18, width: 18 }} />
<IconComponent
data-testid='node-status-sign'
style={{ color: iconColor, height: 18, width: 18 }}
/>
</span>
</Tooltip>
);
Expand Down
5 changes: 5 additions & 0 deletions frontend/src/pages/__snapshots__/ExperimentList.test.tsx.snap
View file
Open in desktop
Expand Up @@ -202,6 +202,7 @@ exports[`ExperimentList renders last 5 runs statuses 1`] = `
}
>
<pure(CheckCircleIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#34a853",
Expand Down Expand Up @@ -237,6 +238,7 @@ exports[`ExperimentList renders last 5 runs statuses 1`] = `
}
>
<pure(ScheduleIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#9aa0a6",
Expand Down Expand Up @@ -272,6 +274,7 @@ exports[`ExperimentList renders last 5 runs statuses 1`] = `
}
>
<pure(ErrorIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#d50000",
Expand Down Expand Up @@ -307,6 +310,7 @@ exports[`ExperimentList renders last 5 runs statuses 1`] = `
}
>
<pure(HelpIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#5f6368",
Expand Down Expand Up @@ -342,6 +346,7 @@ exports[`ExperimentList renders last 5 runs statuses 1`] = `
}
>
<pure(CheckCircleIcon)
data-testid="node-status-sign"
style={
Object {
"color": "#34a853",
Expand Down