Conversation
| if (/snapshot .*doesn't exist/.test(message) && message.includes('.png')) { | ||
| result.name = ErrorName.NO_REF_IMAGE; | ||
| } else if (message.includes('Screenshot comparison failed')) { | ||
| } else if (this._testResult.errors.every(error => this._isScreenshotComparisonError(error))) { |
There was a problem hiding this comment.
why is it not enough to just switch to 2 error options? for example, /Screenshot comparison failed|expect\(.*\)\.toHaveScreenshot\(expected\)failed/
There was a problem hiding this comment.
Playwright uses the same header when it cannot take a screenshot. So I also check that expected, actual, and diff images are attached.
I'll keep toHaveScreenshot support with this check and remove the mixed-error changes from this PR.
| return true; | ||
| } | ||
|
|
||
| if (!/^(?:Error: )?expect\((?:page|locator|Buffer)\)\.(?:toHaveScreenshot|toMatchSnapshot)\(expected\)(?: failed)?$/.test(header)) { |
There was a problem hiding this comment.
toMatchSnapshot should be used for non-image comparison, so we don't need this part here
There was a problem hiding this comment.
It can compare images too, but I agree that we don't need it for this fix. I'll remove it from this PR.
| // Modern Playwright uses the same matcher header for diffs and capture errors. | ||
| const snapshotName = message.match(/^\s*Snapshot: (.+)\.png\s*$/m)?.[1]; | ||
| const states = Object.entries(this._attachmentsByState).filter(([state]) => | ||
| snapshotName ? state === snapshotName : this._testResult.errors.length === 1); |
There was a problem hiding this comment.
with expect.soft error length can be more then 1
There was a problem hiding this comment.
can you, pls, add one more unit with state name "some state"? pwt can replace a space with a dash here
| if (/snapshot .*doesn't exist/.test(message) && message.includes('.png')) { | ||
| result.name = ErrorName.NO_REF_IMAGE; | ||
| } else if (message.includes('Screenshot comparison failed')) { | ||
| } else if (message.includes('Screenshot comparison failed') || |
There was a problem hiding this comment.
we can merge these checks to your _isScreenshotComparisonError function
|
|
||
| private _isScreenshotComparisonError(error: PlaywrightTestResult['errors'][number]): boolean { | ||
| const message = stripAnsi(error.message || ''); | ||
| const header = message.split('\n')[0]; |
There was a problem hiding this comment.
splitting the message into the header and rest can lead to the search error. For example, in case with
await expect(page, 'Home page should match').toHaveScreenshot('custom.png');
header part will consist only text like "Error: Home page should match". In this case the test will be marked as none-image error
commit: |
This branch has not been deployed
2 participants
Playwright 1.59.1 reports screenshot mismatches as:
The adapter only recognizes
Screenshot comparison failed, so it marks these as general errors even when expected/actual/diff images are attached. Accept can then leave a stale error instead of resolving the visual failure.This adds support for the newer page/locator
toHaveScreenshotheader. Playwright uses the same header when it cannot take a screenshot, so the adapter also checks for matching expected/actual/diff attachments. The oldScreenshot comparison failedhandling stays unchanged.Only the adapter and its unit tests change.
toMatchSnapshot, changes to legacy mixed-error handling, and platform handling are outside this PR.Reproduction
With Playwright 1.59.1 and
html-reporter/playwright:Before:
error/Error. After:fail/ImageDiffError.Tests
npm testpasses on Node 20.19.6: lint, Node and jsdom unit tests, and type checks.The full GUI e2e suite and interactive Accept/Undo weren't rerun for this isolated fix.
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en.