fix(651): pre-merge asks — an empty filter loop asserts nothing; fix "1 item need"
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 17s
PR Gates / Docs update reminder (pull_request) Successful in 23s
PR Gates / decisions lifecycle (pull_request) Successful in 25s
Review verdict / Set review-verdict status (pull_request) Successful in 7s
PR Gates / Script tests (pytest) (pull_request) Successful in 46s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 18s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 15s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 5m51s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 19m40s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 21m57s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
review-verdict/h10 Review-verdict: MERGEABLE @ 66c8500 (base: main)

FIX 1: `PlaylistsScreen.test.tsx`'s group-id test filtered POSTs and asserted inside a
`for` loop over the result. On the fixed build that array is empty — the unmatched value
leaves the select at '', so Create is disabled and jsdom won't dispatch its onClick — so
ZERO assertions ran and the title claimed more than the body proved. A future change that
re-enabled Create and POSTed `playlistGroupId: null` would still have passed. Added the
unconditional `expect(posts).toHaveLength(0)` before the loop.

Worth noting this is the ninth instance of the shape on this branch, and the sibling
strengthening in the *same commit* got it right (`RerunCollectionsScreen.test.tsx` uses an
unconditional `toHaveLength(0)`) — so the lesson didn't generalize even one file over. The
rule is: an assertion inside a loop over a filtered collection proves nothing until the
collection's length is asserted.

FIX 2: "1 item need a selection" — the noun was pluralized, the verb wasn't, and singular is
the common case. My test used `/need a selection/i`, which matches both the right and wrong
grammar, so nothing could catch it; it now asserts the exact string '1 item needs a
selection'.

FIX 3: two comments about clicking an already-disabled button read as contradictory policy.
They're not — on the parent the Playlists button was ENABLED, so there the click genuinely
discriminates, while the rerun button was disabled on both sides, making it a restatement of
`toBeDisabled()`. Both comments now say which case they are and why.

Added to #677: the row label falls back to "(no X selected)" on empty `selectedName`
regardless of `selectedId` (the mirror image of the bug fixed here), and an all-unbindable
group list disables Create with no reason shown.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-07-27 05:19:49 +02:00
co-authored by Claude Opus 5
parent 27867e03cf
commit 66c8500e94
3 changed files with 19 additions and 6 deletions
+13 -3
View File
@@ -390,11 +390,16 @@ describe('PlaylistsScreen', () => {
expect(await screen.findByText(/no movie selected/i)).toBeInTheDocument();
expect(screen.queryByText('Cool Movie')).not.toBeInTheDocument();
// LOAD-BEARING 2: Save is gated on it, with a visible reason.
expect(screen.getByText(/need a selection/i)).toBeInTheDocument();
// LOAD-BEARING 2: Save is gated on it, with a visible reason. Asserted as the EXACT string —
// a loose /need a selection/i matched both "1 item need" and "1 items needs", so it could not
// see the grammar it appeared to cover, and singular is the common case.
expect(screen.getByText('1 item needs a selection')).toBeInTheDocument();
expect(screen.getByRole('button', { name: 'Save playlist' })).toBeDisabled();
// LOAD-BEARING 3: and the write is genuinely unreachable, not merely discouraged.
// LOAD-BEARING 3: and the write is genuinely unreachable. Clicking a disabled button is usually
// a no-op restatement of `toBeDisabled()` (see RerunCollectionsScreen.test.tsx, where it is
// deliberately omitted) — it earns its place HERE because on the parent this button was
// ENABLED, so the click actually issued the PUT and this assertion is what discriminates.
fireEvent.click(screen.getByRole('button', { name: 'Save playlist' }));
await new Promise((resolve) => setTimeout(resolve, 0));
expect(
@@ -432,6 +437,11 @@ describe('PlaylistsScreen', () => {
const posts = fetchMock.mock.calls.filter(
([u, i]) => u === '/api/v1/playlists' && (i?.method ?? '').toUpperCase() === 'POST'
);
// Unconditional FIRST: `posts` is empty on the fixed build (the unmatched value leaves the
// select at '', so Create is disabled and jsdom won't dispatch its onClick), which means the
// loop below runs ZERO assertions on its own. Without this line a future change that re-enabled
// Create and POSTed `playlistGroupId: null` would still pass.
expect(posts).toHaveLength(0);
for (const post of posts) {
expect(JSON.parse(String(post[1]?.body)).playlistGroupId).not.toBe(2_147_483_648);
}
+1 -1
View File
@@ -593,7 +593,7 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
name.trim().length === 0
? 'Name is required'
: itemsMissingSelection > 0
? `${itemsMissingSelection} item${itemsMissingSelection === 1 ? '' : 's'} need a selection`
? `${itemsMissingSelection} item${itemsMissingSelection === 1 ? '' : 's'} need${itemsMissingSelection === 1 ? 's' : ''} a selection`
: null;
const save = async () => {
@@ -985,8 +985,11 @@ describe('RerunCollectionsScreen', () => {
await screen.findByPlaceholderText('Rerun collection name');
// Treated as ABSENT — visibly, with the reason — rather than as a selection that fails on write.
// These two ARE the load-bearing assertions; clicking a button already asserted disabled would
// only restate `toBeDisabled()`, so it is deliberately not done here.
// These two ARE the load-bearing assertions. Clicking the disabled Save is deliberately NOT done
// here: this button is disabled on the parent too, so the click would only restate
// `toBeDisabled()`. (PlaylistsScreen.test.tsx does click it — there the parent left the button
// enabled, so the click genuinely discriminates. The difference is what the parent did, not a
// difference of policy.)
expect(screen.getByText('A selection is required')).toBeInTheDocument();
expect(screen.getByRole('button', { name: 'Save rerun collection' })).toBeDisabled();
});