-
-
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(snapshot)!: reset snapshot state for retry
and repeats
#6817
base: main
Are you sure you want to change the base?
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. |
62cc7be
to
7cd37a6
Compare
7cd37a6
to
7cf11f5
Compare
e3239a0
to
c5e4527
Compare
e9f8133
to
b441f8a
Compare
b441f8a
to
3fc11d5
Compare
retry
and repeats
620e2ce
to
4a69ec1
Compare
278ada9
to
e0768a1
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 very good! But we would have to release it as a breaking change (@vitest/snapshot
is a public API and SnapshotState
is exposed on expect.getState()
- there are jest's custom matchers that rely on the jest interface)
packages/snapshot/src/client.ts
Outdated
getSnapshotState(filepath: string): SnapshotState { | ||
const state = this.snapshotStateMap.get(filepath) | ||
if (!state) { | ||
throw new Error('snapshot state not initialized') |
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.
Can we maybe make it more presentable here, too? Since snapshot manager is a public API (even used in webdriverio, I think). Something like The snapshot state for ${file} was not initialised. Did you call manager.setup()?
retry
and repeats
retry
and repeats
08db7bb
to
d9b5f23
Compare
d9b5f23
to
99c64e6
Compare
Only 7 users on public 😃 https://www.npmjs.com/package/@vitest/snapshot?activeTab=dependents I'll check what webdriverio does and see if new API fits them. |
I briefly reviewed
Webdriverio doesn't seem to have |
Description
todo
should we make@vitest/snapshot
non breaking?SnapshotClient
interface a lot to simplify the concept, but it's probably possible to make it non-breaking by keepingtestId
optional.@vitest/snapshot
usage in the wild to see if new API is not bad.I have an attempt to support different snapshot files per test in #6827, but I need to think a bit more to validate the idea and we should separate it from this fix.
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:
.