Skip to content

fix: give each retry attempt the configured timeout, not a fixed second - #276

Open
joshdaugherty wants to merge 1 commit into
pestphp:5.xfrom
joshdaugherty:fix/per-attempt-timeout
Open

joshdaugherty wants to merge 1 commit into
pestphp:5.xfrom
joshdaugherty:fix/per-attempt-timeout

Conversation

@joshdaugherty

Copy link
Copy Markdown

What happens

Execution::waitForExpectation() runs each attempt under a hardcoded Playwright::usingTimeout(1_000, ...). Playwright::setTimeout() only decides how long the loop keeps retrying, and only the final call after the loop gets the configured value. So any page method still routed through the loop that takes more than a second is cut off and run again, even when it fits the configured timeout.

#269 fixed this for click() and the other actions by taking them out of the loop. Everything still in the loop keeps the fixed one-second attempt, and submit() is still in it, although it clicks the form's submit button.

Measured on v5.0.0 with setTimeout(5000), one call each, with a handler that blocks for 1.5 seconds and counts in the page how often it ran. Same result in each of 3 runs:

call runs, v5.0.0 runs, with this change
click() (for comparison; #269 fixes it on 5.x) 5, in 8.0 to 8.4s 1, in 1.8 to 1.9s
submit() 5, in 7.7s 1, in 1.5s
script() 6, in 9.0s 1, in 1.5s

The fix

  • Each attempt gets what is left of the configured timeout instead of a fixed second. An operation that fits the timeout now finishes on its first attempt. Clamping to the remaining time keeps the loop inside the timeout, so an assertion that can never pass still fails in the same bounded time. Retrying is unchanged: an assertion still waits for content that appears later.
  • submit() joins the methods that are not retried, next to click().

This is narrower than #249, which also takes more actions out of the loop, and the two do not conflict.

Tests

  • SubmitTest: a submit handler that takes 1.5s runs once. A plain submit test is added too, since submit() had none.
  • ScriptTest: a script that takes 1.5s runs once. A plain script() test is added too.
  • AssertSeeTest: text that appears after 1.2s is still seen, so retrying still works.

Against unchanged 5.x source, the two "runs once" tests fail and the other new tests pass. The submit() call itself fails there with Timeout 2000ms exceeded, after being retried.

On this branch: the full suite passed 344 tests with 27 skipped. Three tests failed: two iframe tests and "it may visit external URLs". All three load external URLs and fail the same way on unchanged 5.x on this machine. PHPStan and type coverage (100%) pass, and Pint reports nothing on the changed files. Run on Windows 11 with PHP 8.4.

Relates to pestphp/pest#1755.

waitForExpectation() ran every attempt under a hardcoded 1000ms, so
Playwright::setTimeout() only set how long the loop kept retrying. An
operation that fits the configured timeout but takes more than a second
was cut off and run again: a slow script() ran several times, and a slow
submit() submitted several times before timing out.

Each attempt now gets what is left of the configured timeout, so such an
operation finishes on its first attempt. Clamping to the remaining time
keeps the loop inside the timeout, and retrying is unchanged, so an
assertion still waits for text that appears later.

submit() clicks the form's submit button, so it joins click() and the
other actions that are not retried.
@likemusic

Copy link
Copy Markdown
Contributor

Independent reproduction of the remaining half at the current 5.x tip, on a Linux kernel — though with a caveat about how independent it really is.

Environment: WSL2 (Linux 6.18 kernel in a VM, on a Windows host), PHP 8.4.24, Playwright 1.62.1, plugin 5.x at c98e8a5, unchanged source, the suite's own Playwright::setTimeout(2000) from tests/Pest.php. Worth stating plainly: WSL2 is a real Linux kernel and a real Linux userland, but the host is still Windows, so this confirms the behaviour on a second runtime rather than on a genuinely different class of machine. A bare-metal Linux or macOS run would add more than this one does.

The page records every entry and exit itself, so the count does not depend on reading the plugin:

Route::get('/', fn (): string => '<span id="marker">ok</span>
    <script>
        window.log = [];
        window.slow = function () {
            window.log.push("enter@" + Math.round(performance.now()));
            const until = performance.now() + 1500;
            while (performance.now() < until) {}
            window.log.push("leave@" + Math.round(performance.now()));
            return "done";
        };
    </script>');

$page = visit('/');
$page->script('window.slow()');            // 1500ms of work, configured timeout 2000ms
$page->script('window.log.join(" | ")');

Three consecutive runs, identical result each time:

enter@51 | leave@1551 | enter@1552 | leave@3052 | enter@3053 | leave@4553
elapsed 4.52s, then: Timeout 2000ms exceeded.

So the script ran three times, back to back, and the call still failed — with work that fits the configured timeout twice over. The failure is the part I had not expected from reading the code: the caller does not merely wait longer, it gets an exception for an operation that succeeded.

A fourth run of the same test, the first one on a fresh checkout, ran it twice and returned 'done' after 9.69s. So the count is timing-dependent (2 to 3 here against your 6 at setTimeout(5000), which is consistent — a wider window fits more attempts); the direction is not.

One thing worth adding for anyone reading this after #269: the append() reproduction from pestphp/pest#1755 is green now, on this same checkout — 1.73s, one execution — because #269 took append out of the loop. That is what isolates the case above. Every method #269 did not name still spends its side effects on retries, and script() is the sharpest example, since a script can do anything.

Happy to run anything else here if that helps.

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