You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/03/15 01:22:24 UTC
[GitHub] [superset] suddjian opened a new pull request #19148: fix: TimezoneSelector is not reliably testable
suddjian opened a new pull request #19148:
URL: https://github.com/apache/superset/pull/19148
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
When DST switched, this test broke. This new version should be functionally equivalent to the original, but with system time properly mocked and frozen for the duration of the test.
The test has some odd failures that I haven't yet figured out. All I know about them so far is that when I remove the line `await userEvent.type(searchInput, 'mou', { delay: 10 });` they pass. That line seems important, and also it seems like since it passes when the line is removed, the test is not adequately asserting that something actually changed.
Help getting the tests passing would be appreciated.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud edited a comment on pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #19148:
URL: https://github.com/apache/superset/pull/19148#issuecomment-1067484745
I think ideally we'd want to test both DST and non-DST so you'd need to mock both a January and June date.
To fix the tests, all you have to do is simply removing [L85](https://github.com/apache/superset/blob/3a78165d13c309fc946ad52c9470b093fffbeddc/superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx#L85). It was broken because `America/Adak` uses daylight saving time therefore was ranked to a lower position when DST switched. The user input line was broken for you probably because other changes you made in this PR.
I'm not sure we need to change the component code itself. Let me see what I can do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #19148:
URL: https://github.com/apache/superset/pull/19148#discussion_r826514576
##########
File path: superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx
##########
@@ -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', () => {
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian closed pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
suddjian closed pull request #19148:
URL: https://github.com/apache/superset/pull/19148
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19148:
URL: https://github.com/apache/superset/pull/19148#discussion_r827222926
##########
File path: superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx
##########
@@ -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', () => {
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud edited a comment on pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #19148:
URL: https://github.com/apache/superset/pull/19148#issuecomment-1067484745
I think ideally we'd want to test both DST and non-DST so you'd need to mock both a January and June date.
To fix the tests, all you have to do is simply removing [L85](https://github.com/apache/superset/blob/3a78165d13c309fc946ad52c9470b093fffbeddc/superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx#L85). It was broken because `America/Adak` uses daylight saving time therefore was ranked to a lower position when DST switched. The user input line was broken for you probably because the other changes you made.
I'm not sure we need to change the component code itself. Let me see what I can do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19148:
URL: https://github.com/apache/superset/pull/19148#discussion_r826504464
##########
File path: superset-frontend/src/components/TimezoneSelector/index.tsx
##########
@@ -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, []);
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19148:
URL: https://github.com/apache/superset/pull/19148#discussion_r827211081
##########
File path: superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx
##########
@@ -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', () => {
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #19148:
URL: https://github.com/apache/superset/pull/19148#discussion_r827214368
##########
File path: superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx
##########
@@ -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', () => {
Review comment:
You can run `beforeEach` and `afterEach` even without the nesting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #19148:
URL: https://github.com/apache/superset/pull/19148#issuecomment-1067484745
I think ideally we'd want to test both DST and non-DST so you'd need to mock both a January and June date.
To fix the tests, all you have to do is simply removing [L85](https://github.com/apache/superset/blob/3a78165d13c309fc946ad52c9470b093fffbeddc/superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx#L85). It was broken because `America/Adak` uses daylight saving time therefore was ranked to a lower position when timezone switched. The user input line was broken for you probably because the other changes you made.
I'm not sure we need to change the component code itself. Let me see what I can do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #19148: fix: TimezoneSelector is not reliably testable
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19148:
URL: https://github.com/apache/superset/pull/19148#discussion_r827212071
##########
File path: superset-frontend/src/components/TimezoneSelector/TimezoneSelector.test.tsx
##########
@@ -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', () => {
+ beforeAll(() => {
+ jest.useFakeTimers('modern');
+ jest.setSystemTime(new Date('January 10 2022'));
Review comment:
This sets/freezes the system time so that tests will be run under the same conditions every time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org