Make retry stop when the time it was given is up (#96354)
How long do you expect this to wait at worst (before failing)?
```js
await retry(async () => {
expect(await browser.elementById('never-appears').text()).toBe('x')
}, 15_000)
```
After this PR, the answer is 15 seconds. (Well, technically, 15 seconds
+ whatever last attempt is, so 20.)
But before this PR, the answer was 3 minutes.
## What?
There's a bug (IMO) in `retry`. Instead of waiting for the time you give
it, it counts tries.
For example, `retry(fn, 15_000)` reads as "keep trying for 15 seconds".
However, what it actually was doing is divide 15000 by the 500ms
interval and try 30 times, sleeping 500ms in between every try.
This is a problem because **it wasn't measuring the stall time of each
try itself.**
When a try is cheap, it doesn't matter. But a try that looks for an
element in the browser isn't cheap: if the element isn't there, the
selector waits 5 seconds before giving up.
In the example above, each try sits in the selector for its full 5
seconds, and there are 31 of these intervals, plus the 15 seconds of
sleeping in between. This ends up waiting 170103ms, i.e. three minutes
for a wait that says 15 seconds.
Jest kills the test at its own timeout long before that, and the actual
error gets thrown away. What's left is `Exceeded timeout of 120000 ms`,
which doesn't say which assertion never came true. In the one I measured
it was `page.waitForSelector: Timeout 5000ms exceeded`. My agent lost an
hour trying to make sense of this twice now.
## Fix
We count time now. After the fix, the retry watches the clock and stops
when its time is up. It still always makes at least one try. The
interval no longer has to divide the duration, because nothing counts
steps any more. If the last try is slow, we're gonna wait for that last
try to end (so the timeout is still not *strictly* respected) so that we
can show the error in the correct test.
Cheap waits behave exactly as before. In my local run, a try that had to
repeat cost 251ms at the median, 459ms at the worst. So the only calls
this changes are ones that were already failing, and those now fail with
their own error instead of a test timeout. One flaky dev test that used
to die at 120 seconds with nothing to show now fails in 19.5 seconds
saying `Failed to retry within 15000ms (waited 16022ms)`, next to nine
tests that take 2.2 seconds each.
I added some regression tests. Since it's a test helper, a unit test
seemed fine.
## What this means for existing tests
The catch is that a call whose tries are slow now gets less time than it
used to, and this helper has 2110 call sites, so CI is the real check.
It found one file: the hash scroll tests in `navigation.test.ts` wait
with the default 3 seconds, and each try there costs 150-300ms on a
runner, so they were really getting about 5. They need more than 3, so
they failed on two dev shards. I gave them 10.
If this is green, we can merge it in isolation from the rest of the
stack.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
