diff --git a/frontend/src/components/Graph.test.tsx b/frontend/src/components/Graph.test.tsx index 96340be29cb..680e426e88e 100644 --- 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()).toMatchSnapshot(); @@ -149,4 +153,19 @@ describe('Graph', () => { graph.setEdge('node1', 'node2'); expect(shallow()).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().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 index ed8ebe51aeb..f33b5a8b209 100644 --- 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 { +interface GraphErrorBoundaryProps { + onError?: (message: string, additionalInfo: string) => void; +} +class GraphErrorBoundary extends React.Component { + 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 ?
: this.props.children; + } +} + +export class Graph extends React.Component { private LEFT_OFFSET = 100; private TOP_OFFSET = 44; private EDGE_THICKNESS = 2; @@ -154,6 +178,10 @@ export default class Graph extends React.Component { 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 { return color.grey; } } + +const EnhancedGraph = (props: GraphProps & GraphErrorBoundaryProps) => ( + + + +); +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 index f9dca594ca5..454858c968f 100644 --- 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`] = `
`; + +exports[`Graph shows an error message when the graph is invalid 1`] = `"
"`; diff --git a/frontend/src/pages/PipelineDetails.test.tsx b/frontend/src/pages/PipelineDetails.test.tsx index 855ce625998..5a8ee6e10cc 100644 --- a/frontend/src/pages/PipelineDetails.test.tsx +++ b/frontend/src/pages/PipelineDetails.test.tsx @@ -606,7 +606,7 @@ describe('PipelineDetails', () => { tree = shallow(); 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(); await getPipelineVersionTemplateSpy; await TestUtils.flushPromises(); - tree.find('Graph').simulate('click', 'node1'); + clickGraphNode(tree, 'node1'); expect(tree).toMatchSnapshot(); }); @@ -650,7 +650,7 @@ 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(); await getPipelineVersionTemplateSpy; await TestUtils.flushPromises(); @@ -658,3 +658,12 @@ describe('PipelineDetails', () => { 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 index 6e20888da68..98aa3814e65 100644 --- 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' }) + } /> { tree = shallow(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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(); 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 index 28b85bb0638..7d287c0b1c1 100644 --- a/frontend/src/pages/RunDetails.tsx +++ b/frontend/src/pages/RunDetails.tsx @@ -261,6 +261,9 @@ class RunDetails extends Page { graph={graph} selectedNodeId={selectedNodeId} onClick={id => this._selectNode(id)} + onError={(message, additionalInfo) => + this.props.updateBanner({ message, additionalInfo, mode: 'error' }) + } /> - - -
-
- Unable to retrieve node info -
-
-
-
-
- - - Static pipeline graph - -
-
- - - - - -`; - -exports[`PipelineDetails closes side panel when close button is clicked 2`] = ` -
-
- -
-
-
- -
-
- Summary -
- - Hide - -
-
- ID -
-
- test-pipeline-id -
-
- - - Version - - - - test-pipeline-version - - - -
- -
- Uploaded on -
-
- 9/5/2018, 4:03:02 AM -
-
- Description -
- -
- - - - `; +exports[`PipelineDetails shows correct versions in version selector 1`] = ` +
+
+ +
+
+
+ +
+
+ Summary +
+ + Hide + +
+
+ ID +
+
+ test-pipeline-id +
+
+ + + Version + + + + test-pipeline-version + + + +
+ +
+ Uploaded on +
+
+ 9/5/2018, 4:03:02 AM +
+
+ Description +
+ +
+ + +
+
+ Unable to retrieve node info +
+
+
+
+
+ + + Static pipeline graph + +
+
+
+
+
+
+
+`; + exports[`PipelineDetails shows empty pipeline details with empty graph 1`] = `
- - - - - - - - - - - - - - - -