-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
[code-infra] Vitest test migration #44325
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
Conversation
Janpot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're good
JCQuintas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve my own PR, so do as you wish, left comments, mostly for clarification, but everything looks good 👍
Thanks for the time put on this 😆
| # macOS-latest has 3 CPUs, but we get "EMFILE: too many open files" errors with that parallelism | ||
| # Limit Next.js to 2 CPUs to prevent file descriptor exhaustion. Empty string uses os.availableParallelism() | ||
| NEXT_PARALLELISM: ${{ runner.os == 'macOS' && '2' || '' }} | ||
| GITHUB_AUTH: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need these now? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR, but the docs build sometimes flakes without this token due to rate limiting in the github API when not authenticated. Was hitting this flakeyness while working on this PR
| } else { | ||
| id = this.currentTest?.fullTitle() ?? null; | ||
| } | ||
| const id = expect.getState().currentTestName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to remove the isVitest func from this file now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, was also planning a bigger cleanup and harmonization with X after this is merged
| function describeSkipIf(condition: boolean) { | ||
| return condition ? describe.skip : describe; | ||
| }; | ||
| const describeSkipIf = describe.skipIf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use this on X anymore, should we remove this file altogether?
| if (window.navigator.userAgent.includes('jsdom')) { | ||
| this.skip(); | ||
| } | ||
| it.skipIf(window.navigator.userAgent.includes('jsdom'))( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reflow is so annoying to review 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, best to disable whitespace in github files view
|
|
||
| expect(() => { | ||
| setProps({ checked: true }); | ||
| globalThis.didWarnControlledToUncontrolled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird pattern 😆
packages/mui-utils/tsconfig.json
Outdated
| // Remove once: | ||
| // * vitest is at >= 3.2.0 | ||
| // * https://github.com/capricorn86/happy-dom/issues/1444 is solved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this pr is using 4+ already, should these be removed?
Related to mui/mui-x#14508 and #43625
Help toward mui/mui-public#170
This also contains all the actual test changes to make material-ui repo run on vitest, while #43625 contains only config changes