Skip to content

[code-infra] Remove required checkout step #17729

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 3 commits into from
May 7, 2025

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented May 6, 2025

Remove requires: - checkout for CircleCI steps.

Since mui/material-ui#40670, we no longer rely on the checkout step to cache the npm dependencies installation, it turns out that it's faster to start from scratch. So this checkout step doesn't speed up the CI, and only blocks running the cases, we forget to dedupe the installs. So it's slower. This should trim ~2 minutes from the overall wait time for the tests to run, since all tests wait for this to finish.

@JCQuintas JCQuintas self-assigned this May 6, 2025
@JCQuintas JCQuintas added type: enhancement This is not a bug, nor a new feature scope: code-infra labels May 6, 2025
@mui-bot
Copy link

mui-bot commented May 6, 2025

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

Generated by 🚫 dangerJS against e785076

@oliviertassinari
Copy link
Member

oliviertassinari commented May 6, 2025

It doesn't speed up the tests, and only blocks running the cases we forget to dedupe the installs.

Oh, because of mui/material-ui#40670. Is this still true with the latest pnpm version? If yes, then all the repositories we have do this wrong and should be updated. A great way to make the CI at least 60s faster for everyone in the org.

@LukasTy
Copy link
Member

LukasTy commented May 6, 2025

Oh, because of mui/material-ui#40670. Is this still true? If yes, then all the repositories we have do this wrong and should be patched 😆

Yes, we still don't cache the pnpm store on CircleCI.
Are you suggesting bringing back the caching? Or just confirming to move forward with this change for all repos? 🤔

@oliviertassinari
Copy link
Member

oliviertassinari commented May 6, 2025

Are you suggesting bringing back the caching?

I wonder if it's still an up-to-date decision. But yeah, ok, it seems like it, e.g. vercel/next.js#75600 no pnpm caching either for Next.js.

@JCQuintas JCQuintas marked this pull request as ready for review May 7, 2025 07:57
@JCQuintas JCQuintas requested a review from a team May 7, 2025 07:57
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

very nice 👌

@JCQuintas JCQuintas merged commit 53fd0e3 into mui:master May 7, 2025
23 checks passed
@JCQuintas JCQuintas deleted the remove-require-checkout branch May 7, 2025 08:15
@Janpot
Copy link
Member

Janpot commented May 7, 2025

I guess the difference will be that now the dedupe check doesn't block running the rest of the tests anymore. perhaps we should just fold this check into the test_static job?

Porting to core: mui/material-ui#46099.

@JCQuintas
Copy link
Member Author

I guess the difference will be that now the dedupe check doesn't block running the rest of the tests anymore. perhaps we should just fold this check into the test_static job?

I think it would make sense

@michaldudak
Copy link
Member

Porting to Base UI: mui/base-ui#1883

oliviertassinari added a commit to mui/mui-public that referenced this pull request May 8, 2025
oliviertassinari added a commit to mui/pigment-css that referenced this pull request Jun 2, 2025
oliviertassinari added a commit to mui/toolpad that referenced this pull request Jun 2, 2025
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.

6 participants