Skip to content

fix: a flaky plain-link test aborts the whole Browser CI job #1135

Description

@vivek7405

Problem

The positive-control test in packages/core/test/routing/browser/router-js-handled.test.js (L74-84) intermittently aborts the entire Browser CI job, most often on Firefox. When it fires, the run reports 0 failed in all three browsers and then exits 1 with a bare Error while running tests., which is close to the worst possible failure signature: the summary says everything passed, so the natural reading is an infrastructure blip rather than a test problem.

The actual line in the log is:

❌ Tests were interrupted because the page navigated to http://localhost:8000/plain-link-target.
   This can happen when clicking a link, submitting a form or interacting with window.location.  (failed on Firefox)

The test renders a real anchor and clicks it:

render(html`<a href="/plain-link-target">go</a>`, container);
container.querySelector('a').click();
await tick();
assert.ok(fetched.some((u) => u.includes('/plain-link-target')), ...);

The assertion is legitimate and worth keeping: it proves the client router still SPA-navigates a plain link, so the @click-preventDefault fix it guards did not break progressive enhancement. The problem is the failure MODE. The router prevents the default only when it handles the click; whenever it loses that race (slower engine, a slow module load, an unlucky tick), the browser performs a real navigation, web-test-runner's page goes to /plain-link-target, and the whole session dies. One racy click takes down 60 unrelated test files.

Observed on main (the chore: release core 0.7.45 run) and repeatedly on the PR #1126 branch, where it blocked a merge and needed manual job re-runs. It is timing-dependent, not branch-dependent.

Design / approach

Keep the assertion, remove the ability of this test to nuke the run.

Add a capture-phase preventDefault on the container (or document) for the duration of this test only, so the browser can never perform the navigation, while the router's own handling is still observable through the existing fetched spy. Capture phase runs before the router's bubble-phase listener, and preventDefault() does not stop propagation, so the router still sees the click and still records the fetch. The assertion therefore keeps its full meaning:

  • router intercepted, fetched contains the URL, test passes (safety net was redundant);
  • router did NOT intercept, fetched is empty, test fails normally with a useful message instead of aborting the session.

This is the same technique already used in website/test/components/browser/docs-drawer.test.js (its blockNav capture listener in setup), added for exactly this reason when that suite hit the same wall.

Worth auditing the sibling cases in the same file while there: the form-submission tests (L62-72) have the same shape and the same latent failure mode if a submit ever escapes the handler.

Implementation notes (for the implementing agent)

Where to edit:

  • packages/core/test/routing/browser/router-js-handled.test.js, the test at L74-84 ('positive control: a plain <a href> link IS still SPA-navigated by the router'). The file's setup() / teardown() helpers are at the top of the suite; the cleanest place for the guard is inside setup() (registered) and teardown() (removed), so every test in the file is protected rather than just this one.
  • Reference implementation of the same guard: website/test/components/browser/docs-drawer.test.js, the blockNav listener in its setup / teardown (added in PR feat(website): serve the UI gallery at webjs.dev/ui #1126).

Landmines / gotchas:

  • Use the CAPTURE phase. A bubble-phase preventDefault may run after the router's own listener depending on registration order, and the point is to be unconditionally earlier than the browser's default action while still letting the router observe the event.
  • Do not use stopPropagation. That would prevent the router from seeing the click and would silently turn the positive control into a test that asserts nothing (it would fail, but for the wrong reason, and a future reader would "fix" it by deleting the assertion).
  • Do not simply change the href to # or a javascript: URL. The test's value is that it exercises a REAL same-origin path the router should intercept; a fragment link takes a different code path in the router.
  • The 0 failed + exit 1 signature means anyone triaging this from the CI summary alone will conclude it is infra. Consider whether the runner config can be made to surface it better, but that is secondary to fixing the test.
  • web-test-runner aborts the whole SESSION, not the file, so this is a single point of failure for all 61 browser test files.

Invariants to respect:

  • Do not weaken the assertion. AGENTS.md's testing rule is explicit that a browser-facing behaviour needs a browser assertion; this test is the browser assertion for "the router still SPA-navigates a plain link".
  • packages/ is plain .js with JSDoc; no .ts in this tree.

Tests + docs surfaces:

  • The change is itself in the test layer. The counterfactual: temporarily neuter the router's click interception (or point the spy at a URL it will not record) and confirm the test now FAILS with its assertion message rather than aborting the run.
  • Run the full browser suite several times (npm run test:browser) to gain confidence the abort no longer reproduces; the flake is timing-dependent so a single green run proves little.
  • No docs surface: this is test-internal.

Acceptance criteria

  • The plain-link positive control can no longer navigate the test runner page, on any of the three browsers
  • The assertion still proves the router SPA-navigated the link (it fails when the router does not)
  • A counterfactual shows the test failing with its own assertion message rather than aborting the session
  • The sibling form tests in the same file are audited for the same latent failure mode
  • The full browser suite passes repeatedly, including on Firefox

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

Status
Todo

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions