-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: rewrite benchmark reporter without log-update
#7019
fix: rewrite benchmark reporter without log-update
#7019
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@vitest/browser
@vitest/coverage-istanbul
@vitest/expect
@vitest/coverage-v8
@vitest/mocker
@vitest/pretty-format
@vitest/runner
@vitest/snapshot
@vitest/spy
@vitest/ui
@vitest/utils
vite-node
@vitest/web-worker
vitest
@vitest/ws-client
commit: |
3dc0b49
to
b1c700c
Compare
Looks like benchmark runner is reporting tests of nested suites twice. Need to resolve this, otherwise summary is showing too high test count. |
Does this also add a project name to the output? I just noticed we don't print |
2316cf1
to
8a19faa
Compare
@@ -135,16 +137,6 @@ async function runBenchmarkSuite(suite: Suite, runner: NodeBenchmarkRunner) { | |||
suite.result!.duration = performance.now() - start | |||
suite.result!.state = 'pass' | |||
|
|||
tasks |
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.
Did you move the sorting? Why is it removed from here?
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.
0965c72
to
3e9a9b7
Compare
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.
Looks good! Tested radashi locally and the improvement is awesome 👏
About tinyexec
patch, it's not a blocker, but the situation is not ideal since patch is only applied for our local dev, but tinyexec
is also used for external dependencies in a few places.
I agree. I'll replace |
45ba01c
to
c9ad5fd
Compare
c9ad5fd
to
5bb20e1
Compare
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!
Description
PatchesAlternatively I could remove the patch and usetinyexec
for Missingstdout
due to closed stream tinylibs/tinyexec#40.node:child_process
in test cases.log-update
andcli-truncate
-> -92 kB from bundleTest preview release with:
vitest@v2
they could not see previous results during the run aslog-update
limits the rows to the terminal height. Now you can actually scroll up during the run and see previous results. No need to wait for that ~8min run to finish.'pass'
multiple times, fixed.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.