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

feat(runner): add "queued" state #6931

Merged
merged 18 commits into from
Dec 17, 2024

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Nov 19, 2024

Description

Fixes #6915

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Nov 19, 2024

Deploy Preview for vitest-dev ready!

Name Link
🔨 Latest commit 45a433f
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/67596c9550a1cd0008a9999a
😎 Deploy Preview https://deploy-preview-6931--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

pkg-pr-new bot commented Nov 19, 2024

@vitest/browser

npm i https://pkg.pr.new/@vitest/browser@6931

@vitest/coverage-istanbul

npm i https://pkg.pr.new/@vitest/coverage-istanbul@6931

@vitest/expect

npm i https://pkg.pr.new/@vitest/expect@6931

@vitest/coverage-v8

npm i https://pkg.pr.new/@vitest/coverage-v8@6931

@vitest/mocker

npm i https://pkg.pr.new/@vitest/mocker@6931

@vitest/pretty-format

npm i https://pkg.pr.new/@vitest/pretty-format@6931

@vitest/runner

npm i https://pkg.pr.new/@vitest/runner@6931

@vitest/snapshot

npm i https://pkg.pr.new/@vitest/snapshot@6931

@vitest/spy

npm i https://pkg.pr.new/@vitest/spy@6931

@vitest/ui

npm i https://pkg.pr.new/@vitest/ui@6931

@vitest/utils

npm i https://pkg.pr.new/@vitest/utils@6931

vite-node

npm i https://pkg.pr.new/vite-node@6931

vitest

npm i https://pkg.pr.new/vitest@6931

@vitest/web-worker

npm i https://pkg.pr.new/@vitest/web-worker@6931

@vitest/ws-client

npm i https://pkg.pr.new/@vitest/ws-client@6931

commit: 657de56

@sheremet-va
Copy link
Member Author

Note: doesn't seem like it's working as I expected in a real project - https://github.com/zammad/zammad

@sheremet-va sheremet-va force-pushed the feat/add-queued-state branch from b47f145 to 138a163 Compare December 1, 2024 22:42
@sheremet-va sheremet-va added this to the 3.0.0 milestone Dec 9, 2024
@sheremet-va sheremet-va force-pushed the feat/add-queued-state branch from 138a163 to 753456b Compare December 9, 2024 16:20
@sheremet-va
Copy link
Member Author

Note: doesn't seem like it's working as I expected in a real project - https://github.com/zammad/zammad

Tested and it now looks very nice 👌🏻

@sheremet-va
Copy link
Member Author

@AriPerkkio it now shows (0/0) - should we print [queued] instead? 🤔

Copy link
Member

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

@AriPerkkio it now shows (0/0) - should we print [queued] instead? 🤔

We should probably hide test count when testFile.total is 0:

summary.push(
c.bold(c.yellow(` ${F_POINTER} `))
+ formatProjectName(testFile.projectName)
+ testFile.filename
+ c.dim(` ${testFile.completed}/${testFile.total}`),
)

@sheremet-va
Copy link
Member Author

We should probably hide test count when testFile.total is 0:

Do you want to just remove it without having any labels?

> fixtures/example.test.ts
> fixtures/example.test.ts (0/0)
> fixtures/example.test.ts [queued]
> fixtures/example.test.ts (queued)

@sheremet-va
Copy link
Member Author

@AriPerkkio any feedback? 👀

@AriPerkkio
Copy link
Member

Testing this on Vite's pnpm test-serve, something breaks test count. There's 7/0 visible:

image

@sheremet-va
Copy link
Member Author

Testing this on Vite's pnpm test-serve, something breaks test count. There's 7/0 visible:

I will look into it. Any feedback on #6931 (comment) ?

@AriPerkkio
Copy link
Member

I will look into it. Any feedback on #6931 (comment) ?

If label doesn't cause too much flickering, sure. Maybe the icon could also be different, though I'm not sure what color and shape would work best.

@sheremet-va
Copy link
Member Author

Testing this on Vite's pnpm test-serve, something breaks test count. There's 7/0 visible:

Should be fixed

@AriPerkkio
Copy link
Member

Still there 🤔
image

@sheremet-va
Copy link
Member Author

Added another workaround. I agree would be nice to have this statistics in test suites themselves :D

Maybe we can track it in thestate

@AriPerkkio
Copy link
Member

It's still there. Here's the PR on top, main below:

@sheremet-va
Copy link
Member Author

I can't really reproduce it - I've run it on the vite repo several times.

@AriPerkkio
Copy link
Member

AriPerkkio commented Dec 11, 2024

Does it help reproducing if you add this in playground/css/__tests__/same-file-name/css-same-file-name.spec.ts? And run pnpm test-serve playground/css.

// after all other code

test('temp', async () => {
  await new Promise((r) => setTimeout(r, 10_000))
})

E: Oh wait, no. Maybe it's related to the describe.runIf. Skipped tests are now reported 🤔

@AriPerkkio
Copy link
Member

Adding this in createSummary reveals the erroneous reports:

      if (testFile.completed > testFile.total) {
        this.ctx.logger.log(`Error: ${testFile.completed} > ${testFile.total} ${testFile.filename}`)
      }

if (!stats) {
// It's possible that that test finished before it's preparation was even reported
this.onTestFilePrepare(test.file)

We need might need to extend the check to

- if (!stats) {
+ if (!stats || stats.total === 0) {

This is all related to the fact that test runners can skip test and test file phases, e.g. test can be reported as finished even though it was never reported to start.

I'll take closer look at this later today.

@AriPerkkio
Copy link
Member

Verified fix from f91a5f7 in Vite's pnpm test-serve. ✅

During #7069 we'll be fixing this "prevent reporting tests finished if they haven't even been reported as starting" properly. We'll need some state logic in TaskParser (or in it's successor). Then we can remove some complex logic from summary.ts and dot.ts reporters.

@sheremet-va sheremet-va merged commit 5f8d209 into vitest-dev:main Dec 17, 2024
18 checks passed
@sheremet-va sheremet-va deleted the feat/add-queued-state branch December 17, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reporter API: report files before collection
2 participants