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.

Sign up for GitHub

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

Jump to bottom

Vitest switch #4687

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

Vitest switch #4687

wants to merge 51 commits into from

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Feb 18, 2025 •
edited
Loading

Requirements before merge

  • Remove (done) tests
  • Ensure we have coverage setup
  • Check how we differ from the existing karma setup
  • Look into potential duplicate bundle issue (options.hook not called or mangle not applied)
  • Transform options property names in tests with regards to mangle
  • Fix size and performance diff
  • Figure out why tests are running against dist rather than src
  • Fix types in test files
  • Fix TODO comments in tests
  • Get coverage working, looks like it's not reported
  • Make pnpm test:vitest:min work, this somehow makes two tests fail
  • Get tsc to work again... Broke with bump of types/node, this is part of pnpm lint
  • Fix npm run test:mocha
  • Use playwright
  • Restore CI to use npm run lint && npm run test:unit

Follow-ups

  • Get rid of sinon
  • Get rid of sinon-chai

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 18, 2025 •
edited
Loading

📊 Tachometer Benchmark Results

Summary

duration

  • create10k: unsure 🔍 -0% - +0% (-4.02ms - +4.10ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -0% - +0% (-0.04ms - +0.03ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -2% - +2% (-1.42ms - +1.31ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -1% - +0% (-0.09ms - +0.05ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -2% - +1% (-1.26ms - +0.77ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -1% - +5% (-0.03ms - +0.11ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -1% - +1% (-0.39ms - +0.39ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -1% - +5% (-0.15ms - +1.42ms)
    preact-local vs preact-main

usedJSHeapSize

  • create10k: unsure 🔍 -0% - +0% (-0.01ms - +0.02ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -0% - +4% (-0.00ms - +0.05ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -1% - +7% (-0.10ms - +0.69ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -1% - +1% (-0.02ms - +0.02ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -1% - +0% (-0.02ms - +0.01ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -1% - +3% (-0.01ms - +0.04ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
    preact-local vs preact-main

Results

create10k

duration

VersionAvg timevs preact-localvs preact-main
preact-local922.50ms - 928.80ms-unsure 🔍
-0% - +0%
-4.02ms - +4.10ms
preact-main923.05ms - 928.17msunsure 🔍
-0% - +0%
-4.10ms - +4.02ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local19.18ms - 19.20ms-unsure 🔍
-0% - +0%
-0.01ms - +0.02ms
preact-main19.18ms - 19.19msunsure 🔍
-0% - +0%
-0.02ms - +0.01ms		-
filter-list

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.55ms - 16.61ms-unsure 🔍
-0% - +0%
-0.04ms - +0.03ms
preact-main16.56ms - 16.60msunsure 🔍
-0% - +0%
-0.03ms - +0.04ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.56ms - 1.61ms-unsure 🔍
-0% - +4%
-0.00ms - +0.05ms
preact-main1.54ms - 1.58msunsure 🔍
-3% - +0%
-0.05ms - +0.00ms		-
hydrate1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local78.15ms - 80.56ms-unsure 🔍
-2% - +2%
-1.42ms - +1.31ms
preact-main78.78ms - 80.05msunsure 🔍
-2% - +2%
-1.31ms - +1.42ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local9.54ms - 10.11ms-unsure 🔍
-1% - +7%
-0.10ms - +0.69ms
preact-main9.26ms - 9.80msunsure 🔍
-7% - +1%
-0.69ms - +0.10ms		-
many-updates

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.47ms - 16.60ms-unsure 🔍
-1% - +0%
-0.09ms - +0.05ms
preact-main16.52ms - 16.59msunsure 🔍
-0% - +1%
-0.05ms - +0.09ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.77ms - 3.79ms-unsure 🔍
-1% - +1%
-0.02ms - +0.02ms
preact-main3.77ms - 3.79msunsure 🔍
-1% - +1%
-0.02ms - +0.02ms		-
replace1k
  • Browser: chrome-headless
  • Sample size: 100
  • Built by: CI #4554

  • Commit: 5dc412d

duration

VersionAvg timevs preact-localvs preact-main
preact-local65.14ms - 66.23ms-unsure 🔍
-2% - +1%
-1.26ms - +0.77ms
preact-main65.07ms - 66.79msunsure 🔍
-1% - +2%
-0.77ms - +1.26ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local2.98ms - 2.99ms-unsure 🔍
-1% - +0%
-0.02ms - +0.01ms
preact-main2.98ms - 3.00msunsure 🔍
-0% - +1%
-0.01ms - +0.02ms		-

run-warmup-0

VersionAvg timevs preact-localvs preact-main
preact-local29.82ms - 30.62ms-unsure 🔍
-1% - +2%
-0.25ms - +0.69ms
preact-main29.75ms - 30.24msunsure 🔍
-2% - +1%
-0.69ms - +0.25ms		-

run-warmup-1

VersionAvg timevs preact-localvs preact-main
preact-local33.68ms - 34.65ms-unsure 🔍
-4% - +0%
-1.31ms - +0.15ms
preact-main34.19ms - 35.29msunsure 🔍
-0% - +4%
-0.15ms - +1.31ms		-

run-warmup-2

VersionAvg timevs preact-localvs preact-main
preact-local25.25ms - 25.81ms-unsure 🔍
-1% - +2%
-0.17ms - +0.46ms
preact-main25.23ms - 25.54msunsure 🔍
-2% - +1%
-0.46ms - +0.17ms		-

run-warmup-3

VersionAvg timevs preact-localvs preact-main
preact-local26.35ms - 26.96ms-unsure 🔍
-2% - +1%
-0.58ms - +0.31ms
preact-main26.47ms - 27.11msunsure 🔍
-1% - +2%
-0.31ms - +0.58ms		-

run-warmup-4

VersionAvg timevs preact-localvs preact-main
preact-local27.45ms - 28.12ms-unsure 🔍
-0% - +3%
-0.07ms - +0.79ms
preact-main27.15ms - 27.70msunsure 🔍
-3% - +0%
-0.79ms - +0.07ms		-

run-final

VersionAvg timevs preact-localvs preact-main
preact-local20.02ms - 20.39ms-unsure 🔍
-1% - +2%
-0.26ms - +0.39ms
preact-main19.88ms - 20.40msunsure 🔍
-2% - +1%
-0.39ms - +0.26ms		-
text-update
  • Browser: chrome-headless
  • Sample size: 230
  • Built by: CI #4554

  • Commit: 5dc412d

duration

VersionAvg timevs preact-localvs preact-main
preact-local2.04ms - 2.15ms-unsure 🔍
-1% - +5%
-0.03ms - +0.11ms
preact-main2.02ms - 2.10msunsure 🔍
-5% - +1%
-0.11ms - +0.03ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.12ms - 1.12ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main1.12ms - 1.12msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-
todo

duration

VersionAvg timevs preact-localvs preact-main
preact-local34.74ms - 35.19ms-unsure 🔍
-1% - +1%
-0.39ms - +0.39ms
preact-main34.65ms - 35.28msunsure 🔍
-1% - +1%
-0.39ms - +0.39ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.24ms - 1.27ms-unsure 🔍
-1% - +3%
-0.01ms - +0.04ms
preact-main1.23ms - 1.26msunsure 🔍
-3% - +1%
-0.04ms - +0.01ms		-
update10th1k
  • Browser: chrome-headless
  • Sample size: 120
  • Built by: CI #4554

  • Commit: 5dc412d

duration

VersionAvg timevs preact-localvs preact-main
preact-local30.21ms - 31.32ms-unsure 🔍
-1% - +5%
-0.15ms - +1.42ms
preact-main29.57ms - 30.68msunsure 🔍
-5% - +0%
-1.42ms - +0.15ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local2.95ms - 2.96ms-unsure 🔍
-0% - +0%
-0.01ms - +0.01ms
preact-main2.94ms - 2.96msunsure 🔍
-0% - +0%
-0.01ms - +0.01ms		-

tachometer-reporter-action v2 for CI

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 22, 2025 •
edited
Loading

Size Change: -59 B (-0.08%)

Total Size: 78.4 kB

Filename Size Change
debug/dist/debug.js 3.82 kB -14 B (-0.37%)
debug/dist/debug.mjs 3.82 kB -15 B (-0.39%)
debug/dist/debug.module.js 3.82 kB -15 B (-0.39%)
debug/dist/debug.umd.js 3.9 kB -15 B (-0.38%)
ℹ️ View Unchanged
Filename Size
compat/dist/compat.js 4.12 kB
compat/dist/compat.mjs 4.05 kB
compat/dist/compat.module.js 4.05 kB
compat/dist/compat.umd.js 4.19 kB
devtools/dist/devtools.js 260 B
devtools/dist/devtools.mjs 274 B
devtools/dist/devtools.module.js 274 B
devtools/dist/devtools.umd.js 346 B
dist/preact.js 4.74 kB
dist/preact.min.js 4.74 kB
dist/preact.min.module.js 4.76 kB
dist/preact.min.umd.js 4.77 kB
dist/preact.mjs 4.75 kB
dist/preact.module.js 4.75 kB
dist/preact.umd.js 4.77 kB
hooks/dist/hooks.js 1.54 kB
hooks/dist/hooks.mjs 1.57 kB
hooks/dist/hooks.module.js 1.57 kB
hooks/dist/hooks.umd.js 1.61 kB
jsx-runtime/dist/jsxRuntime.js 978 B
jsx-runtime/dist/jsxRuntime.mjs 952 B
jsx-runtime/dist/jsxRuntime.module.js 952 B
jsx-runtime/dist/jsxRuntime.umd.js 1.05 kB
test-utils/dist/testUtils.js 473 B
test-utils/dist/testUtils.mjs 477 B
test-utils/dist/testUtils.module.js 477 B
test-utils/dist/testUtils.umd.js 555 B

compressed-size-action

rschristian
rschristian reviewed Feb 22, 2025
View reviewed changes
@JoviDeCroock JoviDeCroock force-pushed the switch-to-vitest branch 2 times, most recently from da6d806 to 2bfe8b0 Compare February 23, 2025 08:43
@coveralls
Copy link

coveralls commented Feb 23, 2025 •
edited
Loading

Coverage Status

coverage: 99.523% (-0.09%) from 99.61%
when pulling 41d9a27 on switch-to-vitest
into ccd1e71 on main.

@JoviDeCroock JoviDeCroock marked this pull request as ready for review February 24, 2025 09:18
@JoviDeCroock JoviDeCroock force-pushed the switch-to-vitest branch 2 times, most recently from 6f8b8ef to 0a4c379 Compare February 24, 2025 09:21
rschristian
rschristian approved these changes Feb 24, 2025
View reviewed changes
Copy link
Member

@rschristian rschristian left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Lot of work went into this, great job!

However, I'm not in love with merging this as-is, entirely due to the state of Vitest, not any of the work here.

  • It seems slower across the board
  • It's an extra 100MB+ of node_modules & an 800MB in ~/.cache/ms-playright (tried using local Chrome install but I think that'd require different config/setup, it doesn't seem to function properly out-of-the-box)
  • It's a hell of a lot more dependencies
  • We've had to switch to some Vitest-specific APIs. While the "lock-in" doesn't look to be too rough, we are switching from runner agnostic (or meant to be runner agnostic) tools to something that's specific & will need to be changed if we need to migrate again in the future
  • We've encountered a fair number of Vitest bugs in the process of this PR

I suppose most importantly, I don't see a clear benefit to switching at the moment? It's something newer and that has some value, and we do shave a bit of config, but browser testing seems pretty well established, as are our tests, so I don't see any obvious possibilities this opens up. Karma, while deprecated, is super stable & doesn't seem to be holding us back at all.

I know Vitest's maintainers have been hard at work improving things, and with a bit of luck some of this will be addressed soon-ish, but at this particular moment it seems like a rather large regression instead of a clear improvement.

That being said, I don't want to hold this back at all; if people want this then let's go forward with it.

@JoviDeCroock
Copy link
Member Author

It seems slower across the board and It's an extra 100MB+ of node_modules & an 800MB in ~/.cache/ms-playright (tried using local Chrome install but I think that'd require different config/setup, it doesn't seem to function properly out-of-the-box)

I agree, merging this feels like a regression, all though I am convinced that we should eventually move to a new tool because of karma being deprecated and it blocking upgrading our dependencies. I am going to explore the landscape a bit more, maybe there's a middle ground here.

Ideally webdriver would be as fast as playwright but... I am unsure where this is headed in terms of performance.

We've had to switch to some Vitest-specific APIs. While the "lock-in" doesn't look to be to rough, we are switching from runner agnostic (or meant to be runner agnostic) tools to something that's specific & will need to be changed if we need to migrate again in the future

This is more because sinon in more recent versions is very broken, when we use sinon.spyOn it will break the prototype of the underlying object 😅

We've encountered a fair number of Vitest bugs in the process of this PR

This was part of my intention, I wanted to uncover bugs so we can help the vitest people with testing browser mode as I am really looking for a community project to step into the shoes of karma.

The --skipLibCheck flags can probably be removed now that it was added to js-lint.json, right?

Well, sadly, the setting doesn't work 😅

I agree with you, let's hold off on merging this, I will surface this with the vitest people so there's information sharing.

@marvinhagemeister
Copy link
Member

+1 on waiting on some vitest improvements before merging this. I don't mind the switch away from sinon as it hasn't been well maintained either.

@rschristian
Copy link
Member

This is more because sinon in more recent versions is very broken, when we use sinon.spyOn it will break the prototype of the underlying object 😅

Oh fair enough, wasn't quite sure if it was sinon or vitest that was the problem there.

Well, sadly, the setting doesn't work 😅

Huh. I thought the flag would act the exact same there. Admittedly I can't even run test:ts:core at the moment, some Node error... test:ts:compat is a-ok though. Fair enough on that too then.

@JoviDeCroock JoviDeCroock force-pushed the switch-to-vitest branch 5 times, most recently from 97ebbf3 to 5dc412d Compare February 25, 2025 10:26
JoviDeCroock and others added 28 commits March 7, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rschristian rschristian rschristian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JoviDeCroock @coveralls @marvinhagemeister @rschristian @sheremet-va