Skip to content

Commit

Permalink
chore(frontend): Refactor NewExperiment to functional component (kube…
Browse files Browse the repository at this point in the history
…flow#9948)

* Add an alternative component for new experiment page.

* Add unit tests.
Assign default values for useState().

* Move the files to functional_components folder.
Update the imported files from absolute path.

* Move handing experiemnt creation error to useEffect
rename the create helper to createExperiment.

* Move redirection logic after createExperiment() is succeed to
useEffect().

* Fix dependency array for handling createExperiment() succeed case.

* Remove pipeline id in dependency array.
  • Loading branch information
jlyaoyuli authored Sep 1, 2023
1 parent bc5fe57 commit 6b7739d
Show file tree
Hide file tree
Showing 3 changed files with 510 additions and 11 deletions.
28 changes: 17 additions & 11 deletions frontend/src/pages/NewExperiment.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,23 @@
*/

import * as React from 'react';
import BusyButton from '../atoms/BusyButton';
import BusyButton from 'src/atoms/BusyButton';
import Button from '@material-ui/core/Button';
import Input from '../atoms/Input';
import Input from 'src/atoms/Input';
import { V2beta1Experiment } from 'src/apisv2beta1/experiment';
import { Apis } from '../lib/Apis';
import { Page, PageProps } from './Page';
import { RoutePage, QUERY_PARAMS } from '../components/Router';
import { Apis } from 'src/lib/Apis';
import { Page, PageProps } from 'src/pages/Page';
import { RoutePage, QUERY_PARAMS } from 'src/components/Router';
import { TextFieldProps } from '@material-ui/core/TextField';
import { ToolbarProps } from '../components/Toolbar';
import { URLParser } from '../lib/URLParser';
import { ToolbarProps } from 'src/components/Toolbar';
import { URLParser } from 'src/lib/URLParser';
import { classes, stylesheet } from 'typestyle';
import { commonCss, padding, fontsize } from '../Css';
import { logger, errorToMessage } from '../lib/Utils';
import { commonCss, padding, fontsize } from 'src/Css';
import { logger, errorToMessage } from 'src/lib/Utils';
import { NamespaceContext } from 'src/lib/KubeflowClient';
import { getLatestVersion } from './NewRunV2';
import { getLatestVersion } from 'src/pages/NewRunV2';
import { NewExperimentFC } from 'src/pages/functional_components/NewExperimentFC';
import { FeatureKey, isFeatureEnabled } from 'src/features';

interface NewExperimentState {
description: string;
Expand Down Expand Up @@ -201,7 +203,11 @@ export class NewExperiment extends Page<{ namespace?: string }, NewExperimentSta

const EnhancedNewExperiment: React.FC<PageProps> = props => {
const namespace = React.useContext(NamespaceContext);
return <NewExperiment {...props} namespace={namespace} />;
return isFeatureEnabled(FeatureKey.FUNCTIONAL_COMPONENT) ? (
<NewExperimentFC {...props} namespace={namespace} />
) : (
<NewExperiment {...props} namespace={namespace} />
);
};

export default EnhancedNewExperiment;
299 changes: 299 additions & 0 deletions frontend/src/pages/functional_components/NewExperimentFC.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,299 @@
/*
* Copyright 2023 The Kubeflow Authors
*
* 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 { fireEvent, render, screen, waitFor } from '@testing-library/react';
import * as React from 'react';
import { CommonTestWrapper } from 'src/TestWrapper';
import TestUtils from 'src/TestUtils';
import { NewExperimentFC } from './NewExperimentFC';
import { Apis } from 'src/lib/Apis';
import { PageProps } from 'src/pages/Page';
import * as features from 'src/features';
import { RoutePage, QUERY_PARAMS } from 'src/components/Router';

describe('NewExperiment', () => {
const TEST_EXPERIMENT_ID = 'new-experiment-id';
const createExperimentSpy = jest.spyOn(Apis.experimentServiceApiV2, 'createExperiment');
const historyPushSpy = jest.fn();
const updateDialogSpy = jest.fn();
const updateSnackbarSpy = jest.fn();
const updateToolbarSpy = jest.fn();

function generateProps(): PageProps {
return {
history: { push: historyPushSpy } as any,
location: { pathname: RoutePage.NEW_EXPERIMENT } as any,
match: '' as any,
toolbarProps: { actions: {}, breadcrumbs: [], pageTitle: TEST_EXPERIMENT_ID },
updateBanner: () => null,
updateDialog: updateDialogSpy,
updateSnackbar: updateSnackbarSpy,
updateToolbar: updateToolbarSpy,
};
}

beforeEach(() => {
jest.clearAllMocks();
// mock both v2_alpha and functional feature keys are enable.
jest.spyOn(features, 'isFeatureEnabled').mockReturnValue(true);

createExperimentSpy.mockImplementation(() => ({
experiment_id: 'new-experiment-id',
display_name: 'new-experiment-name',
}));
});

it('does not include any action buttons in the toolbar', () => {
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} />
</CommonTestWrapper>,
);

expect(updateToolbarSpy).toHaveBeenCalledWith({
actions: {},
breadcrumbs: [{ displayName: 'Experiments', href: RoutePage.EXPERIMENTS }],
pageTitle: 'New experiment',
});
});

it("enables the 'Next' button when an experiment name is entered", () => {
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} />
</CommonTestWrapper>,
);

const experimentNameInput = screen.getByLabelText(/Experiment name/);
fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } });
const nextButton = screen.getByText('Next');
expect(nextButton.closest('button')?.disabled).toEqual(false);
});

it("re-disables the 'Next' button when an experiment name is cleared after having been entered", () => {
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} />
</CommonTestWrapper>,
);

const experimentNameInput = screen.getByLabelText(/Experiment name/);
fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } });
const nextButton = screen.getByText('Next');
expect(nextButton.closest('button')?.disabled).toEqual(false);

// Remove experiment name
fireEvent.change(experimentNameInput, { target: { value: '' } });
expect(nextButton.closest('button')?.disabled).toEqual(true);
});

it('updates the experiment name', () => {
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} />
</CommonTestWrapper>,
);

const experimentNameInput = screen.getByLabelText(/Experiment name/);
fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } });
expect(experimentNameInput.closest('input')?.value).toBe('new-experiment-name');
});

it('create new experiment', async () => {
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} />
</CommonTestWrapper>,
);

const experimentNameInput = screen.getByLabelText(/Experiment name/);
fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } });
const experimentDescriptionInput = screen.getByLabelText('Description');
fireEvent.change(experimentDescriptionInput, {
target: { value: 'new-experiment-description' },
});
const nextButton = screen.getByText('Next');
expect(nextButton.closest('button')?.disabled).toEqual(false);

fireEvent.click(nextButton);
await waitFor(() => {
expect(createExperimentSpy).toHaveBeenCalledWith(
expect.objectContaining({
description: 'new-experiment-description',
display_name: 'new-experiment-name',
}),
);
});
});

it('create new experiment with namespace provided', async () => {
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} namespace='test-ns' />
</CommonTestWrapper>,
);

const experimentNameInput = screen.getByLabelText(/Experiment name/);
fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } });
const nextButton = screen.getByText('Next');
expect(nextButton.closest('button')?.disabled).toEqual(false);

fireEvent.click(nextButton);
await waitFor(() => {
expect(createExperimentSpy).toHaveBeenCalledWith(
expect.objectContaining({
description: '',
display_name: 'new-experiment-name',
namespace: 'test-ns',
}),
);
});
});

it('navigates to NewRun page upon successful creation', async () => {
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} />
</CommonTestWrapper>,
);

const experimentNameInput = screen.getByLabelText(/Experiment name/);
fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } });
const nextButton = screen.getByText('Next');
expect(nextButton.closest('button')?.disabled).toEqual(false);

fireEvent.click(nextButton);
await waitFor(() => {
expect(createExperimentSpy).toHaveBeenCalledWith(
expect.objectContaining({
description: '',
display_name: 'new-experiment-name',
}),
);
});
expect(historyPushSpy).toHaveBeenCalledWith(
RoutePage.NEW_RUN + `?experimentId=${TEST_EXPERIMENT_ID}` + `&firstRunInExperiment=1`,
);
});

it('includes pipeline ID and version ID in NewRun page query params if present', async () => {
const pipelineId = 'some-pipeline-id';
const pipelineVersionId = 'version-id';
const listPipelineVersionsSpy = jest.spyOn(Apis.pipelineServiceApiV2, 'listPipelineVersions');
listPipelineVersionsSpy.mockImplementation(() => ({
pipeline_versions: [{ pipeline_version_id: pipelineVersionId }],
}));

const props = generateProps();
props.location.search = `?${QUERY_PARAMS.pipelineId}=${pipelineId}`;

render(
<CommonTestWrapper>
<NewExperimentFC {...props} />
</CommonTestWrapper>,
);

const experimentNameInput = screen.getByLabelText(/Experiment name/);
fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } });
const nextButton = screen.getByText('Next');
expect(nextButton.closest('button')?.disabled).toEqual(false);

fireEvent.click(nextButton);
await waitFor(() => {
expect(createExperimentSpy).toHaveBeenCalledWith(
expect.objectContaining({
description: '',
display_name: 'new-experiment-name',
}),
);
});

expect(historyPushSpy).toHaveBeenCalledWith(
RoutePage.NEW_RUN +
`?experimentId=${TEST_EXPERIMENT_ID}` +
`&pipelineId=${pipelineId}` +
`&pipelineVersionId=${pipelineVersionId}` +
`&firstRunInExperiment=1`,
);
});

it('shows snackbar confirmation after experiment is created', async () => {
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} />
</CommonTestWrapper>,
);

const experimentNameInput = screen.getByLabelText(/Experiment name/);
fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } });
const nextButton = screen.getByText('Next');
expect(nextButton.closest('button')?.disabled).toEqual(false);

fireEvent.click(nextButton);
await waitFor(() => {
expect(createExperimentSpy).toHaveBeenCalledWith(
expect.objectContaining({
description: '',
display_name: 'new-experiment-name',
}),
);
});
expect(updateSnackbarSpy).toHaveBeenLastCalledWith({
autoHideDuration: 10000,
message: 'Successfully created new Experiment: new-experiment-name',
open: true,
});
});

it('shows error dialog when experiment creation fails', async () => {
TestUtils.makeErrorResponseOnce(createExperimentSpy, 'There was something wrong!');
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} />
</CommonTestWrapper>,
);

const experimentNameInput = screen.getByLabelText(/Experiment name/);
fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } });
const nextButton = screen.getByText('Next');
expect(nextButton.closest('button')?.disabled).toEqual(false);

fireEvent.click(nextButton);
await waitFor(() => {
expect(createExperimentSpy).toHaveBeenCalled();
});

expect(updateDialogSpy).toHaveBeenCalledWith(
expect.objectContaining({
content: 'There was something wrong!',
title: 'Experiment creation failed',
}),
);
});

it('navigates to experiment list page upon cancellation', () => {
render(
<CommonTestWrapper>
<NewExperimentFC {...generateProps()} />
</CommonTestWrapper>,
);

const cancelButton = screen.getByText('Cancel');
fireEvent.click(cancelButton);

expect(historyPushSpy).toHaveBeenCalledWith(RoutePage.EXPERIMENTS);
});
});
Loading

0 comments on commit 6b7739d

Please sign in to comment.