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

refactor: Moving from jest to vitest #9035

Merged
merged 18 commits into from
May 25, 2023
Merged

refactor: Moving from jest to vitest #9035

merged 18 commits into from
May 25, 2023

Conversation

leog
Copy link
Contributor

@leog leog commented May 22, 2023

What does this PR do?

Moving unit testing framework from Jest to Vitest in order to speed things up.

Going from this:

To this:

Marking a 85.3% decrease in time, even running more tests than Jest.

Type of change

  • Chore (refactoring code, technical debt, workflow improvements)

@leog leog requested a review from hariombalhara as a code owner May 22, 2023 13:11
@leog leog requested a review from a team May 22, 2023 13:11
@vercel
Copy link

vercel bot commented May 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
cal ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 24, 2023 11:15pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
ui ⬜️ Ignored (Inspect) Visit Preview May 24, 2023 11:15pm

@socket-security
Copy link

socket-security bot commented May 22, 2023

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
[email protected] None +8 domenic
[email protected] None +1 eratio
[email protected] None +2 simenb

🚮 Removed packages: @types/[email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]

@github-actions
Copy link
Contributor

github-actions bot commented May 22, 2023

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@leog leog added the ♻️ autoupdate tells kodiak to keep this branch up-to-date label May 22, 2023
@deploysentinel
Copy link

deploysentinel bot commented May 22, 2023

Current Playwright Test Results Summary

✅ 114 Passing - ⚠️ 6 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 05/24/2023 11:34:59pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 27ef504

Started: 05/24/2023 11:20:58pm UTC

⚠️ Flakes

📄   apps/web/playwright/reschedule.e2e.ts • 2 Flakes

Top 1 Common Error Messages

null

2 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Reschedule Tests -- new-booker Should do a booking request reschedule from /bookings
Retry 1Initial Attempt
0.77% (3) 3 / 388 runs
failed over last 7 days
1.80% (7) 7 / 388 runs
flaked over last 7 days
Reschedule Tests -- old-booker Opt in event should be ACCEPTED when rescheduled by OWNER
Retry 1Initial Attempt
0% (0) 0 / 375 runs
failed over last 7 days
2.40% (9) 9 / 375 runs
flaked over last 7 days

📄   apps/web/playwright/event-types.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Event Types tests user -- old-booker can duplicate an existing event type
Retry 1Initial Attempt
0% (0) 0 / 394 runs
failed over last 7 days
1.27% (5) 5 / 394 runs
flaked over last 7 days

📄   apps/web/playwright/embed-code-generator.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Embed Code Generator Tests Event Type Edit Page open Embed Dialog for the Event Type
Retry 1Initial Attempt
3.27% (13) 13 / 397 runs
failed over last 7 days
29.97% (119) 119 / 397 runs
flaked over last 7 days

📄   apps/web/playwright/booking-seats.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Booking with Seats -- new-booker Reschedule for booking with seats -- old-booker Should reschedule booking with seats and if everyone rescheduled it should be deleted
Retry 1Initial Attempt
0% (0) 0 / 406 runs
failed over last 7 days
85.47% (347) 347 / 406 runs
flaked over last 7 days

📄   packages/embeds/embed-core/playwright/tests/action-based.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Popup Tests should be able to reschedule
Retry 2Retry 1Initial Attempt
9.09% (1) 1 / 11 run
failed over last 7 days
36.36% (4) 4 / 11 runs
flaked over last 7 days

View Detailed Build Results


@zomars
Copy link
Member

zomars commented May 22, 2023

I'm loving this! Nice job @leog

Just for completeness sake. What are the drawbacks of abandoning jest in our codebase?

@@ -7,6 +7,7 @@ public

*.lock
*.log
*.test.ts
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some tests need to import prisma before the actual code using prisma gets run, and Prettier was reorganizing the imports everywhere breaking this. I added this to avoid prettier reorganizing imports in tests.

});

it("should filter cities for a valid city name", () => {
expect(filterByCities("San Francisco", cityData)).toMatchInlineSnapshot(`
Array [
Copy link
Member

Choose a reason for hiding this comment

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

I had always thought why it was needed. Now, vitest removing it ❤️

@PeerRich PeerRich removed this pull request from the merge queue due to a manual request May 23, 2023
@PeerRich
Copy link
Member

PeerRich commented May 23, 2023

happy to merge once open questions were answered. changes can be done in a follow up PR

@leog
Copy link
Contributor Author

leog commented May 23, 2023

Just for completeness sake. What are the drawbacks of abandoning jest in our codebase?

@zomars There is no apparent drawback other than immersing ourselves into a new framework which can lead to unknowns although Vitest is being used more and more these days. Not to mention Vitest has been designed with a Jest compatible API. More info about migration and similarities/differences here.

All in all, I think we should give it a chance, the time difference is huge, and locally is ludicrous, see:
image

@PeerRich PeerRich changed the title Moving from jest to vitest refactor: Moving from jest to vitest May 24, 2023
@emrysal emrysal added this pull request to the merge queue May 24, 2023
Merged via the queue into main with commit 734382b May 25, 2023
@emrysal emrysal deleted the chore/vitest-migration branch May 25, 2023 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♻️ autoupdate tells kodiak to keep this branch up-to-date
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants