Compare commits
4
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
7699fcd9dc | ||
|
|
86e7444603 | ||
|
|
2365806d18 | ||
|
|
7d348f243a |
@@ -3928,3 +3928,84 @@ fused half and was missing the retry; it has both now.
|
|||||||
|
|
||||||
Worth generalising: a spec that resizes and then measures is asserting
|
Worth generalising: a spec that resizes and then measures is asserting
|
||||||
about a moving target for the next dozen frames. Fuse, then poll.
|
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.
|
||||||
|
|
||||||
|
## The phone's way into Now Playing was under the artwork (measured 2026-08-20)
|
||||||
|
|
||||||
|
`phone-shell.spec.ts`'s "opens the full-screen now playing" failed in CI
|
||||||
|
on both engines, three times across two branches that could not have
|
||||||
|
caused it, and passed on re-run each time. It was filed as a flake
|
||||||
|
(#150). It is not one: **it depends on which track is playing.**
|
||||||
|
|
||||||
|
`.expand` — the phone's only route into `<now-playing-view>` — is
|
||||||
|
`position: absolute; inset: 0` inside `.cover-art-wrapper`, and
|
||||||
|
`.cover-art` is a **later sibling**. Both have `z-index: auto`, so they
|
||||||
|
tie on paint order and the later one wins. With an `<img>` that costs
|
||||||
|
nothing; with no artwork the placeholder `wa-icon` renders and takes
|
||||||
|
every click aimed at the button underneath it.
|
||||||
|
|
||||||
|
Measured at 390px with `elementFromPoint` at the button's centre:
|
||||||
|
|
||||||
|
| playing track | hit test |
|
||||||
|
|---|---|
|
||||||
|
| has artwork | `button.expand` |
|
||||||
|
| no artwork | **`wa-icon`** |
|
||||||
|
| no artwork, with `z-index: 1` | `button.expand` |
|
||||||
|
|
||||||
|
So on a phone, the only way into the full-screen player stopped working
|
||||||
|
whenever the current song had no cover — and this has nothing to do with
|
||||||
|
the fixture: any library has untagged files.
|
||||||
|
|
||||||
|
Three things worth keeping.
|
||||||
|
|
||||||
|
**"Flaky in CI" was the wrong diagnosis and it cost three cycles.** The
|
||||||
|
spec starts the *first* row of the track list, so which track it plays
|
||||||
|
is the order the scan inserted rows in — the same root cause as #156,
|
||||||
|
one spec over. A test whose subject is a hit test has to *choose* the
|
||||||
|
case that breaks it.
|
||||||
|
|
||||||
|
**The first two hypotheses were both wrong, and both were plausible.**
|
||||||
|
A custom element's upgrade replacing its own contents, and the cover
|
||||||
|
preview's `mouseenter` opening a popup under the pointer. Neither
|
||||||
|
survived contact with `elementFromPoint`, which took a minute and would
|
||||||
|
have saved the other two cycles.
|
||||||
|
|
||||||
|
**And the spec that pins it needs the 90-second track**, because a
|
||||||
|
2-second one finishes before the assertions run — the trap
|
||||||
|
`fixtures.ts` already documents. Note the filter that does *not* work:
|
||||||
|
`library.Track.CoverArt` is empty for all 31 fixture rows, so "the
|
||||||
|
first track with no cover art" selects nothing in particular. The
|
||||||
|
placeholder's presence is asserted instead, which is the property the
|
||||||
|
test actually depends on.
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
import { test, expect } from '../support/fixtures.js';
|
import { test, expect, LONG_TRACK } from '../support/fixtures.js';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The phone shell (plan 016 B2, phase 1).
|
* The phone shell (plan 016 B2, phase 1).
|
||||||
@@ -138,6 +138,78 @@ test.describe('the shell on a phone', () => {
|
|||||||
.toHaveAttribute('data-active-view', 'tracks');
|
.toHaveAttribute('data-active-view', 'tracks');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The same journey with a track that has **no cover art** (#150).
|
||||||
|
*
|
||||||
|
* The test above starts the *first* row of the track list, so which
|
||||||
|
* track it plays is the order the scan inserted them in — and the
|
||||||
|
* answer decided whether it passed. A track with artwork renders an
|
||||||
|
* `<img>`, which is no obstacle; one without renders a placeholder
|
||||||
|
* `wa-icon`, which took every click aimed at the button beneath it,
|
||||||
|
* because that button is absolutely positioned with `z-index: auto`
|
||||||
|
* and the art is a *later* sibling. They tied, and the later one won.
|
||||||
|
*
|
||||||
|
* So this picks a track *for* the property that broke it, which is
|
||||||
|
* the only way the assertion means anything: the version above passes
|
||||||
|
* on a broken build roughly two runs in three, which is exactly how
|
||||||
|
* it came to cost three CI cycles across two branches that could not
|
||||||
|
* have caused it.
|
||||||
|
*/
|
||||||
|
test('opens the full-screen now playing for a track with no art', async ({
|
||||||
|
app,
|
||||||
|
}) => {
|
||||||
|
// `LONG_TRACK` by name, and not "the first track with no
|
||||||
|
// CoverArt": the *library* model reports that field empty for
|
||||||
|
// every row in this fixture (31 of 31), so filtering on it selects
|
||||||
|
// nothing in particular and picked a 2-second track, which had
|
||||||
|
// finished before the assertions ran. The placeholder check below
|
||||||
|
// is what actually holds the property this test needs.
|
||||||
|
const started = await app.evaluate(async (longTitle) => {
|
||||||
|
const tracks = (await window.__yjEvents.call(
|
||||||
|
'library.Library.GetTracks',
|
||||||
|
[0],
|
||||||
|
10_000,
|
||||||
|
)) as { FilePath: string; TrackName: string }[];
|
||||||
|
|
||||||
|
const bare = tracks.find((t) => t.TrackName === longTitle);
|
||||||
|
|
||||||
|
if (!bare) return null;
|
||||||
|
|
||||||
|
await window.__yjEvents.call(
|
||||||
|
'queue.Queue.SetQueue',
|
||||||
|
[[bare.FilePath], 0, false, { type: '', id: 0, label: '' }],
|
||||||
|
10_000,
|
||||||
|
);
|
||||||
|
await window.__yjEvents.call('queue.Queue.Play', [], 5_000);
|
||||||
|
|
||||||
|
return bare.TrackName;
|
||||||
|
}, LONG_TRACK);
|
||||||
|
|
||||||
|
expect(started).toBe(LONG_TRACK);
|
||||||
|
|
||||||
|
await expect(app.getByTestId('now-playing-title')).not.toBeEmpty();
|
||||||
|
|
||||||
|
// **The placeholder is the whole point**, so it is asserted rather
|
||||||
|
// than assumed: this test is about the thing that renders when
|
||||||
|
// there is no artwork. If the fixture ever gives this album a
|
||||||
|
// cover, this fails and says so instead of passing while measuring
|
||||||
|
// the easy case.
|
||||||
|
//
|
||||||
|
// One selector rather than a chain from the host: Playwright's CSS
|
||||||
|
// engine pierces an open shadow root, and chaining from the host
|
||||||
|
// element does not reach into it.
|
||||||
|
await expect(
|
||||||
|
app.locator('now-playing .cover-placeholder'),
|
||||||
|
).toBeAttached();
|
||||||
|
|
||||||
|
await app.getByTestId('open-now-playing').click();
|
||||||
|
|
||||||
|
await expect(app.getByTestId('main-content'))
|
||||||
|
.toHaveAttribute('data-active-view', 'now-playing');
|
||||||
|
|
||||||
|
await app.getByTestId('npv-back').click();
|
||||||
|
});
|
||||||
|
|
||||||
test('offers no way in on a desktop, where the bar is whole', async ({ app }) => {
|
test('offers no way in on a desktop, where the bar is whole', async ({ app }) => {
|
||||||
await app.setViewportSize({ width: 1440, height: 900 });
|
await app.setViewportSize({ width: 1440, height: 900 });
|
||||||
|
|
||||||
|
|||||||
@@ -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',
|
||||||
|
|||||||
@@ -188,6 +188,27 @@ export class NowPlaying extends LitElement {
|
|||||||
cursor: pointer;
|
cursor: pointer;
|
||||||
/* The art shows through; this is a target, not a picture. */
|
/* The art shows through; this is a target, not a picture. */
|
||||||
color: transparent;
|
color: transparent;
|
||||||
|
/* **Above the art, or it is not a target at all** (#150).
|
||||||
|
|
||||||
|
This button is absolutely positioned with z-index auto and
|
||||||
|
the art is a *later* sibling, so the two tie on paint order
|
||||||
|
and the later one wins. With an <img> that costs nothing --
|
||||||
|
an image is not a hit-test obstacle here -- but a track with
|
||||||
|
no artwork renders a placeholder wa-icon, which is, and it
|
||||||
|
takes every click aimed at the button underneath it.
|
||||||
|
|
||||||
|
The failure is therefore per *track*, not per build: on a
|
||||||
|
phone the only way into the full-screen now-playing view
|
||||||
|
stopped working whenever the current song had no cover.
|
||||||
|
Measured with elementFromPoint at the button's centre --
|
||||||
|
wa-icon with a placeholder, button.expand with an image, and
|
||||||
|
button.expand either way once this line exists.
|
||||||
|
|
||||||
|
z-index rather than pointer-events: none on the art, which
|
||||||
|
would take the cover preview's mouseenter with it; and
|
||||||
|
rather than reordering the DOM, which would leave the same
|
||||||
|
tie to be won by the same accident in the other direction. */
|
||||||
|
z-index: 1;
|
||||||
}
|
}
|
||||||
|
|
||||||
.expand:focus-visible {
|
.expand:focus-visible {
|
||||||
|
|||||||
Reference in New Issue
Block a user