fix: honor screenshot pixel tolerances

[lazerg]

expectScreenshot() compares the raw PNG bytes with toMatchSnapshot() first, and only asks Playwright for a pixel comparison once those differ. But both paths out of the catch throw, so a screenshot Playwright reports as a match still fails the test, and maxDiffPixels, threshold and detectAntialiasing never get to decide anything. Chromium's PNG output is not byte-reproducible, so a handful of antialiased pixels is enough to fail a run at random.

Playwright signals a real mismatch by erroring on the expectScreenshot call, which the websocket client already turns into a failed expectation before the loop below it runs. Dropping the trailing throw therefore keeps genuine regressions failing and lets the tolerances apply.

To reproduce, take a screenshot assertion with a committed baseline and re-encode the stored snapshot so the pixels stay identical but the bytes change (appending a PNG tEXt chunk does it). The test fails today and passes with this change.

A separate commit covers what a genuine mismatch then reports. Since 1.61 Playwright puts only Expect failed in the error and sends the pixel comparison result beside it as errorDetails.customErrorMessage, so the client prefers that when the frame carries it. A failing screenshot assertion reads 3659 pixels (ratio 0.01 of all image pixels) are different. instead of Expect failed.

Fixes pestphp/pest#1881

[yeapea]

I think you're underselling this. Your own reasoning kills the other branch too, and the PR leaves it standing.

You say Playwright signals a real mismatch by erroring on the call. That matches what I found. But if that's true, then if (isset($message['result']['diff'])) a few lines above the code you deleted can't fire either. I read Page.expectScreenshot in every minor from 1.53.1 up: 1.53.1 through 1.60.0 all return { log, ...intermediateResult, errorMessage } with the diff in it. 1.61.0, 1.61.1 and 1.62.1 throw instead, and the diff moves onto response.errorDetails.

So the switch happened between 1.60.0 and 1.61.0, and this repo pins ^1.62.1. On that version a real mismatch never gets as far as the foreach — Client::execute() has already thrown. createImageDiffView() never runs, --diff gives you nothing, and what you actually see is Playwright's bare Expect failed instead of the --update-snapshots hint.

That also explains 0e2ce0b ("add support for handling missing image diffs", 2025-08-21). package.json pinned ^1.54.1 back then, so the diff really did come back in the result — the placeholder was only ever papering over the tolerance case you're removing here.

None of which argues against merging this. It's the same bug on the other branch of the same catch. But once this lands, that catch is basically "ask Playwright, and if it doesn't throw we're fine", with a diff viewer nobody can reach. Might be worth wiring it up to errorDetails or deleting it in the same go, so it stops looking like it works.

[lazerg]

Checked the version boundary against the vendored source and it holds. The protocol schema is even more direct than the throw/return change: in 1.60.0 PageExpectScreenshotResult is {diff, errorMessage, actual, previous, timedOut, log}, and in 1.61.0 it drops to {actual} while a new PageExpectScreenshotErrorDetails picks up diff. On ^1.62.1 the result validator strips diff outright, so that branch cannot fire even in principle.

One correction to my own earlier reading. I had assumed the diff was simply lost once Playwright started throwing. It is not. Here is the raw websocket frame on a genuine mismatch, produced by painting a rectangle over the stored snapshot:

keys:                id, error, errorDetails, log
error.error.message: "Expect failed"
errorDetails:        diff (valid PNG, 9529 bytes), customErrorMessage, actual, timedOut, log
customErrorMessage:  "1954 pixels (ratio 0.01 of all image pixels) are different."

So deleting the branch would throw away something that is still reachable. Wiring it up is the better direction.

That part is not local to this catch, though. Client::execute() throws as soon as it sees error.error.message, before it yields anything, so errorDetails never reaches Page. Carrying it through needs an exception type that holds the details, and every call in the plugin goes through that same client path. That is a different design decision from removing a dead throw.

I would keep this PR as the deletion and send the errorDetails wiring straight after. Happy to open that if the maintainers want it.

[yeapea]

Checked the schema boundary against the vendored sources and it lines up. PageExpectScreenshotResult in 1.60.0 is {actual, diff, errorMessage, log, previous, timedOut}; in 1.61.0 it's just {actual}, and PageExpectScreenshotErrorDetails shows up alongside it carrying {actual, customErrorMessage, diff, log, previous, timedOut}. So the result validator strips diff outright, like you say — that branch can't fire even with a willing server.

You're also right that I overstated the loss. I said all you get is the bare Expect failed, which is true of what the plugin surfaces but not of what arrives. customErrorMessage is sitting in the same frame.

That splits your follow-up into two pieces of very different size, though. The image does need somewhere to live, and that's the design decision you're describing. The message doesn't. Client::execute() already reads $response['error']['error']['message'] on the line it throws from — preferring $response['errorDetails']['customErrorMessage'] when it's set is a change in that one spot, with no new exception type and nothing plumbed through Page. On its own that turns Expect failed into 1954 pixels (ratio 0.01 of all image pixels) are different.

Worth knowing where that line sits, whichever way you go: #247 inserts its id check directly above that same if, and #241 rewrites the loop around it. Three open PRs on one method.

One thing I checked that didn't pan out, in case you wonder the same: Frame.expect is the only other protocol method with an ErrorDetails type, so I went looking for whether this reached element assertions too. The plugin never sends expect. Screenshots only.

[lazerg]

Agreed on the split, and the message half is in, as b3eb236. Client::execute() now prefers errorDetails.customErrorMessage when the frame carries it, and falls back to the error message otherwise. The outdated-Playwright check still reads the original message, so that path is untouched. Four lines, kept as its own commit so it can be dropped if the maintainers want this PR to stay a pure deletion.

Verified end to end rather than from the schema: repainted a rectangle over the stored snapshot and ran the test. It now fails with 3659 pixels (ratio 0.01 of all image pixels) are different. where it said Expect failed before. Restoring the snapshot passes again. Lint, types and the 357 unit tests are green.

Nothing else in the plugin reads that text, and expectScreenshot is the only call that sends error details, so no other assertion changes.

On the collision: #247 merges over this cleanly. #241 does conflict, but on the metadata timeout lines, and it already conflicted there before this commit. The new lines sit inside the if body itself, which neither PR touches, so this adds no conflict surface.

The image still needs somewhere to live. Separate PR.