Skip to content

fix: keep the original failure as the previous exception - #268

Merged
MrPunyapal merged 2 commits into
pestphp:5.xfrom
likemusic:fix/preserve-browser-failure-origin
Sep 24, 2026
Merged

MrPunyapal merged 2 commits into
pestphp:5.xfrom
likemusic:fix/preserve-browser-failure-origin

Conversation

@likemusic

Copy link
Copy Markdown
Contributor

Problem

When a browser assertion fails, the exception the test receives has no previous. The error that actually came out of Playwright — and the stack that goes with it — is gone by the time anything can report it.

Measured on 5.x with a probe that catches the failure and prints its internals:

previous: NULL

Root cause

BrowserExpectationFailedException::from() builds a richer message (screenshot path, console logs, JavaScript errors, an exception thrown by the HTTP server) and returns a new ExpectationFailedException:

return new ExpectationFailedException(
    $message,
    $e->getComparisonFailure(),
    $e->getPrevious(), // @phpstan-ignore-line
);

The third argument is $e->getPrevious() — the previous of the original — rather than $e itself. Since the original is typically the root of its own chain, its getPrevious() is null, so the chain ends there and $e is discarded along with its file, line and trace.

The reported location is fine: the new exception's trace still contains the test frame, and PHPUnit filters the plugin's frames out, so the line shown is the call that threw. What is lost is the error underneath it.

This matters most when the failure did not originate in an assertion at all. Protocol errors surface as ExpectationFailedException too (Client::execute() turns any error frame into one), so a Target page, context or browser has been closed or a Playwright timeout reaches the test with its origin stripped — which is part of why transport-level bugs such as #247 and #266 read as application flakiness.

The fix

Pass the original exception:

return new ExpectationFailedException(
    $message,
    $e->getComparisonFailure(),
    $e,
);

After the change the same probe reports:

previous: PHPUnit\Framework\ExpectationFailedException @ src/Api/Concerns/MakesElementAssertions.php:61

The @phpstan-ignore-line is no longer needed and is removed — $e is a Throwable, which the constructor accepts directly, whereas the previous expression needed the suppression.

The visible message is unchanged: from() already copies the original text into $message, so nothing new is printed unless a tool walks the chain.

Tests

tests/Browser/Visit/FailureOriginTest.php fails a real browser assertion and asserts that the exception carries the original one as its previous, with the original message intact. It fails on 5.x as it stands (null is not an instance of ExpectationFailedException) and passes with this change.

composer test:lint, composer test:types and composer test:type-coverage (100%) pass on this branch (PHP 8.4.24, Playwright 1.62.1).

The full suite leaves three failures here — the two IframeTest cases and SingleUrl > it may visit external URLs — all of them Timeout 2000ms exceeded on example.com. They are an artefact of my environment: outbound traffic goes through an authenticated proxy that Chromium does not inherit from the environment. I verified they are unrelated by running those two files with and without this change: 3 failed / 4 passed either way, identical.

likemusic and others added 2 commits September 22, 2026 16:37
`BrowserExpectationFailedException::from()` builds a richer message and
returns a new `ExpectationFailedException`, passing `$e->getPrevious()`
as its third argument rather than `$e`. The original exception is the
root of its own chain, so its previous is null: the error that came out
of Playwright is dropped, along with its file, line and trace.

Protocol errors surface as `ExpectationFailedException` too, so a
closed-target error or a timeout reaches the test with its origin
stripped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@MrPunyapal
MrPunyapal merged commit c98e8a5 into pestphp:5.x Sep 24, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants