[test] Attempt to fix flaky unit tests #14583
Conversation
|
Deploy preview: https://deploy-preview-14583--material-ui-x.netlify.app/ |
docs/next-env.d.ts
Outdated
| @@ -2,4 +2,4 @@ | |||
| /// <reference types="next/image-types/global" /> | |||
|
|
|||
| // NOTE: This file should not be edited | |||
| // see https://nextjs.org/docs/basic-features/typescript for more information. | |||
| // see https://nextjs.org/docs/pages/building-your-application/configuring/typescript for more information. | |||
There was a problem hiding this comment.
This is generated after running pnpm start
| @@ -86,6 +86,10 @@ describe('<DataGridPremium /> - Row grouping', () => { | |||
| describe('Setting grouping criteria', () => { | |||
| clock.withFakeTimers(); | |||
|
|
|||
| after(() => { | |||
| clock.restore(); | |||
There was a problem hiding this comment.
We seem to be calling it after each test already: https://github.com/mui/material-ui/blob/bddf126c978ae50dd67ffe0ff143dbb1e42eec3d/packages-internal/test-utils/src/createRenderer.tsx#L392
I'm OK with calling it in the after hook as well if it's less flaky with that.
But how about doing that in the withFakeTimers function? https://github.com/mui/material-ui/blob/bddf126c978ae50dd67ffe0ff143dbb1e42eec3d/packages-internal/test-utils/src/createRenderer.tsx#L430-L438
There was a problem hiding this comment.
I've tested that assumption in another PR and can conclude that the after block does not seem to get called. 🤔
I will test it more locally with the actual package, but adding a console.info in the after block does not log anything when running a test having a describe with withFakeTimers.
There was a problem hiding this comment.
There was a problem hiding this comment.
This is weird indeed.
Can you reproduce the flakiness locally? If so, please share some details, and I can look into it.
There was a problem hiding this comment.
I wasn't able to ever reproduce those flaky tests locally. 🙈
And I also reproduced flaky tests with on this PR. 💡
There was a problem hiding this comment.
This agenda didn't produce any stable results.
I'm exploring a more long-term/stable solution. 😉
|
Btw, while working on #14508 I noticed there are two instances that makes the tests somewhat unstable, these |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
I suspect that the fake clock configuration might be leaking outside the intended scope and producing flaky unit tests (i.e. 1, 2, 3).
This PR aims to ensure that the timers mocking is reset after each describe block.After more testing I concluded that manual clock restoring doesn't help at all as everything is already done.
I'm exploring the next best thing—avoid faking clock whenever possible and where needed, ensure that only
Dateis mocked to prevent running into cases where faked timeouts are bleeding into an async test.The current solution in a nutshell:
@testing-library/user-eventto Pickers and test packages dependencies to avoid the need of passingclickandkeyboardfunction down to utility functionsBut we are already bringing in this dependency in
test/performance-chartspackage... 🤷clock: 'fake'whenever possibleuserEvent.clickanduserEvent.keyboardinstead of the syncfireEventutility