yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

make ui-test: play-in-context flakes ~1 in 3 full-suite runs on main #138

New Issue
Closed
opened 2026-08-19 18:25:47 +00:00 by yonlu · 3 comments
yonlu commented 2026-08-19 18:25:47 +00:00
Owner
Copy Link
Copy Source

Report

test/components/play-in-context.test.ts > "queues the list as displayed
and starts on that row" fails intermittently in a full make ui-test
run, on pristine main (bb7dde1), with no local changes.

AssertionError: expected [ 12, '/music/track-11.mp3' ]
              to deeply equal [ 12, '/music/track-3.mp3' ]

Rate. 1 failure in 3 consecutive full-suite runs on clean main.
Tripped over because it failed a pre-push hook on a branch containing
only lefthook.yml and scripts/codegen-check.sh — zero frontend
changes — which is what identified it as pre-existing rather than
introduced.

What is ruled out

  • Not caused by any change in the quick-wins batch: it reproduces on
    main with a clean tree.
  • Not an inherent failure of the spec: vitest run play-in-context
    passes 3/3 in isolation.
  • Not reproducible by running the suspected polluter before it:
    --fileParallelism=false aria-tail.test.ts play-in-context.test.ts
    passes, as does a five-file parallel subset of every track-list-ish
    suite (45 tests, twice). It appears to need the full suite's timing.

The lead, stated as a lead

The two file paths differ in sort order, not in identity:
track-11 sorts before track-3 as a string, and /music/track-3.mp3
is what the spec expects. So the list under test was sorted by title
when the assertion ran, and the spec assumes the unsorted/default order.

track-list.ts persists its sort to localStorage
(track-list-sort-field, track-list-sort-direction, ~line 2048) and
restores it in restoreSortPreferences(). frontend/test/setup.ts
clears neither: it resets the Wails harness and the fixtures, and
nothing touches storage. aria-tail.test.ts:100 activates the Title
column header from the keyboard, which is a code path that writes those
keys.

That is a coherent story and it did not reproduce on demand, so it
is a hypothesis, not the cause. Whatever the mechanism, a spec whose
result depends on what an earlier spec left in storage is the shape of
the problem.

Direction

Two things worth doing regardless of the diagnosis:

  1. Clear localStorage in setup.ts's beforeEach, beside
    resetHarness(). Several components persist there — the track list's
    sort and column widths, now-playing's scroll mode (whose own tests
    already clean up by hand, which is the tell that this is missing) —
    and cross-file leakage through a shared origin is invisible until it
    is intermittent.
  2. Make the spec state its order. If the assertion depends on the
    list not being sorted, it should set the sort it wants rather than
    inherit a default, so a future sort change is a spec change and not a
    mystery.

Note for whoever picks this up

make ui-test is a required CI check, so this can fail an unrelated PR
and send the author looking at their own diff. That is what happened
here.

**Report** `test/components/play-in-context.test.ts` > "queues the list as displayed and starts on that row" fails intermittently in a **full** `make ui-test` run, on pristine `main` (bb7dde1), with no local changes. ``` AssertionError: expected [ 12, '/music/track-11.mp3' ] to deeply equal [ 12, '/music/track-3.mp3' ] ``` **Rate.** 1 failure in 3 consecutive full-suite runs on clean `main`. Tripped over because it failed a `pre-push` hook on a branch containing *only* `lefthook.yml` and `scripts/codegen-check.sh` — zero frontend changes — which is what identified it as pre-existing rather than introduced. **What is ruled out** - Not caused by any change in the quick-wins batch: it reproduces on `main` with a clean tree. - Not an inherent failure of the spec: `vitest run play-in-context` passes 3/3 in isolation. - **Not** reproducible by running the suspected polluter before it: `--fileParallelism=false aria-tail.test.ts play-in-context.test.ts` passes, as does a five-file parallel subset of every track-list-ish suite (45 tests, twice). It appears to need the full suite's timing. **The lead, stated as a lead** The two file paths differ in *sort order*, not in identity: `track-11` sorts before `track-3` as a string, and `/music/track-3.mp3` is what the spec expects. So the list under test was sorted by title when the assertion ran, and the spec assumes the unsorted/default order. `track-list.ts` persists its sort to `localStorage` (`track-list-sort-field`, `track-list-sort-direction`, ~line 2048) and restores it in `restoreSortPreferences()`. `frontend/test/setup.ts` clears neither: it resets the Wails harness and the fixtures, and nothing touches storage. `aria-tail.test.ts:100` activates the Title column header from the keyboard, which is a code path that writes those keys. That is a coherent story and it did **not** reproduce on demand, so it is a hypothesis, not the cause. Whatever the mechanism, a spec whose result depends on what an earlier spec left in storage is the shape of the problem. **Direction** Two things worth doing regardless of the diagnosis: 1. **Clear `localStorage` in `setup.ts`'s `beforeEach`**, beside `resetHarness()`. Several components persist there — the track list's sort and column widths, `now-playing`'s scroll mode (whose own tests already clean up by hand, which is the tell that this is missing) — and cross-file leakage through a shared origin is invisible until it is intermittent. 2. **Make the spec state its order.** If the assertion depends on the list not being sorted, it should set the sort it wants rather than inherit a default, so a future sort change is a spec change and not a mystery. **Note for whoever picks this up** `make ui-test` is a required CI check, so this can fail an unrelated PR and send the author looking at their own diff. That is what happened here.
yonlu added the Area/Library-UI
Reviewed
Confirmed
1
Priority
Medium
3
Kind/Bug labels 2026-08-19 18:25:47 +00:00
logan commented 2026-08-24 10:12:02 +00:00
Collaborator
Copy Link
Copy Source

Another occurrence, in CI this time — the rate is what this issue
is about, so recording it.

Run 553 attempt 1 (check job, Component and store suite step),
against PR #218, a branch whose diff is entirely backend/riff,
backend/metadata and backend/tagwriter — no frontend file touched:

FAIL |chromium| test/components/play-in-context.test.ts > double-clicking a row in the track list > queues the list as displayed and starts on that row
AssertionError: expected [ 12, '/music/track-11.mp3' ] to deeply equal [ 12, '/music/track-3.mp3' ]

Byte-identical to the report. Attempt 2 of the same commit passed, so
it is still intermittent and still costs a full check plus the
e2e job, which needs: check and is skipped rather than run.

Two things this adds to the report: it happens on the CI container as
well as locally, and make ui-test passed on the same commit on my
machine before the push — so the ~1 in 3 is not machine-specific.

**Another occurrence, in CI this time** — the rate is what this issue is about, so recording it. Run 553 attempt 1 (`check` job, `Component and store suite` step), against PR #218, a branch whose diff is entirely `backend/riff`, `backend/metadata` and `backend/tagwriter` — no frontend file touched: ``` FAIL |chromium| test/components/play-in-context.test.ts > double-clicking a row in the track list > queues the list as displayed and starts on that row AssertionError: expected [ 12, '/music/track-11.mp3' ] to deeply equal [ 12, '/music/track-3.mp3' ] ``` Byte-identical to the report. Attempt 2 of the same commit passed, so it is still intermittent and still costs a full `check` **plus** the `e2e` job, which `needs: check` and is skipped rather than run. Two things this adds to the report: it happens on the CI container as well as locally, and `make ui-test` passed on the same commit on my machine before the push — so the ~1 in 3 is not machine-specific.
logan self-assigned this 2026-08-24 10:37:04 +00:00
logan added the
Status
In Progress
 label 2026-08-24 10:37:04 +00:00
logan commented 2026-08-24 10:37:04 +00:00
Collaborator
Copy Link
Copy Source

Picking this up on fix/138-ui-test-storage-leak.

The mechanism reproduces on demand, which is what the "skip
intermittent failures" rule wanted before spending a run on it. Two
measurements, both on the frontend tree as it stands on main:

  1. A temporary probe in test/setup.ts — a root beforeEach that
    throws when localStorage is not empty at the start of a test —
    fails 11 test-starts in a single full run with exactly
    ["track-list-sort-field","track-list-sort-direction"] already set
    (plus 8 with cover-grid-size and 5 with
    track-list-column-widths). So the leak is real and common; what
    varies run to run is only which files land behind the polluter.
  2. Injecting those two keys and running the spec alone gives the
    report's failure byte for byte:
    AssertionError: expected [ 12, '/music/track-11.mp3' ] to deeply equal [ 12, '/music/track-3.mp3' ].

Why the reporter's sequential subset did not reproduce it.
@vitest/browser-playwright opens one BrowserContext per session
and files run in parallel across sessions, so localStorage is shared
by whichever files happen to share a tab and is not shared across
tabs. Two files on the command line usually do not land in the order —
or the tab — that matters; the full suite has ~99 chances to.

That also settles the safety question about the fix: because files
within a session run sequentially, a localStorage.clear() in
beforeEach cannot clobber a concurrently-running file's storage.

Doing both halves of the Direction as written, and nothing else.

Picking this up on `fix/138-ui-test-storage-leak`. **The mechanism reproduces on demand**, which is what the "skip intermittent failures" rule wanted before spending a run on it. Two measurements, both on the frontend tree as it stands on `main`: 1. A temporary probe in `test/setup.ts` — a root `beforeEach` that *throws* when `localStorage` is not empty at the start of a test — fails **11** test-starts in a single full run with exactly `["track-list-sort-field","track-list-sort-direction"]` already set (plus 8 with `cover-grid-size` and 5 with `track-list-column-widths`). So the leak is real and common; what varies run to run is only *which* files land behind the polluter. 2. Injecting those two keys and running the spec alone gives the report's failure byte for byte: `AssertionError: expected [ 12, '/music/track-11.mp3' ] to deeply equal [ 12, '/music/track-3.mp3' ]`. **Why the reporter's sequential subset did not reproduce it.** `@vitest/browser-playwright` opens **one `BrowserContext` per session** and files run in parallel across sessions, so `localStorage` is shared by whichever files happen to share a tab and is *not* shared across tabs. Two files on the command line usually do not land in the order — or the tab — that matters; the full suite has ~99 chances to. That also settles the safety question about the fix: because files within a session run sequentially, a `localStorage.clear()` in `beforeEach` cannot clobber a concurrently-running file's storage. Doing both halves of the Direction as written, and nothing else.
logan commented 2026-08-24 11:01:19 +00:00
Collaborator
Copy Link
Copy Source

PR: #219 — CI green (run
554: check and e2e both success).

The cause, since the issue filed it as a lead. The lead was right
about the sort and about restoreSortPreferences(); what was missing
was why it did not reproduce sequentially. @vitest/browser-playwright
opens one BrowserContext per session and runs several files in it
one after another, so localStorage is shared by whichever files
happen to share a tab and is not shared across tabs. Two files named
on the command line rarely land in the tab, or the order, that matters;
the full suite has ~99 chances to.

Measured on main with a probe that throws when a test starts with a
non-empty localStorage: 24 polluted test-starts in one full run —
11 with the two sort keys, 8 with cover-grid-size, 5 with
track-list-column-widths — cascading into 248 failures. So
play-in-context is the reporter, not the extent.

Both halves of the Direction shipped, nothing else. 0 polluted
test-starts after the fix; eight consecutive clean full runs; and the
spec was shown to fail at its new first assertion when both halves are
defeated with the reported pollution injected.

Leaving Status/In Progress on until the PR merges.

PR: https://git.ljones.me/yonlu/yellowjacket/pulls/219 — CI green (run 554: `check` and `e2e` both success). **The cause, since the issue filed it as a lead.** The lead was right about the sort and about `restoreSortPreferences()`; what was missing was why it did not reproduce sequentially. `@vitest/browser-playwright` opens **one `BrowserContext` per session** and runs several files in it one after another, so `localStorage` is shared by whichever files happen to share a tab and is *not* shared across tabs. Two files named on the command line rarely land in the tab, or the order, that matters; the full suite has ~99 chances to. Measured on `main` with a probe that throws when a test starts with a non-empty `localStorage`: **24** polluted test-starts in one full run — 11 with the two sort keys, 8 with `cover-grid-size`, 5 with `track-list-column-widths` — cascading into 248 failures. So `play-in-context` is the reporter, not the extent. Both halves of the Direction shipped, nothing else. 0 polluted test-starts after the fix; eight consecutive clean full runs; and the spec was shown to fail at its new first assertion when both halves are defeated with the reported pollution injected. Leaving `Status/In Progress` on until the PR merges.
logan closed this issue 2026-08-25 16:37:42 +00:00
gitea-actions bot removed the
Status
In Progress
 label 2026-08-25 16:37:51 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Library-UI Kind/Bug
Priority
Medium
3
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees logan
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#138