Skip to content

fix: await the matching navigation command response - #266

Merged
MrPunyapal merged 2 commits into
pestphp:5.xfrom
cyppe:fix/await-navigation-command-response
Oct 26, 2026
Merged

MrPunyapal merged 2 commits into
pestphp:5.xfrom
cyppe:fix/await-navigation-command-response

Conversation

@cyppe

@cyppe cyppe commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

A test can continue while its requested page is still loading. Client::execute() currently treats any incoming load-state event matching waitUntil as completion of the navigation, even when that event belongs to another document or frame. This PR makes navigation wait for Playwright's response to the command that initiated it.

How it works today

Page::goto() and Page::reload() send a command with a unique request ID and waitUntil: 'load'. The WebSocket carries both command responses and asynchronous browser events. While reading that stream, Client::execute() stops on either:

  • A response whose ID matches the command's request ID.
  • Any event whose params.add matches waitUntil.

The second condition does not establish that the requested navigation completed. For example:

Pest sends:       goto('/products'), request ID A, waitUntil 'load'
Playwright emits: load-state event from the initial document or a child frame
Pest returns:    visit() is considered complete
Test executes:   a script or accessibility check
Playwright sends: the actual response to request A

The next test action can therefore run against the initial blank document or during navigation. The real command response is also left unread; if navigation failed, its error is not reported by the navigation call that caused it.

We encountered this while expanding an application's browser suite to Firefox: accessibility checks saw <html><head></head><body></body></html>, and other checks failed with an execution context destroyed during navigation. The deterministic unit tests below reproduce the underlying response-ordering bug independently of that application.

How it works after this PR

Client::execute() continues consuming events until it receives the response with the current command's request ID. Playwright already implements waitUntil for the navigation command, so that response is the completion boundary:

Pest sends:       goto('/products'), request ID A, waitUntil 'load'
Playwright emits: unrelated load-state events; Pest keeps reading
Playwright sends: response to request A after its navigation condition is met
Pest returns:    the next test action can run against the requested document

If the command returns an error after an unrelated load event, the existing error handling now reports it during the navigation call. The fix removes the event-based early exit; it adds no sleeps, extra readiness conditions or changes to the public browser API.

Regression coverage

  • Two deterministic unit cases (goto and reload) deliver initial-document and child-frame load events before the matching command response. Both prove that the client consumes the actual response.
  • A third unit case delivers a load event followed by a navigation error and verifies that the error is raised.
  • A browser test reads document.title immediately after visit() and navigate(), without an intervening element assertion that could hide premature completion.

All three unit cases fail before the fix and pass afterward. The browser regression passes in Chromium, Firefox and WebKit with Playwright 1.63.0. The full composer test run reports 344 passed and 27 platform skips; lint, type coverage and PHPStan pass. The existing PHPUnit cacheResult deprecation is still reported.

@likemusic

Copy link
Copy Markdown
Contributor

Tried this against 5.x together with #247, since both change the same loop in Client::execute(). Result: they are compatible, and the combination is coherent.

Applied on top of a78ede5 (current 5.x), #247 first, then this one — both apply cleanly, no conflict. What the loop becomes:

// from #247
if (isset($response['id']) && $response['id'] !== $requestId) {
    continue;
}
…
yield $response;

// from this PR
if (isset($response['id']) && $response['id'] === $requestId) {
    break;
}

That is one rule stated twice from both ends — wait for your own response, skip what belongs to someone else — so the two PRs reinforce rather than overlap.

Measured on PHP 8.4.24 / Playwright 1.62.1 / Chromium, Linux:

  • All 8 tests from both PRs pass together with both applied (19 assertions).
  • The unit tests are genuine regression tests: with the patches reverted and only the test files kept, 5 of 7 fail. They pin the behaviour precisely.
  • Full suite with both applied: 347 passed, 27 skipped, 1 failed — the one failure is SingleUrl > it may visit external URLs, an artefact of my environment (outbound traffic goes through a proxy Chromium does not inherit). Nothing else regressed.

One caveat about the browser test in this PR, offered as data rather than criticism: NavigationCompletionTest passes on unpatched 5.x here — 3 runs out of 3, on Chromium. So on this setup it does not demonstrate the bug, and would not catch a regression of it. The unit test does, reliably. You mentioned hitting this while expanding a suite to Firefox, so the browser-level timing may well differ there; worth noting in case a reviewer runs it on Chromium and concludes the bug is not real.

For what it is worth, I also tried building a browser-level repro of the stranded-frame symptom from scratch (a navigation deliberately left unsettled at the end of a test, then a clean test after it) and could not make it fail on demand either. The unit-stub approach both of you took looks like the right call.

@Caio-Tera

Copy link
Copy Markdown

Another independent reproduction on v5.1.2 (Chromium, Playwright 1.62, --parallel), with a deterministic test that needs no machine load:

Route::get('/slow', function () { usleep(1_500_000); return 'slow'; });

$page = visit('/fast');
$page->navigate('/slow');                    // /slow is served twice
$answers = array_map(fn ($i) => $page->script("() => {$i}"), range(1, 5));
// v5.1.2: [1, 1, 2, 3, 4]: every call reads the previous call's reply

Red 3/3 on v5.1.2. With
#280, #266, #247 and #262 applied together on top of v5.1.2 they apply cleanly, the test goes green 3/3, and our app’s browser suite (realtime tests that were flaky under --parallel) passed 5/5. The four fix different links of the same chain: the retried goto (#280), the goto that returns on load before its own reply (#266), stale frames read by the next call (#247), and processors returning before their own frame (#262).

Heads-up: #277 (merged 2026-10-25) touched the same loop in Client::execute(), so #247 now conflicts with 5.x.

@MrPunyapal
MrPunyapal merged commit 8347da2 into pestphp:5.x Oct 26, 2026
2 checks passed
@MrPunyapal

Copy link
Copy Markdown
Member

Thanks

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.

4 participants