Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
2365806d18 | ||
|
|
7d348f243a | ||
|
|
ddd04623f7 | ||
|
|
9ad1477b1e |
@@ -3891,3 +3891,73 @@ scan takes a minute and is worth running before demoting anybody's links
|
|||||||
— `explore-album-details`'s tracklist (number / title / artist /
|
— `explore-album-details`'s tracklist (number / title / artist /
|
||||||
duration) is the one that plausibly *is* mostly link, and is the one
|
duration) is the one that plausibly *is* mostly link, and is the one
|
||||||
#5 is about to add selection to.
|
#5 is about to add selection to.
|
||||||
|
|
||||||
|
## A layout is still moving when a guard says it has arrived (measured 2026-08-20)
|
||||||
|
|
||||||
|
`album-dropdown.spec.ts` failed with `Expected 80, Received 10` twice
|
||||||
|
over two sessions, and #133 already strengthened its guard from
|
||||||
|
"scrollable at all" to "has at least the range the assertion needs".
|
||||||
|
That was necessary and could not be sufficient, and the reason is
|
||||||
|
structural rather than a matter of thresholds: **a guard and the write
|
||||||
|
it guards are separate CDP round trips**, so the page is free to
|
||||||
|
re-lay-out between them. Polling harder cannot close a window between
|
||||||
|
two moments; only removing the window can.
|
||||||
|
|
||||||
|
Measured directly, sampling `scrollHeight - clientHeight` on
|
||||||
|
`.grid-scroll-container` every frame across a 1440x900 → 900x600 resize,
|
||||||
|
three runs:
|
||||||
|
|
||||||
|
| t (ms) | range |
|
||||||
|
|---|---|
|
||||||
|
| 0 | 0 |
|
||||||
|
| 1 | **88** |
|
||||||
|
| 8–14 | 330 (settled) |
|
||||||
|
|
||||||
|
88 satisfies a guard asking for 80 and is not the settled value, so the
|
||||||
|
guard can pass while the grid is one layout pass from done. Under
|
||||||
|
full-suite load the transient is worse — the observed failure had 10 —
|
||||||
|
which is why it shows up on the second run of a suite and not in ten
|
||||||
|
consecutive runs of the file alone (0/10 both before and after the fix).
|
||||||
|
|
||||||
|
The shape to write instead: **one page-side call that performs the
|
||||||
|
action and returns what it observes**, with `expect.poll` retrying
|
||||||
|
*that*. `scrollTo()` sets `scrollTop` and returns `scrollTop`, so the
|
||||||
|
assertion is about what the grid did rather than about what it was
|
||||||
|
ready to do. `layout-overflow.spec.ts`'s sidebar probe already had the
|
||||||
|
fused half and was missing the retry; it has both now.
|
||||||
|
|
||||||
|
Worth generalising: a spec that resizes and then measures is asserting
|
||||||
|
about a moving target for the next dozen frames. Fuse, then poll.
|
||||||
|
|
||||||
|
## "The first N tracks" is not a way to ask for an ordinary one (2026-08-20)
|
||||||
|
|
||||||
|
`queue-selection.spec.ts` staged its queue from the first few rows of
|
||||||
|
`library.Library.GetTracks(0)` and clicked a track *name*, which
|
||||||
|
`explore-link` routes to that track's **album** page. Four tracks in the
|
||||||
|
fixture library have no album at all — `01 Tone A`, `02 Tone B`,
|
||||||
|
`Title Only`, `no-tags-at-all` — and a name with nothing to route to
|
||||||
|
renders as **plain text**, not as a link.
|
||||||
|
|
||||||
|
Two things follow, and the second is the sharper one.
|
||||||
|
|
||||||
|
**The order is the scan's.** `GetTracks` returns `audio_files.id` order,
|
||||||
|
i.e. the order the scan inserted rows, which depends on concurrency and
|
||||||
|
directory traversal. Locally the first eight are all from two proper
|
||||||
|
albums, so the spec passed twice over; CI rebuilds its seed with a real
|
||||||
|
scan, got a different eight, and failed on both engines. This is the
|
||||||
|
same family as "a seed freezes every default it has already persisted" —
|
||||||
|
the fixture library is not a list, it is a *set* with an incidental
|
||||||
|
order, and no spec should depend on that order.
|
||||||
|
|
||||||
|
**A loose locator hid it.** The row was located with
|
||||||
|
`.locator('.explore-link').first()`, and a row has two — the title and
|
||||||
|
the artist. When the title is plain text, `first()` silently resolves to
|
||||||
|
the **artist** link, so the click went somewhere real and the assertion
|
||||||
|
was about a destination the test had not exercised. `.track-title
|
||||||
|
.explore-link` is the locator that says which one it means; the loose
|
||||||
|
one turned a fixture problem into a mystery.
|
||||||
|
|
||||||
|
The general rule for this repo's fixture library: it is deliberately
|
||||||
|
full of edge cases (untagged, unicode, duplicates, extremes), so a spec
|
||||||
|
that wants an *ordinary* track has to **say so** — filter on the
|
||||||
|
property it depends on rather than slicing.
|
||||||
|
|||||||
@@ -110,27 +110,30 @@ test.describe('the album dropdown', () => {
|
|||||||
await app.setViewportSize({ width: 900, height: 600 });
|
await app.setViewportSize({ width: 900, height: 600 });
|
||||||
|
|
||||||
try {
|
try {
|
||||||
// Wait for the range the assertion below actually needs, not for
|
// The container has to be a scroller at all, which is the thing
|
||||||
// "scrollable at all" (#133). The guard used to be
|
// the defect behind this spec broke and is a property rather
|
||||||
// `scrollHeight > clientHeight + 40` while the next line asks to
|
// than a moment.
|
||||||
// reach 80, so any range in 41-79 satisfied it and could not
|
|
||||||
// satisfy the assertion — and the grid passes through exactly
|
|
||||||
// that while it settles, because it recomputes its columns after
|
|
||||||
// the resize rather than during it. The settled range here is
|
|
||||||
// 330, so this waits rather than weakening anything.
|
|
||||||
await expect
|
await expect
|
||||||
.poll(() => scrollRange(app))
|
.poll(() => scrollRange(app))
|
||||||
.toMatchObject({ room: true, overflowY: 'auto' });
|
.toMatchObject({ overflowY: 'auto' });
|
||||||
|
|
||||||
await app.evaluate((target) => {
|
// **Scrolling it and reading it back are one round trip** (#151).
|
||||||
const sc = document
|
//
|
||||||
.querySelector('cover-grid')
|
// #133 made the guard ask for the range this needs rather than
|
||||||
?.shadowRoot?.querySelector('.grid-scroll-container');
|
// for "scrollable at all", which was necessary and is not
|
||||||
|
// sufficient: a guard and the write it guards are separate
|
||||||
if (sc) sc.scrollTop = target;
|
// `evaluate` calls, so the grid can satisfy the range and settle
|
||||||
}, SCROLL_TARGET);
|
// out of it before the write lands. It still does — observed as
|
||||||
|
// `Expected 80, Received 10` in the second of three consecutive
|
||||||
expect(await scrollTop(app)).toBe(SCROLL_TARGET);
|
// full-suite runs, with the spec green alone on the same app
|
||||||
|
// straight afterwards.
|
||||||
|
//
|
||||||
|
// Polling harder cannot close a window between two moments; only
|
||||||
|
// removing the window can. So the probe sets `scrollTop` and
|
||||||
|
// returns what it reads back, in one page-side call, and the
|
||||||
|
// poll retries *that* — which also means the assertion is about
|
||||||
|
// what the grid did rather than about what it was ready to do.
|
||||||
|
await expect.poll(() => scrollTo(app, SCROLL_TARGET)).toBe(SCROLL_TARGET);
|
||||||
|
|
||||||
// And the dropdown it opens is on screen, wherever the manager
|
// And the dropdown it opens is on screen, wherever the manager
|
||||||
// decides that leaves the scroll. It is *not* "the position is
|
// decides that leaves the scroll. It is *not* "the position is
|
||||||
@@ -261,7 +264,7 @@ async function closeDropdown(app: Page): Promise<void> {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Whether the grid can scroll at all, which decides if a probe can move. */
|
/** Whether the grid is a scroller at all, which is what the bug broke. */
|
||||||
async function scrollRange(app: Page) {
|
async function scrollRange(app: Page) {
|
||||||
return app.evaluate((target) => {
|
return app.evaluate((target) => {
|
||||||
const sc = document
|
const sc = document
|
||||||
@@ -269,22 +272,37 @@ async function scrollRange(app: Page) {
|
|||||||
?.shadowRoot?.querySelector('.grid-scroll-container');
|
?.shadowRoot?.querySelector('.grid-scroll-container');
|
||||||
|
|
||||||
return {
|
return {
|
||||||
// `room` is the precondition of the assertion that follows it:
|
// Reported for the failure message rather than waited on: `room`
|
||||||
// enough range to actually reach the target. A threshold below
|
// was the guard #133 strengthened, and #151 is that a guard in
|
||||||
// what the caller depends on is not a guard.
|
// its own round trip cannot speak for the write in the next one.
|
||||||
|
// `scrollTo` below is the assertion now; this says *why* it did
|
||||||
|
// not reach the target when it does not.
|
||||||
room: !!sc && sc.scrollHeight - sc.clientHeight >= target,
|
room: !!sc && sc.scrollHeight - sc.clientHeight >= target,
|
||||||
overflowY: sc ? getComputedStyle(sc).overflowY : '',
|
overflowY: sc ? getComputedStyle(sc).overflowY : '',
|
||||||
};
|
};
|
||||||
}, SCROLL_TARGET);
|
}, SCROLL_TARGET);
|
||||||
}
|
}
|
||||||
|
|
||||||
async function scrollTop(app: Page): Promise<number> {
|
/**
|
||||||
return app.evaluate(
|
* Scroll the grid and report where it actually landed, in one call.
|
||||||
() =>
|
*
|
||||||
document
|
* The whole point is that the set and the read share a moment: a
|
||||||
.querySelector('cover-grid')
|
* `scrollTop` write is clamped to the range *at the instant it lands*,
|
||||||
?.shadowRoot?.querySelector('.grid-scroll-container')?.scrollTop ?? -1,
|
* so reading it back in a second round trip asks a container that may
|
||||||
);
|
* have re-laid out in between.
|
||||||
|
*/
|
||||||
|
async function scrollTo(app: Page, target: number): Promise<number> {
|
||||||
|
return app.evaluate((to) => {
|
||||||
|
const sc = document
|
||||||
|
.querySelector('cover-grid')
|
||||||
|
?.shadowRoot?.querySelector('.grid-scroll-container');
|
||||||
|
|
||||||
|
if (!sc) return -1;
|
||||||
|
|
||||||
|
sc.scrollTop = to;
|
||||||
|
|
||||||
|
return sc.scrollTop;
|
||||||
|
}, target);
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Whether the open dropdown is inside the scroll container's viewport. */
|
/** Whether the open dropdown is inside the scroll container's viewport. */
|
||||||
|
|||||||
@@ -132,22 +132,34 @@ test.describe('the app fits in its own window', () => {
|
|||||||
// be dragged here, but a scaled display or a large system font can
|
// be dragged here, but a scaled display or a large system font can
|
||||||
// still land the layout in it, and clipping the nav with no scroll
|
// still land the layout in it, and clipping the nav with no scroll
|
||||||
// is the failure that made Settings unreachable.
|
// is the failure that made Settings unreachable.
|
||||||
const reachable = await app.locator('app-sidebar').evaluate((el) => {
|
//
|
||||||
const settings = el.shadowRoot?.querySelector<HTMLElement>(
|
// The scroll and the measurement share one `evaluate` — #151's
|
||||||
'[data-testid="nav-settings"]',
|
// rule, which this already had — and the whole probe is polled,
|
||||||
);
|
// which it did not: a viewport change settles asynchronously, so a
|
||||||
|
// single attempt reads whatever the sidebar happened to be doing.
|
||||||
|
// The probe is safe to repeat because scrolling to the bottom twice
|
||||||
|
// is scrolling to the bottom.
|
||||||
|
await expect
|
||||||
|
.poll(() =>
|
||||||
|
app.locator('app-sidebar').evaluate((el) => {
|
||||||
|
const settings = el.shadowRoot?.querySelector<HTMLElement>(
|
||||||
|
'[data-testid="nav-settings"]',
|
||||||
|
);
|
||||||
|
|
||||||
if (!settings) return null;
|
if (!settings) return null;
|
||||||
|
|
||||||
el.scrollTop = el.scrollHeight;
|
el.scrollTop = el.scrollHeight;
|
||||||
|
|
||||||
const item = settings.getBoundingClientRect();
|
const item = settings.getBoundingClientRect();
|
||||||
const pane = el.getBoundingClientRect();
|
const pane = el.getBoundingClientRect();
|
||||||
|
|
||||||
return item.bottom <= Math.ceil(pane.bottom) && item.top >= Math.floor(pane.top);
|
return (
|
||||||
});
|
item.bottom <= Math.ceil(pane.bottom) &&
|
||||||
|
item.top >= Math.floor(pane.top)
|
||||||
expect(reachable).toBe(true);
|
);
|
||||||
|
}),
|
||||||
|
)
|
||||||
|
.toBe(true);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -93,10 +93,31 @@ async function queueSixAndOpen(app: Page): Promise<void> {
|
|||||||
'library.Library.GetTracks',
|
'library.Library.GetTracks',
|
||||||
[0],
|
[0],
|
||||||
10_000,
|
10_000,
|
||||||
)) as { FilePath: string; TrackName: string }[];
|
)) as { FilePath: string; TrackName: string; Album: string }[];
|
||||||
|
|
||||||
const long = tracks.find((t) => t.TrackName === longTitle);
|
const long = tracks.find((t) => t.TrackName === longTitle);
|
||||||
const rest = tracks.filter((t) => t.TrackName !== longTitle).slice(0, 5);
|
|
||||||
|
/**
|
||||||
|
* **Tracks that have an album**, which is a requirement of one of
|
||||||
|
* the tests and was previously left to luck (#156).
|
||||||
|
*
|
||||||
|
* `explore-link` routes a track name to its *album's* page, so a
|
||||||
|
* track with no album renders a name that navigates nowhere — and
|
||||||
|
* the fixture library deliberately contains two (`01 Tone A`,
|
||||||
|
* `02 Tone B`). Which tracks arrive first is `audio_files.id`
|
||||||
|
* order, i.e. the order the **scan** inserted them, which depends
|
||||||
|
* on concurrency and directory traversal: locally the first eight
|
||||||
|
* all had albums and the spec passed twice over, and CI rebuilds
|
||||||
|
* its seed with a real scan and got a different eight.
|
||||||
|
*
|
||||||
|
* Asking for what the test needs is the fix. It is not a
|
||||||
|
* narrowing: every assertion here wants an ordinary track, and
|
||||||
|
* "the first five rows" was never a way to ask for one in a
|
||||||
|
* library whose whole purpose is edge cases.
|
||||||
|
*/
|
||||||
|
const rest = tracks
|
||||||
|
.filter((t) => t.TrackName !== longTitle && t.Album !== '')
|
||||||
|
.slice(0, 5);
|
||||||
|
|
||||||
// Index 3 is the long one: far enough down that a shift-extend has
|
// Index 3 is the long one: far enough down that a shift-extend has
|
||||||
// room either side of it.
|
// room either side of it.
|
||||||
@@ -223,7 +244,14 @@ test.describe('selecting in the queue with a mouse', () => {
|
|||||||
.poll(() => selected(app), { timeout: HIGHLIGHT_MS })
|
.poll(() => selected(app), { timeout: HIGHLIGHT_MS })
|
||||||
.toEqual([1]);
|
.toEqual([1]);
|
||||||
|
|
||||||
await row(app, 2).locator('.explore-link').first().click();
|
// `.track-title .explore-link`, not `.explore-link` first(): a row
|
||||||
|
// has two, and which one `first()` finds depends on whether the
|
||||||
|
// *title* is a link at all. It is not, for a track with no album —
|
||||||
|
// `explore-link` renders plain text where it cannot route — so the
|
||||||
|
// loose locator silently clicked the **artist** instead and the
|
||||||
|
// assertion below was about a different destination than the one
|
||||||
|
// being exercised (#156).
|
||||||
|
await row(app, 2).locator('.track-title .explore-link').click();
|
||||||
|
|
||||||
await expect(app.getByTestId('main-content')).toHaveAttribute(
|
await expect(app.getByTestId('main-content')).toHaveAttribute(
|
||||||
'data-active-view',
|
'data-active-view',
|
||||||
|
|||||||
Reference in New Issue
Block a user