-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(browser): run toMatchScreenshot only once when used with expect.element
#9132
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
Changes from 3 commits
22eff98
39d89be
9560251
aba0475
24c294e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -421,4 +421,114 @@ describe('.toMatchScreenshot', () => { | |
|
|
||
| expect(errorMessage).matches(/^Could not capture a stable screenshot within 100ms\.$/m) | ||
| }) | ||
|
|
||
| // Only run this test if snapshots aren't being updated | ||
| test.runIf(server.config.snapshotOptions.updateSnapshot !== 'all')( | ||
| 'runs only once after resolving the element', | ||
| async ({ onTestFinished }) => { | ||
| const filename = `toMatchScreenshot-runs-only-once-after-resolving-the-element-1` | ||
| const path = join( | ||
| '__screenshots__', | ||
| 'toMatchScreenshot.test.ts', | ||
| ) | ||
|
|
||
| const screenshotPath = join( | ||
| path, | ||
| `${filename}-${server.browser}-${server.platform}.png` | ||
| ) | ||
|
|
||
| // Create baseline screenshot with original colors | ||
| renderTestCase([ | ||
| 'oklch(39.6% 0.141 25.723)', | ||
| 'oklch(40.5% 0.101 131.063)', | ||
| 'oklch(37.9% 0.146 265.522)', | ||
| ]) | ||
| const locator = page.getByTestId(dataTestId) | ||
|
|
||
| await locator.screenshot({ | ||
| save: true, | ||
| path: screenshotPath, | ||
| }) | ||
|
|
||
| onTestFinished(async () => { | ||
| await server.commands.removeFile(screenshotPath) | ||
| }) | ||
|
|
||
| // Remove element, then re-render with inverted colors after a delay | ||
| document.body.innerHTML = '' | ||
|
|
||
| const renderDelay = 500 | ||
| setTimeout(() => { | ||
| renderTestCase([ | ||
| 'oklch(37.9% 0.146 265.522)', | ||
| 'oklch(40.5% 0.101 131.063)', | ||
| 'oklch(39.6% 0.141 25.723)', | ||
| ]) | ||
| }, renderDelay) | ||
|
|
||
| const start = performance.now() | ||
|
|
||
| // Expected behavior: | ||
| // 1. `expect.element()` polls until element exists (~500ms) | ||
| // 2. `toMatchScreenshot()` runs ONCE and fails (colors don't match baseline) | ||
| // | ||
| // If `toMatchScreenshot()` polled internally, it would retry for 30s. | ||
| // By checking the elapsed time we verify it only ran once. | ||
|
|
||
| let errorMessage: string | ||
|
|
||
| try { | ||
| await expect.element(locator).toMatchScreenshot() | ||
| } catch (error) { | ||
| errorMessage = error.message | ||
| } | ||
|
|
||
| expect(typeof errorMessage === 'string') | ||
|
|
||
| const [referencePath, actualPath, diffPath] = extractToMatchScreenshotPaths( | ||
| errorMessage, | ||
| filename, | ||
| ) | ||
|
|
||
| expect(typeof referencePath).toBe('string') | ||
| expect(typeof actualPath).toBe('string') | ||
| expect(typeof diffPath).toBe('string') | ||
|
|
||
| expect(referencePath).toMatch(new RegExp(`${screenshotPath}$`)) | ||
|
|
||
| onTestFinished(async () => { | ||
| await Promise.all([ | ||
| server.commands.removeFile(actualPath), | ||
| server.commands.removeFile(diffPath), | ||
| ]) | ||
| }) | ||
|
|
||
| expect( | ||
| errorMessage | ||
| .replace(/(?:\d+)(.*?)(?:0\.\d{2})/, '<pixels>$1<ratio>') | ||
| .replace(referencePath, '<reference>') | ||
| .replace(actualPath, '<actual>') | ||
| .replace(diffPath, '<diff>') | ||
| ).toMatchInlineSnapshot(` | ||
| expect(element).toMatchScreenshot() | ||
|
|
||
| Screenshot does not match the stored reference. | ||
| <pixels> pixels (ratio <ratio>) differ. | ||
|
|
||
| Reference screenshot: | ||
| <reference> | ||
|
|
||
| Actual screenshot: | ||
| <actual> | ||
|
|
||
| Diff image: | ||
| <diff> | ||
| `) | ||
|
|
||
| const elapsed = performance.now() - start | ||
|
|
||
| // Allow some buffer for screenshot capture + diffing | ||
| expect(elapsed).toBeLessThan(renderDelay + 500) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Locally, 500ms are enough for all browsers to finish. Might be worth monitoring on CI to see if it's flaky.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like it's failing in CI. Maybe I can just check |
||
| }, | ||
| ) | ||
| }) | ||
Uh oh!
There was an error while loading. Please reload this page.
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 doesn't do anything, did you mean
expect.assert? We should maybe throw an error if the methods are not called 😄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.
Editing fail on my part, it was an
ifbefore but forgot to change===with).toBe(😅Good catch, thanks!