diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs index 6a1e5237b..5489f64ec 100644 --- a/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs +++ b/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs @@ -354,21 +354,26 @@ public class ScheduleItemResponseRoundTripTests // // Properties deliberately NOT compared. The set is EMPTY, and that is a finding rather than an // oversight. The first version exempted Id on the reasoning that "the PUT replaces the item - // set, so B's rows are new rows with new ids". That is not what this endpoint does: - // ToReplaceCommand forwards r.Id, so ReplaceProgramScheduleItemsHandler takes its id-based - // reconcile path and updates each row IN PLACE. So Id compares equal and the exemption was + // set, so B's rows are new rows with new ids". That is not what this endpoint does: both of + // ReplaceProgramScheduleItemsHandler's branches reuse the existing row for a same-order, + // same-subtype payload — the id-based reconcile and the positional fallback each do + // `built.Id = existing.Id` and SetValues. So Id compares equal and the exemption was // unnecessary, resting on a rationale that was the opposite of the code. // - // What comparing Id does NOT do, measured rather than assumed: it does not pin the #252 - // id-preservation invariant. Replacing `r.Id` with `null` in ToReplaceCommand — which sends the - // handler down the create-new-rows path — leaves this test GREEN, because a fresh database - // reassigns the same ids after the old rows are deleted. The assertion is true and cheap, not - // discriminating. The test that actually pins in-place reconcile is - // ReplaceProgramScheduleItemsReconcileTests ("in-place: id preserved"); do not read this one as - // a second copy of it. + // What comparing Id does NOT do, and this is stated narrowly on purpose because two earlier + // versions of this comment asserted a MECHANISM that the code contradicted. Measured: replacing + // `r.Id` with `null` in ToReplaceCommand leaves this test GREEN. Read from the handler: an + // all-null payload gives `requestIds.Count == 0` and so takes the positional fallback, which + // also preserves ids in place. Both branches therefore agree on Id for this fixture, so NO + // id-less mutation of it can redden the Id comparison — the assertion is true and cheap, not + // discriminating. What separates the branches is a payload that REORDERS, which is what + // ReplaceProgramScheduleItemsReconcileTests covers in + // Reorder_ById_Should_Move_State_With_The_Logical_Item_Not_The_Slot and + // Insert_ById_In_Middle_Should_Keep_Existing_Ids_And_State. Those are the tests that pin + // in-place reconcile; do not read this one as a second copy of them. // - // Any name added here must still exist on the record (asserted below), so renaming a field - // cannot leave a stale exemption silently exempting nothing. + // Any name added here must still exist on ScheduleItemResponseModel (asserted below), so + // renaming a field cannot leave a stale exemption silently exempting nothing. private static readonly System.Collections.Generic.HashSet RoundTripExemptProperties = new(StringComparer.Ordinal); @@ -402,8 +407,8 @@ public class ScheduleItemResponseRoundTripTests // Collection-valued members (WatermarkIds, Watermarks, GraphicsElementIds, // GraphicsElements). The elementwise walk still delegates to each element's Equals, // so it is value equality only because those elements are records - // (NamedIdResponseModel); a future non-record element type would silently be - // compared by REFERENCE inside this loop. It is also order-sensitive, which is + // (NamedIdResponseModel) or value types (the int id lists); a future element type that + // is neither would silently be compared by REFERENCE inside this loop. It is also order-sensitive, which is // correct for these ordered lists but would be wrong for an unordered type such as // a dictionary-valued property. actual.ShouldNotBeNull($"{property.Name} was null on the round-tripped item"); diff --git a/docs/decisions/records/testing/deny-path-at-production-config-value.md b/docs/decisions/records/testing/deny-path-at-production-config-value.md index 11ab97045..9fd1cb444 100644 --- a/docs/decisions/records/testing/deny-path-at-production-config-value.md +++ b/docs/decisions/records/testing/deny-path-at-production-config-value.md @@ -44,6 +44,17 @@ Ask not only "which value did the test use" but "which code did the value flow t `ResolveKey` returns before touching the live config volume; that is a test-isolation detail and deliberately not their subject. +**What the mutation proof here does and does not show, because the honest version is weaker than +the headline.** Flipping the shipped default `true` -> `false` reddens +`Read_Gating_Is_Required_When_The_Setting_Is_Absent` and only that test. That establishes the +*absent* cell is load-bearing. It also shows the three explicitly-configured `true`/`True`/`TRUE` +cases — the cells this rule is named for — are NOT independently load-bearing against that mutation: +a mistyped configuration key is caught, but by the `false` cases, not the `true` ones. The only +mutation that would redden `"True"`/`"TRUE"` alone is a hand-rolled case-sensitive parse replacing +`GetValue`. They are kept as behaviour coverage of the real binder rather than deleted, and the +distinction is written down instead of being smoothed over: a matrix that is *complete* is not +thereby a matrix in which every cell *discriminates*. + **Why no check enforces this.** Deciding whether a test exercised a production value means reading test source and judging intent — a string-matching predicate over code, which `docs/defect-shapes-773.md` §4 argues against and which this repo has withdrawn twice after diff --git a/docs/decisions/records/testing/full-replace-asserts-field-list.md b/docs/decisions/records/testing/full-replace-asserts-field-list.md index 57af624aa..91d320fc0 100644 --- a/docs/decisions/records/testing/full-replace-asserts-field-list.md +++ b/docs/decisions/records/testing/full-replace-asserts-field-list.md @@ -41,17 +41,30 @@ contradicted the code — the failure this record is about, committed inside the Two further details. Every exempted name is asserted to still *exist* on the record, because a stale exemption exempts nothing while reading in review as a considered decision. And the anti-vacuity check is a **pin** (`ShouldBe(55)`), not a `>=` floor: a floor lets properties vanish silently, which -is the one-sided version of the both-directions rule this record invokes. Comparing against the +is the one-sided version of the both-directions rule this record invokes. (` +testing.guard-derives-population-from-source` endorses a floor on its canonical example; this is +strictly stronger, not a contradiction of it.) Comparing against the reflected count minus exemptions would be tautological — both sides come from the same reflection — so the number is written down and must be bumped deliberately. It was obtained by raising the pin and reading the failure, not counted off the source. -**What comparing `Id` does NOT do**, stated because the plausible version of this sentence is wrong. -It is tempting to say it now pins #252's id-preservation invariant. It does not, and the check is -cheap: replacing the forwarded `r.Id` with `null` — which sends the handler down the create-new-rows -path — leaves this test **green**, because a fresh database reassigns the same ids once the old rows -are deleted. The assertion is true and free, not discriminating. -`ReplaceProgramScheduleItemsReconcileTests` is what pins in-place reconcile. +**What comparing `Id` does NOT do**, stated narrowly because two earlier drafts of this paragraph +asserted a MECHANISM the code contradicted — inside the record whose subject is exactly that. What +was measured: replacing the forwarded `r.Id` with `null` leaves the round-trip test **green**. What +the handler does, read rather than inferred: an all-null payload gives `requestIds.Count == 0` and +takes the *positional fallback*, which — like the id-based reconcile — reuses the existing row +(`built.Id = existing.Id`). Both branches preserve ids for a same-order, same-subtype payload, so no +id-less mutation of that fixture can redden the `Id` comparison. It is true and cheap, not +discriminating. What separates the branches is a payload that REORDERS, which is what +`ReplaceProgramScheduleItemsReconcileTests` pins in +`Reorder_ById_Should_Move_State_With_The_Logical_Item_Not_The_Slot` and +`Insert_ById_In_Middle_Should_Keep_Existing_Ids_And_State`. + +The generalisable part is not the `Id` detail. It is that three successive drafts here each supplied +a confident causal story for a correct observation, and two of them were wrong. **Record the +measurement and the code path you actually read; do not narrate why the system behaves that way +unless that too was measured.** + **A completeness guarantee on the COMPARER is not one on the FIXTURE.** The seed builder feeding this round trip is still a hand-written list. A new nullable field is now reflected and compared — as @@ -69,13 +82,17 @@ item ids. That is false, and the counterexamples live in the same subsystem the - `PlayoutItem.SchedulingContext` is a persisted column holding a serialized `BlockSchedulingContext(BlockId, **BlockItemId**, Enumerator, Seed, Index)`. - `ReplaceBlockItemsHandler` renumbers `BlockItem.Id`, after which - `ProcessSchedulingContextHandler` resolves nothing and falls back to - `new BlockContextBlockItem(id, null, null)` — so the troubleshooting view for an already-built - playout item loses its collection type and name. + `ReplaceBlockItemsHandler` deletes and reinserts, so the stored `BlockItemId` no longer refers to + the row it was written for. `ProcessSchedulingContextHandler` looks it up: it either resolves + nothing and falls back to `new BlockContextBlockItem(id, null, null)`, or — if the id was reused by + a different item — resolves to the WRONG one. Which of the two happens was not measured, so the + claim here is only that the reference is orphaned; the second case is the worse one and is the + reason not to write this off. - `PlayoutItem.GuideGroup` is set to `effectiveBlock.TemplateItemId` and persisted, while - `ReplaceTemplateItemsHandler` renumbers `TemplateItem.Id`. `GuideGroup` is the XMLTV grouping key, - so renumbered ids can collide with values already persisted on older items. + `ReplaceTemplateItemsHandler` deletes and reinserts. `GuideGroup` is the XMLTV grouping key, so a + reused id colliding with one already persisted on older items is a HAZARD rather than a + demonstrated consequence — unmeasured, and narrowed by the projector grouping only contiguous runs + that also share `GuideStart`/`GuideFinish`. The accurate statement is narrower: **no progression or enumerator state is keyed to those ids**, so the replace does not reset scheduling position — but two persisted *diagnostic/grouping* fields do