Skip to content

[code-infra] Split date-picker test files #16825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 6, 2025

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Mar 4, 2025

These are improvements made during the vitest PR

I'm moving the changes out so that PR is leaner.

  • Split describes.**.test.tsx from x-date-pickers* packages into smaller, faster files.
    • The main issue is that these files had a lot of tests to run, which made them extremely slow, where some files could take up to 50s to finish.
    • divided into:
      • describeConformance.*
      • describeValue.*
      • describeValidation.*
    • Some exceptions like describeValueMultiInput.DesktopDateRangePicker.test.tsx and describeValueSingleInput.DesktopDateRangePicker.test.tsx had to be split into even smaller files.
  • Smaller changes like removing unnecessary clock in describeConformance tests

Ofc, suggestions on how to name the files are welcome, I went with the function names to be easier to understand. :)

@JCQuintas JCQuintas self-assigned this Mar 4, 2025
@mui-bot
Copy link

mui-bot commented Mar 5, 2025

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

Generated by 🚫 dangerJS against e8ed4f7

@JCQuintas JCQuintas changed the title Vitest file split [code-infra] Split date-picker test files Mar 5, 2025
@JCQuintas JCQuintas requested review from flaviendelangle, LukasTy, noraleonte and a team March 5, 2025 11:16
@JCQuintas JCQuintas added the type: enhancement This is not a bug, nor a new feature label Mar 5, 2025
@JCQuintas JCQuintas marked this pull request as ready for review March 5, 2025 11:18
@flaviendelangle
Copy link
Member

Could you describe why splitting those files improve the tests?
Is it because we run more tests in parallel and those files contain so many tests that they end up being a bottleneck?

@JCQuintas
Copy link
Member Author

Could you describe why splitting those files improve the tests? Is it because we run more tests in parallel and those files contain so many tests that they end up being a bottleneck?

One of the main issues with vitest we have in CI is that it gets "stuck" at some steps. It was caused by both OOM or CPU-bottlenecks.

While disabling file parallelisation in CI solves most of the issues, the date-pickers package would still get stuck in the same couple files from time to time.

Mostly:

  • describes.DesktopDateTimeRangePicker.test.tsx
  • describes.MobileDateTimePicker.test.tsx
  • describes.DesktopDateTimePicker.test.tsx
  • describes.DesktopDateTimeRangePicker.test.tsx
  • describes.MobileDateRangePicker.test.tsx

And probably more.

I could have fixed those files specifically, but decided to implement a more systematic approach so it is harder for it to happen in the future.

@flaviendelangle
Copy link
Member

OK
If it improves CI reliability, I'm all in favor of doing the split.
In term of pure DX, I don't think the split helps, but it's not the main argument here clearly 👍

@JCQuintas
Copy link
Member Author

OK If it improves CI reliability, I'm all in favor of doing the split. In term of pure DX, I don't think the split helps, but it's not the main argument here clearly 👍

Indeed, I went with a filename matches function pattern to be clearer. Since when replicating these tests in the future you can leverage what is already there. (read: copy/paste) And replace what is useful for you.

In the end these test files were just running other functions, which themselves run a lot of other functions/files, so most meaningful interactions would be in different files anyways.

It is not as simple as before, but it should be quite simple to replicate in the future 😅

@JCQuintas JCQuintas merged commit 7c80083 into mui:master Mar 6, 2025
18 checks passed
@JCQuintas JCQuintas deleted the date-pickers-file-division branch March 6, 2025 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test type: enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants