[code-infra] Stabilize fake timers in regression tests #19719
Conversation
Janpot
added
type: bug
|
Deploy preview: https://deploy-preview-19719--material-ui-x.netlify.app/ Bundle size report
|
|
The flaky visual test seems to return the seconds as 53 and sometimes as 54. Why do you think it's related to Could it be some fake timer (or the lack of a fake timer) and a test that runs more slowly in some cases, thus resulting in a different second? |
The exact timestamp that is being visualized is |
Why not just change the ts? |
Could we update the timestamp to have 0 milliseconds instead of 500? |
|
That's what I did in my last commit. It's a draft PR, I'm still researching this. I'm still not sure that's what's at play here, it suspect |
Janpot
changed the title
.toLocaleString
| setPrevShouldAdvanceTime(props.shouldAdvanceTime); | ||
| } | ||
|
|
||
| return ready ? props.children : null; |
There was a problem hiding this comment.
Little side-quest in some weird code I ran into. It's problematic when shouldAdvanceTime prop changes while ready is true it would render the children once with the wrong fake clock. We can get rid of the effect and instead set up the clock straight in the render.
test/regressions/index.tsx
Outdated
| import { createBrowserRouter, RouterProvider, Outlet, NavLink, useNavigate } from 'react-router'; | ||
| import { Globals } from '@react-spring/web'; | ||
| import { setupFakeClock, restoreFakeClock } from '../utils/setupFakeClock'; // eslint-disable-line | ||
| import { restoreFakeClock } from '../utils/setupFakeClock'; // eslint-disable-line |
There was a problem hiding this comment.
So we just need to import the file to set up the fake clock, right?
| import { restoreFakeClock } from '../utils/setupFakeClock'; // eslint-disable-line | |
| import '../utils/setupFakeClock'; // eslint-disable-line |
There was a problem hiding this comment.
The lint disable rule was hiding this. Updated as well
| const [dispose, setDispose] = React.useState<(() => void) | null>(null); | ||
| const [prevShouldAdvanceTime, setPrevShouldAdvanceTime] = React.useState(props.shouldAdvanceTime); |
There was a problem hiding this comment.
Do we need state for these? Wouldn't refs work?
There was a problem hiding this comment.
As per https://react.dev/reference/react/useRef#caveats, I don't believe a ref would be safe under concurrent rendering.
Do not write or read
ref.currentduring rendering, except for initialization. This makes your component’s behavior unpredictable.
Dynamically importing the test suites causes fake timers to advance in non-deterministic way. Instead we can just import them statically,
setupFakeClockis already being called when its module loads, so no need to wrap an async import with it.Also noticed this weird component that attempts to set up fake timers in an effect, but would absolutely not work as intended as soon as
readyistrue. Don't understand what it's for really, but I corrected the implementation at the local level, it's now correctly updating the fake clock whenevershouldAdvanceTimechanges.I'm also including a optimizedeps plugin that fixes the jsx in .js files. True vite dev mode for this app still doesn't work due to CJS/ESM issues with the markdown loader. Still to investigate further, but already including this step.
The screenshot is changing to ...:53, this is the correct value. The one on master is from when timers advanced a bit in unpredictable way.