-
Notifications
You must be signed in to change notification settings - Fork 57
Migrate jest tets to vitest #2396
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
base: trunk
Are you sure you want to change the base?
Conversation
|
I took a quick look at Buildkite to compare unit test times between this PR and #2465. Looks like migrating to Vitest basically cut them in half 🤩
|
📊 Performance Test ResultsComparing 6e64da8 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
bcotrim
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.
Amazing work! @katinthehatsite
All 1,138 tests pass without any errors.
A couple of notes:
- Some logs from Studio CLI are polluting the test outputs. We could consider mocking ora globally in vitest.setup.ts or adding it to the alias config.
- There are still some references to "Jest" in some files. For good measure we should probably clean these up:
- CLAUDE.md:47
- docs/ai-instructions.md
- packages/eslint-plugin-studio/ - still uses Jest (could be a follow-up)
|
Thanks for taking time to do the review @bcotrim ! All great suggestions, I will take a look at them today 👀 |
|
@bcotrim I made some further changes based on your suggestions, let me know what you think. |
|
I forgot to mention:
Let's do this as a follow-up because there are lots of changes in this PR already 😓 I will open an issue for this |
|
Created a follow up issue https://linear.app/a8c/issue/STU-1268/migrate-packageseslint-plugin-studio-to-vitest |
bcotrim
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.
Thanks for addressing the changes and creating the follow-up issue @katinthehatsite!
Sorry for being picky, but there are still some warning logs during test execution. Could we address those before merging?
No worries at all, looking 👀 |
|
I made some further updates, let me know what you think! |
Related issues
Fixes STU-1207
Proposed Changes
This PR migrates from
jesttovitest.Testing Instructions
vitestconfig and setup files and feel free to add suggestionsNote: I initially wanted to split the
setupfiles fornodeanddombut after doing that, I noticed that they had quite a bit in common for now so decided to leave as is. We can also do the split if we think it is necessary.In some cases, it was tricky to mock things so the mocks might be longer, especially since
vitestis quite strict with enforcing types.Pre-merge Checklist