-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(browser): introduce toMatchScreenshot
for Visual Regression Testing
#8041
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
base: main
Are you sure you want to change the base?
feat(browser): introduce toMatchScreenshot
for Visual Regression Testing
#8041
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
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 overall, just some code style issues
6a8e1ab
to
d5848f0
Compare
I moved the types around to what starts to hopefully make some sense. I created a I've also rebased and resolved the conflicts from main. |
packages/browser/jest-dom.d.ts
Outdated
@@ -1,6 +1,7 @@ | |||
// Disable automatic exports. | |||
|
|||
import { ARIARole } from './aria-role.ts' | |||
import { ComparatorRegistry, ScreenshotMatcherOptions } from './context.js' |
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 know this creates a circular dependency between this file and context.d.ts
, was wondering if it's fine tho.
To fix it I would have to move ScreenshotOptions
out of context.d.ts
and, as it's using Locator
, at least half of the other types declared in it 😅
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.
If it works, it works 😄
dbd7baa
to
2412183
Compare
79afa30
to
addc827
Compare
@@ -56,7 +56,7 @@ function parseInspector(inspect: string | undefined | boolean | number) { | |||
return { host, port: Number(port) || defaultInspectPort } | |||
} | |||
|
|||
export function resolveApiServerConfig<Options extends ApiConfig & UserConfig>( | |||
export function resolveApiServerConfig<Options extends ApiConfig & Omit<UserConfig, 'expect'>>( |
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.
This is provided with the browser
object, the new browser.expect
property conflicts with expect
. I noticed options.expect
is not used tho, so instead of removing it at runtime I removed it from the type.
toMatchScreenshot?: { | ||
[ComparatorName in keyof ToMatchScreenshotComparators]: | ||
{ | ||
/** | ||
* The name of the comparator to use for visual diffing. | ||
* | ||
* @defaultValue `'pixelmatch'` | ||
*/ | ||
comparatorName?: ComparatorName | ||
comparatorOptions?: ToMatchScreenshotComparators[ComparatorName] | ||
} | ||
}[keyof ToMatchScreenshotComparators] & ToMatchScreenshotOptions |
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.
This type is a bit chaotic, but without generics the only solution is to create discriminated unions.
The interface is compatible with ScreenshotComparatorRegistry
, so I used that to extend it in the providers. In the future this will allow to add more comparators and have them typed correctly in the config by augmenting ScreenshotComparatorRegistry
.
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.
Code looks good to me, but we need a lot more tests for this feature
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.
Code looks good to me, but we need a lot more tests for this feature
ff2c9ad
to
8649b09
Compare
7fab222
to
682bbd1
Compare
I changed the testing approach: instead of relying on pre-generated screenshots, the tests now create them dynamically during execution. This makes the tests more stable and avoids failures across different OSes, browser versions, or rendering environments. All artifacts are cleaned up automatically after the tests run.
|
ab114c5
to
246d7c0
Compare
while (signal.aborted === false) { | ||
if (decodedBaseline === null) { | ||
decodedBaseline = takeDecodedScreenshot(screenshotArgument) | ||
} | ||
|
||
const [image1, image2] = await Promise.all([ | ||
decodedBaseline, | ||
takeDecodedScreenshot(screenshotArgument), | ||
]) | ||
|
||
const comparatorResult = (await comparator( | ||
image1, | ||
image2, | ||
{ ...comparatorOptions, createDiff: false }, | ||
)).pass | ||
|
||
decodedBaseline = image2 | ||
|
||
if (comparatorResult) { | ||
break | ||
} | ||
|
||
retries += 1 | ||
} |
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.
Playwright uses a backoff mechanism based on predefined timeouts ([0, 100, 250, 500]
, fallbacks to 1_000
) when retrying.
I'm not sure if I should implement that, if each tests waits 100ms to take a screenshot again, it adds up.
Right now, it's surprisingly hard not to get a stable screenshot in a few retries, but this doesn't mean the timing is consistent across runs. When using Playwright as a provider, consistency should not be an issue because they allow disabling animations. With WebdriverIO I don't know as I have no experience with it.
c611092
to
e2776e9
Compare
d900731
to
c7c6219
Compare
0600986
to
7476ee6
Compare
f84095c
to
71a616e
Compare
Let me know if you don't want the merge commit in the PR's history, I will rebase and remove it once we're done with the review. Force-pushing after a rebase moves comments up in the "load more" hidden section every time I update from |
Description
This PR introduces initial support for Visual Regression Testing for Vitest via a new
toMatchScreenshot
assertion.Related issue: #6265
In this initial iteration:
pixelmatch
as the comparator.The logic to get a stable screenshot follows Playwright's approach (with some differences):
The command has 6 possible outcomes:
#01
fail
#02
fail
#03
fail
#04
pass
#05
pass
#06
fail
The client matcher expects an
Element
orLocator
along with:To-do
resolveScreenshotPath
andresolveDiffPath
functionsannotation
API (feat: annotation API #7953) to show screenshot pathspixelmatch
comparatorPlease 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:
.