timothy/ersatztv
1
0
Fork 0
Code Issues 54 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

ElasticSearchIndex.UpdateSong has no regression test — an Elastic-only reintroduction of the #701 mutation stays CI-green #824

New Issue
Closed
opened 2026-08-23 00:07:30 +02:00 by timothy · 2 comments
timothy commented 2026-08-23 00:07:30 +02:00
Owner
Copy Link
Copy Source

Deferred from #701 and named there rather than papered over.

The gap

#701 removed metadata.Artists ??= [] / AlbumArtists ??= [] from both search indexers, but only LuceneSearchIndex gained a regression test (ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests). ElasticSearchIndex.UpdateSong holds an independent copy of the same logic, so reintroducing the mutation there alone leaves the whole suite green.

The Lucene fixture is not extendable as-is: it injects a Lucene IndexWriter into a private field, whereas ElasticSearchIndex needs an ElasticsearchClient, i.e. a live server or a stubbed transport.

Why it was not done in #701

The alternative on the table was a source-text guard asserting the string ??= does not appear near those fields. That was rejected: the ??= idiom is correct on navigation collections and appears 92 times across the app projects, so a text guard is either noisy or so narrowly scoped that it stops matching the day someone reformats the line. A brittle guard that reads as coverage is the failure #781 and docs/defect-shapes-773.md §5.3 warn about.

Options

  1. Inject a non-network ElasticsearchClient transport and run the same tracked-entity assertions.
  2. Extract the document-construction step from both indexers into one shared, directly testable unit, so the assertion covers both by construction. This is the fix-the-boundary-not-the-site shape and is probably the better answer — but it is a refactor of two large files and wants its own review.

Prefer whichever actually executes; do not close this with a test that cannot be shown to go red.

Done-when

  • ElasticSearchIndex.UpdateSong is covered by a test that is WITNESSED RED when the ??= mutation is restored (quote the red)
  • The test asserts the entity is not mutated, not merely that the produced document looks right
  • Adversarial review passed
Deferred from #701 and named there rather than papered over. ## The gap #701 removed `metadata.Artists ??= []` / `AlbumArtists ??= []` from **both** search indexers, but only `LuceneSearchIndex` gained a regression test (`ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests`). `ElasticSearchIndex.UpdateSong` holds an independent copy of the same logic, so reintroducing the mutation there alone leaves the whole suite green. The Lucene fixture is not extendable as-is: it injects a Lucene `IndexWriter` into a private field, whereas `ElasticSearchIndex` needs an `ElasticsearchClient`, i.e. a live server or a stubbed transport. ## Why it was not done in #701 The alternative on the table was a source-text guard asserting the string `??=` does not appear near those fields. That was rejected: the `??=` idiom is correct on navigation collections and appears 92 times across the app projects, so a text guard is either noisy or so narrowly scoped that it stops matching the day someone reformats the line. A brittle guard that reads as coverage is the failure `#781` and `docs/defect-shapes-773.md` §5.3 warn about. ## Options 1. Inject a non-network `ElasticsearchClient` transport and run the same tracked-entity assertions. 2. Extract the document-construction step from both indexers into one shared, directly testable unit, so the assertion covers both by construction. This is the `fix-the-boundary-not-the-site` shape and is probably the better answer — but it is a refactor of two large files and wants its own review. Prefer whichever actually executes; do not close this with a test that cannot be shown to go red. ## Done-when - [x] `ElasticSearchIndex.UpdateSong` is covered by a test that is WITNESSED RED when the `??=` mutation is restored (quote the red) - [x] The test asserts the entity is not mutated, not merely that the produced document looks right - [x] Adversarial review passed
timothy added the priority: medium label 2026-08-23 01:07:17 +02:00
timothy commented 2026-08-29 11:22:40 +02:00
Author
Owner
Copy Link
Copy Source

Claiming — bundled session closing out both #701 deferrals (#823 + #824) together, because both must edit the same decision record (media.nullable-primitive-collection-mutation) and splitting them across two sessions would produce conflicting edits to its rule/mechanics text.

Claude Code / Opus 5, orchestrator tier. Work happens in a worktree off origin/main.

Claiming — bundled session closing out both #701 deferrals (#823 + #824) together, because both must edit the same decision record (`media.nullable-primitive-collection-mutation`) and splitting them across two sessions would produce conflicting edits to its `rule`/`mechanics` text. Claude Code / Opus 5, orchestrator tier. Work happens in a worktree off `origin/main`.
timothy added the in-progress label 2026-08-29 11:22:47 +02:00
timothy commented 2026-08-29 21:56:51 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped in PR #879 (merged as 1afad0851). ElasticSearchIndex.UpdateSong is now covered by ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs, which drives the real indexer against a real TvContext. No production change to ElasticSearchIndex.cs — this was coverage only.

Root cause: #701 removed metadata.Artists ??= [] from both search indexers but only Lucene gained a fixture, so an Elastic-only reintroduction left the whole suite green. Not a code defect — a coverage hole that nothing could notice.

Decisions/conventions changed: media.nullable-primitive-collection-mutation's mechanics: now describes the shipped Elastic fixture and its two traps instead of saying the gap exists. ThrowOnWarningLogger lifted to ErsatzTV.Tests/Support so both fixtures share it.

Reusable knowledge:

  1. Elastic.Transport.InMemoryRequestInvoker is public in the pinned version, and ElasticsearchClientSettings(NodePool, IRequestInvoker) accepts it — so an Elasticsearch-backed component is testable with no server, no socket and no new package, injected into the private _client the way #701 injects the Lucene IndexWriter. UpdateItems never runs _client ??= CreateClient(), so the injected instance is the one used. An initial recon pass reported this as UNPROVEN; enumerating the DLL's types directly refuted that.
  2. The canned response must carry X-Elastic-Product: Elasticsearch. The client runs a product check and otherwise throws UnsupportedProductException into UpdateSong's catch — which would have made the fixture a green measurement of the error path. That is exactly how it first failed, caught by the ThrowOnWarningLogger rather than by luck. An empty response body fails the same way.
  3. Assert a document id as the last path segment, not a substring. ShouldContain(songId) passes off the index name once that name carries digits, so it silently stops discriminating.
  4. The boundary fix matters more than the site fix. SearchIndexMutationCoverageTests derives the ISearchIndex population from the declaring assembly, so a third indexer cannot reopen this hole unnoticed. Its claim stops where the check does — no static check can prove a named fixture actually drives its indexer, so it forces a human to look rather than proving coverage.

Verification: The mutation was executed, not asserted: restoring ??= in ElasticSearchIndex alone reddens the new fixture on metadata.Artists should be null but was [] while the Lucene fixture stays green — the #824 hole demonstrated rather than described. The mirror was also witnessed: restoring it in LuceneSearchIndex alone reddens Lucene and leaves Elastic green, confirming the shared-logger extraction did not cost #701 its proof and that the two fixtures are independently load-bearing. Three further mutations redden the coverage guard (indexer dropped from the covered set; two indexers mapped to one fixture; a fixture declaring no runnable test).

Deferred: none.

Docs updated: docs/decisions/records/media/nullable-primitive-collection-mutation.md, docs/decisions/README.md (regenerated), docs/guard-inventory.md.

## Closing record **Outcome:** Shipped in PR #879 (merged as `1afad0851`). `ElasticSearchIndex.UpdateSong` is now covered by `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs`, which drives the **real** indexer against a real `TvContext`. **No production change to `ElasticSearchIndex.cs` — this was coverage only.** **Root cause:** #701 removed `metadata.Artists ??= []` from both search indexers but only Lucene gained a fixture, so an Elastic-only reintroduction left the whole suite green. Not a code defect — a coverage hole that nothing could notice. **Decisions/conventions changed:** `media.nullable-primitive-collection-mutation`'s `mechanics:` now describes the shipped Elastic fixture and its two traps instead of saying the gap exists. `ThrowOnWarningLogger` lifted to `ErsatzTV.Tests/Support` so both fixtures share it. **Reusable knowledge:** 1. **`Elastic.Transport.InMemoryRequestInvoker` is public in the pinned version**, and `ElasticsearchClientSettings(NodePool, IRequestInvoker)` accepts it — so an Elasticsearch-backed component is testable with no server, no socket and no new package, injected into the private `_client` the way #701 injects the Lucene `IndexWriter`. `UpdateItems` never runs `_client ??= CreateClient()`, so the injected instance is the one used. An initial recon pass reported this as UNPROVEN; enumerating the DLL's types directly refuted that. 2. **The canned response must carry `X-Elastic-Product: Elasticsearch`.** The client runs a product check and otherwise throws `UnsupportedProductException` **into** `UpdateSong`'s catch — which would have made the fixture a green measurement of the error path. That is exactly how it first failed, caught by the `ThrowOnWarningLogger` rather than by luck. An empty response body fails the same way. 3. **Assert a document id as the last path segment, not a substring.** `ShouldContain(songId)` passes off the index name once that name carries digits, so it silently stops discriminating. 4. **The boundary fix matters more than the site fix.** `SearchIndexMutationCoverageTests` derives the `ISearchIndex` population from the declaring assembly, so a third indexer cannot reopen this hole unnoticed. Its claim stops where the check does — no static check can prove a named fixture actually *drives* its indexer, so it forces a human to look rather than proving coverage. **Verification:** The mutation was executed, not asserted: restoring `??=` in `ElasticSearchIndex` **alone** reddens the new fixture on `metadata.Artists should be null but was []` while the **Lucene fixture stays green** — the #824 hole demonstrated rather than described. The mirror was also witnessed: restoring it in `LuceneSearchIndex` alone reddens Lucene and leaves Elastic green, confirming the shared-logger extraction did not cost #701 its proof and that the two fixtures are independently load-bearing. Three further mutations redden the coverage guard (indexer dropped from the covered set; two indexers mapped to one fixture; a fixture declaring no runnable test). **Deferred:** none. **Docs updated:** `docs/decisions/records/media/nullable-primitive-collection-mutation.md`, `docs/decisions/README.md` (regenerated), `docs/guard-inventory.md`.
timothy removed the in-progress label 2026-08-29 21:56:58 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels priority: medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#824