Files
ersatztv/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs
T
timothyandClaude Opus 5 3951fcf516
Build ErsatzTV Image / CI toolchain image resolves (pull_request) Successful in 6s
Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 18s
PR Gates / Fix proofs (Proves trailers) (pull_request) Successful in 9s
PR Gates / decisions lifecycle (pull_request) Successful in 14s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
review-verdict/h10 Awaiting review verdict for 3951fcf
Review verdict / Set review-verdict status (pull_request_target) Successful in 16s
PR Gates / Docs update reminder (pull_request) Successful in 11s
PR Gates / Script lint and tests (ruff + pytest) (pull_request) Successful in 7m52s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Canceled after 0s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Canceled after 0s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Canceled after 0s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Canceled after 0s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Canceled after 0s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Canceled after 0s
fix(823): stop patching the record by grep — a second ?? [] survived, and my "swept it" claim was false
Round-four review. One HIGH, again in the decision record, and the previous
commit message asserted this exact class was cleared. It was not.

THE HIGH, and the reason it recurred.

A second sentence still described the rejected reading: "The guard form is
`?? []` into a local rather than this record's Optional(x).Flatten(), a STATED
deviation". The shipped guard is `?? AllDaysOfWeek()`. That sentence is the one
that dictates guard FORM to the next implementer, so it would have taught the
`[]` reading the same record spends a paragraph calling data corruption -- and
it had already propagated into docs/decisions/README.md, the mandated entry
point, which carries `rule:` verbatim.

The mechanism, not the sentence, is the defect. I swept with a regex keyed on
"null" plus a reading word; this sentence talks about guard FORM and contains
neither, so it could not match. That is grepping the retracted WORDING instead
of sweeping the CONCEPT, which is exactly what this corpus warns about -- and
three rounds in a row have now found a defect introduced by the previous
round's targeted string edit. So the fix is not another targeted edit: the
whole `rule:` field was split into its 39 sentences and read back one by one
against the code. Everything below came out of that pass rather than a grep.

Its secondary damage is worth recording because it is the shape of a rationale
that outlives its claim: the deviation was justified by ".ToList() allocates
for nothing", which is now BOTH irrelevant to the choice AND false about the
shipped code, since AllDaysOfMonth()/AllMonthsOfYear() are themselves
Enumerable.Range(...).ToList() on exactly the null path it describes.

- The opening sentence of `rule:` prescribed Optional(x).Flatten() as THE
  read-site form. It is the sentence most likely to be read in isolation, and
  it is wrong for six of the eight columns. It now separates the universal half
  (a LOCAL, never assigned back) from the half that is not (the substituted
  value), and names where each applies.
- `signals:` had never been touched, so roughly 60% of `rule:` was unreachable
  by the discovery surface built for it -- no AlternateScheduleSelector, no
  mapper, no "unrestricted", and its paths: list named none of the files this
  work touched. It also advertised "Optional Flatten hoisted local" as the
  form, which is precisely what the six do NOT use.
- The body prose was still entirely about SongMetadata while `rule:` had grown
  a whole second subject. Added the two results that contradicted the prior
  reasoning, in prose, where a reader meets them.

A REAL BUG in my own guard, not just prose:

  fixture.IsAbstract.ShouldBeFalse(...)

A C# `static class` compiles to `abstract sealed`, and NUnit runs tests
declared in one -- this repo already has such a fixture
(AlternateScheduleSelectorTests is `public static class`). So the check I added
one commit ago to reject an un-runnable fixture would have falsely reddened a
perfectly good static one. Now rejects an abstract BASE (abstract and NOT
sealed), which is the case NUnit actually cannot instantiate.

A SURVIVING MUTANT the added controls did not kill:

AnyDate was 2024-03-06. With a day <= 12 a CROSS-WIRED substitution survives
the whole fixture -- `DaysOfMonth ?? AllMonthsOfYear()` hands back 1..12, which
still contains day 6, so every assertion passes while the guard substitutes the
wrong set. Moved to 2024-03-20, still a Wednesday in March, outside 1..12.
Measured both ways rather than reasoned: the cross-wire mutant passes the old
fixture and FAILS 2 of 11 on the new one.

Also re-witnessed, because I had modified that file and never re-proved it:
restoring `??=` in LuceneSearchIndex reddens the LUCENE fixture (1 red, 1
green) -- the exact mirror of the Elastic mutation. Extracting
ThrowOnWarningLogger did not cost #701 its proof, and the two fixtures are
independently load-bearing in both directions.

The record is now 73 prose lines, over the 60-line WARNING ceiling. Stated
rather than trimmed: it is 42nd of 42 records over that line, and the added
content is distinct findings (a second subject, a migration analysis and three
residuals), not redundancy against a sibling.

Local gate: ErsatzTV.Tests 2091 passed / 6 skipped (the three fixtures' MySQL
halves), Core.Tests 697/1 -- 0 failures. Format clean, no BOM. decisions
validate OK.

Refs #823
Refs #824

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW
2026-08-29 21:00:22 +02:00

127 lines
7.4 KiB
C#

using System.Reflection;
using ErsatzTV.Core.Interfaces.Search;
using ErsatzTV.Infrastructure.Search;
using NUnit.Framework;
using NUnit.Framework.Interfaces;
using Shouldly;
namespace ErsatzTV.Tests.Integration;
/// <summary>
/// ersatztv#824. The defect that produced #824 was not that <c>ElasticSearchIndex</c> was hard to
/// test — it was that NOTHING NOTICED it had no cover. #701 fixed two independent copies of the same
/// <c>UpdateSong</c> logic and pinned one; the suite stayed green, and the gap survived on a
/// hand-written list of what had been covered (namely, one entry).
/// <para>
/// So the covered set is compared against a population DERIVED FROM THE ASSEMBLY rather than
/// restated: a third <see cref="ISearchIndex" /> implementation reddens this test until it is
/// given a mutation fixture of its own. That is
/// <c>testing.guard-derives-population-from-source</c> applied to a test population instead of a
/// file population.
/// </para>
/// <para>
/// <b>Scope, stated rather than implied.</b> The derivation is over the assembly that declares
/// both indexers (<c>ErsatzTV.Infrastructure</c>). An implementation added in a DIFFERENT
/// assembly is outside what this sees — it is not covered and this test cannot say so. Both
/// implementations have lived here since the interface existed, so the narrower scope buys a
/// guard that cannot be defeated by an unrelated assembly load order; widening it to every
/// loaded assembly would be the false-confidence version of the same check.
/// </para>
/// </summary>
[TestFixture]
public class SearchIndexMutationCoverageTests
{
/// <summary>
/// The indexers whose <c>UpdateSong</c> is pinned against the ersatztv#701 mutation. Referenced by
/// TYPE, so renaming an indexer or deleting a fixture is a compile error rather than a silent
/// divergence.
/// </summary>
private static readonly Dictionary<Type, Type> CoveredBy = new()
{
[typeof(LuceneSearchIndex)] = typeof(SongIndexerMetadataMutationTests),
[typeof(ElasticSearchIndex)] = typeof(ElasticSongIndexerMetadataMutationTests)
};
[Test]
public void Every_ISearchIndex_Implementation_Has_A_Metadata_Mutation_Fixture()
{
List<Type> implementations = typeof(LuceneSearchIndex).Assembly
.GetTypes()
.Where(t => t is { IsAbstract: false, IsInterface: false })
.Where(t => typeof(ISearchIndex).IsAssignableFrom(t))
.OrderBy(t => t.FullName, StringComparer.Ordinal)
.ToList();
// The set comparison below would ALREADY fail on an empty derivation, because the expected side
// is non-empty -- so this floor is not load-bearing for correctness and saying it is would be a
// false claim about a check. It is a DIAGNOSTIC: it separates "the reflection stopped finding
// types" (a moved type, a renamed interface) from "someone added an indexer", which the set
// comparison alone reports identically.
implementations.Count.ShouldBeGreaterThanOrEqualTo(
2,
"the ISearchIndex population derivation found almost nothing -- this is the derivation "
+ "breaking, not an indexer being added");
implementations.ShouldBe(
CoveredBy.Keys.OrderBy(t => t.FullName, StringComparer.Ordinal),
ignoreOrder: false,
"every ISearchIndex implementation needs an UpdateSong metadata-mutation fixture — see "
+ "ersatztv#824, where a second copy of the same logic went uncovered and an Elastic-only "
+ "reintroduction of `metadata.Artists ??= []` left the whole suite green");
// Comparing the KEYS alone would leave the mapping half-checked: a third indexer could be pointed
// at an EXISTING fixture, or at a fixture class holding no runnable test, and the set comparison
// above would still pass. Both are closed here. What NO static check can establish is that the
// named fixture actually DRIVES its indexer -- that is stated as a residual on this guard rather
// than implied away, and it is why the record claims a third implementation cannot be added
// WITHOUT NOTICE, not that it cannot be mis-covered.
CoveredBy.Values.Distinct().Count().ShouldBe(
CoveredBy.Count,
"two indexers are mapped to the SAME fixture, so one of them is not actually covered");
foreach ((Type indexer, Type fixture) in CoveredBy)
{
// Test-ness is decided by NUnit's INTERFACES, not by a hand-listed set of attribute types.
// A list has to be kept in step with NUnit and silently falsely-reddens whatever it omits --
// an earlier revision listed TestAttribute alone (so a [TestCase]-only fixture failed), then
// three types (so a [Theory] one did). ITestBuilder/ISimpleTestBuilder is what NUnit itself
// dispatches on, so it cannot fall behind the vocabulary.
//
// DeclaredOnly is load-bearing: without it a fixture that merely SUBCLASSES another inherits
// its [Test] and satisfies this while driving the wrong indexer -- and Values.Distinct()
// cannot catch that, since the two Types differ. Static is included because NUnit runs a
// public static test method in a non-static fixture, and omitting the flag would falsely
// redden one.
const BindingFlags TestMethods =
BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly;
static bool NeverRuns(object[] attributes) =>
attributes.Any(a => a is ExplicitAttribute or IgnoreAttribute);
bool declaresARunnableTest = fixture
.GetMethods(TestMethods)
.Select(m => m.GetCustomAttributes(inherit: true))
.Any(attrs =>
attrs.Any(a => a is ITestBuilder or ISimpleTestBuilder) && !NeverRuns(attrs));
declaresARunnableTest.ShouldBeTrue(
$"{fixture.Name} is named as the mutation fixture for {indexer.Name} but DECLARES no "
+ "runnable test method of its own, so it proves nothing");
// The same "wired is not running" failure at fixture level, in its three forms: an abstract
// class NUnit will not instantiate, and [Explicit]/[Ignore]. The indexer population at the top
// of this method already filters IsAbstract; the fixture side needs the mirror of that.
// `IsAbstract` ALONE is wrong here and would have been a false red: a C# `static class`
// compiles to `abstract sealed`, and NUnit runs tests declared in one -- this repo already has
// such a fixture (`AlternateScheduleSelectorTests`). What must be rejected is an abstract BASE
// (abstract and NOT sealed), which NUnit cannot instantiate.
(fixture.IsAbstract && !fixture.IsSealed).ShouldBeFalse(
$"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is an abstract "
+ "base class, so NUnit never runs it");
NeverRuns(fixture.GetCustomAttributes(inherit: true)).ShouldBeFalse(
$"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is [Explicit] or "
+ "[Ignore]d, so it never runs and proves nothing");
}
}
}