pestphp / pestphp/pest

[Bug]: Browser plugin — evaluate()/script() can return another call's result (processResultResponse ignores the request id)

Open
#1,910 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
11.7k
Forks
538
Avg merge
4d 11h
Merged PRs (30d)
8

Description

What Happened

Webpage::script() / Page::evaluate() can return the result of an earlier, unrelated call, because the value is picked by shape instead of by request id.

Client::execute() yields every websocket frame it reads until it sees one whose id matches its own $requestId:

// src/Playwright/Client.php
$requestId = uniqid();
// ...
while (true) {
    $response = json_decode($this->fetch($this->websocketConnection), true);
    // ...
    yield $response;                                  // <- every frame is yielded
    if (isset($response['id']) && $response['id'] === $requestId) {
        break;
    }
}

processResultResponse() then consumes that generator and returns the first frame carrying result.value, without ever comparing id:

// src/Playwright/Concerns/InteractsWithPlaywright.php
private function processResultResponse(Generator $response): mixed
{
    foreach ($response as $message) {
        if (isset($message['result']['value'])) {
            return JavaScriptSerializer::parseValue($message['result']['value']);
        }
    }

    return null;
}

So if a frame belonging to a previous request is still in the stream when the next evaluate() runs, that frame's value is returned as this call's result. The call succeeds and returns plausible data — it is simply the wrong data, which makes it very hard to spot.

Observed in CI. A helper that evaluates window.__pestBrowser.jsErrors returned {text: "…", count: 2} — the return value of the previous script() call in the same test, which read a card's text. Nothing was wrong with the page.

The same root cause surfaces as a hard crash through assertNoJavaScriptErrors():

// src/Api/Concerns/MakesConsoleAssertions.php
expect($javaScriptErrors)->toBeEmpty(sprintf(
    '…: %s',
    // …
    implode(', ', array_map(fn (array $log) => $log['message'], $javaScriptErrors)),
));

Two problems compound here: javaScriptErrors() may receive some other call's value (not a list of array{message: string}), and the failure message is built by sprintf(...) before the expectation runs, so array_map(fn (array $log) => …) throws

TypeError: {closure:…assertNoJavaScriptErrors():86}(): Argument #1 ($log) must be of type array, string given

even when the assertion would have passed. The real value is swallowed and the test reports a TypeError in plugin internals.

How to Reproduce

Any browser test that makes two script() calls in a row can hit it; it is a race, so it reproduces intermittently and much more often on a loaded machine (a shared CI runner). The shape is:

$page = visit('/some-page');

$first  = $page->script('(() => ({ text: "hello", count: 2 }))()');
$second = $page->script('(() => ["a", "b"])()');   // may return {text: "hello", count: 2}

We hit it on a self-hosted runner in roughly 1 of 3 runs of a 164-test browser suite; on a developer laptop it is rare.

Suggested fixes:

  1. processResultResponse() should only accept the frame whose id matches the request that produced the generator (the id is known in Client::execute(), so either filter there or pass it down).
  2. Independently, build the failure message lazily (or defensively) in assertNoJavaScriptErrors(), so a malformed entry cannot turn a passing assertion into a TypeError that hides the value.

Happy to send a PR if the direction above looks right.

Pest Version

pestphp/pest v5.1.3, pestphp/pest-plugin-browser v5.0.1

PHP Version

8.4.23

Operating System

macOS (developer machines) and Linux (CI runner, container image)

Notes

Laravel 13 app, LaravelHttpServer driver, playwright 1.62.1.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/Playwright/Client.php and src/Playwright/Concerns/InteractsWithPlaywright.php, then inspect assertNoJavaScriptErrors() in src/Api/Concerns/MakesConsoleAssertions.php. Reproduce the issue with consecutive script() calls and a loaded browser test environment. Done means responses are matched to the originating request and assertion failures do not raise the reported TypeError, with regression coverage for both cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.