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

[UI] Fix UI crash when invalid pipeline uploaded #2774

Merged
merged 4 commits into from
Dec 26, 2019
Merged

[UI] Fix UI crash when invalid pipeline uploaded #2774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5c684d
Fix UI crashing when invalid pipeline uploaded
Bobgy Dec 26, 2019
a799449
Fix existing tests
Bobgy Dec 26, 2019
d5629c7
Use dive instead of shallow
Bobgy Dec 26, 2019
362093e
Add test case for error behavior
Bobgy Dec 26, 2019
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
23 changes: 21 additions & 2 deletions frontend/src/components/Graph.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

import * as dagre from 'dagre';
import * as React from 'react';
import { shallow } from 'enzyme';
import Graph from './Graph';
import { shallow, mount } from 'enzyme';
import EnhancedGraph, { Graph } from './Graph';
import SuccessIcon from '@material-ui/icons/CheckCircle';
import Tooltip from '@material-ui/core/Tooltip';

Expand All @@ -43,6 +43,10 @@ const newNode = (label: string, isPlaceHolder?: boolean, color?: string, icon?:
width: 10,
});

beforeEach(() => {
jest.restoreAllMocks();
});

describe('Graph', () => {
it('handles an empty graph', () => {
expect(shallow(<Graph graph={newGraph()} />)).toMatchSnapshot();
Expand Down Expand Up @@ -149,4 +153,19 @@ describe('Graph', () => {
graph.setEdge('node1', 'node2');
expect(shallow(<Graph graph={graph} selectedNodeId='node3' />)).toMatchSnapshot();
});

it('shows an error message when the graph is invalid', () => {
const consoleErrorSpy = jest.spyOn(console, 'error');
consoleErrorSpy.mockImplementation(() => null);
const graph = newGraph();
graph.setEdge('node1', 'node2');
const onError = jest.fn();
expect(mount(<EnhancedGraph graph={graph} onError={onError} />).html()).toMatchSnapshot();
expect(onError).toHaveBeenCalledTimes(1);
const [message, additionalInfo] = onError.mock.calls[0];
expect(message).toEqual('There was an error rendering the graph.');
expect(additionalInfo).toEqual(
"There was an error rendering the graph. This is likely a bug in Kubeflow Pipelines. Error message: 'Graph definition is invalid. Cannot get node by 'node1'.'.",
);
});
});
39 changes: 38 additions & 1 deletion frontend/src/components/Graph.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,31 @@ interface GraphState {
hoveredNode?: string;
}

export default class Graph extends React.Component<GraphProps, GraphState> {
interface GraphErrorBoundaryProps {
onError?: (message: string, additionalInfo: string) => void;
}
class GraphErrorBoundary extends React.Component<GraphErrorBoundaryProps> {
state = {
hasError: false,
};

componentDidCatch(error: Error): void {
const message = 'There was an error rendering the graph.';
const additionalInfo = `${message} This is likely a bug in Kubeflow Pipelines. Error message: '${error.message}'.`;
if (this.props.onError) {
this.props.onError(message, additionalInfo);
}
this.setState({
hasError: true,
});
}

render() {
return this.state.hasError ? <div className={css.root} /> : this.props.children;
}
}

export class Graph extends React.Component<GraphProps, GraphState> {
private LEFT_OFFSET = 100;
private TOP_OFFSET = 44;
private EDGE_THICKNESS = 2;
Expand Down Expand Up @@ -154,6 +178,10 @@ export default class Graph extends React.Component<GraphProps, GraphState> {
if (i === 1) {
const sourceNode = graph.node(edgeInfo.v);

if (!sourceNode) {
throw new Error(`Graph definition is invalid. Cannot get node by '${edgeInfo.v}'.`);
}

// Set the edge's first segment to start at the bottom or top of the source node.
yStart = downwardPointingSegment
? sourceNode.y + sourceNode.height / 2 - 3
Expand Down Expand Up @@ -380,3 +408,12 @@ export default class Graph extends React.Component<GraphProps, GraphState> {
return color.grey;
}
}

const EnhancedGraph = (props: GraphProps & GraphErrorBoundaryProps) => (
<GraphErrorBoundary onError={props.onError}>
<Graph {...props} />
</GraphErrorBoundary>
);
EnhancedGraph.displayName = 'EnhancedGraph';

export default EnhancedGraph;
2 changes: 2 additions & 0 deletions frontend/src/components/__snapshots__/Graph.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1864,3 +1864,5 @@ exports[`Graph renders a graph with two disparate nodes 1`] = `
</div>
</div>
`;

exports[`Graph shows an error message when the graph is invalid 1`] = `"<div class=\\"root\\"></div>"`;
15 changes: 12 additions & 3 deletions frontend/src/pages/PipelineDetails.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ describe('PipelineDetails', () => {
tree = shallow(<PipelineDetails {...generateProps()} />);
await getPipelineVersionTemplateSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'some-node-id');
clickGraphNode(tree, 'some-node-id');
expect(tree.state('selectedNodeId')).toBe('some-node-id');
expect(tree).toMatchSnapshot();
});
Expand All @@ -627,7 +627,7 @@ describe('PipelineDetails', () => {
tree = shallow(<PipelineDetails {...generateProps()} />);
await getPipelineVersionTemplateSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
expect(tree).toMatchSnapshot();
});

Expand All @@ -650,11 +650,20 @@ describe('PipelineDetails', () => {
expect(tree).toMatchSnapshot();
});

it('closes side panel when close button is clicked', async () => {
it('shows correct versions in version selector', async () => {
tree = shallow(<PipelineDetails {...generateProps()} />);
await getPipelineVersionTemplateSpy;
await TestUtils.flushPromises();
expect(tree.state('versions')).toHaveLength(1);
expect(tree).toMatchSnapshot();
});
});

function clickGraphNode(wrapper: ShallowWrapper, nodeId: string) {
// TODO: use dom events instead
wrapper
.find('EnhancedGraph')
.dive()
.dive()
.simulate('click', nodeId);
}
3 changes: 3 additions & 0 deletions frontend/src/pages/PipelineDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,9 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> {
graph={this.state.graph}
selectedNodeId={selectedNodeId}
onClick={id => this.setStateSafe({ selectedNodeId: id })}
onError={(message, additionalInfo) =>
this.props.updateBanner({ message, additionalInfo, mode: 'error' })
}
/>

<SidePanel
Expand Down
51 changes: 30 additions & 21 deletions frontend/src/pages/RunDetails.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
expect(tree.state('selectedNodeDetails')).toHaveProperty('id', 'node1');
expect(tree).toMatchSnapshot();
});
Expand All @@ -589,7 +589,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
expect(tree.state('selectedNodeDetails')).toHaveProperty(
'phaseMessage',
'This step is in ' + testRun.run!.status + ' state with this message: some test message',
Expand All @@ -611,7 +611,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
await pathsParser;
await pathsWithStepsParser;
await loaderSpy;
Expand Down Expand Up @@ -652,7 +652,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -669,7 +669,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -685,7 +685,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -701,7 +701,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
await TestUtils.flushPromises();
expect(tree.state('selectedNodeDetails')).toHaveProperty('id', 'node1');
tree.find('SidePanel').simulate('close');
Expand All @@ -717,7 +717,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -743,7 +743,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -764,7 +764,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -789,7 +789,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand Down Expand Up @@ -817,7 +817,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand Down Expand Up @@ -906,7 +906,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -922,7 +922,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -946,7 +946,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -967,7 +967,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -992,7 +992,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -1018,7 +1018,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand Down Expand Up @@ -1048,7 +1048,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -1070,7 +1070,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand All @@ -1093,7 +1093,7 @@ describe('RunDetails', () => {
tree = shallow(<RunDetails {...generateProps()} />);
await getRunSpy;
await TestUtils.flushPromises();
tree.find('Graph').simulate('click', 'node1');
clickGraphNode(tree, 'node1');
tree
.find('MD2Tabs')
.at(1)
Expand Down Expand Up @@ -1219,3 +1219,12 @@ describe('RunDetails', () => {
});
});
});

function clickGraphNode(wrapper: ShallowWrapper, nodeId: string) {
// TODO: use dom events instead
wrapper
.find('EnhancedGraph')
.dive()
.dive()
.simulate('click', nodeId);
}
3 changes: 3 additions & 0 deletions frontend/src/pages/RunDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ class RunDetails extends Page<RunDetailsProps, RunDetailsState> {
graph={graph}
selectedNodeId={selectedNodeId}
onClick={id => this._selectNode(id)}
onError={(message, additionalInfo) =>
this.props.updateBanner({ message, additionalInfo, mode: 'error' })
}
/>

<SidePanel
Expand Down
Loading