fix(e2e): one helper for the reload-until-appears waits, and don't let a transient reload error abort them

[corneliusroemer-agent]

Fixes an integration test flake where a reload issued moments after a navigation gets rejected by the browser, which then aborts the whole wait it was part of.

Also refactors similar polling into a shared helper to reduce verbosity and improve maintainability. Lots of line number reductions in the tests!

Flake looks like this (run 33173217408, chromium, on main):

  1) [chromium-with-dep] › tests/specs/features/sequence-fasta.dependent.spec.ts:6:9 › Sequence FASTA endpoint › returns valid FASTA with CORS headers 

    Error: page.reload: Protocol error (Page.reload): Not attached to an active page
    Call log:
      - waiting for navigation until "load"


       at pages/search.page.ts:307

      305 |             .poll(
      306 |                 async () => {
    > 307 |                     await this.page.reload();
          |                                     ^
      308 |                     accessions = await this.getAccessionVersions();
      309 |                     return accessions.length;
      310 |                 },
        at _test.expect.poll.message (/home/runner/work/loculus/loculus/integration-tests/tests/pages/search.page.ts:307:37)
        at SearchPage.waitForSequencesInSearch (/home/runner/work/loculus/loculus/integration-tests/tests/pages/search.page.ts:304:9)
        at /home/runner/work/loculus/loculus/integration-tests/tests/specs/features/sequence-fasta.dependent.spec.ts:9:35

Details

What is actually going on

The message is misleading twice over: nothing was slow, and nothing was missing from the search results.

The reload was rejected 2.6 ms after it was sent. The line before it had just clicked the "Ebola Sudan" link, which navigates to /ebola-sudan and gets redirected to /ebola-sudan/search; the reload arrived about 2 ms after that new page's response finished, while the browser was still swapping in the new frame. Chrome refuses a reload during that window, and the phrase it uses is "Not attached to an active page". The page was fine before and after — the trace still has video frames from it after the error, and the sibling test three lines down in the same file did the exact same thing successfully 3.5 seconds later.

The reason a 2.6 ms hiccup ends a 60 second wait is the part worth knowing: expect.poll retries while the matcher fails, but if the function being polled throws, the error escapes immediately. The timeout and intervals never come into play, and the poll's own message is never printed. I measured this against the exact Playwright version we use — a function that throws on its first call ends the poll in 3 ms. So any polled function that does something, rather than just reading a value, needs to handle its own errors.

That is also why the failure is so hard to read: the reported error is the reload, not the wait, so the log looks like a page that died rather than a wait that was interrupted. The giveaway is in the line above it in the job log — the test is recorded as taking (1.0s), where a genuine timeout would say (60.0s).

Every page.reload() in the integration tests (15 of them) sits inside a loop that reloads until something appears, so all of them can be killed this way. One of them is in readonly.setup.ts, where an abort fails every dependent test in the run.

The waits were also written out longhand fifteen times, in two shapes: an expect.poll block, and a bare while (!visible) { reload; wait 2s }. They are now reloadAndPoll(page, read, { message }) and reloadUntilVisible(page, locator, { message }).

How rare is it

Once, in the 179 failed test jobs I could still get logs for (9 days, 359 finished runs). Logs expire after two to three weeks so that is as far back as it goes.

It is worth fixing anyway because the cost of the fix is one extra loop iteration, and because the failure it produces is actively misleading — it points at a page that has died rather than at whatever the test was waiting for.

Things you might want to decide differently

I match on the error text rather than catching everything. Catching everything is shorter, but a real failure like a refused connection would then retry silently until the wait's timeout with its cause thrown away, instead of failing straight away and saying why. Matching text means we may meet a fourth wording eventually and have to add it; happy to swap it for a blanket catch if you would rather have that.

The console.warn is there so a burst of these still shows up in the job log rather than vanishing. It is Node-side, so it does not reach the console-warnings fixture, which only listens to messages logged inside the browser page — pages/CliPage.ts already logs this way. Easy to drop if it looks noisy.

The nine while loops previously had no timeout at all, so they could only fail by using up the enclosing test's whole budget. They now get the helper's default of 90 seconds, which is the more generous of the two values the existing polls of this same "wait for a released sequence to show up" kind already use. This is a behaviour change, not a free win, for two of them: sequence-version-banners has a 300 second test budget and override-hidden-fields has 200, so a wait that legitimately took 150 seconds used to pass and would now fail at 90. I think 90 is right — nothing in this suite should need two and a half minutes for a sequence to appear in search, and if something does I would rather see it named than see a bare test timeout — but say the word and I will raise it. For lineage-field (95 second budget) and submission-flow (120) the test timeout fires first regardless, so bounding them changes only the error message. The ninth is in search.page.ts, whose callers set their own budgets.

Left alone deliberately

Separately, and not fixed here: waitForSequencesInSearch polls for up to 60 seconds while the per-test timeout is also 60 seconds, so the test always runs out of time first and the poll's message ("Expected at least N sequences to appear in search results") can never actually be printed. That means a genuinely slow indexing run cannot report itself as slow indexing — it reports as a bare test timeout. waitForAccessionVersionInSearch has the same problem. Worth a follow-up; it did not cause this failure.

This does not overlap with #7198, which fixes a different flake in the same file and does not touch any reload.

🚀 Preview: Add preview label to enable

[claude]

This PR may be related to: #5118 (epic: Unflake integration tests) — this fix addresses one instance of test-infra flakiness that the epic is tracking.

[claude]

Claude finished @corneliusroemer's task in 3m 23s —— View job

---

Review

• Read the diff
• Review the new helper (utils/reload-helpers.ts)
• Check call-site behaviour changes
• Check CI
• Post review

The diagnosis is right and unusually well evidenced — expect.poll really does propagate a throw from its callback instead of retrying it, so any polled function that acts rather than just reads needs its own error handling. Collapsing fifteen longhand waits into two helpers is a clear win.

Three inline comments, one of which I think matters:

1. reload-helpers.ts — the tolerance stops at the reload, but read() runs immediately afterwards against the same still-swapping frame and can throw the same three errors. The abort is still reachable, just attributed to the read.
2. search.page.ts:300 — re-reading after the poll means the returned array is no longer the one the assertion passed on; callers that destructure [{ accession }] would get a TypeError instead of a wait failure.
3. readonly.setup.ts:88 — worth keeping the message where the matcher output alone doesn't say what is being counted.

Two smaller things not worth their own threads:

• reloadAndPoll returns an unchained expect.poll builder, so a caller that forgets the matcher gets a silent no-op — the reload never runs and the test passes. no-floating-promises won't catch it, since expect.poll returns a proxy rather than a promise. Only two call sites today, so low risk; just something to keep in mind if the helper spreads.
• The nine converted while loops go from a flat 2s interval to [2000, 5000], i.e. 5s from the second retry on. Negligible, but it does mean a sequence that appears at t=3s is now noticed at t=7s.

On the three questions in the description: matching on error text over a blanket catch is the right call for exactly the reason you give — a refused connection should fail loudly and immediately, not retry silently for 90 seconds with its cause discarded. The console.warn seems fine and precedented. And 90s reads right to me; if something genuinely takes longer than that for a sequence to hit search, a named failure is more useful than the bare test timeout those two tests would produce today.

I couldn't run the suite (no permission to npm ci here). Format Check passed on CI; Integration tests were still running when I looked.
· branch fix/integration-tests-reload-race

[Copilot]

Pull request overview

This PR hardens Playwright integration tests against a Chromium-specific transient page.reload() failure that can abort expect.poll waits, and consolidates repeated “reload-until-condition” patterns into a shared helper to reduce duplication across the e2e suite.

Changes:

• Added reloadAndPoll / reloadUntilVisible helpers that retry page.reload() on known transient reload errors and centralize polling intervals/timeouts.
• Refactored multiple specs/fixtures/page-object waits to use the new helpers instead of bespoke expect.poll blocks and ad-hoc while loops.
• Updated SearchPage waiting utilities to use the shared helper for reload-based polling.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
integration-tests/tests/utils/reload-helpers.ts	Introduces shared reload+poll helpers and transient reload error handling.
integration-tests/tests/specs/features/submission-flow.spec.ts	Replaces custom reload polling with reloadUntilVisible.
integration-tests/tests/specs/features/sequence-version-banners.spec.ts	Uses reloadUntilVisible for reload-based waits.
integration-tests/tests/specs/features/search/override-hidden-fields.spec.ts	Switches long-running reload loops to reloadUntilVisible (now default-timeout bounded).
integration-tests/tests/specs/features/search/lineage-field.spec.ts	Refactors reload loop into reloadUntilVisible.
integration-tests/tests/specs/features/review-restricted.spec.ts	Refactors poll into reloadUntilVisible with explicit timeout.
integration-tests/tests/readonly.setup.ts	Uses reloadAndPoll for setup-time reload polling.
integration-tests/tests/pages/search.page.ts	Refactors page-object reload/poll waits to use reloadAndPoll / reloadUntilVisible.
integration-tests/tests/fixtures/sequence.fixture.ts	Uses reloadUntilVisible and removes now-unused expect import.

Suppressed comments (1)

integration-tests/tests/specs/features/search/override-hidden-fields.spec.ts:83

• This wait is now capped by reloadUntilVisible’s 90s default, which conflicts with the test’s own comment/timeout indicating this flow can legitimately exceed 90s in CI. Consider passing an explicit timeout here as well.

    await reloadUntilVisible(page, page.getByRole('cell', { name: '2012-12-13' }));

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[theosanderson]

lgtm