Skip to content

[test] Refactor most Pickers test to async#14583

Draft
LukasTy wants to merge 25 commits intomui:masterfrom
LukasTy:attempt-fix-flaky-unit-tests
Draft

[test] Refactor most Pickers test to async#14583
LukasTy wants to merge 25 commits intomui:masterfrom
LukasTy:attempt-fix-flaky-unit-tests

Conversation

@LukasTy
Copy link
Member

@LukasTy LukasTy commented Sep 11, 2024

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 Date is mocked to prevent running into cases where faked timeouts are bleeding into an async test.

The current solution in a nutshell:

  • Add @testing-library/user-event to Pickers and test packages dependencies to avoid the need of passing click and keyboard function down to utility functions
    • if you think that more verbose tests are a good compromise to avoid this dependency, I can refactor it.
      But we are already bringing in this dependency in test/performance-charts package... 🤷
  • Remove clock: 'fake' whenever possible
  • Use userEvent.click and userEvent.keyboard instead of the sync fireEvent utility

@LukasTy LukasTy added test internal Behind-the-scenes enhancement. Formerly called “core”. labels Sep 11, 2024
@LukasTy LukasTy requested a review from a team September 11, 2024 14:01
@LukasTy LukasTy self-assigned this Sep 11, 2024
@mui-bot
Copy link

mui-bot commented Sep 11, 2024

Deploy preview: https://deploy-preview-14583--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 8e7281f

@LukasTy LukasTy removed the request for review from a team September 11, 2024 14:21
@LukasTy LukasTy marked this pull request as draft September 11, 2024 14:23

// 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.
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 is generated after running pnpm start

@LukasTy LukasTy marked this pull request as ready for review September 11, 2024 20:17
@LukasTy LukasTy requested a review from a team September 11, 2024 20:17
@JCQuintas
Copy link
Member

Btw, while working on #14508 I noticed there are two instances that makes the tests somewhat unstable, these clocks, and anything that changes moment.locale

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

I don't understand how this works, but I don't want to block this PR if it reduces the flakiness 😅

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 19, 2024
@cherniavskii cherniavskii mentioned this pull request Sep 19, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 19, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 30, 2024
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 9, 2024
@LukasTy LukasTy marked this pull request as draft October 11, 2024 15:29
@LukasTy
Copy link
Member Author

LukasTy commented Oct 11, 2024

I'm parking this PR until we move away from mocha. (#14508)
it appears to be the main problem in ensuring the test stability.
The configuration seems to be leaking outside to other tests and causing unexpected test failures. 🤷


Hopefully, #14927 can reduce the existing test:unit pipeline flakiness in the meantime. 🤞

@LukasTy LukasTy changed the title [test] Attempt to fix flaky unit tests [test] Refactor most Pickers test to async Oct 14, 2024
@LukasTy LukasTy added scope: pickers Changes related to the date/time pickers. and removed test labels Oct 14, 2024
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 16, 2024
@LukasTy LukasTy added the on hold There is a blocker, we need to wait. label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Behind-the-scenes enhancement. Formerly called “core”. on hold There is a blocker, we need to wait. PR: out-of-date The pull request has merge conflicts and can't be merged. scope: pickers Changes related to the date/time pickers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants