Skip to content

fix: ignore Playwright replies that belong to an earlier request - #284

Open
aimerge-studio wants to merge 1 commit into
pestphp:5.xfrom
aimerge-studio:fix/stale-replies
Open

aimerge-studio wants to merge 1 commit into
pestphp:5.xfrom
aimerge-studio:fix/stale-replies

Conversation

@aimerge-studio

Copy link
Copy Markdown

Problem

On a slow machine or CI runner, tests that call refresh() (or navigate) intermittently fail with

Pest\Browser\Playwright\Page::javaScriptErrors(): Return value must be of type array, int returned

or with an assertion receiving a value that belongs to the previous call.

Cause

Client::execute() yields every message it reads until it sees its own reply, and throws on any message that carries an error, whichever request that error belongs to.

  1. Page::reload() sends reload with waitUntil: load. The loop breaks on the matching loadstate event, which can arrive before the reload's own reply. That reply stays in the socket. With the 1 s per-attempt timeout of waitForExpectation(), it arrives later as Timeout 1000ms exceeded.
  2. The next request, e.g. an evaluateExpression, reads that stale error first and throws. The attempt counts as failed and is retried.
  3. The first evaluateExpression's reply is still unread. The retry reads it as its own result, because processResultResponse() takes the first result.value it sees. From then on every call receives the previous call's reply, until the test ends. javaScriptErrors() receives the integer meant for the preceding assertScript().

Instrumented trace from a failing run (ids shortened):

SEND req=…b83e2 reload
     req=…b83e2 reload  method=loadstate add=load           <- loop breaks here, before its own reply
SEND req=…cba3 evaluateExpression
     req=…cba3  ERROR msg_id=…b83e2 Timeout 1000ms exceeded.  <- the reload's late reply fails this evaluate
SEND req=…1cde evaluateExpression                           <- retry
     req=…1cde  msg_id=…cba3 result.value                    <- receives the previous evaluate's value

Fix

Skip any message whose id is set and differs from the current request's id. Such a message answers a request that has already stopped reading; it must neither fail the current request nor be taken as its result. Events carry no id and pass through unchanged, so waitUntil and __create__ handling keep working.

Tests

tests/Unit/Playwright/ClientTest.php feeds execute() a scripted socket: a stale error and a stale value with foreign ids, an event, then the request's own reply. It asserts that only the event and the own reply are yielded, and that a request's own error still throws.

In the project where this showed up, the affected browser test failed 5 of 9 runs before the change and passed 6 of 6 after it.

🤖 Generated with Claude Code

A navigation with waitUntil returns on its load event, which can arrive
before its own reply. That reply stayed in the socket, and the next
request read it: a late error failed an unrelated request, and a late
value was taken as its result, so every later call received the
previous call's reply.
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.

1 participant