Avoid retrying side-effecting browser actions#227
Conversation
|
Thanks for this, the PR here appears to have solved every issue I’d been having with duplicate clicks, form input etc. with the browser plugin and hasn’t introduced any negative side effects to any of my tests. Now tests that were sometimes flakey because the browser plugin would double click or type text multiple times sometimes are working as expected every time, hopefully this can be merged soon. |
|
Same here, would be amazing if this PR can be looked at. |
|
Again thanks for this, it does fix the navigation-hang case. But I'm following up on my previous comment because when I applied this PR to my test-suite I got two deterministic regressions, both on a Both pass on stock 4.x and fail (15s timeout on the type) with this PR, reproducibly in isolation. The retry loop was silently compensating for a race: GuessLocator resolves the target eagerly (via ->count()), and if the reveal-click's effect hasn't landed yet, it falls back to getByText() and hangs. Run-once removes that cushion, so the race surfaces. The underlying issue is really navigation-specific, so disabling retries for every action feels broader than needed. Would an opt-in ->press('Submit', ['noWaitAfter' => true])
->assertPathIs('/next-page'); In my tests that fixes the hang (the navigating press opts out of the nav-settle wait; assertPathIs is still retried and waits for the URL to commit) without touching the type-after-reveal tests. |
|
Made a PR for that if it would be the preferred solution |
Summary
I had an issue where
click()was timing out on some pages, even though the click visibly occurred in the browser when using --debug.This appears to relate to the behaviour described in:
Related to pestphp/pest#1511
What changed
This PR updates
AwaitableWebpageso state-changing browser actions are no longer routed throughExecution::waitForExpectation().This includes actions such as:
clickpressfillcheckselectdragattachAssertions and read-style expectations still use the existing retry loop.
Why
Execution::waitForExpectation()retries the callback using a 1000ms Playwright timeout. That behaviour is useful for assertions, because repeatedly checking whether something is true is usually safe.For example, retrying an assertion such as
assertSee()is useful because the page may need time to update.However, this retry behaviour is less suitable for browser actions. Actions such as
click()andpress()can change the page, trigger JavaScript, submit forms, start navigation, or update application state.In these cases, the action may already have succeeded, but Pest may still report
Timeout 1000ms exceededif the operation did not complete within the 1000ms attempt window. Retrying the action can then cause further issues because:This PR avoids the pattern where a browser action visibly succeeds, but Pest still fails because the action was handled through the expectation retry loop.
Behaviour after this change
State-changing browser actions are attempted once and use the configured Playwright/browser timeout.
Assertions continue to be retried until the configured timeout expires.
This preserves the useful retry behaviour for assertions, while avoiding unsafe retries for actions that may alter browser or application state.
Side-effecting / state-changing actions
A side-effecting action is an operation that changes something.
In browser testing, this means the command does not merely observe the page. It may alter:
Examples include
click,press,fill,check,select,drag, andattach.These actions should be awaited using the configured action timeout, but not retried in the same way as assertions.