Skip to content

Commit

Permalink
refactor: clean up mock fixtures and re-enable tests for executions (#…
Browse files Browse the repository at this point in the history
…130)

* test: adding test data for node executions

* test: mocks and refactoring to re-enable NodeExecutionDetails tests

* chore: lint error

* test: getting first test for NE table working again

* test: mocks and a couple of tests for NE table

* refactor: msw handlers to use a backing map and return 404s

* test: more tests for NE table

* test: adding fixture for dynamic external workflow

* test: using mock fixture for sub workflow tests

* test: move remaining mocks to fixtures and fix tests

* test: re-enabling more execution tests

* fix: removing global query handlers for caching entitiesq

* test: re-enable ExecutionNodeViews tests

* fix: typo in import path

* fix: show DataError by default for failed queries

* chore: documentation

* chore: pr feedback
  • Loading branch information
schottra authored Jan 4, 2021
1 parent 6f73c41 commit 4168210
Show file tree
Hide file tree
Showing 44 changed files with 2,471 additions and 856 deletions.
3 changes: 2 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ module.exports = {
'jest/valid-title': 'off',
'@typescript-eslint/no-var-requires': 'off',
'@typescript-eslint/ban-types': 'warn',
'@typescript-eslint/no-empty-function': 'warn'
'@typescript-eslint/no-empty-function': 'warn',
'@typescript-eslint/explicit-module-boundary-types': 'off'
}
};
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@
"react-intersection-observer": "^8.25.1",
"react-json-tree": "^0.11.0",
"react-loading-skeleton": "^1.1.2",
"react-query": "^3.2.0-beta",
"react-query": "^3.3.0",
"react-query-devtools": "^3.0.0-beta",
"react-router": "^5.0.1",
"react-router-dom": "^5.0.1",
Expand Down
7 changes: 7 additions & 0 deletions src/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,10 @@ export interface ResourceDefinition {
// TODO: This should probably be an enum
type: string;
}

/** Converts type `T` to one in which all fields are optional and
* all fields in nested objects are also optional.
*/
export type DeepPartial<T> = {
[P in keyof T]?: DeepPartial<T[P]>;
};
4 changes: 3 additions & 1 deletion src/components/Cache/utils.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import * as objectHash from 'object-hash';

export type KeyableType = any[] | object | string | symbol;

/** Generic cache key generator. For object, will generate a unique hash.
* Strings are passed through for convenience.
*/
export function getCacheKey(id: any[] | object | string | symbol): string {
export function getCacheKey(id: KeyableType): string {
if (typeof id === 'symbol') {
return id.toString();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ export const ExecutionNodeViews: React.FC<ExecutionNodeViewsProps> = ({
nodeExecutionsRequestConfig
} = useExecutionNodeViewsState(execution, appliedFilters);

// TODO: Handle error case (table usually rendered the error)
const renderNodeExecutionsTable = (nodeExecutions: NodeExecution[]) => (
<NodeExecutionsRequestConfigContext.Provider
value={nodeExecutionsRequestConfig}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { keyBy } from 'lodash';
import { endNodeId, NodeExecution, startNodeId } from 'models';
import { Workflow, WorkflowId } from 'models/Workflow';
import * as React from 'react';
import { useQuery } from 'react-query';
import { useQuery, useQueryClient } from 'react-query';
import { NodeExecutionsContext } from '../contexts';
import { NodeExecutionDetailsPanelContent } from './NodeExecutionDetailsPanelContent';
import { TaskExecutionNodeRenderer } from './TaskExecutionNodeRenderer/TaskExecutionNodeRenderer';
Expand All @@ -22,7 +22,7 @@ export const ExecutionWorkflowGraph: React.FC<ExecutionWorkflowGraphProps> = ({
workflowId
}) => {
const workflowQuery = useQuery<Workflow, Error>(
makeWorkflowQuery(workflowId)
makeWorkflowQuery(useQueryClient(), workflowId)
);
const nodeExecutionsById = React.useMemo(
() => keyBy(nodeExecutions, 'id.nodeId'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ const NodeExecutionLinkContent: React.FC<{
}> = ({ execution }) => {
const commonStyles = useCommonStyles();
const styles = useStyles();
useStyles();
const { workflowNodeMetadata } = execution.closure;
if (!workflowNodeMetadata) {
return null;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,93 +1,99 @@
import { render } from '@testing-library/react';
import { createQueryClient } from 'components/data/queryCache';
import { createMockExecutionEntities } from 'components/Executions/__mocks__/createMockExecutionEntities';
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import { filterLabels } from 'components/Executions/filters/constants';
import { nodeExecutionStatusFilters } from 'components/Executions/filters/statusFilters';
import { oneFailedTaskWorkflow } from 'mocks/data/fixtures/oneFailedTaskWorkflow';
import { insertFixture } from 'mocks/data/insertFixture';
import { mockServer } from 'mocks/server';
import { Execution } from 'models/Execution/types';
import * as React from 'react';
import { QueryClient, QueryClientProvider } from 'react-query';
import {
ExecutionNodeViews,
ExecutionNodeViewsProps
} from '../ExecutionNodeViews';
import { createTestQueryClient } from 'test/utils';
import { tabs } from '../constants';
import { ExecutionNodeViews } from '../ExecutionNodeViews';

// We don't need to verify the content of the graph component here and it is
// difficult to make it work correctly in a test environment.
jest.mock('../ExecutionWorkflowGraph.tsx', () => ({
ExecutionWorkflowGraph: () => null
}));

// TODO: Update this to use MSW and re-enable
describe.skip('ExecutionNodeViews', () => {
// ExecutionNodeViews uses query params for NE list, so we must match them
// for the list to be returned properly
const baseQueryParams = {
filters: '',
'sort_by.direction': 'ASCENDING',
'sort_by.key': 'created_at'
};

describe('ExecutionNodeViews', () => {
let queryClient: QueryClient;
let props: ExecutionNodeViewsProps;
let execution: Execution;
let fixture: ReturnType<typeof oneFailedTaskWorkflow.generate>;

beforeEach(() => {
queryClient = createQueryClient();
const { workflowExecution } = createMockExecutionEntities({
workflowName: 'SampleWorkflow',
nodeExecutionCount: 2
});
fixture = oneFailedTaskWorkflow.generate();
execution = fixture.workflowExecutions.top.data;
insertFixture(mockServer, fixture);
const nodeExecutions = fixture.workflowExecutions.top.nodeExecutions;

props = { execution: workflowExecution };
mockServer.insertNodeExecutionList(
execution.id,
Object.values(nodeExecutions).map(({ data }) => data),
baseQueryParams
);
mockServer.insertNodeExecutionList(
execution.id,
[nodeExecutions.failedNode.data],
{ ...baseQueryParams, filters: 'value_in(phase,FAILED)' }
);
queryClient = createTestQueryClient();
});

it('is disabled', () => {});

const renderViews = () =>
render(
<QueryClientProvider client={queryClient}>
<ExecutionNodeViews {...props} />
<ExecutionNodeViews execution={execution} />
</QueryClientProvider>
);

// it('only applies filter when viewing the nodes tab', async () => {
// const { getByText } = renderViews();
// const nodesTab = await waitFor(() => getByText(tabs.nodes.label));
// const graphTab = await waitFor(() => getByText(tabs.graph.label));
it('maintains filter when switching back to nodes tab', async () => {
const { nodeExecutions } = fixture.workflowExecutions.top;
const failedNodeName = nodeExecutions.failedNode.data.id.nodeId;
const succeededNodeName = nodeExecutions.pythonNode.data.id.nodeId;

// fireEvent.click(nodesTab);
// const statusButton = await waitFor(() =>
// getByText(filterLabels.status)
// );
// fireEvent.click(statusButton);
// const successFilter = await waitFor(() =>
// getByText(nodeExecutionStatusFilters.succeeded.label)
// );
const { getByText, queryByText } = renderViews();
const nodesTab = await waitFor(() => getByText(tabs.nodes.label));
const graphTab = await waitFor(() => getByText(tabs.graph.label));

// mockListNodeExecutions.mockClear();
// fireEvent.click(successFilter);
// await waitFor(() => mockListNodeExecutions.mock.calls.length > 0);
// // Verify at least one filter is passed
// expect(mockListNodeExecutions).toHaveBeenCalledWith(
// expect.anything(),
// expect.objectContaining({
// filter: expect.arrayContaining([
// expect.objectContaining({ key: expect.any(String) })
// ])
// })
// );
// Ensure we are on Nodes tab
fireEvent.click(nodesTab);
await waitFor(() => getByText(succeededNodeName));

// fireEvent.click(statusButton);
// await waitForElementToBeRemoved(successFilter);
// mockListNodeExecutions.mockClear();
// fireEvent.click(graphTab);
// await waitFor(() => mockListNodeExecutions.mock.calls.length > 0);
// // No filter expected on the graph tab
// expect(mockListNodeExecutions).toHaveBeenCalledWith(
// expect.anything(),
// expect.objectContaining({ filter: [] })
// );
const statusButton = await waitFor(() =>
getByText(filterLabels.status)
);

// mockListNodeExecutions.mockClear();
// fireEvent.click(nodesTab);
// await waitFor(() => mockListNodeExecutions.mock.calls.length > 0);
// // Verify (again) at least one filter is passed, after changing back to
// // nodes tab.
// expect(mockListNodeExecutions).toHaveBeenCalledWith(
// expect.anything(),
// expect.objectContaining({
// filter: expect.arrayContaining([
// expect.objectContaining({ key: expect.any(String) })
// ])
// })
// );
// });
// Apply 'Failed' filter and wait for list to include only the failed item
fireEvent.click(statusButton);
const failedFilter = await waitFor(() =>
screen.getByLabelText(nodeExecutionStatusFilters.failed.label)
);

// Wait for succeeded task to disappear and ensure failed task remains
fireEvent.click(failedFilter);
await waitFor(() => queryByText(succeededNodeName) == null);
await waitFor(() =>
expect(getByText(failedNodeName)).toBeInTheDocument()
);

// Switch to the Graph tab
fireEvent.click(statusButton);
fireEvent.click(graphTab);
await waitFor(() => queryByText(failedNodeName) == null);

// Switch back to Nodes Tab and verify filter still applied
fireEvent.click(nodesTab);
await waitFor(() => getByText(failedNodeName));
expect(queryByText(succeededNodeName)).toBeNull();
});
});
Original file line number Diff line number Diff line change
@@ -1,62 +1,50 @@
import { render, waitFor } from '@testing-library/react';
import { mockAPIContextValue } from 'components/data/__mocks__/apiContext';
import { APIContext } from 'components/data/apiContext';
import {
cacheStatusMessages,
viewSourceExecutionString
} from 'components/Executions/constants';
import {
NodeExecutionDetails,
NodeExecutionDisplayType
} from 'components/Executions/types';
import { Core } from 'flyteidl';
import { basicPythonWorkflow } from 'mocks/data/fixtures/basicPythonWorkflow';
import { insertFixture } from 'mocks/data/insertFixture';
import { mockServer } from 'mocks/server';
import { NodeExecution, TaskNodeMetadata } from 'models';
import { createMockNodeExecutions } from 'models/Execution/__mocks__/mockNodeExecutionsData';
import { mockExecution as mockTaskExecution } from 'models/Execution/__mocks__/mockTaskExecutionsData';
import { listTaskExecutions } from 'models/Execution/api';
import * as React from 'react';
import { QueryClient, QueryClientProvider } from 'react-query';
import { MemoryRouter } from 'react-router';
import { Routes } from 'routes';
import { makeIdentifier } from 'test/modelUtils';
import { createTestQueryClient } from 'test/utils';
import { NodeExecutionDetailsPanelContent } from '../NodeExecutionDetailsPanelContent';

// TODO: Update this to use MSW and re-enable
describe.skip('NodeExecutionDetails', () => {
describe('NodeExecutionDetails', () => {
let fixture: ReturnType<typeof basicPythonWorkflow.generate>;
let execution: NodeExecution;
let details: NodeExecutionDetails;
let mockListTaskExecutions: jest.Mock<ReturnType<
typeof listTaskExecutions
>>;
let queryClient: QueryClient;

beforeEach(() => {
const { executions } = createMockNodeExecutions(1);
details = {
displayType: NodeExecutionDisplayType.PythonTask,
displayId: 'com.flyte.testTask',
cacheKey: 'abcdefg'
};
execution = executions[0];
mockListTaskExecutions = jest.fn().mockResolvedValue({ entities: [] });
fixture = basicPythonWorkflow.generate();
execution =
fixture.workflowExecutions.top.nodeExecutions.pythonNode.data;
insertFixture(mockServer, fixture);
queryClient = createTestQueryClient();
});

const renderComponent = () =>
render(
<MemoryRouter>
<APIContext.Provider
value={mockAPIContextValue({
listTaskExecutions: mockListTaskExecutions
})}
>
<QueryClientProvider client={queryClient}>
<NodeExecutionDetailsPanelContent
nodeExecutionId={execution.id}
/>
</APIContext.Provider>
</QueryClientProvider>
</MemoryRouter>
);

it('renders displayId', async () => {
const { queryByText } = renderComponent();
await waitFor(() => {});
expect(queryByText(details.displayId)).toBeInTheDocument();
it('renders name for task nodes', async () => {
const { name } = fixture.tasks.python.id;
const { getByText } = renderComponent();
await waitFor(() => expect(getByText(name)));
});

describe('with cache information', () => {
Expand All @@ -72,6 +60,7 @@ describe.skip('NodeExecutionDetails', () => {
}
};
execution.closure.taskNodeMetadata = taskNodeMetadata;
mockServer.insertNodeExecution(execution);
});

[
Expand All @@ -84,9 +73,10 @@ describe.skip('NodeExecutionDetails', () => {
].forEach(cacheStatusValue =>
it(`renders correct status for ${Core.CatalogCacheStatus[cacheStatusValue]}`, async () => {
taskNodeMetadata.cacheStatus = cacheStatusValue;
mockServer.insertNodeExecution(execution);
const { getByText } = renderComponent();
await waitFor(() =>
getByText(cacheStatusMessages[cacheStatusValue])
expect(getByText(cacheStatusMessages[cacheStatusValue]))
);
})
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { useConditionalQuery } from 'components/hooks/useConditionalQuery';
import { every } from 'lodash';
import {
Execution,
executionSortFields,
Expand All @@ -8,6 +7,8 @@ import {
NodeExecution,
SortDirection
} from 'models';
import { useQueryClient } from 'react-query';
import { executionRefreshIntervalMs } from '../constants';
import { makeNodeExecutionListQuery } from '../nodeExecutionQueries';
import { executionIsTerminal, nodeExecutionIsTerminal } from '../utils';

Expand All @@ -26,11 +27,18 @@ export function useExecutionNodeViewsState(
};

const shouldEnableQuery = (executions: NodeExecution[]) =>
every(executions, nodeExecutionIsTerminal) &&
executionIsTerminal(execution);
!executionIsTerminal(execution) ||
executions.some(ne => !nodeExecutionIsTerminal(ne));

const nodeExecutionsQuery = useConditionalQuery(
makeNodeExecutionListQuery(execution.id, nodeExecutionsRequestConfig),
{
...makeNodeExecutionListQuery(
useQueryClient(),
execution.id,
nodeExecutionsRequestConfig
),
refetchInterval: executionRefreshIntervalMs
},
shouldEnableQuery
);

Expand Down
Loading

0 comments on commit 4168210

Please sign in to comment.