Skip to content

Commit 488805c

Browse files
Bobgyk8s-ci-robot
Bobgy
authored and
k8s-ci-robot
committed
[UI] Fix UI crash when invalid pipeline uploaded (#2774)
* Fix UI crashing when invalid pipeline uploaded * Fix existing tests * Use dive instead of shallow * Add test case for error behavior
1 parent 840979c commit 488805c

9 files changed

+325
-222
lines changed

‎frontend/src/components/Graph.test.tsx

+21-2
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@
1616

1717
import * as dagre from 'dagre';
1818
import * as React from 'react';
19-
import { shallow } from 'enzyme';
20-
import Graph from './Graph';
19+
import { shallow, mount } from 'enzyme';
20+
import EnhancedGraph, { Graph } from './Graph';
2121
import SuccessIcon from '@material-ui/icons/CheckCircle';
2222
import Tooltip from '@material-ui/core/Tooltip';
2323

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

46+
beforeEach(() => {
47+
jest.restoreAllMocks();
48+
});
49+
4650
describe('Graph', () => {
4751
it('handles an empty graph', () => {
4852
expect(shallow(<Graph graph={newGraph()} />)).toMatchSnapshot();
@@ -149,4 +153,19 @@ describe('Graph', () => {
149153
graph.setEdge('node1', 'node2');
150154
expect(shallow(<Graph graph={graph} selectedNodeId='node3' />)).toMatchSnapshot();
151155
});
156+
157+
it('shows an error message when the graph is invalid', () => {
158+
const consoleErrorSpy = jest.spyOn(console, 'error');
159+
consoleErrorSpy.mockImplementation(() => null);
160+
const graph = newGraph();
161+
graph.setEdge('node1', 'node2');
162+
const onError = jest.fn();
163+
expect(mount(<EnhancedGraph graph={graph} onError={onError} />).html()).toMatchSnapshot();
164+
expect(onError).toHaveBeenCalledTimes(1);
165+
const [message, additionalInfo] = onError.mock.calls[0];
166+
expect(message).toEqual('There was an error rendering the graph.');
167+
expect(additionalInfo).toEqual(
168+
"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'.'.",
169+
);
170+
});
152171
});

‎frontend/src/components/Graph.tsx

+38-1
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,31 @@ interface GraphState {
110110
hoveredNode?: string;
111111
}
112112

113-
export default class Graph extends React.Component<GraphProps, GraphState> {
113+
interface GraphErrorBoundaryProps {
114+
onError?: (message: string, additionalInfo: string) => void;
115+
}
116+
class GraphErrorBoundary extends React.Component<GraphErrorBoundaryProps> {
117+
state = {
118+
hasError: false,
119+
};
120+
121+
componentDidCatch(error: Error): void {
122+
const message = 'There was an error rendering the graph.';
123+
const additionalInfo = `${message} This is likely a bug in Kubeflow Pipelines. Error message: '${error.message}'.`;
124+
if (this.props.onError) {
125+
this.props.onError(message, additionalInfo);
126+
}
127+
this.setState({
128+
hasError: true,
129+
});
130+
}
131+
132+
render() {
133+
return this.state.hasError ? <div className={css.root} /> : this.props.children;
134+
}
135+
}
136+
137+
export class Graph extends React.Component<GraphProps, GraphState> {
114138
private LEFT_OFFSET = 100;
115139
private TOP_OFFSET = 44;
116140
private EDGE_THICKNESS = 2;
@@ -154,6 +178,10 @@ export default class Graph extends React.Component<GraphProps, GraphState> {
154178
if (i === 1) {
155179
const sourceNode = graph.node(edgeInfo.v);
156180

181+
if (!sourceNode) {
182+
throw new Error(`Graph definition is invalid. Cannot get node by '${edgeInfo.v}'.`);
183+
}
184+
157185
// Set the edge's first segment to start at the bottom or top of the source node.
158186
yStart = downwardPointingSegment
159187
? sourceNode.y + sourceNode.height / 2 - 3
@@ -380,3 +408,12 @@ export default class Graph extends React.Component<GraphProps, GraphState> {
380408
return color.grey;
381409
}
382410
}
411+
412+
const EnhancedGraph = (props: GraphProps & GraphErrorBoundaryProps) => (
413+
<GraphErrorBoundary onError={props.onError}>
414+
<Graph {...props} />
415+
</GraphErrorBoundary>
416+
);
417+
EnhancedGraph.displayName = 'EnhancedGraph';
418+
419+
export default EnhancedGraph;

‎frontend/src/components/__snapshots__/Graph.test.tsx.snap

+2
Original file line numberDiff line numberDiff line change
@@ -1864,3 +1864,5 @@ exports[`Graph renders a graph with two disparate nodes 1`] = `
18641864
</div>
18651865
</div>
18661866
`;
1867+
1868+
exports[`Graph shows an error message when the graph is invalid 1`] = `"<div class=\\"root\\"></div>"`;

‎frontend/src/pages/PipelineDetails.test.tsx

+12-3
Original file line numberDiff line numberDiff line change
@@ -606,7 +606,7 @@ describe('PipelineDetails', () => {
606606
tree = shallow(<PipelineDetails {...generateProps()} />);
607607
await getPipelineVersionTemplateSpy;
608608
await TestUtils.flushPromises();
609-
tree.find('Graph').simulate('click', 'some-node-id');
609+
clickGraphNode(tree, 'some-node-id');
610610
expect(tree.state('selectedNodeId')).toBe('some-node-id');
611611
expect(tree).toMatchSnapshot();
612612
});
@@ -627,7 +627,7 @@ describe('PipelineDetails', () => {
627627
tree = shallow(<PipelineDetails {...generateProps()} />);
628628
await getPipelineVersionTemplateSpy;
629629
await TestUtils.flushPromises();
630-
tree.find('Graph').simulate('click', 'node1');
630+
clickGraphNode(tree, 'node1');
631631
expect(tree).toMatchSnapshot();
632632
});
633633

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

653-
it('closes side panel when close button is clicked', async () => {
653+
it('shows correct versions in version selector', async () => {
654654
tree = shallow(<PipelineDetails {...generateProps()} />);
655655
await getPipelineVersionTemplateSpy;
656656
await TestUtils.flushPromises();
657657
expect(tree.state('versions')).toHaveLength(1);
658658
expect(tree).toMatchSnapshot();
659659
});
660660
});
661+
662+
function clickGraphNode(wrapper: ShallowWrapper, nodeId: string) {
663+
// TODO: use dom events instead
664+
wrapper
665+
.find('EnhancedGraph')
666+
.dive()
667+
.dive()
668+
.simulate('click', nodeId);
669+
}

‎frontend/src/pages/PipelineDetails.tsx

+3
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,9 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> {
270270
graph={this.state.graph}
271271
selectedNodeId={selectedNodeId}
272272
onClick={id => this.setStateSafe({ selectedNodeId: id })}
273+
onError={(message, additionalInfo) =>
274+
this.props.updateBanner({ message, additionalInfo, mode: 'error' })
275+
}
273276
/>
274277

275278
<SidePanel

‎frontend/src/pages/RunDetails.test.tsx

+30-21
Original file line numberDiff line numberDiff line change
@@ -569,7 +569,7 @@ describe('RunDetails', () => {
569569
tree = shallow(<RunDetails {...generateProps()} />);
570570
await getRunSpy;
571571
await TestUtils.flushPromises();
572-
tree.find('Graph').simulate('click', 'node1');
572+
clickGraphNode(tree, 'node1');
573573
expect(tree.state('selectedNodeDetails')).toHaveProperty('id', 'node1');
574574
expect(tree).toMatchSnapshot();
575575
});
@@ -589,7 +589,7 @@ describe('RunDetails', () => {
589589
tree = shallow(<RunDetails {...generateProps()} />);
590590
await getRunSpy;
591591
await TestUtils.flushPromises();
592-
tree.find('Graph').simulate('click', 'node1');
592+
clickGraphNode(tree, 'node1');
593593
expect(tree.state('selectedNodeDetails')).toHaveProperty(
594594
'phaseMessage',
595595
'This step is in ' + testRun.run!.status + ' state with this message: some test message',
@@ -611,7 +611,7 @@ describe('RunDetails', () => {
611611
tree = shallow(<RunDetails {...generateProps()} />);
612612
await getRunSpy;
613613
await TestUtils.flushPromises();
614-
tree.find('Graph').simulate('click', 'node1');
614+
clickGraphNode(tree, 'node1');
615615
await pathsParser;
616616
await pathsWithStepsParser;
617617
await loaderSpy;
@@ -652,7 +652,7 @@ describe('RunDetails', () => {
652652
tree = shallow(<RunDetails {...generateProps()} />);
653653
await getRunSpy;
654654
await TestUtils.flushPromises();
655-
tree.find('Graph').simulate('click', 'node1');
655+
clickGraphNode(tree, 'node1');
656656
tree
657657
.find('MD2Tabs')
658658
.at(1)
@@ -669,7 +669,7 @@ describe('RunDetails', () => {
669669
tree = shallow(<RunDetails {...generateProps()} />);
670670
await getRunSpy;
671671
await TestUtils.flushPromises();
672-
tree.find('Graph').simulate('click', 'node1');
672+
clickGraphNode(tree, 'node1');
673673
tree
674674
.find('MD2Tabs')
675675
.at(1)
@@ -685,7 +685,7 @@ describe('RunDetails', () => {
685685
tree = shallow(<RunDetails {...generateProps()} />);
686686
await getRunSpy;
687687
await TestUtils.flushPromises();
688-
tree.find('Graph').simulate('click', 'node1');
688+
clickGraphNode(tree, 'node1');
689689
tree
690690
.find('MD2Tabs')
691691
.at(1)
@@ -701,7 +701,7 @@ describe('RunDetails', () => {
701701
tree = shallow(<RunDetails {...generateProps()} />);
702702
await getRunSpy;
703703
await TestUtils.flushPromises();
704-
tree.find('Graph').simulate('click', 'node1');
704+
clickGraphNode(tree, 'node1');
705705
await TestUtils.flushPromises();
706706
expect(tree.state('selectedNodeDetails')).toHaveProperty('id', 'node1');
707707
tree.find('SidePanel').simulate('close');
@@ -717,7 +717,7 @@ describe('RunDetails', () => {
717717
tree = shallow(<RunDetails {...generateProps()} />);
718718
await getRunSpy;
719719
await TestUtils.flushPromises();
720-
tree.find('Graph').simulate('click', 'node1');
720+
clickGraphNode(tree, 'node1');
721721
tree
722722
.find('MD2Tabs')
723723
.at(1)
@@ -743,7 +743,7 @@ describe('RunDetails', () => {
743743
tree = shallow(<RunDetails {...generateProps()} />);
744744
await getRunSpy;
745745
await TestUtils.flushPromises();
746-
tree.find('Graph').simulate('click', 'node1');
746+
clickGraphNode(tree, 'node1');
747747
tree
748748
.find('MD2Tabs')
749749
.at(1)
@@ -764,7 +764,7 @@ describe('RunDetails', () => {
764764
tree = shallow(<RunDetails {...generateProps()} />);
765765
await getRunSpy;
766766
await TestUtils.flushPromises();
767-
tree.find('Graph').simulate('click', 'node1');
767+
clickGraphNode(tree, 'node1');
768768
tree
769769
.find('MD2Tabs')
770770
.at(1)
@@ -789,7 +789,7 @@ describe('RunDetails', () => {
789789
tree = shallow(<RunDetails {...generateProps()} />);
790790
await getRunSpy;
791791
await TestUtils.flushPromises();
792-
tree.find('Graph').simulate('click', 'node1');
792+
clickGraphNode(tree, 'node1');
793793
tree
794794
.find('MD2Tabs')
795795
.at(1)
@@ -817,7 +817,7 @@ describe('RunDetails', () => {
817817
tree = shallow(<RunDetails {...generateProps()} />);
818818
await getRunSpy;
819819
await TestUtils.flushPromises();
820-
tree.find('Graph').simulate('click', 'node1');
820+
clickGraphNode(tree, 'node1');
821821
tree
822822
.find('MD2Tabs')
823823
.at(1)
@@ -906,7 +906,7 @@ describe('RunDetails', () => {
906906
tree = shallow(<RunDetails {...generateProps()} />);
907907
await getRunSpy;
908908
await TestUtils.flushPromises();
909-
tree.find('Graph').simulate('click', 'node1');
909+
clickGraphNode(tree, 'node1');
910910
tree
911911
.find('MD2Tabs')
912912
.at(1)
@@ -922,7 +922,7 @@ describe('RunDetails', () => {
922922
tree = shallow(<RunDetails {...generateProps()} />);
923923
await getRunSpy;
924924
await TestUtils.flushPromises();
925-
tree.find('Graph').simulate('click', 'node1');
925+
clickGraphNode(tree, 'node1');
926926
tree
927927
.find('MD2Tabs')
928928
.at(1)
@@ -946,7 +946,7 @@ describe('RunDetails', () => {
946946
tree = shallow(<RunDetails {...generateProps()} />);
947947
await getRunSpy;
948948
await TestUtils.flushPromises();
949-
tree.find('Graph').simulate('click', 'node1');
949+
clickGraphNode(tree, 'node1');
950950
tree
951951
.find('MD2Tabs')
952952
.at(1)
@@ -967,7 +967,7 @@ describe('RunDetails', () => {
967967
tree = shallow(<RunDetails {...generateProps()} />);
968968
await getRunSpy;
969969
await TestUtils.flushPromises();
970-
tree.find('Graph').simulate('click', 'node1');
970+
clickGraphNode(tree, 'node1');
971971
tree
972972
.find('MD2Tabs')
973973
.at(1)
@@ -992,7 +992,7 @@ describe('RunDetails', () => {
992992
tree = shallow(<RunDetails {...generateProps()} />);
993993
await getRunSpy;
994994
await TestUtils.flushPromises();
995-
tree.find('Graph').simulate('click', 'node1');
995+
clickGraphNode(tree, 'node1');
996996
tree
997997
.find('MD2Tabs')
998998
.at(1)
@@ -1018,7 +1018,7 @@ describe('RunDetails', () => {
10181018
tree = shallow(<RunDetails {...generateProps()} />);
10191019
await getRunSpy;
10201020
await TestUtils.flushPromises();
1021-
tree.find('Graph').simulate('click', 'node1');
1021+
clickGraphNode(tree, 'node1');
10221022
tree
10231023
.find('MD2Tabs')
10241024
.at(1)
@@ -1048,7 +1048,7 @@ describe('RunDetails', () => {
10481048
tree = shallow(<RunDetails {...generateProps()} />);
10491049
await getRunSpy;
10501050
await TestUtils.flushPromises();
1051-
tree.find('Graph').simulate('click', 'node1');
1051+
clickGraphNode(tree, 'node1');
10521052
tree
10531053
.find('MD2Tabs')
10541054
.at(1)
@@ -1070,7 +1070,7 @@ describe('RunDetails', () => {
10701070
tree = shallow(<RunDetails {...generateProps()} />);
10711071
await getRunSpy;
10721072
await TestUtils.flushPromises();
1073-
tree.find('Graph').simulate('click', 'node1');
1073+
clickGraphNode(tree, 'node1');
10741074
tree
10751075
.find('MD2Tabs')
10761076
.at(1)
@@ -1093,7 +1093,7 @@ describe('RunDetails', () => {
10931093
tree = shallow(<RunDetails {...generateProps()} />);
10941094
await getRunSpy;
10951095
await TestUtils.flushPromises();
1096-
tree.find('Graph').simulate('click', 'node1');
1096+
clickGraphNode(tree, 'node1');
10971097
tree
10981098
.find('MD2Tabs')
10991099
.at(1)
@@ -1219,3 +1219,12 @@ describe('RunDetails', () => {
12191219
});
12201220
});
12211221
});
1222+
1223+
function clickGraphNode(wrapper: ShallowWrapper, nodeId: string) {
1224+
// TODO: use dom events instead
1225+
wrapper
1226+
.find('EnhancedGraph')
1227+
.dive()
1228+
.dive()
1229+
.simulate('click', nodeId);
1230+
}

‎frontend/src/pages/RunDetails.tsx

+3
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,9 @@ class RunDetails extends Page<RunDetailsProps, RunDetailsState> {
261261
graph={graph}
262262
selectedNodeId={selectedNodeId}
263263
onClick={id => this._selectNode(id)}
264+
onError={(message, additionalInfo) =>
265+
this.props.updateBanner({ message, additionalInfo, mode: 'error' })
266+
}
264267
/>
265268

266269
<SidePanel

0 commit comments

Comments
 (0)