Support vitest 5 - #86
GuillaumeLagrange wants to merge 5 commits into
Conversation
| } | ||
| } | ||
|
|
||
| function patchRunBenchmarks(): void { |
There was a problem hiding this comment.
By the way, you can do that just in a setup file without overriding the runner. This should also make it support the browser(?)
If there is any feedback, I would love to address them on vitest side; we would also like to use codspeed after Vitest 5 is out in our own repo
There was a problem hiding this comment.
@sheremet-va thanks for the heads up, currently trying out this approach.
The main pain point I am running into is that we are struggling to highjack the benchmark runner when we are in simulation/analysis mode.
When in this mode, what we used to do is highjack the whole run function, to have a very simple sequence of
- warmup
- manually call optimizer
- start valgrind instrumentation
- execute code once
- stop
But I'm struggling a bit to find a good way to replicate the behavior without overriding the whole runner.
I still do not have a proper api request, nor am I sure it's the way to go for the analysis mode, becuase maybe running browser mode under valgrind is not a good idea in the first place, and the main motivation for not highjacking the whole runner is actually to be as compatible as possible with the browser mode.
There was a problem hiding this comment.
Could it be possible to have a way for the plugin to hook/customize the createTinybench function? Maybe by defining some sort of abstract "benchmark backend" interface that we could fullfill, that would default to a tinybench implementation. It would also make it easier to switch away from tinybench on your end at some point.
The way tinybench is instanciated/ran does not allow for much flexibility on our end, and I cannot find a good implementation without overriding the whole runner as we did before for our analysis mode.
I'd love to exchange over this over discord/slack/a call if you want, or we can keep things here on github.
There was a problem hiding this comment.
Feel free to join the discord - https://chat.vitest.dev/
I will give you the ecosystem role, we can discuss it there. I just need your nickname
There was a problem hiding this comment.
I've joined and pinged on via MP, my nickname on the server is the same is github
25ef927 to
e04f0da
Compare
Merging this PR will regress 4 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | switch 1 |
204.5 µs | 689.9 µs | -70.37% |
| ❌ | Memory | wait 500ms |
15.5 KB | 21.5 KB | -27.99% |
| ❌ | Simulation | iterative fibo 20 |
26.9 µs | 30.7 µs | -12.36% |
| ❌ | WallTime | test_recursive_cached_fibo_10 |
2.3 µs | 2.6 µs | -12.09% |
| ⚡ | Simulation | recursive fibo 10 |
1,372.1 µs | 299.9 µs | ×4.6 |
| ⚡ | Simulation | recursive fibo 15 |
636.9 µs | 352.1 µs | +80.88% |
| ⚡ | WallTime | switch 1 |
84 ns | 72 ns | +16.67% |
| ⚡ | WallTime | test_iterative_fibo_10 |
120 ns | 108 ns | +11.11% |
| 🆕 | WallTime | iterative fibo 15 |
N/A | 672 ns | N/A |
| 🆕 | WallTime | iterative fibo 20 |
N/A | 672 ns | N/A |
| 🆕 | WallTime | recursive fibo 15 |
N/A | 20.4 µs | N/A |
| 🆕 | WallTime | recursive fibo 20 |
N/A | 219.2 µs | N/A |
| 🆕 | WallTime | iterative |
N/A | 552 ns | N/A |
| 🆕 | WallTime | recursive |
N/A | 20.2 µs | N/A |
| 🆕 | WallTime | iterative |
N/A | 564 ns | N/A |
| 🆕 | WallTime | recursive |
N/A | 218.9 µs | N/A |
| 🆕 | WallTime | fibo 10 |
N/A | 2.3 µs | N/A |
| 🆕 | WallTime | fibo 15 |
N/A | 20.3 µs | N/A |
| 🆕 | WallTime | long body |
N/A | 267.2 µs | N/A |
| 🆕 | WallTime | short body |
N/A | 1.5 µs | N/A |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing cod-2931-prepare-compatibility-with-vitest-5 (9007e6e) with main (3258048)
Footnotes
-
53 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
f511dee to
c3a356b
Compare
Greptile SummaryThis PR adds Vitest 5 support to the CodSpeed Vitest plugin. The main changes are:
Confidence Score: 4/5The new Vitest 5 path can record distinct benchmarks under one CodSpeed URI, and legacy fallback can skip instrumentation.
packages/vitest-plugin/src/v5/provider.ts and packages/vitest-plugin/src/vitestBackend.ts Important Files Changed
|
| let setupHappened = false; | ||
| let teardownHappened = false; | ||
|
|
||
| // TODO: Check if this can be avoided |
There was a problem hiding this comment.
This new TODO has no Linear issue reference, which violates the repository rule for TODO comments. Please change it to the TODO(COD-XXX): ... form or remove it.
| // TODO: Check if this can be avoided | |
| // TODO(COD-XXX): Check if this can be avoided |
Rule Used: Every TODO comment must reference a Linear issue: ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/vitest-plugin/src/globalSetup.ts
Line: 15
Comment:
**Untracked TODO Comment**
This new TODO has no Linear issue reference, which violates the repository rule for TODO comments. Please change it to the `TODO(COD-XXX): ...` form or remove it.
```suggestion
// TODO(COD-XXX): Check if this can be avoided
```
**Rule Used:** Every TODO comment must reference a Linear issue: ... ([source](https://app.greptile.com/codspeed/-/custom-context?memory=65193bc9-f65b-477d-9521-104b5aac5931))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
eec7ec7 to
eb2042e
Compare
|
Hi @GuillaumeLagrange, what's the status of this PR? We would need it for Astro |
Hey! The support for the benchmark provider has been merged and released upstream by vitest, we plan to merge and release this PR early this week! Don't hesitate to continue pinging me to get updates, and sorry about the delay |
A plugin behaves differently depending on whether CodSpeed drives the run, and each mode takes a different path through the plugin, so all three have to be run when developing or reviewing one. Write down the commands and the expected output per mode, and ignore the local `.codspeed` result directory those runs produce. Co-Authored-By: Claude <noreply@anthropic.com>
eb2042e to
d6abe82
Compare
Vitest 5 removed the dedicated `NodeBenchmarkRunner` and the `vitest/runners` / `vitest/suite` entrypoints. Benchmarks are now declared through a `bench` test-context fixture and executed by a `benchmark.provider`: a module Vitest hands the registered functions, their options and the declaring test, and whose results it treats as authoritative. Detect the installed Vitest generation and select the integration behind a `VitestBackend` abstraction so the rest of the plugin never inspects the version: - v3/4 keep the custom benchmark runner per instrument mode. Their Vitest lookups move behind `legacy/compat`, which owns the types of the removed benchmark backend so the package type-checks against Vitest 5. - v5 registers CodSpeed as the benchmark provider, which runs the analysis and walltime modes off the registrations it receives. On v5, benchmark URIs carry the registration name after the test path, so a test declaring several benchmarks through `bench.compare()` reports one benchmark per registration. Gate the injection on the instrument mode: Vitest 5 clones its benchmark project after the Vite config hooks ran, so a benchmark run can no longer be detected from the incoming config, and CodSpeed only ever drives benchmark runs. Without CodSpeed the plugin injects nothing and Vitest runs the benchmarks through its own tinybench provider. Vitest 5 also runs `globalSetup` once per project, including the cloned benchmark project, so make setup and teardown idempotent instead of failing the run on the second teardown. Bump the plugin's own dev dependency to Vitest 5 and move its benches to the fixture API, reading imported bindings into a local before the measured loop so the module-runner getters stay out of the samples. Refs COD-2931 Co-Authored-By: Claude <noreply@anthropic.com>
Now that the plugin's own dev dependency tracks Vitest 5, add a dedicated Vitest 4 example so the legacy (v3/4) benchmark seam keeps explicit coverage alongside the existing with-vitest-v3 example. Mirrors that example, pinning vitest ^4.1.9. Refs COD-2931 Co-Authored-By: Claude <noreply@anthropic.com>
Benchmark the plugin against Vitest 5 in CI the way the v3 and v4 examples do for their majors, using the `bench` fixture and `bench.compare()`. Refs COD-2931 Co-Authored-By: Claude <noreply@anthropic.com>
Benchmarks are declared through the `bench` test-context fixture on Vitest 5; keep the top-level `bench()` form as a note for Vitest 3 and 4. Drop the fallback log line from the sample output, which the plugin no longer prints. Refs COD-2931 Co-Authored-By: Claude <noreply@anthropic.com>
d6abe82 to
9007e6e
Compare
No description provided.