Skip to content
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

[code-infra] Make tests on React 18 part of pipeline #16933

Merged
merged 2 commits into from
Mar 12, 2025

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Mar 12, 2025

Implement decision discussed here: https://mui-org.slack.com/archives/C011VC970AW/p1741612073853199

Makes test run on React 18 on each pipeline run. This aligns the behavior with base-ui repo.

Note

test_static will be fixed with #16934.

@LukasTy LukasTy added enhancement This is not a bug, nor a new feature scope: infra Org infrastructure work going on behind the scenes labels Mar 12, 2025
@LukasTy LukasTy self-assigned this Mar 12, 2025
@mui-bot
Copy link

mui-bot commented Mar 12, 2025

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

Generated by 🚫 dangerJS against c8d9963

@LukasTy LukasTy requested review from a team March 12, 2025 10:40
Copy link
Member

@JCQuintas JCQuintas left a comment

Choose a reason for hiding this comment

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

Should we make the react 18 pipelines required?

@LukasTy
Copy link
Member Author

LukasTy commented Mar 12, 2025

Should we make the react 18 pipelines required?

Agreed. 👍
I take it the configuration is on GH settings that almost no one has access to? 😃

@JCQuintas
Copy link
Member

Should we make the react 18 pipelines required?

Agreed. 👍 I take it the configuration is on GH settings that almost no one has access to? 😃

yes

@LukasTy
Copy link
Member Author

LukasTy commented Mar 12, 2025

Should we make the react 18 pipelines required?

@DanailH, maybe you have access to adjust these settings? 🤔

@LukasTy LukasTy enabled auto-merge (squash) March 12, 2025 11:25
@LukasTy LukasTy merged commit b90db3f into mui:master Mar 12, 2025
20 checks passed
@LukasTy LukasTy deleted the make-react-18-tests-part-of-pipeline branch March 12, 2025 11:38
Copy link

Cherry-pick PRs will be created targeting branches: v7.x

LukasTy added a commit to LukasTy/mui-x that referenced this pull request Mar 14, 2025
@oliviertassinari oliviertassinari added scope: code-infra Specific to the core-infra product and removed scope: infra Org infrastructure work going on behind the scenes labels Mar 15, 2025
@oliviertassinari oliviertassinari changed the title [infra] Make tests on React 18 part of pipeline [code-infra] Make tests on React 18 part of pipeline Mar 15, 2025
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 15, 2025

👍 I have almost never seen CI checks that only runs on the default branch work in practice.

I guess covering React v17 is needed but would be overkill, so not pragmatic, makes sense to skip it:

SCR-20250315-podp

https://tools-public.mui.com/prod/pages/npmVersion?package=react-dom.


I take it the configuration is on GH settings that almost no one has access to?

This is configured in https://github.com/mui/mui-x/settings/rules, per https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization

SCR-20250315-pqwm

we need Admin. So yeah, only 5 people can do this: https://github.com/orgs/mui/teams/company-admin/members.

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Mar 15, 2025
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 23, 2025

@LukasTy It seems that this creates a problem with Argos CI. We generate screenshots for React 18 and React 19, for example https://app.argos-ci.com/mui/mui-x/builds/33556/147652868 and https://app.argos-ci.com/mui/mui-x/builds/33555/147652013 but it only reports the latest build in the PR #17104.

Now, if the CSS is wrong, React 19 tests should catch it for React 18 too. If the behavior changes for React 18, unit tests should catch. So I struggle to see a case where those React 18 visual regression tests adds coverage. Maybe we can simply remove React 18 screenshots? Otherwise, I imagine that the solution is https://argos-ci.com/docs/build-splitting#practical-application.

@LukasTy
Copy link
Member Author

LukasTy commented Mar 24, 2025

@oliviertassinari Thanks for noting it. 👍
Indeed, there is little to no need to always run regressions on React 18 as well.
I've raised a PR to remove that step: #17108.

However, I've kept the regressions step when the additional_tests pipeline is manually triggered. 😉

mui-x/.circleci/config.yml

Lines 390 to 393 in a04c849

- test_regressions:
<<: *default-context
name: test_regressions_additional
react-version: << pipeline.parameters.with-react-version >>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes enhancement This is not a bug, nor a new feature scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants