fix: TimezoneSelector is not reliably testable #19148
Conversation
|
|
||
| const TimezoneSelector = ({ onTimezoneChange, timezone }: TimezoneProps) => { | ||
| const currentDate = useMemo(moment, []); |
There was a problem hiding this 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.
| expect(onTimezoneChange).toHaveBeenCalledTimes(1); | ||
| expect(onTimezoneChange).toHaveBeenCalledWith('America/Nassau'); | ||
| }); | ||
| describe('TimezoneSelector', () => { |
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this comment.
You can run beforeEach and afterEach even without the nesting.
There was a problem hiding this 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.
|
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. It was broken because I'm not sure we need to change the component code itself. Let me see what I can do. |
| jest.useFakeTimers('modern'); | ||
| jest.setSystemTime(new Date('January 10 2022')); |
There was a problem hiding this comment.
This sets/freezes the system time so that tests will be run under the same conditions every time.
SUMMARY
When DST switched, this test suite broke. This new version of the component and its tests 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. Since the test passes when the line is removed, I would presume that the test is also not adequately asserting that something actually changed after the user input.Help getting the tests passing would be appreciated.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION