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: TimezoneSelector is not reliably testable #19148

Closed
wants to merge 1 commit into from
Closed

fix: TimezoneSelector is not reliably testable #19148

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
165 changes: 89 additions & 76 deletions superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,91 +27,104 @@ jest.spyOn(moment.tz, 'guess').mockReturnValue('America/New_York');
const getSelectOptions = () =>
waitFor(() => document.querySelectorAll('.ant-select-item-option-content'));

it('use the timezone from `moment` if no timezone provided', () => {
const onTimezoneChange = jest.fn();
render(<TimezoneSelector onTimezoneChange={onTimezoneChange} />);
expect(onTimezoneChange).toHaveBeenCalledTimes(1);
expect(onTimezoneChange).toHaveBeenCalledWith('America/Nassau');
});
describe('TimezoneSelector', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I thought we have agreed on not adding describe? https://github.com/apache/superset/wiki/Testing-Guidelines-and-Best-Practices#avoid-nesting-when-youre-testing

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I wasn't familiar with this pattern. I don't think I agree that never adding describe is desirable. These blocks are important when you want to do setup and cleanup reliably.

Copy link
Member

Choose a reason for hiding this comment

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

You can run beforeEach and afterEach even without the nesting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that makes more sense then. I still do feel that it can be useful sometimes for categorization or setting up different shared initial conditions, but I agree with preferring to avoid in most cases.

beforeAll(() => {
jest.useFakeTimers('modern');
jest.setSystemTime(new Date('January 10 2022'));
Comment on lines +32 to +33
Copy link
Member Author

Choose a reason for hiding this comment

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

This sets/freezes the system time so that tests will be run under the same conditions every time.

});

it('update to closest deduped timezone when timezone is provided', async () => {
const onTimezoneChange = jest.fn();
render(
<TimezoneSelector
onTimezoneChange={onTimezoneChange}
timezone="America/Los_Angeles"
/>,
);
expect(onTimezoneChange).toHaveBeenCalledTimes(1);
expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Vancouver');
});
afterAll(() => {
jest.useRealTimers();
});

it('use the default timezone when an invalid timezone is provided', async () => {
const onTimezoneChange = jest.fn();
render(
<TimezoneSelector onTimezoneChange={onTimezoneChange} timezone="UTC" />,
);
expect(onTimezoneChange).toHaveBeenCalledTimes(1);
expect(onTimezoneChange).toHaveBeenLastCalledWith('Africa/Abidjan');
});
it('use the timezone from `moment` if no timezone provided', () => {
const onTimezoneChange = jest.fn();
render(<TimezoneSelector onTimezoneChange={onTimezoneChange} />);
expect(onTimezoneChange).toHaveBeenCalledTimes(1);
expect(onTimezoneChange).toHaveBeenCalledWith('America/Nassau');
});

it('can select a timezone values and returns canonical value', async () => {
const onTimezoneChange = jest.fn();
render(
<TimezoneSelector
onTimezoneChange={onTimezoneChange}
timezone="America/Nassau"
/>,
);
it('update to closest deduped timezone when timezone is provided', async () => {
const onTimezoneChange = jest.fn();
render(
<TimezoneSelector
onTimezoneChange={onTimezoneChange}
timezone="America/Los_Angeles"
/>,
);
expect(onTimezoneChange).toHaveBeenCalledTimes(1);
expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Vancouver');
});

const searchInput = screen.getByRole('combobox', {
name: 'Timezone selector',
it('use the default timezone when an invalid timezone is provided', async () => {
const onTimezoneChange = jest.fn();
render(
<TimezoneSelector onTimezoneChange={onTimezoneChange} timezone="UTC" />,
);
expect(onTimezoneChange).toHaveBeenCalledTimes(1);
expect(onTimezoneChange).toHaveBeenLastCalledWith('Africa/Abidjan');
});
expect(searchInput).toBeInTheDocument();
userEvent.click(searchInput);
const isDaylight = moment(moment.now()).isDST();

const selectedTimezone = isDaylight
? 'GMT -04:00 (Eastern Daylight Time)'
: 'GMT -05:00 (Eastern Standard Time)';
it('can select a timezone values and returns canonical value', async () => {
const onTimezoneChange = jest.fn();
render(
<TimezoneSelector
onTimezoneChange={onTimezoneChange}
timezone="America/Nassau"
/>,
);

// selected option ranks first
const options = await getSelectOptions();
expect(options[0]).toHaveTextContent(selectedTimezone);
const searchInput = screen.getByRole('combobox', {
name: 'Timezone selector',
});
expect(searchInput).toBeInTheDocument();
userEvent.click(searchInput);
const isDaylight = moment().isDST();

// others are ranked by offset
expect(options[1]).toHaveTextContent('GMT -11:00 (Pacific/Pago_Pago)');
expect(options[2]).toHaveTextContent('GMT -10:00 (Hawaii Standard Time)');
expect(options[3]).toHaveTextContent('GMT -10:00 (America/Adak)');
const selectedTimezone = isDaylight
? 'GMT -04:00 (Eastern Daylight Time)'
: 'GMT -05:00 (Eastern Standard Time)';

// search for mountain time
await userEvent.type(searchInput, 'mou', { delay: 10 });
// selected option ranks first
const options = await getSelectOptions();
expect(options[0]).toHaveTextContent(selectedTimezone);

const findTitle = isDaylight
? 'GMT -06:00 (Mountain Daylight Time)'
: 'GMT -07:00 (Mountain Standard Time)';
const selectOption = await screen.findByTitle(findTitle);
expect(selectOption).toBeInTheDocument();
userEvent.click(selectOption);
expect(onTimezoneChange).toHaveBeenCalledTimes(1);
expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Cambridge_Bay');
});
// others are ranked by offset
expect(options[1]).toHaveTextContent('GMT -11:00 (Pacific/Pago_Pago)');
expect(options[2]).toHaveTextContent('GMT -10:00 (Hawaii Standard Time)');
expect(options[3]).toHaveTextContent('GMT -10:00 (America/Adak)');

// search for mountain time
await userEvent.type(searchInput, 'mou', { delay: 10 });

it('can update props and rerender with different values', async () => {
const onTimezoneChange = jest.fn();
const { rerender } = render(
<TimezoneSelector
onTimezoneChange={onTimezoneChange}
timezone="Asia/Dubai"
/>,
);
expect(screen.getByTitle('GMT +04:00 (Asia/Dubai)')).toBeInTheDocument();
rerender(
<TimezoneSelector
onTimezoneChange={onTimezoneChange}
timezone="Australia/Perth"
/>,
);
expect(screen.getByTitle('GMT +08:00 (Australia/Perth)')).toBeInTheDocument();
expect(onTimezoneChange).toHaveBeenCalledTimes(0);
const findTitle = isDaylight
? 'GMT -06:00 (Mountain Daylight Time)'
: 'GMT -07:00 (Mountain Standard Time)';
const selectOption = await screen.findByTitle(findTitle);
expect(selectOption).toBeInTheDocument();
userEvent.click(selectOption);
expect(onTimezoneChange).toHaveBeenCalledTimes(1);
expect(onTimezoneChange).toHaveBeenLastCalledWith('America/Cambridge_Bay');
});

it('can update props and rerender with different values', async () => {
const onTimezoneChange = jest.fn();
const { rerender } = render(
<TimezoneSelector
onTimezoneChange={onTimezoneChange}
timezone="Asia/Dubai"
/>,
);
expect(screen.getByTitle('GMT +04:00 (Asia/Dubai)')).toBeInTheDocument();
rerender(
<TimezoneSelector
onTimezoneChange={onTimezoneChange}
timezone="Australia/Perth"
/>,
);
expect(
screen.getByTitle('GMT +08:00 (Australia/Perth)'),
).toBeInTheDocument();
expect(onTimezoneChange).toHaveBeenCalledTimes(0);
});
});
99 changes: 57 additions & 42 deletions superset-frontend/src/components/TimezoneSelector/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import React, { useEffect, useMemo } from 'react';
import React, { useCallback, useEffect, useMemo } from 'react';
import moment from 'moment-timezone';
import { t } from '@superset-ui/core';
import { Select } from 'src/components';
Expand Down Expand Up @@ -45,28 +45,15 @@ const offsetsToName = {
'060': ['GMT Standard Time - London', 'British Summer Time'],
};

const currentDate = moment();
const JANUARY = moment([2021, 1]);
const JULY = moment([2021, 7]);

// january and july are assumed to be reliable differentiators to determine DST.
// There is definitely no way this assumption could come back to bite us :grin:
const getOffsetKey = (name: string) =>
JANUARY.tz(name).utcOffset().toString() +
JULY.tz(name).utcOffset().toString();

const getTimezoneName = (name: string) => {
const offsets = getOffsetKey(name);
return (
(currentDate.tz(name).isDST()
? offsetsToName[offsets]?.[1]
: offsetsToName[offsets]?.[0]) || name
);
};

export interface TimezoneProps {
onTimezoneChange: (value: string) => void;
timezone?: string | null;
}

const ALL_ZONES = moment.tz
.countries()
.map(country => moment.tz.zonesForCountry(country, true))
Expand All @@ -83,33 +70,61 @@ ALL_ZONES.forEach(zone => {
}
});

const TIMEZONE_OPTIONS = TIMEZONES.map(zone => ({
label: `GMT ${moment
.tz(currentDate, zone.name)
.format('Z')} (${getTimezoneName(zone.name)})`,
value: zone.name,
offsets: getOffsetKey(zone.name),
timezoneName: zone.name,
}));

const TIMEZONE_OPTIONS_SORT_COMPARATOR = (
a: typeof TIMEZONE_OPTIONS[number],
b: typeof TIMEZONE_OPTIONS[number],
) =>
moment.tz(currentDate, a.timezoneName).utcOffset() -
moment.tz(currentDate, b.timezoneName).utcOffset();

// pre-sort timezone options by time offset
TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARATOR);

const matchTimezoneToOptions = (timezone: string) =>
TIMEZONE_OPTIONS.find(option => option.offsets === getOffsetKey(timezone))
?.value || DEFAULT_TIMEZONE.value;
export interface TimezoneProps {
onTimezoneChange: (value: string) => void;
timezone?: string | null;
}

interface TimezoneOption {
label: string;
value: string;
offsets: string;
timezoneName: string;
}

const TimezoneSelector = ({ onTimezoneChange, timezone }: TimezoneProps) => {
const currentDate = useMemo(moment, []);
Copy link
Member Author

Choose a reason for hiding this comment

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

currentDate needs to be instantiated in the component instead of at the module root in order to pick up the system time mock. All the rest of the changes in this component stem from that requirement.


const getTimezoneName = useCallback(
(name: string) => {
const offsets = getOffsetKey(name);
return (
(currentDate.tz(name).isDST()
? offsetsToName[offsets]?.[1]
: offsetsToName[offsets]?.[0]) || name
);
},
[currentDate],
);

const sortComparator = useCallback(
(a: TimezoneOption, b: TimezoneOption) =>
moment.tz(currentDate, a.timezoneName).utcOffset() -
moment.tz(currentDate, b.timezoneName).utcOffset(),
[currentDate],
);

const timezoneOptions = useMemo(() => {
const options = TIMEZONES.map(zone => ({
label: `GMT ${moment
.tz(currentDate, zone.name)
.format('Z')} (${getTimezoneName(zone.name)})`,
value: zone.name,
offsets: getOffsetKey(zone.name),
timezoneName: zone.name,
}));
// pre-sort timezone options by time offset
options.sort(sortComparator);
return options;
}, [currentDate, sortComparator]);

const validTimezone = useMemo(
() => matchTimezoneToOptions(timezone || moment.tz.guess()),
[timezone],
() =>
timezoneOptions.find(
option =>
option.offsets === getOffsetKey(timezone || moment.tz.guess()),
)?.value || DEFAULT_TIMEZONE.value,
[timezoneOptions, timezone],
);

// force trigger a timezone update if provided `timezone` is not invalid
Expand All @@ -125,8 +140,8 @@ const TimezoneSelector = ({ onTimezoneChange, timezone }: TimezoneProps) => {
css={{ minWidth: MIN_SELECT_WIDTH }} // smallest size for current values
onChange={tz => onTimezoneChange(tz as string)}
value={validTimezone}
options={TIMEZONE_OPTIONS}
sortComparator={TIMEZONE_OPTIONS_SORT_COMPARATOR}
options={timezoneOptions}
sortComparator={sortComparator}
/>
);
};
Expand Down