feat: Fetch Node Executions directly if possible (#109)

* chore: update flyteidl

* feat: support proper parent node executions

* test: first step of tests for new child loading

* test: remaining tests for new child node fetching

* fix: skip fetching task executions for isParentNode: false

package.json

@@ -78,7 +78,7 @@
     "@commitlint/cli": "^8.3.5",
     "@commitlint/config-conventional": "^8.3.4",
     "@date-io/moment": "1.3.9",
-    "@lyft/flyteidl": "^0.18.4",
+    "@lyft/flyteidl": "^0.18.9",
     "@material-ui/core": "^4.0.0",
     "@material-ui/icons": "^4.0.0",
     "@material-ui/pickers": "^3.2.2",

src/components/Executions/Tables/NodeExecutionChildren.tsx

@@ -40,13 +40,14 @@ export const NodeExecutionChildren: React.FC<NodeExecutionChildrenProps> = ({
                 ));
                 const key = `group-${name}`;
                 return showNames ? (
-                    <div key={key}>
+                    <div key={key} role="list">
                         <div
                             className={classnames(
                                 { [tableStyles.borderTop]: groupIndex > 0 },
                                 tableStyles.borderBottom,
                                 tableStyles.childGroupLabel
                             )}
+                            title={name}
                             style={childGroupLabelStyle}
                         >
                             <Typography
@@ -59,7 +60,9 @@ export const NodeExecutionChildren: React.FC<NodeExecutionChildrenProps> = ({
                         <div>{rows}</div>
                     </div>
                 ) : (
-                    <div key={key}>{rows}</div>
+                    <div key={key} role="list">
+                        {rows}
+                    </div>
                 );
             })}
         </>

src/components/Executions/Tables/NodeExecutionRow.tsx

@@ -93,6 +93,7 @@ export const NodeExecutionRow: React.FC<NodeExecutionRowProps> = ({
 
     return (
         <div
+            role="listitem"
             className={classnames(tableStyles.row, {
                 [selectedClassName]: selected
             })}

src/components/Executions/Tables/RowExpander.tsx

@@ -2,6 +2,7 @@ import { IconButton } from '@material-ui/core';
 import ChevronRight from '@material-ui/icons/ChevronRight';
 import ExpandMore from '@material-ui/icons/ExpandMore';
 import * as React from 'react';
+import { titleStrings } from './constants';
 
 /** A simple expand/collapse arrow to be rendered next to row items. */
 export const RowExpander: React.FC<{
@@ -12,6 +13,7 @@ export const RowExpander: React.FC<{
         disableRipple={true}
         disableTouchRipple={true}
         size="small"
+        title={titleStrings.expandRow}
         onClick={onClick}
     >
         {expanded ? <ExpandMore /> : <ChevronRight />}

src/components/Executions/Tables/constants.ts

@@ -15,3 +15,8 @@ export const nodeExecutionsTableColumnWidths = {
     phase: 150,
     startedAt: 200
 };
+
+export const titleStrings = {
+    expandRow: 'Expand row',
+    groupName: 'Group name'
+};

src/components/Executions/Tables/test/NodeExecutionsTable.test.tsx

@@ -1,4 +1,10 @@
-import { render, waitFor } from '@testing-library/react';
+import {
+    fireEvent,
+    getAllByRole,
+    getByTitle,
+    render,
+    waitFor
+} from '@testing-library/react';
 import { mockAPIContextValue } from 'components/data/__mocks__/apiContext';
 import { APIContext, APIContextValue } from 'components/data/apiContext';
 import { createMockExecutionEntities } from 'components/Executions/__mocks__/createMockExecutionEntities';
@@ -13,29 +19,39 @@ import { ExecutionDataCache } from 'components/Executions/types';
 import { createExecutionDataCache } from 'components/Executions/useExecutionDataCache';
 import { fetchStates } from 'components/hooks/types';
 import { Core } from 'flyteidl';
+import { cloneDeep } from 'lodash';
 import {
+    CompiledNode,
+    Execution,
     FilterOperationName,
     getTask,
     NodeExecution,
+    nodeExecutionQueryParams,
     RequestConfig,
+    TaskExecution,
     TaskNodeMetadata,
+    Workflow,
     WorkflowExecutionIdentifier
 } from 'models';
 import { createMockExecution } from 'models/__mocks__/executionsData';
 import {
+    createMockTaskExecutionForNodeExecution,
     createMockTaskExecutionsListResponse,
     mockExecution as mockTaskExecution
 } from 'models/Execution/__mocks__/mockTaskExecutionsData';
 import {
     getExecution,
+    listNodeExecutions,
     listTaskExecutionChildren,
     listTaskExecutions
 } from 'models/Execution/api';
 import { mockTasks } from 'models/Task/__mocks__/mockTaskData';
 import * as React from 'react';
 import { makeIdentifier } from 'test/modelUtils';
+import { obj } from 'test/utils';
 import { Identifier } from 'typescript';
 import { State } from 'xstate';
+import { titleStrings } from '../constants';
 import {
     NodeExecutionsTable,
     NodeExecutionsTableProps
@@ -48,6 +64,8 @@ describe('NodeExecutionsTable', () => {
     let dataCache: ExecutionDataCache;
     let requestConfig: RequestConfig;
     let mockNodeExecutions: NodeExecution[];
+    let mockNodes: CompiledNode[];
+    let mockWorkflow: Workflow;
     let mockGetExecution: jest.Mock<ReturnType<typeof getExecution>>;
     let mockGetTask: jest.Mock<ReturnType<typeof getTask>>;
     let mockListTaskExecutions: jest.Mock<ReturnType<
@@ -56,9 +74,13 @@ describe('NodeExecutionsTable', () => {
     let mockListTaskExecutionChildren: jest.Mock<ReturnType<
         typeof listTaskExecutionChildren
     >>;
+    let mockListNodeExecutions: jest.Mock<ReturnType<
+        typeof listNodeExecutions
+    >>;
 
     beforeEach(() => {
         const {
+            nodes,
             nodeExecutions,
             workflow,
             workflowExecution
@@ -67,8 +89,11 @@ describe('NodeExecutionsTable', () => {
             nodeExecutionCount: 2
         });
 
+        mockNodes = nodes;
         mockNodeExecutions = nodeExecutions;
+        mockWorkflow = workflow;
 
+        mockListNodeExecutions = jest.fn().mockResolvedValue({ entities: [] });
         mockListTaskExecutions = jest.fn().mockResolvedValue({ entities: [] });
         mockListTaskExecutionChildren = jest
             .fn()
@@ -85,6 +110,7 @@ describe('NodeExecutionsTable', () => {
         apiContext = mockAPIContextValue({
             getExecution: mockGetExecution,
             getTask: mockGetTask,
+            listNodeExecutions: mockListNodeExecutions,
             listTaskExecutions: mockListTaskExecutions,
             listTaskExecutionChildren: mockListTaskExecutionChildren
         });
@@ -103,7 +129,7 @@ describe('NodeExecutionsTable', () => {
         };
 
         props = {
-            value: nodeExecutions,
+            value: mockNodeExecutions,
             lastError: null,
             state: State.from(fetchStates.LOADED),
             moreItemsAvailable: false,
@@ -130,16 +156,198 @@ describe('NodeExecutionsTable', () => {
         const { queryAllByText } = renderTable();
         await waitFor(() => {});
 
-        const node = dataCache.getNodeForNodeExecution(
-            mockNodeExecutions[0].id
-        );
+        const node = dataCache.getNodeForNodeExecution(mockNodeExecutions[0]);
         const taskId = node?.node.taskNode?.referenceId;
         expect(taskId).toBeDefined();
         const task = dataCache.getTaskTemplate(taskId!);
         expect(task).toBeDefined();
         expect(queryAllByText(task!.id.name)[0]).toBeInTheDocument();
     });
 
+    describe('for nodes with children', () => {
+        let parentNodeExecution: NodeExecution;
+        let childNodeExecutions: NodeExecution[];
+        beforeEach(() => {
+            parentNodeExecution = mockNodeExecutions[0];
+        });
+
+        const expandParentNode = async (container: HTMLElement) => {
+            const expander = await waitFor(() =>
+                getByTitle(container, titleStrings.expandRow)
+            );
+            fireEvent.click(expander);
+            return await waitFor(() => getAllByRole(container, 'list'));
+        };
+
+        describe('with isParentNode flag', () => {
+            beforeEach(() => {
+                const id = parentNodeExecution.id;
+                const { nodeId } = id;
+                childNodeExecutions = [
+                    {
+                        ...parentNodeExecution,
+                        id: { ...id, nodeId: `${nodeId}-child1` },
+                        metadata: { retryGroup: '0', specNodeId: nodeId }
+                    },
+                    {
+                        ...parentNodeExecution,
+                        id: { ...id, nodeId: `${nodeId}-child2` },
+                        metadata: { retryGroup: '0', specNodeId: nodeId }
+                    },
+                    {
+                        ...parentNodeExecution,
+                        id: { ...id, nodeId: `${nodeId}-child1` },
+                        metadata: { retryGroup: '1', specNodeId: nodeId }
+                    },
+                    {
+                        ...parentNodeExecution,
+                        id: { ...id, nodeId: `${nodeId}-child2` },
+                        metadata: { retryGroup: '1', specNodeId: nodeId }
+                    }
+                ];
+                mockNodeExecutions[0].metadata = { isParentNode: true };
+                mockListNodeExecutions.mockResolvedValue({
+                    entities: childNodeExecutions
+                });
+            });
+
+            it('correctly fetches children', async () => {
+                const { getByText } = renderTable();
+                await waitFor(() => getByText(mockNodeExecutions[0].id.nodeId));
+                expect(mockListNodeExecutions).toHaveBeenCalledWith(
+                    expect.anything(),
+                    expect.objectContaining({
+                        params: {
+                            [nodeExecutionQueryParams.parentNodeId]:
+                                parentNodeExecution.id.nodeId
+                        }
+                    })
+                );
+                expect(mockListTaskExecutionChildren).not.toHaveBeenCalled();
+            });
+
+            it('does not fetch children if flag is false', async () => {
+                mockNodeExecutions[0].metadata = { isParentNode: false };
+                const { getByText } = renderTable();
+                await waitFor(() => getByText(mockNodeExecutions[0].id.nodeId));
+                expect(mockListNodeExecutions).not.toHaveBeenCalled();
+                expect(mockListTaskExecutionChildren).not.toHaveBeenCalled();
+            });
+
+            it('correctly renders groups', async () => {
+                const { container } = renderTable();
+                const childGroups = await expandParentNode(container);
+                expect(childGroups).toHaveLength(2);
+            });
+        });
+
+        describe('without isParentNode flag, using taskNodeMetadata ', () => {
+            let taskExecutions: TaskExecution[];
+            beforeEach(() => {
+                taskExecutions = [0, 1].map(retryAttempt =>
+                    createMockTaskExecutionForNodeExecution(
+                        parentNodeExecution.id,
+                        mockNodes[0],
+                        retryAttempt,
+                        { isParent: true }
+                    )
+                );
+                childNodeExecutions = [
+                    {
+                        ...parentNodeExecution
+                    }
+                ];
+                mockNodeExecutions = mockNodeExecutions.slice(0, 1);
+                // In the case of a non-isParent node execution, API should not
+                // return anything from the list endpoint
+                mockListNodeExecutions.mockResolvedValue({ entities: [] });
+                mockListTaskExecutions.mockImplementation(async id => {
+                    const entities =
+                        id.nodeId === parentNodeExecution.id.nodeId
+                            ? taskExecutions
+                            : [];
+                    return { entities };
+                });
+                mockListTaskExecutionChildren.mockResolvedValue({
+                    entities: childNodeExecutions
+                });
+            });
+
+            it('correctly fetches children', async () => {
+                const { getByText } = renderTable();
+                await waitFor(() => getByText(mockNodeExecutions[0].id.nodeId));
+                expect(mockListNodeExecutions).not.toHaveBeenCalled();
+                expect(mockListTaskExecutionChildren).toHaveBeenCalledWith(
+                    expect.objectContaining(taskExecutions[0].id),
+                    expect.anything()
+                );
+            });
+
+            it('correctly renders groups', async () => {
+                // We returned two task execution attempts, each with children
+                const { container } = renderTable();
+                const childGroups = await expandParentNode(container);
+                expect(childGroups).toHaveLength(2);
+            });
+        });
+
+        describe('without isParentNode flag, using workflowNodeMetadata', () => {
+            let childExecution: Execution;
+            let childNodeExecutions: NodeExecution[];
+            beforeEach(() => {
+                childExecution = cloneDeep(executionContext.execution);
+                childExecution.id.name = 'childExecution';
+                dataCache.insertExecution(childExecution);
+                dataCache.insertWorkflowExecutionReference(
+                    childExecution.id,
+                    mockWorkflow.id
+                );
+
+                childNodeExecutions = cloneDeep(mockNodeExecutions);
+                childNodeExecutions.forEach(
+                    ne => (ne.id.executionId = childExecution.id)
+                );
+                mockNodeExecutions[0].closure.workflowNodeMetadata = {
+                    executionId: childExecution.id
+                };
+                mockGetExecution.mockImplementation(async id => {
+                    if (id.name !== childExecution.id.name) {
+                        throw new Error(
+                            `Unexpected call to getExecution with execution id: ${obj(
+                                id
+                            )}`
+                        );
+                    }
+                    return childExecution;
+                });
+                mockListNodeExecutions.mockImplementation(async id => {
+                    const entities =
+                        id.name === childExecution.id.name
+                            ? childNodeExecutions
+                            : [];
+                    return { entities };
+                });
+            });
+
+            it('correctly fetches children', async () => {
+                const { getByText } = renderTable();
+                await waitFor(() => getByText(mockNodeExecutions[0].id.nodeId));
+                expect(mockListNodeExecutions).toHaveBeenCalledWith(
+                    expect.objectContaining({ name: childExecution.id.name }),
+                    expect.anything()
+                );
+            });
+
+            it('correctly renders groups', async () => {
+                // We returned a single WF execution child, so there should only
+                // be one child group
+                const { container } = renderTable();
+                const childGroups = await expandParentNode(container);
+                expect(childGroups).toHaveLength(1);
+            });
+        });
+    });
+
     it('requests child node executions using configuration from context', async () => {
         const { taskExecutions } = createMockTaskExecutionsListResponse(1);
         taskExecutions[0].isParent = true;

src/components/Executions/TaskExecutionsList/utils.ts

@@ -13,7 +13,17 @@ export function getUniqueTaskExecutionName({ id }: TaskExecution) {
     return `${name}${suffix}`;
 }
 
-export function formatRetryAttempt(attempt: number): string {
+export function formatRetryAttempt(
+    attempt: number | string | undefined
+): string {
+    let parsed =
+        typeof attempt === 'number'
+            ? attempt
+            : Number.parseInt(`${attempt}`, 10);
+    if (Number.isNaN(parsed)) {
+        parsed = 0;
+    }
+
     // Retry attempts are zero-based, so incrementing before formatting
-    return `Attempt ${leftPaddedNumber(attempt + 1, 2)}`;
+    return `Attempt ${leftPaddedNumber(parsed + 1, 2)}`;
 }