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
|
||||
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).
|
||||
@@ -138,6 +138,78 @@ test.describe('the shell on a phone', () => {
|
||||
.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 }) => {
|
||||
await app.setViewportSize({ width: 1440, height: 900 });
|
||||
|
||||
|
||||
@@ -93,10 +93,31 @@ async function queueSixAndOpen(app: Page): Promise<void> {
|
||||
'library.Library.GetTracks',
|
||||
[0],
|
||||
10_000,
|
||||
)) as { FilePath: string; TrackName: string }[];
|
||||
)) as { FilePath: string; TrackName: string; Album: string }[];
|
||||
|
||||
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
|
||||
// room either side of it.
|
||||
@@ -223,7 +244,14 @@ test.describe('selecting in the queue with a mouse', () => {
|
||||
.poll(() => selected(app), { timeout: HIGHLIGHT_MS })
|
||||
.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(
|
||||
'data-active-view',
|
||||
|
||||
@@ -188,6 +188,27 @@ export class NowPlaying extends LitElement {
|
||||
cursor: pointer;
|
||||
/* The art shows through; this is a target, not a picture. */
|
||||
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 {
|
||||
|
||||
Reference in New Issue
Block a user