Pulls most functions out of Status and into StatusUtils. Fixes run du…

…ration bug (kubeflow#1262)

frontend/src/lib/StatusUtils.test.tsx

@@ -0,0 +1,120 @@
+/*
+ * Copyright 2018 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { NodePhase, hasFinished, statusBgColors, statusToBgColor, checkIfTerminated } from './StatusUtils';
+
+describe('StatusUtils', () => {
+
+  describe('hasFinished', () => {
+    [NodePhase.ERROR, NodePhase.FAILED, NodePhase.SUCCEEDED, NodePhase.SKIPPED, NodePhase.TERMINATED].forEach(status => {
+      it(`returns \'true\' if status is: ${status}`, () => {
+        expect(hasFinished(status)).toBe(true);
+      });
+    });
+
+    [NodePhase.PENDING, NodePhase.RUNNING, NodePhase.UNKNOWN, NodePhase.TERMINATING].forEach(status => {
+      it(`returns \'false\' if status is: ${status}`, () => {
+        expect(hasFinished(status)).toBe(false);
+      });
+    });
+
+    it('returns \'false\' if status is undefined', () => {
+      expect(hasFinished(undefined)).toBe(false);
+    });
+
+    it('returns \'false\' if status is invalid', () => {
+      expect(hasFinished('bad phase' as any)).toBe(false);
+    });
+  });
+
+  describe('statusToBgColor', () => {
+    it('handles an invalid phase', () => {
+      const consoleSpy = jest.spyOn(console, 'log').mockImplementationOnce(() => null);
+      expect(statusToBgColor('bad phase' as any)).toEqual(statusBgColors.notStarted);
+      expect(consoleSpy).toHaveBeenLastCalledWith('Unknown node phase:', 'bad phase');
+    });
+
+    it('handles an \'Unknown\' phase', () => {
+      const consoleSpy = jest.spyOn(console, 'log').mockImplementationOnce(() => null);
+      expect(statusToBgColor(NodePhase.UNKNOWN)).toEqual(statusBgColors.notStarted);
+      expect(consoleSpy).toHaveBeenLastCalledWith('Unknown node phase:', 'Unknown');
+    });
+
+    it('returns color \'not started\' if status is undefined', () => {
+      const consoleSpy = jest.spyOn(console, 'log').mockImplementationOnce(() => null);
+      expect(statusToBgColor(undefined)).toEqual(statusBgColors.notStarted);
+      expect(consoleSpy).toHaveBeenLastCalledWith('Unknown node phase:', undefined);
+    });
+
+    it('returns color \'not started\' if status is \'Pending\'', () => {
+      expect(statusToBgColor(NodePhase.PENDING)).toEqual(statusBgColors.notStarted);
+    });
+
+    [NodePhase.ERROR, NodePhase.FAILED].forEach(status => {
+      it(`returns color \'error\' if status is: ${status}`, () => {
+        expect(statusToBgColor(status)).toEqual(statusBgColors.error);
+      });
+    });
+
+    [NodePhase.RUNNING, NodePhase.TERMINATING].forEach(status => {
+      it(`returns color \'running\' if status is: ${status}`, () => {
+        expect(statusToBgColor(status)).toEqual(statusBgColors.running);
+      });
+    });
+
+    [NodePhase.SKIPPED, NodePhase.TERMINATED].forEach(status => {
+      it(`returns color \'terminated or skipped\' if status is: ${status}`, () => {
+        expect(statusToBgColor(status)).toEqual(statusBgColors.terminatedOrSkipped);
+      });
+    });
+
+    it('returns color \'succeeded\' if status is \'Succeeded\'', () => {
+      expect(statusToBgColor(NodePhase.SUCCEEDED)).toEqual(statusBgColors.succeeded);
+    });
+  });
+
+  describe('checkIfTerminated', () => {
+    it('returns status \'terminated\' if status is \'failed\' and error message is \'terminated\'', () => {
+      expect(checkIfTerminated(NodePhase.FAILED, 'terminated')).toEqual(NodePhase.TERMINATED);
+    });
+
+    [
+      NodePhase.SUCCEEDED,
+      NodePhase.ERROR,
+      NodePhase.SKIPPED,
+      NodePhase.PENDING,
+      NodePhase.RUNNING,
+      NodePhase.TERMINATING,
+      NodePhase.UNKNOWN
+    ].forEach(status => {
+      it(`returns the original status, even if message is 'terminated', if status is: ${status}`, () => {
+        expect(checkIfTerminated(status, 'terminated')).toEqual(status);
+      });
+    });
+
+    it('returns \'failed\' if status is \'failed\' and no error message is provided', () => {
+      expect(checkIfTerminated(NodePhase.FAILED)).toEqual(NodePhase.FAILED);
+    });
+
+    it('returns \'failed\' if status is \'failed\' and empty error message is provided', () => {
+      expect(checkIfTerminated(NodePhase.FAILED, '')).toEqual(NodePhase.FAILED);
+    });
+
+    it('returns \'failed\' if status is \'failed\' and arbitrary error message is provided', () => {
+      expect(checkIfTerminated(NodePhase.FAILED, 'some random error')).toEqual(NodePhase.FAILED);
+    });
+  });
+});

frontend/src/lib/StatusUtils.ts

@@ -0,0 +1,93 @@
+/*
+ * Copyright 2019 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { logger } from '../lib/Utils';
+
+export const statusBgColors = {
+  error: '#fce8e6',
+  notStarted: '#f7f7f7',
+  running: '#e8f0fe',
+  succeeded: '#e6f4ea',
+  terminatedOrSkipped: '#f1f3f4',
+  warning: '#fef7f0',
+};
+
+export enum NodePhase {
+  ERROR = 'Error',
+  FAILED = 'Failed',
+  PENDING = 'Pending',
+  RUNNING = 'Running',
+  SKIPPED = 'Skipped',
+  SUCCEEDED = 'Succeeded',
+  TERMINATING = 'Terminating',
+  TERMINATED = 'Terminated',
+  UNKNOWN = 'Unknown',
+}
+
+export function hasFinished(status?: NodePhase): boolean {
+  switch (status) {
+    case NodePhase.SUCCEEDED: // Fall through
+    case NodePhase.FAILED: // Fall through
+    case NodePhase.ERROR: // Fall through
+    case NodePhase.SKIPPED: // Fall through
+    case NodePhase.TERMINATED:
+      return true;
+    case NodePhase.PENDING: // Fall through
+    case NodePhase.RUNNING: // Fall through
+    case NodePhase.TERMINATING: // Fall through
+    case NodePhase.UNKNOWN:
+      return false;
+    default:
+      return false;
+  }
+}
+
+export function statusToBgColor(status?: NodePhase, nodeMessage?: string): string {
+  status = checkIfTerminated(status, nodeMessage);
+  switch (status) {
+    case NodePhase.ERROR:
+      // fall through
+    case NodePhase.FAILED:
+      return statusBgColors.error;
+    case NodePhase.PENDING:
+      return statusBgColors.notStarted;
+    case NodePhase.TERMINATING:
+      // fall through
+    case NodePhase.RUNNING:
+      return statusBgColors.running;
+    case NodePhase.SUCCEEDED:
+      return statusBgColors.succeeded;
+    case NodePhase.SKIPPED:
+      // fall through
+    case NodePhase.TERMINATED:
+      return statusBgColors.terminatedOrSkipped;
+    case NodePhase.UNKNOWN:
+      // fall through
+    default:
+      logger.verbose('Unknown node phase:', status);
+      return statusBgColors.notStarted;
+  }
+}
+
+export function checkIfTerminated(status?: NodePhase, nodeMessage?: string): NodePhase | undefined {
+  // Argo considers terminated runs as having "Failed", so we have to examine the failure message to
+  // determine why the run failed.
+  if (status === NodePhase.FAILED && nodeMessage === 'terminated') {
+    status = NodePhase.TERMINATED;
+  }
+  return status;
+}
+

frontend/src/lib/Utils.test.ts

@@ -21,6 +21,7 @@ import {
   getRunDuration,
   getRunDurationFromWorkflow,
 } from './Utils';
+import { NodePhase } from './StatusUtils';
 
 describe('Utils', () => {
   describe('log', () => {
@@ -102,10 +103,20 @@ describe('Utils', () => {
       expect(getRunDuration(run)).toBe('-');
     });
 
+    it('handles run which has not finished', () => {
+      const run = {
+        created_at: new Date(2018, 1, 2, 3, 55, 30).toISOString(),
+        finished_at: new Date(2018, 1, 3, 3, 56, 25).toISOString(),
+        status: NodePhase.RUNNING,
+      } as any;
+      expect(getRunDuration(run)).toBe('-');
+    });
+
     it('computes seconds', () => {
       const run = {
         created_at: new Date(2018, 1, 2, 3, 55, 30).toISOString(),
         finished_at: new Date(2018, 1, 3, 3, 56, 25).toISOString(),
+        status: NodePhase.SUCCEEDED,
       } as any;
       expect(getRunDuration(run)).toBe('0:00:55');
     });
@@ -114,6 +125,7 @@ describe('Utils', () => {
       const run = {
         created_at: new Date(2018, 1, 2, 3, 55, 10).toISOString(),
         finished_at: new Date(2018, 1, 3, 3, 59, 25).toISOString(),
+        status: NodePhase.SUCCEEDED,
       } as any;
       expect(getRunDuration(run)).toBe('0:04:15');
     });
@@ -122,6 +134,7 @@ describe('Utils', () => {
       const run = {
         created_at: new Date(2018, 1, 2, 3, 55, 10).toISOString(),
         finished_at: new Date(2018, 1, 3, 4, 55, 10).toISOString(),
+        status: NodePhase.SUCCEEDED,
       } as any;
       expect(getRunDuration(run)).toBe('1:00:00');
     });
@@ -130,6 +143,7 @@ describe('Utils', () => {
       const run = {
         created_at: new Date(2018, 1, 2, 3, 55, 10).toISOString(),
         finished_at: new Date(2018, 1, 3, 4, 56, 11).toISOString(),
+        status: NodePhase.SUCCEEDED,
       } as any;
       expect(getRunDuration(run)).toBe('1:01:01');
     });
@@ -138,6 +152,7 @@ describe('Utils', () => {
       const run = {
         created_at: new Date(2018, 1, 2, 3, 55, 13).toISOString(),
         finished_at: new Date(2018, 1, 2, 3, 55, 11).toISOString(),
+        status: NodePhase.SUCCEEDED,
       } as any;
       expect(getRunDuration(run)).toBe('-0:00:02');
     });

frontend/src/lib/Utils.ts

@@ -18,6 +18,7 @@ import { ApiRun } from '../apis/run';
 import { ApiTrigger } from '../apis/job';
 import { Workflow } from '../../third_party/argo-ui/argo_template';
 import { isFunction } from 'lodash';
+import { hasFinished, NodePhase } from './StatusUtils';
 
 export const logger = {
   error: (...args: any[]) => {
@@ -75,7 +76,7 @@ function getDuration(start: Date, end: Date): string {
 }
 
 export function getRunDuration(run?: ApiRun): string {
-  if (!run || !run.created_at || !run.finished_at) {
+  if (!run || !run.created_at || !run.finished_at || !hasFinished(run.status as NodePhase)) {
     return '-';
   }
 

frontend/src/lib/WorkflowParser.test.ts

@@ -15,7 +15,7 @@
  */
 
 import WorkflowParser, { StorageService } from './WorkflowParser';
-import { NodePhase } from '../pages/Status';
+import { NodePhase } from '../lib/StatusUtils';
 import { color } from '../Css';
 import { Constants } from './Constants';
 

frontend/src/lib/WorkflowParser.ts

@@ -18,9 +18,10 @@ import * as dagre from 'dagre';
 import IconWithTooltip from '../atoms/IconWithTooltip';
 import MoreIcon from '@material-ui/icons/MoreHoriz';
 import { Workflow, NodeStatus, Parameter } from '../../third_party/argo-ui/argo_template';
-import { statusToIcon, NodePhase, hasFinished, statusToBgColor } from '../pages/Status';
+import { statusToIcon } from '../pages/Status';
 import { color } from '../Css';
 import { Constants } from './Constants';
+import { NodePhase, statusToBgColor, hasFinished } from './StatusUtils';
 
 export enum StorageService {
   GCS = 'gcs',

frontend/src/pages/ExperimentList.test.tsx

@@ -22,7 +22,7 @@ import { ApiFilter, PredicateOp } from '../apis/filter';
 import { ApiResourceType, RunStorageState } from '../apis/run';
 import { Apis } from '../lib/Apis';
 import { ExpandState } from '../components/CustomTable';
-import { NodePhase } from './Status';
+import { NodePhase } from '../lib/StatusUtils';
 import { PageProps } from './Page';
 import { ReactWrapper, ShallowWrapper, shallow } from 'enzyme';
 import { RoutePage, QUERY_PARAMS } from '../components/Router';

frontend/src/pages/ExperimentList.tsx

@@ -24,13 +24,14 @@ import { ApiListExperimentsResponse, ApiExperiment } from '../apis/experiment';
 import { ApiResourceType, ApiRun, RunStorageState } from '../apis/run';
 import { Apis, ExperimentSortKeys, ListRequest, RunSortKeys } from '../lib/Apis';
 import { Link } from 'react-router-dom';
+import { NodePhase } from '../lib/StatusUtils';
 import { Page } from './Page';
 import { RoutePage, RouteParams } from '../components/Router';
 import { ToolbarProps } from '../components/Toolbar';
 import { classes } from 'typestyle';
 import { commonCss, padding } from '../Css';
 import { logger } from '../lib/Utils';
-import { statusToIcon, NodePhase } from './Status';
+import { statusToIcon } from './Status';
 
 interface DisplayExperiment extends ApiExperiment {
   last5Runs?: ApiRun[];

frontend/src/pages/RunDetails.test.tsx

@@ -21,13 +21,13 @@ import TestUtils from '../TestUtils';
 import WorkflowParser from '../lib/WorkflowParser';
 import { ApiRunDetail, ApiResourceType, RunStorageState } from '../apis/run';
 import { Apis } from '../lib/Apis';
+import { NodePhase } from '../lib/StatusUtils';
 import { OutputArtifactLoader } from '../lib/OutputArtifactLoader';
 import { PageProps } from './Page';
 import { PlotType } from '../components/viewers/Viewer';
 import { RouteParams, RoutePage, QUERY_PARAMS } from '../components/Router';
 import { Workflow } from 'third_party/argo-ui/argo_template';
 import { shallow, ShallowWrapper } from 'enzyme';
-import { NodePhase } from './Status';
 
 describe('RunDetails', () => {
   const updateBannerSpy = jest.fn();

frontend/src/pages/RunDetails.tsx

@@ -32,7 +32,7 @@ import WorkflowParser from '../lib/WorkflowParser';
 import { ApiExperiment } from '../apis/experiment';
 import { ApiRun, RunStorageState } from '../apis/run';
 import { Apis } from '../lib/Apis';
-import { NodePhase, statusToIcon, hasFinished } from './Status';
+import { NodePhase, hasFinished } from '../lib/StatusUtils';
 import { OutputArtifactLoader } from '../lib/OutputArtifactLoader';
 import { Page } from './Page';
 import { RoutePage, RouteParams } from '../components/Router';
@@ -44,6 +44,7 @@ import { commonCss, padding, color, fonts, fontsize } from '../Css';
 import { componentMap } from '../components/viewers/ViewerContainer';
 import { flatten } from 'lodash';
 import { formatDateString, getRunDurationFromWorkflow, logger, errorToMessage } from '../lib/Utils';
+import { statusToIcon } from './Status';
 
 enum SidePaneTab {
   ARTIFACTS,

frontend/src/pages/RunList.test.tsx

@@ -23,7 +23,7 @@ import { ApiFilter, PredicateOp } from '../apis/filter';
 import { ApiRun, ApiRunDetail, ApiResourceType, ApiRunMetric, RunMetricFormat, RunStorageState } from '../apis/run';
 import { Apis, RunSortKeys, ListRequest } from '../lib/Apis';
 import { MetricMetadata } from '../lib/RunUtils';
-import { NodePhase } from './Status';
+import { NodePhase } from '../lib/StatusUtils';
 import { ReactWrapper, ShallowWrapper, shallow } from 'enzyme';
 import { range } from 'lodash';
 

frontend/src/pages/RunList.tsx

@@ -20,12 +20,13 @@ import RunUtils, { MetricMetadata } from '../../src/lib/RunUtils';
 import { ApiRun, ApiResourceType, RunMetricFormat, ApiRunMetric, RunStorageState, ApiRunDetail } from '../../src/apis/run';
 import { Apis, RunSortKeys, ListRequest } from '../lib/Apis';
 import { Link, RouteComponentProps } from 'react-router-dom';
-import { NodePhase, statusToIcon } from './Status';
+import { NodePhase } from '../lib/StatusUtils';
 import { PredicateOp, ApiFilter } from '../apis/filter';
 import { RoutePage, RouteParams, QUERY_PARAMS } from '../components/Router';
 import { URLParser } from '../lib/URLParser';
 import { commonCss, color } from '../Css';
 import { formatDateString, logger, errorToMessage, getRunDuration } from '../lib/Utils';
+import { statusToIcon } from './Status';
 import { stylesheet } from 'typestyle';
 
 const css = stylesheet({