feat: adds more identifying information for node executions (#70)

* feat: show workflow node name beneath node execution ids * feat: updating DetailsPanel info for NodeExecutions * fix lint errors * adding tests for new formatter function * test: adding new test for NodeExecutionDetails * test: adding test for new code in NodeExecutionsTable
flyteorg · Jun 30, 2020 · e11befc · e11befc
1 parent 90362e2
commit e11befc
Show file tree

Hide file tree

Showing 10 changed files with 222 additions and 34 deletions.
diff --git a/src/common/formatters.ts b/src/common/formatters.ts
@@ -144,3 +144,12 @@ export function ensureUrlWithProtocol(url: string): string {
     }
     return url;
 }
+
+/** Formats a number into a string with leading zeros to ensure a consistent
+ * width.
+ * Example: 1 will be '01'
+ *          10 will be '10'
+ */
+export function leftPaddedNumber(value: number, length: number): string {
+    return value.toString().padStart(length, '0');
+}
diff --git a/src/common/test/formatters.spec.ts b/src/common/test/formatters.spec.ts
@@ -1,5 +1,4 @@
 import { millisecondsToDuration } from 'common/utils';
-import { Admin } from 'flyteidl';
 import {
     subSecondString,
     unknownValueString,
@@ -10,10 +9,10 @@ import {
     dateFromNow,
     dateWithFromNow,
     ensureUrlWithProtocol,
-    fixedRateToString,
     formatDate,
     formatDateLocalTimezone,
     formatDateUTC,
+    leftPaddedNumber,
     millisecondsToHMS,
     protobufDurationToHMS
 } from '../formatters';
@@ -177,3 +176,24 @@ describe('ensureUrlWithProtocol', () => {
         })
     );
 });
+
+describe('leftPaddedNumber', () => {
+    const cases: [number, number, string][] = [
+        [1, 0, '1'],
+        [10, 0, '10'],
+        [0, 1, '0'],
+        [0, 2, '00'],
+        [1, 1, '1'],
+        [1, 2, '01'],
+        [1, 3, '001'],
+        [10, 1, '10'],
+        [10, 2, '10'],
+        [10, 3, '010']
+    ];
+
+    cases.forEach(([value, width, expected]) =>
+        it(`should produce ${expected} with input (${value}, ${width})`, () => {
+            expect(leftPaddedNumber(value, width)).toEqual(expected);
+        })
+    );
+});
diff --git a/src/components/Executions/ExecutionDetails/NodeExecutionDetails.tsx b/src/components/Executions/ExecutionDetails/NodeExecutionDetails.tsx
@@ -35,6 +35,9 @@ const useStyles = makeStyles((theme: Theme) => {
         content: {
             overflowY: 'auto'
         },
+        displayId: {
+            marginBottom: theme.spacing(1)
+        },
         header: {
             borderBottom: `${theme.spacing(1)}px solid ${theme.palette.divider}`
         },
@@ -60,8 +63,7 @@ const useStyles = makeStyles((theme: Theme) => {
         title: {
             alignItems: 'flex-start',
             display: 'flex',
-            justifyContent: 'space-between',
-            marginBottom: theme.spacing(1)
+            justifyContent: 'space-between'
         }
     };
 });
@@ -189,6 +191,16 @@ export const NodeExecutionDetails: React.FC<NodeExecutionDetailsProps> = ({
                             <Close />
                         </IconButton>
                     </Typography>
+                    <Typography
+                        className={classnames(
+                            commonStyles.textWrapped,
+                            styles.displayId
+                        )}
+                        variant="subtitle1"
+                        color="textSecondary"
+                    >
+                        {execution.displayId}
+                    </Typography>
                     {statusContent}
                     <ExecutionTypeDetails execution={execution} />
                 </div>

diff --git a/src/components/Executions/ExecutionDetails/test/NodeExecutionDetails.test.tsx b/src/components/Executions/ExecutionDetails/test/NodeExecutionDetails.test.tsx
@@ -0,0 +1,45 @@
+import { render, waitFor } from '@testing-library/react';
+import { mockAPIContextValue } from 'components/data/__mocks__/apiContext';
+import { APIContext } from 'components/data/apiContext';
+import {
+    DetailedNodeExecution,
+    NodeExecutionDisplayType
+} from 'components/Executions/types';
+import { createMockNodeExecutions } from 'models/Execution/__mocks__/mockNodeExecutionsData';
+import { listTaskExecutions } from 'models/Execution/api';
+import * as React from 'react';
+import { NodeExecutionDetails } from '../NodeExecutionDetails';
+
+describe('NodeExecutionDetails', () => {
+    let execution: DetailedNodeExecution;
+    let mockListTaskExecutions: jest.Mock<ReturnType<
+        typeof listTaskExecutions
+    >>;
+    beforeEach(() => {
+        const { executions } = createMockNodeExecutions(1);
+        execution = {
+            ...executions[0],
+            displayType: NodeExecutionDisplayType.PythonTask,
+            displayId: 'com.flyte.testTask',
+            cacheKey: 'abcdefg'
+        };
+        mockListTaskExecutions = jest.fn().mockResolvedValue({ entities: [] });
+    });
+
+    const renderComponent = () =>
+        render(
+            <APIContext.Provider
+                value={mockAPIContextValue({
+                    listTaskExecutions: mockListTaskExecutions
+                })}
+            >
+                <NodeExecutionDetails execution={execution} />
+            </APIContext.Provider>
+        );
+
+    it('renders displayId', async () => {
+        const { queryByText } = renderComponent();
+        await waitFor(() => {});
+        expect(queryByText(execution.displayId)).toBeInTheDocument();
+    });
+});
diff --git a/src/components/Executions/Tables/nodeExecutionColumns.tsx b/src/components/Executions/Tables/nodeExecutionColumns.tsx
@@ -49,7 +49,14 @@ export function generateColumns(
 ): NodeExecutionColumnDefinition[] {
     return [
         {
-            cellRenderer: props => <NodeExecutionName {...props} />,
+            cellRenderer: props => (
+                <>
+                    <NodeExecutionName {...props} />
+                    <Typography variant="subtitle1" color="textSecondary">
+                        {props.execution.displayId}
+                    </Typography>
+                </>
+            ),
             className: styles.columnName,
             key: 'name',
             label: 'node'

diff --git a/src/components/Executions/Tables/test/NodeExecutionsTable.test.tsx b/src/components/Executions/Tables/test/NodeExecutionsTable.test.tsx
@@ -0,0 +1,76 @@
+import { render, waitFor } from '@testing-library/react';
+import { mapNodeExecutionDetails } from 'components';
+import { mockAPIContextValue } from 'components/data/__mocks__/apiContext';
+import { APIContext } from 'components/data/apiContext';
+import { DetailedNodeExecution } from 'components/Executions/types';
+import {
+    createMockWorkflow,
+    createMockWorkflowClosure
+} from 'models/__mocks__/workflowData';
+import { createMockNodeExecutions } from 'models/Execution/__mocks__/mockNodeExecutionsData';
+import { listTaskExecutions } from 'models/Execution/api';
+import { mockTasks } from 'models/Task/__mocks__/mockTaskData';
+import * as React from 'react';
+import {
+    NodeExecutionsTable,
+    NodeExecutionsTableProps
+} from '../NodeExecutionsTable';
+
+describe('NodeExecutionsTable', () => {
+    let props: NodeExecutionsTableProps;
+    let mockNodeExecutions: DetailedNodeExecution[];
+    let mockListTaskExecutions: jest.Mock<ReturnType<
+        typeof listTaskExecutions
+    >>;
+
+    beforeEach(() => {
+        mockListTaskExecutions = jest.fn().mockResolvedValue({ entities: [] });
+        const {
+            executions: mockExecutions,
+            nodes: mockNodes
+        } = createMockNodeExecutions(1);
+
+        const mockWorkflow = createMockWorkflow('SampleWorkflow');
+        const mockWorkflowClosure = createMockWorkflowClosure();
+        const compiledWorkflow = mockWorkflowClosure.compiledWorkflow!;
+        const {
+            primary: { template },
+            tasks
+        } = compiledWorkflow;
+        template.nodes = template.nodes.concat(mockNodes);
+        compiledWorkflow.tasks = tasks.concat(mockTasks);
+        mockWorkflow.closure = mockWorkflowClosure;
+
+        mockNodeExecutions = mapNodeExecutionDetails(
+            mockExecutions,
+            mockWorkflow
+        );
+
+        props = {
+            value: mockNodeExecutions,
+            lastError: null,
+            loading: false,
+            moreItemsAvailable: false,
+            fetch: jest.fn()
+        };
+    });
+
+    const renderTable = () =>
+        render(
+            <APIContext.Provider
+                value={mockAPIContextValue({
+                    listTaskExecutions: mockListTaskExecutions
+                })}
+            >
+                <NodeExecutionsTable {...props} />
+            </APIContext.Provider>
+        );
+
+    it('renders displayId for nodes', async () => {
+        const { queryByText } = renderTable();
+        await waitFor(() => {});
+
+        const node = mockNodeExecutions[0];
+        expect(queryByText(node.displayId)).toBeInTheDocument();
+    });
+});
diff --git a/src/components/Executions/TaskExecutionsList/TaskExecutionsListItem.tsx b/src/components/Executions/TaskExecutionsList/TaskExecutionsListItem.tsx
@@ -11,7 +11,7 @@ import { ExecutionStatusBadge } from '../ExecutionStatusBadge';
 import { TaskExecutionDetails } from './TaskExecutionDetails';
 import { TaskExecutionError } from './TaskExecutionError';
 import { TaskExecutionLogs } from './TaskExecutionLogs';
-import { getUniqueTaskExecutionName } from './utils';
+import { formatRetryAttempt } from './utils';
 
 const useStyles = makeStyles((theme: Theme) => ({
     detailsLink: {
@@ -43,7 +43,7 @@ export const TaskExecutionsListItem: React.FC<TaskExecutionsListItemProps> = ({
     const styles = useStyles();
     const { closure, isParent } = taskExecution;
     const { error } = closure;
-    const headerText = getUniqueTaskExecutionName(taskExecution);
+    const headerText = formatRetryAttempt(taskExecution.id.retryAttempt);
     const taskHasStarted = closure.phase >= TaskExecutionPhase.QUEUED;
     return (
         <div className={commonStyles.detailsPanelCard}>

diff --git a/src/components/Executions/TaskExecutionsList/test/utils.spec.ts b/src/components/Executions/TaskExecutionsList/test/utils.spec.ts
@@ -1,5 +1,5 @@
 import { obj } from 'test/utils';
-import { getUniqueTaskExecutionName } from '../utils';
+import { formatRetryAttempt, getUniqueTaskExecutionName } from '../utils';
 
 describe('getUniqueTaskExecutionName', () => {
     const cases: [{ name: string; retryAttempt: number }, string][] = [
@@ -20,3 +20,16 @@ describe('getUniqueTaskExecutionName', () => {
             ).toEqual(expected))
     );
 });
+
+describe('formatRetryAttempt', () => {
+    const cases: [number, string][] = [
+        [0, 'Attempt 01'],
+        [1, 'Attempt 02'],
+        [2, 'Attempt 03']
+    ];
+
+    cases.forEach(([input, expected]) =>
+        it(`should return ${expected} for ${input}`, () =>
+            expect(formatRetryAttempt(input)).toEqual(expected))
+    );
+});
diff --git a/src/components/Executions/TaskExecutionsList/utils.ts b/src/components/Executions/TaskExecutionsList/utils.ts
@@ -1,3 +1,4 @@
+import { leftPaddedNumber } from 'common/formatters';
 import { TaskExecution } from 'models';
 
 /** Generates a unique name for a task execution, suitable for display in a
@@ -11,3 +12,8 @@ export function getUniqueTaskExecutionName({ id }: TaskExecution) {
     const suffix = retryAttempt > 0 ? ` (${retryAttempt + 1})` : '';
     return `${name}${suffix}`;
 }
+
+export function formatRetryAttempt(attempt: number): string {
+    // Retry attempts are zero-based, so incrementing before formatting
+    return `Attempt ${leftPaddedNumber(attempt + 1, 2)}`;
+}
diff --git a/tsconfig.json b/tsconfig.json
@@ -1,28 +1,28 @@
 {
-    "compilerOptions": {
-        "allowSyntheticDefaultImports": true,
-        "allowJs": true,
-        "baseUrl": "src",
-        "checkJs": false,
-        "downlevelIteration": true,
-        "emitDecoratorMetadata": true,
-        "experimentalDecorators": true,
-        "importHelpers": true,
-        "jsx": "react",
-        "lib": ["es2015", "es2017", "dom", "dom.iterable"],
-        "module": "commonjs",
-        "moduleResolution": "node",
-        "noFallthroughCasesInSwitch": true,
-        "noImplicitReturns": true,
-        "noUnusedLocals": false,
-        "noUnusedParameters": false,
-        "outDir": "./dist",
-        "removeComments": false,
-        "resolveJsonModule": true,
-        "skipLibCheck": true,
-        "sourceMap": true,
-        "strict": true,
-        "target": "es6",
-        "types": ["node", "webpack-env", "jest"]
-    }
+  "compilerOptions": {
+    "allowSyntheticDefaultImports": true,
+    "allowJs": true,
+    "baseUrl": "src",
+    "checkJs": false,
+    "downlevelIteration": true,
+    "emitDecoratorMetadata": true,
+    "experimentalDecorators": true,
+    "importHelpers": true,
+    "jsx": "react",
+    "lib": ["es2015", "es2017", "dom", "dom.iterable"],
+    "module": "commonjs",
+    "moduleResolution": "node",
+    "noFallthroughCasesInSwitch": true,
+    "noImplicitReturns": true,
+    "noUnusedLocals": false,
+    "noUnusedParameters": false,
+    "outDir": "./dist",
+    "removeComments": false,
+    "resolveJsonModule": true,
+    "skipLibCheck": true,
+    "sourceMap": true,
+    "strict": true,
+    "target": "es6",
+    "types": ["node", "webpack-env", "jest"]
+  }
 }