[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
kubeflow · Dec 26, 2019 · 488805c · 488805c
1 parent 840979c
commit 488805c
Show file tree

Hide file tree

Showing 9 changed files with 325 additions and 222 deletions.
diff --git a/frontend/src/components/Graph.test.tsx b/frontend/src/components/Graph.test.tsx
@@ -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';
 
@@ -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();
@@ -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'.'.",
+    );
+  });
 });
diff --git a/frontend/src/components/Graph.tsx b/frontend/src/components/Graph.tsx
@@ -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;
@@ -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
@@ -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;
diff --git a/frontend/src/components/__snapshots__/Graph.test.tsx.snap b/frontend/src/components/__snapshots__/Graph.test.tsx.snap
@@ -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>"`;
diff --git a/frontend/src/pages/PipelineDetails.test.tsx b/frontend/src/pages/PipelineDetails.test.tsx
@@ -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();
   });
@@ -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();
   });
 
@@ -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);
+}
diff --git a/frontend/src/pages/PipelineDetails.tsx b/frontend/src/pages/PipelineDetails.tsx
@@ -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

diff --git a/frontend/src/pages/RunDetails.test.tsx b/frontend/src/pages/RunDetails.test.tsx
@@ -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();
   });
@@ -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',
@@ -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;
@@ -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)
@@ -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)
@@ -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)
@@ -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');
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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);
+}
diff --git a/frontend/src/pages/RunDetails.tsx b/frontend/src/pages/RunDetails.tsx
@@ -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