From 724eaa14268d9aecdfc209d5523cd33bfea144ac Mon Sep 17 00:00:00 2001 From: Yasser Elsayed Date: Wed, 7 Nov 2018 12:51:14 -0800 Subject: [PATCH 1/5] wip RecurringRunDetails tests --- frontend/src/components/Router.tsx | 4 +- .../src/pages/RecurringRunDetails.test.tsx | 155 ++++++++++++++++++ frontend/src/pages/RecurringRunDetails.tsx | 9 +- .../RecurringRunDetails.test.tsx.snap | 123 ++++++++++++++ 4 files changed, 284 insertions(+), 7 deletions(-) create mode 100644 frontend/src/pages/RecurringRunDetails.test.tsx create mode 100644 frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap diff --git a/frontend/src/components/Router.tsx b/frontend/src/components/Router.tsx index 0cee872d23e..b82b3fe5e13 100644 --- a/frontend/src/components/Router.tsx +++ b/frontend/src/components/Router.tsx @@ -28,7 +28,7 @@ import NewExperiment from '../pages/NewExperiment'; import NewRun from '../pages/NewRun'; import PipelineDetails from '../pages/PipelineDetails'; import PipelineList from '../pages/PipelineList'; -import RecurringRunConfig from '../pages/RecurringRunDetails'; +import RecurringRunDetails from '../pages/RecurringRunDetails'; import RunDetails from '../pages/RunDetails'; import SideNav from './SideNav'; import Snackbar, { SnackbarProps } from '@material-ui/core/Snackbar'; @@ -109,7 +109,7 @@ class Router extends React.Component<{}, RouteComponentState> { { path: RoutePage.PIPELINES, Component: PipelineList }, { path: RoutePage.PIPELINE_DETAILS, Component: PipelineDetails }, { path: RoutePage.RUNS, Component: ExperimentsAndRuns, view: ExperimentsAndRunsTab.RUNS }, - { path: RoutePage.RECURRING_RUN, Component: RecurringRunConfig }, + { path: RoutePage.RECURRING_RUN, Component: RecurringRunDetails }, { path: RoutePage.RUN_DETAILS, Component: RunDetails }, { path: RoutePage.COMPARE, Component: Compare }, ]; diff --git a/frontend/src/pages/RecurringRunDetails.test.tsx b/frontend/src/pages/RecurringRunDetails.test.tsx new file mode 100644 index 00000000000..11994ee6b33 --- /dev/null +++ b/frontend/src/pages/RecurringRunDetails.test.tsx @@ -0,0 +1,155 @@ +/* + * 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 * as React from 'react'; +import RecurringRunDetails from './RecurringRunDetails'; +import TestUtils from '../TestUtils'; +import produce from 'immer'; +import { ApiJob, ApiResourceType } from '../apis/job'; +import { Apis } from '../lib/Apis'; +import { PageProps } from './Page'; +import { RouteParams, RoutePage } from '../components/Router'; +import { shallow } from 'enzyme'; + +describe('RecurringRunDetails', () => { + const updateBannerSpy = jest.fn(); + const updateDialogSpy = jest.fn(); + const updateSnackbarSpy = jest.fn(); + const updateToolbarSpy = jest.fn(); + const historyPushSpy = jest.fn(); + const getJobSpy = jest.spyOn(Apis.jobServiceApi, 'getJob'); + const getExperimentSpy = jest.spyOn(Apis.experimentServiceApi, 'getExperiment'); + + const fullTestJob = { + created_at: new Date(2018, 8, 5, 4, 3, 2), + description: 'test job description', + enabled: true, + id: 'test-job-id', + max_concurrency: '50', + name: 'test job', + pipeline_spec: { pipeline_id: 'some-pipeline-id' }, + trigger: { + periodic_schedule: { + end_time: new Date(2018, 10, 9, 8, 7, 6), + interval_second: '3600', + start_time: new Date(2018, 9, 8, 7, 6), + } + }, + } as ApiJob; + + function generateProps(): PageProps { + return { + history: { push: historyPushSpy } as any, + location: '' as any, + match: { params: { [RouteParams.runId]: fullTestJob.id }, isExact: true, path: '', url: '' }, + toolbarProps: RecurringRunDetails.prototype.getInitialToolbarState(), + updateBanner: updateBannerSpy, + updateDialog: updateDialogSpy, + updateSnackbar: updateSnackbarSpy, + updateToolbar: updateToolbarSpy, + }; + } + + beforeEach(() => { + updateBannerSpy.mockClear(); + updateDialogSpy.mockClear(); + updateSnackbarSpy.mockClear(); + updateToolbarSpy.mockClear(); + getJobSpy.mockImplementation(() => fullTestJob); + getExperimentSpy.mockImplementation(); + }); + + it('renders a recurring run with periodic schedule', () => { + const tree = shallow(); + tree.setState({ + run: fullTestJob, + }); + expect(tree).toMatchSnapshot(); + }); + + it('renders a recurring run with cron schedule', () => { + const tree = shallow(); + tree.setState({ + run: produce(fullTestJob, draft => { + draft.trigger = { + cron_schedule: { + cron: '* * * 0 0 !', + end_time: new Date(2018, 10, 9, 8, 7, 6), + start_time: new Date(2018, 9, 8, 7, 6), + } + }; + }), + }); + expect(tree).toMatchSnapshot(); + }); + + it('loads the recurring run given its id in query params', async () => { + fullTestJob.resource_references = []; + const tree = shallow(); + await (tree.instance() as RecurringRunDetails).load(); + expect(getJobSpy).toHaveBeenLastCalledWith(fullTestJob.id); + expect(getExperimentSpy).not.toHaveBeenCalled(); + }); + + it('shows All runs -> run name when there is no experiment', async () => { + fullTestJob.resource_references = []; + const tree = shallow(); + await (tree.instance() as RecurringRunDetails).load(); + expect(updateToolbarSpy).toHaveBeenLastCalledWith(expect.objectContaining({ + breadcrumbs: [ + { displayName: 'All runs', href: RoutePage.RUNS }, + { displayName: fullTestJob.name, href: '' }, + ], + })); + }); + + it('loads the recurring run and its experiment if it has one', async () => { + fullTestJob.resource_references = [{ key: { id: 'test-experiment-id', type: ApiResourceType.EXPERIMENT } }]; + const tree = shallow(); + await (tree.instance() as RecurringRunDetails).load(); + expect(getJobSpy).toHaveBeenLastCalledWith(fullTestJob.id); + expect(getExperimentSpy).toHaveBeenLastCalledWith('test-experiment-id'); + }); + + it('shows Experiments -> Experiment name -> run name when there is no experiment', async () => { + fullTestJob.resource_references = [{ key: { id: 'test-experiment-id', type: ApiResourceType.EXPERIMENT } }]; + getExperimentSpy.mockImplementation(id => ({ id, name: 'test experiment name' })); + const tree = shallow(); + await (tree.instance() as RecurringRunDetails).load(); + expect(updateToolbarSpy).toHaveBeenLastCalledWith(expect.objectContaining({ + breadcrumbs: [ + { displayName: 'Experiments', href: RoutePage.EXPERIMENTS }, + { + displayName: 'test experiment name', + href: RoutePage.EXPERIMENT_DETAILS.replace( + ':' + RouteParams.experimentId, 'test-experiment-id'), + }, + { displayName: fullTestJob.name, href: '' }, + ], + })); + }); + + it('shows error banner if run cannot be fetched', async () => { + fullTestJob.resource_references = []; + TestUtils.makeErrorResponseOnce(getJobSpy, 'woops!'); + const tree = shallow(); + await (tree.instance() as RecurringRunDetails).load(); + expect(updateBannerSpy).toHaveBeenCalledTimes(2); // Once to clear, once to show error + expect(updateBannerSpy).toHaveBeenLastCalledWith( + `Error: failed to retrieve recurring run: ${fullTestJob.id}.`, {}); + }); + +}); diff --git a/frontend/src/pages/RecurringRunDetails.tsx b/frontend/src/pages/RecurringRunDetails.tsx index dce341d5fd6..1fa710f0f78 100644 --- a/frontend/src/pages/RecurringRunDetails.tsx +++ b/frontend/src/pages/RecurringRunDetails.tsx @@ -22,7 +22,7 @@ import { ApiJob } from '../apis/job'; import { Apis } from '../lib/Apis'; import { Page } from './Page'; import { RoutePage, RouteParams } from '../components/Router'; -import { ToolbarActionConfig, ToolbarProps } from '../components/Toolbar'; +import { ToolbarActionConfig, Breadcrumb, ToolbarProps } from '../components/Toolbar'; import { classes } from 'typestyle'; import { commonCss, padding } from '../Css'; import { formatDateString, enabledDisplayString, logger, errorToMessage } from '../lib/Utils'; @@ -32,7 +32,7 @@ interface RecurringRunConfigState { run: ApiJob | null; } -class RecurringRunConfig extends Page<{}, RecurringRunConfigState> { +class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { constructor(props: any) { super(props); @@ -162,7 +162,7 @@ class RecurringRunConfig extends Page<{}, RecurringRunConfigState> { if (relatedExperimentId) { experiment = await Apis.experimentServiceApi.getExperiment(relatedExperimentId); } - const breadcrumbs: Array<{ displayName: string, href: string }> = []; + const breadcrumbs: Breadcrumb[] = []; if (experiment) { breadcrumbs.push( { displayName: 'Experiments', href: RoutePage.EXPERIMENTS }, @@ -189,7 +189,6 @@ class RecurringRunConfig extends Page<{}, RecurringRunConfigState> { this.setState({ run }); } catch (err) { await this.showPageError(`Error: failed to retrieve recurring run: ${runId}.`, err); - logger.error(`Error loading recurring run: ${runId}`, err); } } @@ -242,4 +241,4 @@ class RecurringRunConfig extends Page<{}, RecurringRunConfigState> { } } -export default RecurringRunConfig; +export default RecurringRunDetails; diff --git a/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap b/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap new file mode 100644 index 00000000000..5495835388e --- /dev/null +++ b/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap @@ -0,0 +1,123 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`RecurringRunDetails renders a recurring run with cron schedule 1`] = ` +
+
+
+ Recurring run details +
+ +
+ Run trigger +
+ +
+
+`; + +exports[`RecurringRunDetails renders a recurring run with periodic schedule 1`] = ` +
+
+
+ Recurring run details +
+ +
+ Run trigger +
+ +
+
+`; From e685a27e22b838bd62fc7279186e8ae3e8c72272 Mon Sep 17 00:00:00 2001 From: Yasser Elsayed Date: Sat, 10 Nov 2018 21:52:27 -0800 Subject: [PATCH 2/5] more recurring run details tests --- .../src/pages/RecurringRunDetails.test.tsx | 200 +++++++++++++++++- frontend/src/pages/RecurringRunDetails.tsx | 17 +- 2 files changed, 201 insertions(+), 16 deletions(-) diff --git a/frontend/src/pages/RecurringRunDetails.test.tsx b/frontend/src/pages/RecurringRunDetails.test.tsx index 11994ee6b33..503a007c581 100644 --- a/frontend/src/pages/RecurringRunDetails.test.tsx +++ b/frontend/src/pages/RecurringRunDetails.test.tsx @@ -22,8 +22,15 @@ import { ApiJob, ApiResourceType } from '../apis/job'; import { Apis } from '../lib/Apis'; import { PageProps } from './Page'; import { RouteParams, RoutePage } from '../components/Router'; +import { ToolbarActionConfig } from '../components/Toolbar'; import { shallow } from 'enzyme'; +class TestRecurringRunDetails extends RecurringRunDetails { + public async _deleteDialogClosed(confirmed: boolean): Promise { + super._deleteDialogClosed(confirmed); + } +} + describe('RecurringRunDetails', () => { const updateBannerSpy = jest.fn(); const updateDialogSpy = jest.fn(); @@ -31,6 +38,9 @@ describe('RecurringRunDetails', () => { const updateToolbarSpy = jest.fn(); const historyPushSpy = jest.fn(); const getJobSpy = jest.spyOn(Apis.jobServiceApi, 'getJob'); + const deleteJobSpy = jest.spyOn(Apis.jobServiceApi, 'deleteJob'); + const enableJobSpy = jest.spyOn(Apis.jobServiceApi, 'enableJob'); + const disableJobSpy = jest.spyOn(Apis.jobServiceApi, 'disableJob'); const getExperimentSpy = jest.spyOn(Apis.experimentServiceApi, 'getExperiment'); const fullTestJob = { @@ -64,12 +74,19 @@ describe('RecurringRunDetails', () => { } beforeEach(() => { + historyPushSpy.mockClear(); updateBannerSpy.mockClear(); updateDialogSpy.mockClear(); updateSnackbarSpy.mockClear(); updateToolbarSpy.mockClear(); getJobSpy.mockImplementation(() => fullTestJob); + getJobSpy.mockClear(); + deleteJobSpy.mockClear(); + deleteJobSpy.mockImplementation(); + enableJobSpy.mockClear(); + disableJobSpy.mockClear(); getExperimentSpy.mockImplementation(); + getExperimentSpy.mockClear(); }); it('renders a recurring run with periodic schedule', () => { @@ -78,6 +95,7 @@ describe('RecurringRunDetails', () => { run: fullTestJob, }); expect(tree).toMatchSnapshot(); + tree.unmount(); }); it('renders a recurring run with cron schedule', () => { @@ -94,12 +112,13 @@ describe('RecurringRunDetails', () => { }), }); expect(tree).toMatchSnapshot(); + tree.unmount(); }); it('loads the recurring run given its id in query params', async () => { fullTestJob.resource_references = []; - const tree = shallow(); - await (tree.instance() as RecurringRunDetails).load(); + shallow(); + await TestUtils.flushPromises(); expect(getJobSpy).toHaveBeenLastCalledWith(fullTestJob.id); expect(getExperimentSpy).not.toHaveBeenCalled(); }); @@ -107,28 +126,30 @@ describe('RecurringRunDetails', () => { it('shows All runs -> run name when there is no experiment', async () => { fullTestJob.resource_references = []; const tree = shallow(); - await (tree.instance() as RecurringRunDetails).load(); + await TestUtils.flushPromises(); expect(updateToolbarSpy).toHaveBeenLastCalledWith(expect.objectContaining({ breadcrumbs: [ { displayName: 'All runs', href: RoutePage.RUNS }, { displayName: fullTestJob.name, href: '' }, ], })); + tree.unmount(); }); it('loads the recurring run and its experiment if it has one', async () => { fullTestJob.resource_references = [{ key: { id: 'test-experiment-id', type: ApiResourceType.EXPERIMENT } }]; const tree = shallow(); - await (tree.instance() as RecurringRunDetails).load(); + await TestUtils.flushPromises(); expect(getJobSpy).toHaveBeenLastCalledWith(fullTestJob.id); expect(getExperimentSpy).toHaveBeenLastCalledWith('test-experiment-id'); + tree.unmount(); }); it('shows Experiments -> Experiment name -> run name when there is no experiment', async () => { fullTestJob.resource_references = [{ key: { id: 'test-experiment-id', type: ApiResourceType.EXPERIMENT } }]; getExperimentSpy.mockImplementation(id => ({ id, name: 'test experiment name' })); const tree = shallow(); - await (tree.instance() as RecurringRunDetails).load(); + await TestUtils.flushPromises(); expect(updateToolbarSpy).toHaveBeenLastCalledWith(expect.objectContaining({ breadcrumbs: [ { displayName: 'Experiments', href: RoutePage.EXPERIMENTS }, @@ -140,16 +161,179 @@ describe('RecurringRunDetails', () => { { displayName: fullTestJob.name, href: '' }, ], })); + tree.unmount(); }); it('shows error banner if run cannot be fetched', async () => { fullTestJob.resource_references = []; TestUtils.makeErrorResponseOnce(getJobSpy, 'woops!'); const tree = shallow(); - await (tree.instance() as RecurringRunDetails).load(); + await TestUtils.flushPromises(); expect(updateBannerSpy).toHaveBeenCalledTimes(2); // Once to clear, once to show error - expect(updateBannerSpy).toHaveBeenLastCalledWith( - `Error: failed to retrieve recurring run: ${fullTestJob.id}.`, {}); + expect(updateBannerSpy).toHaveBeenLastCalledWith(expect.objectContaining({ + additionalInfo: 'woops!', + message: `Error: failed to retrieve recurring run: ${fullTestJob.id}. Click Details for more information.`, + mode: 'error', + })); + tree.unmount(); + }); + + it('shows error banner if has experiment but experiment cannot be fetched', async () => { + fullTestJob.resource_references = [{ key: { id: 'test-experiment-id', type: ApiResourceType.EXPERIMENT } }]; + TestUtils.makeErrorResponseOnce(getExperimentSpy, 'woops!'); + const tree = shallow(); + await TestUtils.flushPromises(); + expect(updateBannerSpy).toHaveBeenCalledTimes(2); // Once to clear, once to show error + expect(updateBannerSpy).toHaveBeenLastCalledWith(expect.objectContaining({ + additionalInfo: 'woops!', + message: `Error: failed to retrieve recurring run: ${fullTestJob.id}. Click Details for more information.`, + mode: 'error', + })); + tree.unmount(); + }); + + it('has a Refresh button, clicking it refreshes the run details', async () => { + const tree = shallow(); + const instance = tree.instance() as RecurringRunDetails; + const refreshBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Refresh'); + expect(refreshBtn).toBeDefined(); + expect(getJobSpy).toHaveBeenCalledTimes(1); + await refreshBtn!.action(); + expect(getJobSpy).toHaveBeenCalledTimes(2); + tree.unmount(); + }); + + it('shows enabled Disable, and disabled Enable buttons if the run is enabled', async () => { + fullTestJob.enabled = true; + const tree = shallow(); + await TestUtils.flushPromises(); + expect(updateToolbarSpy).toHaveBeenCalledTimes(2); + const lastToolbarButtons = updateToolbarSpy.mock.calls[1][0].actions as ToolbarActionConfig[]; + const enableBtn = lastToolbarButtons.find(b => b.title === 'Enable'); + expect(enableBtn).toBeDefined(); + expect(enableBtn!.disabled).toBe(true); + const disableBtn = lastToolbarButtons.find(b => b.title === 'Disable'); + expect(disableBtn).toBeDefined(); + expect(disableBtn!.disabled).toBe(false); + tree.unmount(); + }); + + it('shows enabled Disable, and disabled Enable buttons if the run is disabled', async () => { + fullTestJob.enabled = false; + const tree = shallow(); + await TestUtils.flushPromises(); + expect(updateToolbarSpy).toHaveBeenCalledTimes(2); + const lastToolbarButtons = updateToolbarSpy.mock.calls[1][0].actions as ToolbarActionConfig[]; + const enableBtn = lastToolbarButtons.find(b => b.title === 'Enable'); + expect(enableBtn).toBeDefined(); + expect(enableBtn!.disabled).toBe(false); + const disableBtn = lastToolbarButtons.find(b => b.title === 'Disable'); + expect(disableBtn).toBeDefined(); + expect(disableBtn!.disabled).toBe(true); + tree.unmount(); + }); + + it('shows enabled Disable, and disabled Enable buttons if the run is undefined', async () => { + fullTestJob.enabled = undefined; + const tree = shallow(); + await TestUtils.flushPromises(); + expect(updateToolbarSpy).toHaveBeenCalledTimes(2); + const lastToolbarButtons = updateToolbarSpy.mock.calls[1][0].actions as ToolbarActionConfig[]; + const enableBtn = lastToolbarButtons.find(b => b.title === 'Enable'); + expect(enableBtn).toBeDefined(); + expect(enableBtn!.disabled).toBe(false); + const disableBtn = lastToolbarButtons.find(b => b.title === 'Disable'); + expect(disableBtn).toBeDefined(); + expect(disableBtn!.disabled).toBe(true); + tree.unmount(); + }); + + it('shows a delete button', async () => { + const tree = shallow(); + const instance = tree.instance() as RecurringRunDetails; + const deleteBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Refresh'); + expect(deleteBtn).toBeDefined(); + tree.unmount(); + }); + + it('shows delete dialog when delete button is clicked', async () => { + const tree = shallow(); + const instance = tree.instance() as RecurringRunDetails; + const deleteBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Delete'); + await deleteBtn!.action(); + expect(updateDialogSpy).toHaveBeenLastCalledWith(expect.objectContaining({ + title: 'Delete this recurring run?', + })); + tree.unmount(); + }); + + it('calls delete API when delete confirmation dialog button is clicked', async () => { + const tree = shallow(); + await TestUtils.flushPromises(); + const instance = tree.instance() as RecurringRunDetails; + const deleteBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Delete'); + await deleteBtn!.action(); + const call = updateDialogSpy.mock.calls[0][0]; + const confirmBtn = call.buttons.find((b: any) => b.text === 'Delete'); + await confirmBtn.onClick(); + expect(deleteJobSpy).toHaveBeenCalledTimes(1); + expect(deleteJobSpy).toHaveBeenLastCalledWith('test-job-id'); + tree.unmount(); + }); + + it('does not call delete API when delete cancel dialog button is clicked', async () => { + const tree = shallow(); + await TestUtils.flushPromises(); + const instance = tree.instance() as RecurringRunDetails; + const deleteBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Delete'); + await deleteBtn!.action(); + const call = updateDialogSpy.mock.calls[0][0]; + const confirmBtn = call.buttons.find((b: any) => b.text === 'Cancel'); + await confirmBtn.onClick(); + expect(deleteJobSpy).not.toHaveBeenCalled(); + // Should not reroute + expect(historyPushSpy).not.toHaveBeenCalled(); + tree.unmount(); + }); + + it('redirects back to parent experiment after delete', async () => { + const tree = shallow(); + await TestUtils.flushPromises(); + const instance = tree.instance() as TestRecurringRunDetails; + await instance._deleteDialogClosed(true); + expect(historyPushSpy).toHaveBeenCalledTimes(1); + expect(historyPushSpy).toHaveBeenLastCalledWith(RoutePage.EXPERIMENTS); + tree.unmount(); + }); + + it('shows snackbar after successful deletion', async () => { + const tree = shallow(); + await TestUtils.flushPromises(); + const instance = tree.instance() as TestRecurringRunDetails; + await instance._deleteDialogClosed(true); + expect(updateSnackbarSpy).toHaveBeenCalledTimes(1); + expect(updateSnackbarSpy).toHaveBeenLastCalledWith({ + message: 'Successfully deleted recurring run: ' + fullTestJob.name, + open: true, + }); + tree.unmount(); + }); + + it('shows error dialog after failing deletion', async () => { + TestUtils.makeErrorResponseOnce(deleteJobSpy, 'could not delete'); + const tree = shallow(); + await TestUtils.flushPromises(); + const instance = tree.instance() as TestRecurringRunDetails; + await instance._deleteDialogClosed(true); + await TestUtils.flushPromises(); + expect(updateDialogSpy).toHaveBeenCalledTimes(1); + expect(updateDialogSpy).toHaveBeenLastCalledWith(expect.objectContaining({ + content: 'could not delete', + title: 'Failed to delete recurring run', + })); + // Should not reroute + expect(historyPushSpy).not.toHaveBeenCalled(); + tree.unmount(); }); }); diff --git a/frontend/src/pages/RecurringRunDetails.tsx b/frontend/src/pages/RecurringRunDetails.tsx index 1fa710f0f78..5a174a2a2f3 100644 --- a/frontend/src/pages/RecurringRunDetails.tsx +++ b/frontend/src/pages/RecurringRunDetails.tsx @@ -25,7 +25,7 @@ import { RoutePage, RouteParams } from '../components/Router'; import { ToolbarActionConfig, Breadcrumb, ToolbarProps } from '../components/Toolbar'; import { classes } from 'typestyle'; import { commonCss, padding } from '../Css'; -import { formatDateString, enabledDisplayString, logger, errorToMessage } from '../lib/Utils'; +import { formatDateString, enabledDisplayString, errorToMessage } from '../lib/Utils'; import { triggerDisplayString } from '../lib/TriggerUtils'; interface RecurringRunConfigState { @@ -181,18 +181,20 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { }); const toolbarActions = [...this.props.toolbarProps.actions]; - toolbarActions[1].disabled = run.enabled === true; - toolbarActions[2].disabled = run.enabled === false; + toolbarActions[1].disabled = !!run.enabled; + toolbarActions[2].disabled = !run.enabled; this.props.updateToolbar({ actions: toolbarActions, breadcrumbs }); this.setState({ run }); } catch (err) { - await this.showPageError(`Error: failed to retrieve recurring run: ${runId}.`, err); + const errorMessage = await errorToMessage(err); + await this.showPageError( + `Error: failed to retrieve recurring run: ${runId}.`, new Error(errorMessage)); } } - private async _setEnabledState(enabled: boolean): Promise { + protected async _setEnabledState(enabled: boolean): Promise { if (this.state.run) { const toolbarActions = [...this.props.toolbarProps.actions]; @@ -215,11 +217,11 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { } } - private _updateToolbar(actions: ToolbarActionConfig[]): void { + protected _updateToolbar(actions: ToolbarActionConfig[]): void { this.props.updateToolbar({ breadcrumbs: this.props.toolbarProps.breadcrumbs, actions }); } - private async _deleteDialogClosed(deleteConfirmed: boolean): Promise { + protected async _deleteDialogClosed(deleteConfirmed: boolean): Promise { if (deleteConfirmed) { // TODO: Show spinner during wait. try { @@ -235,7 +237,6 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { } catch (err) { const errorMessage = await errorToMessage(err); this.showErrorDialog('Failed to delete recurring run', errorMessage); - logger.error('Deleting recurring run failed with error:', err); } } } From ba6d3e625084d94ea6faf7abed64c2962d09353e Mon Sep 17 00:00:00 2001 From: Yasser Elsayed Date: Sun, 11 Nov 2018 01:20:45 -0800 Subject: [PATCH 3/5] more tests, covering all essential flows --- .../src/pages/RecurringRunDetails.test.tsx | 101 ++++++++++++++---- frontend/src/pages/RecurringRunDetails.tsx | 2 +- .../RecurringRunDetails.test.tsx.snap | 30 ++++++ 3 files changed, 112 insertions(+), 21 deletions(-) diff --git a/frontend/src/pages/RecurringRunDetails.test.tsx b/frontend/src/pages/RecurringRunDetails.test.tsx index 503a007c581..4d748977b43 100644 --- a/frontend/src/pages/RecurringRunDetails.test.tsx +++ b/frontend/src/pages/RecurringRunDetails.test.tsx @@ -43,22 +43,7 @@ describe('RecurringRunDetails', () => { const disableJobSpy = jest.spyOn(Apis.jobServiceApi, 'disableJob'); const getExperimentSpy = jest.spyOn(Apis.experimentServiceApi, 'getExperiment'); - const fullTestJob = { - created_at: new Date(2018, 8, 5, 4, 3, 2), - description: 'test job description', - enabled: true, - id: 'test-job-id', - max_concurrency: '50', - name: 'test job', - pipeline_spec: { pipeline_id: 'some-pipeline-id' }, - trigger: { - periodic_schedule: { - end_time: new Date(2018, 10, 9, 8, 7, 6), - interval_second: '3600', - start_time: new Date(2018, 9, 8, 7, 6), - } - }, - } as ApiJob; + let fullTestJob: ApiJob = {}; function generateProps(): PageProps { return { @@ -74,6 +59,26 @@ describe('RecurringRunDetails', () => { } beforeEach(() => { + fullTestJob = { + created_at: new Date(2018, 8, 5, 4, 3, 2), + description: 'test job description', + enabled: true, + id: 'test-job-id', + max_concurrency: '50', + name: 'test job', + pipeline_spec: { + parameters: [{ name: 'param1', value: 'value1' }], + pipeline_id: 'some-pipeline-id', + }, + trigger: { + periodic_schedule: { + end_time: new Date(2018, 10, 9, 8, 7, 6), + interval_second: '3600', + start_time: new Date(2018, 9, 8, 7, 6), + } + }, + } as ApiJob; + historyPushSpy.mockClear(); updateBannerSpy.mockClear(); updateDialogSpy.mockClear(); @@ -84,7 +89,9 @@ describe('RecurringRunDetails', () => { deleteJobSpy.mockClear(); deleteJobSpy.mockImplementation(); enableJobSpy.mockClear(); + enableJobSpy.mockImplementation(); disableJobSpy.mockClear(); + disableJobSpy.mockImplementation(); getExperimentSpy.mockImplementation(); getExperimentSpy.mockClear(); }); @@ -116,7 +123,6 @@ describe('RecurringRunDetails', () => { }); it('loads the recurring run given its id in query params', async () => { - fullTestJob.resource_references = []; shallow(); await TestUtils.flushPromises(); expect(getJobSpy).toHaveBeenLastCalledWith(fullTestJob.id); @@ -124,7 +130,6 @@ describe('RecurringRunDetails', () => { }); it('shows All runs -> run name when there is no experiment', async () => { - fullTestJob.resource_references = []; const tree = shallow(); await TestUtils.flushPromises(); expect(updateToolbarSpy).toHaveBeenLastCalledWith(expect.objectContaining({ @@ -165,7 +170,6 @@ describe('RecurringRunDetails', () => { }); it('shows error banner if run cannot be fetched', async () => { - fullTestJob.resource_references = []; TestUtils.makeErrorResponseOnce(getJobSpy, 'woops!'); const tree = shallow(); await TestUtils.flushPromises(); @@ -204,7 +208,6 @@ describe('RecurringRunDetails', () => { }); it('shows enabled Disable, and disabled Enable buttons if the run is enabled', async () => { - fullTestJob.enabled = true; const tree = shallow(); await TestUtils.flushPromises(); expect(updateToolbarSpy).toHaveBeenCalledTimes(2); @@ -248,6 +251,64 @@ describe('RecurringRunDetails', () => { tree.unmount(); }); + it('calls disable API when disable button is clicked, refreshes the page', async () => { + const tree = shallow(); + await TestUtils.flushPromises(); + const instance = tree.instance() as RecurringRunDetails; + const disableBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Disable'); + await disableBtn!.action(); + expect(disableJobSpy).toHaveBeenCalledTimes(1); + expect(disableJobSpy).toHaveBeenLastCalledWith('test-job-id'); + expect(getJobSpy).toHaveBeenCalledTimes(2); + expect(getJobSpy).toHaveBeenLastCalledWith('test-job-id'); + tree.unmount(); + }); + + it('shows error dialog if disable fails', async () => { + const tree = shallow(); + TestUtils.makeErrorResponseOnce(disableJobSpy, 'could not disable'); + await TestUtils.flushPromises(); + const instance = tree.instance() as RecurringRunDetails; + const disableBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Disable'); + await disableBtn!.action(); + expect(updateDialogSpy).toHaveBeenCalledTimes(1); + expect(updateDialogSpy).toHaveBeenLastCalledWith(expect.objectContaining({ + content: 'could not disable', + title: 'Failed to disable recurring run', + })); + tree.unmount(); + }); + + it('shows error dialog if enable fails', async () => { + fullTestJob.enabled = false; + const tree = shallow(); + TestUtils.makeErrorResponseOnce(enableJobSpy, 'could not enable'); + await TestUtils.flushPromises(); + const instance = tree.instance() as RecurringRunDetails; + const enableBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Enable'); + await enableBtn!.action(); + expect(updateDialogSpy).toHaveBeenCalledTimes(1); + expect(updateDialogSpy).toHaveBeenLastCalledWith(expect.objectContaining({ + content: 'could not enable', + title: 'Failed to enable recurring run', + })); + tree.unmount(); + }); + + it('calls enable API when enable button is clicked, refreshes the page', async () => { + fullTestJob.enabled = false; + const tree = shallow(); + await TestUtils.flushPromises(); + const instance = tree.instance() as RecurringRunDetails; + const enableBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Enable'); + await enableBtn!.action(); + expect(enableJobSpy).toHaveBeenCalledTimes(1); + expect(enableJobSpy).toHaveBeenLastCalledWith('test-job-id'); + expect(getJobSpy).toHaveBeenCalledTimes(2); + expect(getJobSpy).toHaveBeenLastCalledWith('test-job-id'); + tree.unmount(); + }); + it('shows a delete button', async () => { const tree = shallow(); const instance = tree.instance() as RecurringRunDetails; diff --git a/frontend/src/pages/RecurringRunDetails.tsx b/frontend/src/pages/RecurringRunDetails.tsx index 5a174a2a2f3..b02e05e0b22 100644 --- a/frontend/src/pages/RecurringRunDetails.tsx +++ b/frontend/src/pages/RecurringRunDetails.tsx @@ -209,7 +209,7 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { } catch (err) { const errorMessage = await errorToMessage(err); this.showErrorDialog( - `Failed to ${enabled ? 'enable' : 'disable'} recurring schedule`, errorMessage); + `Failed to ${enabled ? 'enable' : 'disable'} recurring run`, errorMessage); } finally { toolbarActions[buttonIndex].busy = false; this._updateToolbar(toolbarActions); diff --git a/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap b/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap index 5495835388e..762c8f5e306 100644 --- a/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap +++ b/frontend/src/pages/__snapshots__/RecurringRunDetails.test.tsx.snap @@ -57,6 +57,21 @@ exports[`RecurringRunDetails renders a recurring run with cron schedule 1`] = ` ] } /> +
+ Run parameters +
+ `; @@ -118,6 +133,21 @@ exports[`RecurringRunDetails renders a recurring run with periodic schedule 1`] ] } /> +
+ Run parameters +
+ `; From f5d20bd1bb95dca33e5b3245ddf472f3e440cf60 Mon Sep 17 00:00:00 2001 From: Yasser Elsayed Date: Mon, 12 Nov 2018 22:08:13 -0800 Subject: [PATCH 4/5] pr comments --- frontend/src/pages/Page.tsx | 4 +- .../src/pages/RecurringRunDetails.test.tsx | 44 ++++++------ frontend/src/pages/RecurringRunDetails.tsx | 72 +++++++++++-------- 3 files changed, 66 insertions(+), 54 deletions(-) diff --git a/frontend/src/pages/Page.tsx b/frontend/src/pages/Page.tsx index 4f8b7af06c7..422efb085e7 100644 --- a/frontend/src/pages/Page.tsx +++ b/frontend/src/pages/Page.tsx @@ -53,12 +53,12 @@ export abstract class Page extends React.Component

{ this.props.updateBanner({}); } - public async showPageError(message: string, error?: Error): Promise { + public async showPageError(message: string, error?: Error, mode?: 'error' | 'warning'): Promise { const errorMessage = await errorToMessage(error); this.props.updateBanner({ additionalInfo: errorMessage ? errorMessage : undefined, message: message + (errorMessage ? ' Click Details for more information.' : ''), - mode: 'error', + mode: mode || 'error', refresh: this.refresh.bind(this), }); } diff --git a/frontend/src/pages/RecurringRunDetails.test.tsx b/frontend/src/pages/RecurringRunDetails.test.tsx index 4d748977b43..a3aaf9d9c06 100644 --- a/frontend/src/pages/RecurringRunDetails.test.tsx +++ b/frontend/src/pages/RecurringRunDetails.test.tsx @@ -17,7 +17,6 @@ import * as React from 'react'; import RecurringRunDetails from './RecurringRunDetails'; import TestUtils from '../TestUtils'; -import produce from 'immer'; import { ApiJob, ApiResourceType } from '../apis/job'; import { Apis } from '../lib/Apis'; import { PageProps } from './Page'; @@ -96,40 +95,38 @@ describe('RecurringRunDetails', () => { getExperimentSpy.mockClear(); }); - it('renders a recurring run with periodic schedule', () => { + it('renders a recurring run with periodic schedule', async () => { const tree = shallow(); - tree.setState({ - run: fullTestJob, - }); + await TestUtils.flushPromises(); expect(tree).toMatchSnapshot(); tree.unmount(); }); - it('renders a recurring run with cron schedule', () => { + it('renders a recurring run with cron schedule', async () => { + fullTestJob.trigger = { + cron_schedule: { + cron: '* * * 0 0 !', + end_time: new Date(2018, 10, 9, 8, 7, 6), + start_time: new Date(2018, 9, 8, 7, 6), + } + }; const tree = shallow(); - tree.setState({ - run: produce(fullTestJob, draft => { - draft.trigger = { - cron_schedule: { - cron: '* * * 0 0 !', - end_time: new Date(2018, 10, 9, 8, 7, 6), - start_time: new Date(2018, 9, 8, 7, 6), - } - }; - }), - }); + await TestUtils.flushPromises(); expect(tree).toMatchSnapshot(); tree.unmount(); }); it('loads the recurring run given its id in query params', async () => { - shallow(); + // The run id is in the router match object, defined inside generateProps + const tree = shallow(); await TestUtils.flushPromises(); expect(getJobSpy).toHaveBeenLastCalledWith(fullTestJob.id); expect(getExperimentSpy).not.toHaveBeenCalled(); + tree.unmount(); }); it('shows All runs -> run name when there is no experiment', async () => { + // The run id is in the router match object, defined inside generateProps const tree = shallow(); await TestUtils.flushPromises(); expect(updateToolbarSpy).toHaveBeenLastCalledWith(expect.objectContaining({ @@ -150,7 +147,7 @@ describe('RecurringRunDetails', () => { tree.unmount(); }); - it('shows Experiments -> Experiment name -> run name when there is no experiment', async () => { + it('shows Experiments -> Experiment name -> run name when there is an experiment', async () => { fullTestJob.resource_references = [{ key: { id: 'test-experiment-id', type: ApiResourceType.EXPERIMENT } }]; getExperimentSpy.mockImplementation(id => ({ id, name: 'test experiment name' })); const tree = shallow(); @@ -182,7 +179,7 @@ describe('RecurringRunDetails', () => { tree.unmount(); }); - it('shows error banner if has experiment but experiment cannot be fetched', async () => { + it('shows warning banner if has experiment but experiment cannot be fetched. still loads run', async () => { fullTestJob.resource_references = [{ key: { id: 'test-experiment-id', type: ApiResourceType.EXPERIMENT } }]; TestUtils.makeErrorResponseOnce(getExperimentSpy, 'woops!'); const tree = shallow(); @@ -190,9 +187,10 @@ describe('RecurringRunDetails', () => { expect(updateBannerSpy).toHaveBeenCalledTimes(2); // Once to clear, once to show error expect(updateBannerSpy).toHaveBeenLastCalledWith(expect.objectContaining({ additionalInfo: 'woops!', - message: `Error: failed to retrieve recurring run: ${fullTestJob.id}. Click Details for more information.`, - mode: 'error', + message: `Error: failed to retrieve this recurring run\'s experiment. Click Details for more information.`, + mode: 'warning', })); + expect(tree.state('run')).toEqual(fullTestJob); tree.unmount(); }); @@ -311,6 +309,7 @@ describe('RecurringRunDetails', () => { it('shows a delete button', async () => { const tree = shallow(); + await TestUtils.flushPromises(); const instance = tree.instance() as RecurringRunDetails; const deleteBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Refresh'); expect(deleteBtn).toBeDefined(); @@ -319,6 +318,7 @@ describe('RecurringRunDetails', () => { it('shows delete dialog when delete button is clicked', async () => { const tree = shallow(); + await TestUtils.flushPromises(); const instance = tree.instance() as RecurringRunDetails; const deleteBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Delete'); await deleteBtn!.action(); diff --git a/frontend/src/pages/RecurringRunDetails.tsx b/frontend/src/pages/RecurringRunDetails.tsx index b02e05e0b22..9d1c103c9bd 100644 --- a/frontend/src/pages/RecurringRunDetails.tsx +++ b/frontend/src/pages/RecurringRunDetails.tsx @@ -155,43 +155,55 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { this.clearBanner(); const runId = this.props.match.params[RouteParams.runId]; + let run: ApiJob; try { - const run = await Apis.jobServiceApi.getJob(runId); - const relatedExperimentId = RunUtils.getFirstExperimentReferenceId(run); - let experiment: ApiExperiment | undefined; - if (relatedExperimentId) { + run = await Apis.jobServiceApi.getJob(runId); + } catch (err) { + const errorMessage = await errorToMessage(err); + await this.showPageError( + `Error: failed to retrieve recurring run: ${runId}.`, new Error(errorMessage)); + return; + } + + const relatedExperimentId = RunUtils.getFirstExperimentReferenceId(run); + let experiment: ApiExperiment | undefined; + if (relatedExperimentId) { + try { experiment = await Apis.experimentServiceApi.getExperiment(relatedExperimentId); - } - const breadcrumbs: Breadcrumb[] = []; - if (experiment) { - breadcrumbs.push( - { displayName: 'Experiments', href: RoutePage.EXPERIMENTS }, - { - displayName: experiment.name!, - href: RoutePage.EXPERIMENT_DETAILS.replace(':' + RouteParams.experimentId, experiment.id!) - }); - } else { - breadcrumbs.push( - { displayName: 'All runs', href: RoutePage.RUNS } + } catch (err) { + const errorMessage = await errorToMessage(err); + await this.showPageError( + `Error: failed to retrieve this recurring run\'s experiment.`, + new Error(errorMessage), + 'warning' ); } - breadcrumbs.push({ - displayName: run ? run.name! : runId, - href: '', - }); + } + const breadcrumbs: Breadcrumb[] = []; + if (experiment) { + breadcrumbs.push( + { displayName: 'Experiments', href: RoutePage.EXPERIMENTS }, + { + displayName: experiment.name!, + href: RoutePage.EXPERIMENT_DETAILS.replace(':' + RouteParams.experimentId, experiment.id!) + }); + } else { + breadcrumbs.push( + { displayName: 'All runs', href: RoutePage.RUNS } + ); + } + breadcrumbs.push({ + displayName: run ? run.name! : runId, + href: '', + }); - const toolbarActions = [...this.props.toolbarProps.actions]; - toolbarActions[1].disabled = !!run.enabled; - toolbarActions[2].disabled = !run.enabled; + const toolbarActions = [...this.props.toolbarProps.actions]; + toolbarActions[1].disabled = !!run.enabled; + toolbarActions[2].disabled = !run.enabled; - this.props.updateToolbar({ actions: toolbarActions, breadcrumbs }); + this.props.updateToolbar({ actions: toolbarActions, breadcrumbs }); - this.setState({ run }); - } catch (err) { - const errorMessage = await errorToMessage(err); - await this.showPageError( - `Error: failed to retrieve recurring run: ${runId}.`, new Error(errorMessage)); - } + this.setState({ run }); } protected async _setEnabledState(enabled: boolean): Promise { From 1667f62eaba88dd3ad38de03e20b82436f3d2404 Mon Sep 17 00:00:00 2001 From: Yasser Elsayed Date: Wed, 14 Nov 2018 16:40:05 -0800 Subject: [PATCH 5/5] pr comments --- frontend/src/pages/RecurringRunDetails.test.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frontend/src/pages/RecurringRunDetails.test.tsx b/frontend/src/pages/RecurringRunDetails.test.tsx index a3aaf9d9c06..7385b50de9c 100644 --- a/frontend/src/pages/RecurringRunDetails.test.tsx +++ b/frontend/src/pages/RecurringRunDetails.test.tsx @@ -357,6 +357,9 @@ describe('RecurringRunDetails', () => { tree.unmount(); }); + // TODO: need to test the dismiss path too--when the dialog is dismissed using ESC + // or clicking outside it, it should be treated the same way as clicking Cancel. + it('redirects back to parent experiment after delete', async () => { const tree = shallow(); await TestUtils.flushPromises();