Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: AlertReportCronScheduler tests #19288

Merged
merged 1 commit into from
Apr 20, 2022
Merged

fix: AlertReportCronScheduler tests #19288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 122 additions & 42 deletions superset-frontend/src/views/CRUD/alert/components/AlertReportCronScheduler.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,58 +16,138 @@
* specific language governing permissions and limitations
* under the License.
*/

import React from 'react';
import { ReactWrapper } from 'enzyme';
import { styledMount as mount } from 'spec/helpers/theming';
import { CronPicker } from 'src/components/CronPicker';
import { Input } from 'src/components/Input';
import { AlertReportCronScheduler } from './AlertReportCronScheduler';
import { render, screen, waitFor, within } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import { act } from 'react-dom/test-utils';

import {
AlertReportCronScheduler,
AlertReportCronSchedulerProps,
} from './AlertReportCronScheduler';

const createProps = (props: Partial<AlertReportCronSchedulerProps> = {}) => ({
onChange: jest.fn(),
value: '* * * * *',
...props,
});

test('should render', () => {
const props = createProps();
render(<AlertReportCronScheduler {...props} />);

// Text found in the first radio option
expect(screen.getByText('Every')).toBeInTheDocument();
// Text found in the second radio option
expect(screen.getByText('CRON Schedule')).toBeInTheDocument();
});

test('only one radio option should be enabled at a time', () => {
const props = createProps();
const { container } = render(<AlertReportCronScheduler {...props} />);

expect(screen.getByTestId('picker')).toBeChecked();
expect(screen.getByTestId('input')).not.toBeChecked();

const pickerContainer = container.querySelector(
'.react-js-cron-select',
) as HTMLElement;
const inputContainer = screen.getByTestId('input-content');

expect(within(pickerContainer).getAllByRole('combobox')[0]).toBeEnabled();
expect(inputContainer.querySelector('input[name="crontab"]')).toBeDisabled();

userEvent.click(screen.getByTestId('input'));

expect(within(pickerContainer).getAllByRole('combobox')[0]).toBeDisabled();
expect(inputContainer.querySelector('input[name="crontab"]')).toBeEnabled();

userEvent.click(screen.getByTestId('picker'));

expect(within(pickerContainer).getAllByRole('combobox')[0]).toBeEnabled();
expect(inputContainer.querySelector('input[name="crontab"]')).toBeDisabled();
});

test('picker mode updates correctly', async () => {
const onChangeCallback = jest.fn();
const props = createProps({
onChange: onChangeCallback,
});

const { container } = render(<AlertReportCronScheduler {...props} />);

describe('AlertReportCronScheduler', () => {
let wrapper: ReactWrapper;
expect(screen.getByTestId('picker')).toBeChecked();

it('calls onChnage when value chnages', () => {
const onChangeMock = jest.fn();
wrapper = mount(
<AlertReportCronScheduler value="* * * * *" onChange={onChangeMock} />,
);
const pickerContainer = container.querySelector(
'.react-js-cron-select',
) as HTMLElement;

const changeValue = '1,7 * * * *';
const firstSelect = within(pickerContainer).getAllByRole('combobox')[0];
act(() => {
userEvent.click(firstSelect);
});

wrapper.find(CronPicker).props().setValue(changeValue);
expect(onChangeMock).toHaveBeenLastCalledWith(changeValue);
expect(await within(pickerContainer).findByText('day')).toBeInTheDocument();
act(() => {
userEvent.click(within(pickerContainer).getByText('day'));
});

it.skip('sets input value when cron picker changes', () => {
const onChangeMock = jest.fn();
wrapper = mount(
<AlertReportCronScheduler value="* * * * *" onChange={onChangeMock} />,
);
expect(onChangeCallback).toHaveBeenLastCalledWith('* * * * *');

const secondSelect = container.querySelector(
'.react-js-cron-hours .ant-select-selector',
) as HTMLElement;
await waitFor(() => {
expect(secondSelect).toBeInTheDocument();
});

act(() => {
userEvent.click(secondSelect);
});

const changeValue = '1,7 * * * *';
expect(await screen.findByText('9')).toBeInTheDocument();
act(() => {
userEvent.click(screen.getByText('9'));
});

wrapper.find(CronPicker).props().setValue(changeValue);
// TODO fix this class-style assertion that doesn't work on function components
// @ts-ignore
expect(wrapper.find(Input).state().value).toEqual(changeValue);
await waitFor(() => {
expect(onChangeCallback).toHaveBeenLastCalledWith('* 9 * * *');
});
});

it('calls onChange when input value changes', () => {
const onChangeMock = jest.fn();
wrapper = mount(
<AlertReportCronScheduler value="* * * * *" onChange={onChangeMock} />,
);

const changeValue = '1,7 * * * *';
const event = {
target: { value: changeValue },
} as React.FocusEvent<HTMLInputElement>;

const inputProps = wrapper.find(Input).props();
if (inputProps.onBlur) {
inputProps.onBlur(event);
}
expect(onChangeMock).toHaveBeenLastCalledWith(changeValue);
test('input mode updates correctly', async () => {
const onChangeCallback = jest.fn();
const props = createProps({
onChange: onChangeCallback,
});

render(<AlertReportCronScheduler {...props} />);

const inputContainer = screen.getByTestId('input-content');
userEvent.click(screen.getByTestId('input'));

const input = inputContainer.querySelector(
'input[name="crontab"]',
) as HTMLElement;
await waitFor(() => {
expect(input).toBeEnabled();
});

userEvent.clear(input);
expect(input).toHaveValue('');

const value = '* 10 2 * *';
await act(async () => {
await userEvent.type(input, value, { delay: 1 });
});

await waitFor(() => {
expect(input).toHaveValue(value);
});

act(() => {
userEvent.click(inputContainer);
});

expect(onChangeCallback).toHaveBeenLastCalledWith(value);
});
48 changes: 31 additions & 17 deletions superset-frontend/src/views/CRUD/alert/components/AlertReportCronScheduler.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,44 +16,59 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useState, useCallback, useRef, FunctionComponent } from 'react';
import React, { useState, useCallback, useRef, FocusEvent } from 'react';
import { t, useTheme } from '@superset-ui/core';

import { AntdInput } from 'src/components';
import { AntdInput, RadioChangeEvent } from 'src/components';
import { Input } from 'src/components/Input';
import { Radio } from 'src/components/Radio';
import { CronPicker, CronError } from 'src/components/CronPicker';
import { StyledInputContainer } from 'src/views/CRUD/alert/AlertReportModal';

interface AlertReportCronSchedulerProps {
export interface AlertReportCronSchedulerProps {
value: string;
onChange: (change: string) => any;
}

export const AlertReportCronScheduler: FunctionComponent<AlertReportCronSchedulerProps> =
export const AlertReportCronScheduler: React.FC<AlertReportCronSchedulerProps> =
({ value, onChange }) => {
const theme = useTheme();
const inputRef = useRef<AntdInput>(null);
const [scheduleFormat, setScheduleFormat] = useState<'picker' | 'input'>(
'picker',
);

const handleRadioButtonChange = useCallback(
(e: RadioChangeEvent) => setScheduleFormat(e.target.value),
[],
);

const customSetValue = useCallback(
(newValue: string) => {
onChange(newValue);
inputRef.current?.setValue(newValue);
},
[inputRef, onChange],
);

const handleBlur = useCallback(
(event: FocusEvent<HTMLInputElement>) => {
onChange(event.target.value);
},
[onChange],
);

const handlePressEnter = useCallback(() => {
onChange(inputRef.current?.input.value || '');
}, [onChange]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diegomedina248 do you think we need useCallback for simple Input component props?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't hurt nor improve in this case, but I believe it's cleaner

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit more code and a bit more computation imo, which is my only concern with not much performance impact, but it's not a big deal. I'll cc @rusackas if he has any opinions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but to your comment of it being cleaner, @diegomedina248 I meant to also ask in what way?


const [error, onError] = useState<CronError>();

return (
<>
<Radio.Group
onChange={e => setScheduleFormat(e.target.value)}
value={scheduleFormat}
>
<Radio.Group onChange={handleRadioButtonChange} value={scheduleFormat}>
<div className="inline-container add-margin">
<Radio value="picker" />
<Radio data-test="picker" value="picker" />
<CronPicker
clearButton={false}
value={value}
Expand All @@ -64,9 +79,12 @@ export const AlertReportCronScheduler: FunctionComponent<AlertReportCronSchedule
/>
</div>
<div className="inline-container add-margin">
<Radio value="input" />
<Radio data-test="input" value="input" />
<span className="input-label">CRON Schedule</span>
<StyledInputContainer className="styled-input">
<StyledInputContainer
data-test="input-content"
className="styled-input"
>
<div className="input-container">
<Input
type="text"
Expand All @@ -75,12 +93,8 @@ export const AlertReportCronScheduler: FunctionComponent<AlertReportCronSchedule
style={error ? { borderColor: theme.colors.error.base } : {}}
placeholder={t('CRON expression')}
disabled={scheduleFormat !== 'input'}
onBlur={event => {
onChange(event.target.value);
}}
onPressEnter={() => {
onChange(inputRef.current?.input.value || '');
}}
onBlur={handleBlur}
onPressEnter={handlePressEnter}
/>
</div>
</StyledInputContainer>
Expand Down