ReviewOS

stacks/stx

test(signals): fetch-interceptors records one test's request against another, and fails under --randomize

#2003
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Summary

packages/stx/test/signals/fetch-interceptors.test.ts fails intermittently because a request made by one test is recorded against another. It is invisible in file order and reproduces readily under --randomize, which is also why the suite's assertion count drifts run to run while every test still passes.

Reproduction

bun test packages/stx/test/signals/fetch-interceptors.test.ts --randomize

3 to 4 runs in 6 fail. Two of the failures seen:

(fail) configureFetch reaches every primitive (#1855) > adds a header to useMutation
  expect(seen).toHaveLength(1)   Expected length: 1   Received length: 2

(fail) a headers function is resolved (#1855) > useMutation evaluates headers per request
  expect(headerOf(seen[0], 'X-Trace')).toBe('m')   Expected: "m"   Received: undefined

The second is the clearer symptom: seen[0] is not the request that test made. It belongs to an earlier test and arrived first.

Cause

Most tests in the file fire a request without awaiting it — g.window.stx.useFetch('/api/a', { immediate: false }).refetch() is the shape under test — and the error-path tests leave a primitive that retries. Those primitives stay alive after their test ends, and the globalThis.fetch mock installed by the next test records whatever they do next.

So the next test sees a request it never made, and seen[0] / toHaveLength(1) describe the wrong thing.

What does not fix it

Two attempts, both measured over 6+ randomized runs:

  1. Capturing a fresh array per test in the mock's closure (so a straggler lands in the array belonging to the test that caused it). Sound for a request that completes between tests, and it changes nothing measurable: 4/6 runs still failed, against 3/6 without it. The dominant path is a live primitive firing during a later test, which reaches that test's mock however the array is held.
  2. Draining in afterEach (60ms before restoring fetch). 3/8 still failed. The stragglers are not merely in flight; they are periodic.

What would fix it

The requests have to be attributable to the test that caused them, or the primitives have to stop when their test does:

  1. Dispose the primitives between tests. The real defect — a useQuery or useFetch outliving its test and retrying into the next one is also what an application would hit on unmount. Worth checking whether the runtime exposes a way to stop one; if it does not, that gap is the more interesting bug.
  2. Or give each test its own URL and assert on the entries matching it rather than on seen[0] and seen.length. Mechanical, but five assertions read URLs back (:138, :449, :458, :468, :475) and one of those exercises a derived URL, so it needs care rather than a blanket rename.

Why it matters beyond this file

The full suite's expect() count varies run to run — 32,618 / 32,623 / 32,624 / 32,629 across four runs — while the same 13,718 tests pass. Every directory is stable in isolation, so at least one cross-file interaction of this kind is live in CI today, and a suite that varies is a suite that will eventually hide a real regression rather than report it.

9 days ago

Fixed in 0de85a0 — 0/12 randomized runs fail, against 3–4/6 before.

Two leaks, and the first masked the second.

Primitives outlived their test. They are built outside any scope, so their onDestroy lands on the runtime's global queue and nothing in a test drains it: a useFetch kept its AbortController, a useQuery kept its refetchInterval and its visibilitychange listener, and both went on firing into whichever fetch mock was installed next. afterEach now runs that queue. An application never sees this — a scope tears down its own callbacks and stx:load drains the global ones.

The CSRF prime config was sticky. csrf settings persist across configureFetch({}) by design (installing an Authorization hook is not a request to stop sending the token), so the prime block left prime: '/api/forms/abc' installed and a later mutation issued a priming GET before its own request. That was the second call the failing assertions were counting — not a straggler at all.

Restored to enabled-with-defaults rather than csrf: false: a later test expects the token still echoed, so turning it off simply traded one order dependency for another. That was my first attempt, and running it 12 times is what caught it.

Also rejected along the way, both measured rather than reasoned about: capturing a fresh seen array per test in the mock's closure (4/6 still failed, against 3/6 without — noise), and draining 60ms in afterEach (3/8 still failed).


A second, unrelated order dependency is still open, found by randomizing the whole directory rather than the one file:

bun test packages/stx/test/signals --randomize
(fail) preferred media runtime composables > exposes preferred media helpers as callable signals
  expect(preferredDark()).toBe(true)   Received: false

Passes alone; fails 7/8 randomized directory runs. It stubs window.matchMedia in beforeAll and useMediaQuery reads window.matchMedia at call time with no caching, so at failure time the stub is not in place — which points at window being replaced between files rather than at the composable. Pairing it with each file that ran immediately before it in a failing run does not reproduce, so it is cumulative rather than one bad neighbour.

Leaving this issue open for that.

9 days ago

Both halves are fixed. Closing.

The half this issue describes

0de85a095e test(signals): stop fetch-interceptors leaking state between its own tests fixed it: primitives built outside any scope kept their AbortController, refetchInterval and visibilitychange listener and went on firing into the next test's fetch mock, and a sticky CSRF prime config issued an extra priming GET that the failing assertions were counting.

Re-measured on main at 0.2.345:

bun test packages/stx/test/signals/fetch-interceptors.test.ts --randomize
  run 1..6:  32 pass  0 fail

6 of 6, against the 3 to 4 in 6 that failed when this was filed.

The half that commit handed to this issue

That commit's message recorded a second, unrelated order dependency in preferred-media.test.ts and left it here. Fixed in 76fcc5651d.

Same shape, state that outlives the test that set it. The preference map is seeded once at describe scope:

const matches = new Map<string, boolean>([
  ['(prefers-color-scheme: dark)', true],
  ...
])

and the "updates media query signals when the browser preference changes" test flips dark to false to prove a change propagates, without putting it back. In file order the first test read true and passed before the flip ever happened. In any order where it ran second it read the leftover, so expect(preferredDark()).toBe(true) received false, which is the failure the randomized directory run was surfacing:

(fail) preferred media runtime composables > exposes preferred media helpers as callable signals

Reseeded per test, with the registered listeners dropped alongside, since each test registers its own through usePreferredDark() and keeping the earlier ones delivers a change to signals whose test has already finished.

Verified both ways at file level, which is where the cause lives:

runs failing
before 76fcc5651d3 of 6
after0 of 6

The randomized directory sweep is clean on the runs completed so far. If anything further turns up there it will be a different test, and worth its own issue rather than a third tenant here.

9 days ago

Completing the record, since the comment above was written while the sweep was still running.

A third order dependency did turn up, and it is fixed rather than outstanding: toast theme resolution > falls back to light when the app says nothing, failing because this directory's preferred-media.test.ts replaced window.matchMedia in beforeAll and never restored it. The toast test asserts the light fallback on the stated grounds that "happy-dom reports no dark preference", which held until that file had run. Deterministic in that order, which is why it presented as flaky:

bun test …/preferred-media.test.ts …/toast-theme.test.ts   1 fail
bun test …/toast-theme.test.ts                             0 fail

Fixed in 1276409c01 by restoring the global in afterAll. Both orders pass, 8 of 8.

Worth recording that the intra-file reseed in 76fcc5651d made this one more likely, not less: it leaves the leaked mock reporting dark=true more consistently than the old leftover did. The reseed is still correct for the failure this issue named; it just was not sufficient on its own.

Randomized sweeps of the whole packages/stx/test/signals directory, 6 runs each:

runs failing
reseed only, global still leaking2 of 6 (both the toast test)
after 1276409c010 of 6

No fourth dependency surfaced, so nothing further to split out.

Sign in to comment on this issue.