-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(ui): auto reload coverage iframe after test run #5242
Conversation
✅ Deploy Preview for fastidious-cascaron-4ded94 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Using the |
That's an interesting idea, but it feels a bit overkill? With the custom reporter, we can have access to For now, I went with adding |
@@ -7,6 +7,8 @@ export interface Reporter { | |||
onPathsCollected?: (paths?: string[]) => Awaitable<void> | |||
onCollected?: (files?: File[]) => Awaitable<void> | |||
onFinished?: (files?: File[], errors?: unknown[]) => Awaitable<void> | |||
/** @internal */ | |||
onFinishedReportCoverage?: () => Awaitable<void> |
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.
Is there any other way? I don't like exposing this even with @internal
flag 🤔
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.
I think it's necessary to notify websocket client in one way or another.
Actually there's one thing possible, which is to expose only WebSocketEvents.onFinishedReportCoverage
without Reporter.onFinishedReportCoverage
and it would look like this 468c057
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.
I like it 👍🏻
I think it's more of a bug fix than a feature. |
…orter`
Description
I tested a simple iframe reloading and tested by
pnpm -C examples/basic test:ui --coverage
:Show demo
2024-02-20.13-35-10.webm
But actually I'm worrying that there might be some race condition here:
vitest/packages/vitest/src/node/core.ts
Lines 529 to 530 in a479115
If
reportCoverage
takes significant time, then reloading iframe duringonFinished
might end up fetching old coverage html.To avoid this race condition, it might be necessary to either:
onReportCoverageFinished
) just for this, orreportCoverage
beforeonFinished
(this would likely mess up terminal output for coveragetext
reporter) orI'm leaning towards the 1st option, but what do you think?
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:
.