fix(510): pin the blank-guard's is-Custom discriminator, verified by mutation
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 13s
PR Gates / Docs update reminder (pull_request) Successful in 16s
Review verdict / Set review-verdict status (pull_request) Successful in 4s
PR Gates / decisions lifecycle (pull_request) Successful in 24s
PR Gates / Script tests (pytest) (pull_request) Successful in 35s
review-verdict/h10 Review-verdict: MERGEABLE @ bc1a37f
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m32s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 9s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 8s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 15m57s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m17s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 13s
PR Gates / Docs update reminder (pull_request) Successful in 16s
Review verdict / Set review-verdict status (pull_request) Successful in 4s
PR Gates / decisions lifecycle (pull_request) Successful in 24s
PR Gates / Script tests (pytest) (pull_request) Successful in 35s
review-verdict/h10 Review-verdict: MERGEABLE @ bc1a37f
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m32s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 9s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 8s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 15m57s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m17s
Round-4 review found the fall-through tests, while now falsifiable, still did
not pin the whole guard. Two gaps, both closed and both verified by running the
mutation rather than by asserting the test would catch it:
1. All three fall-through tests used only " ", so narrowing
IsNullOrWhiteSpace to `image == " "` would have passed while breaking
fall-through for null and "" -- and null is the form the API actually
persists. Parameterized over null, "" and " ".
2. Nothing pinned the guard's `ImageSource is Custom` clause. This is the
sharper of the two: a ChannelLogo watermark's Image is NORMALLY blank
(the API writes Image = null for every non-Custom source), so dropping the
discriminator would send every playout-item ChannelLogo watermark down the
fall-through path instead of resolving the channel's own logo -- with a
fully green suite. Added
Blank_Image_ChannelLogo_Playout_Item_Watermark_Should_Win_And_Not_Fall_Through,
which distinguishes the two levels by watermark Id so a fall-through is
observable even though both resolve to the same cached path.
Mutation results (each mutation applied on its own, then reverted):
drop `is Custom` from the guard -> 1 failure, and the new test is the ONLY
test that catches it
IsNullOrWhiteSpace -> == " " -> the null and "" parameterized cases fail
Negative control re-measured on the final 32-case fixture: 19 fail against the
origin/main resolver. The 13 that pass both ways pin deliberately preserved
behavior plus the positive control, which the record now states explicitly
along with the mutation table.
This round's lesson, recorded in the record: a test's NAME is not evidence it
pins what it claims, and a whole-file revert cannot show that a test aimed at a
specific clause actually reaches that clause -- only mutating the clause can.
Gates: 2661 tests green across 4 projects, 225/225 script tests (the gate I
skipped before the last push), decisions-validate OK, format exit 0, no BOMs.
refs #510
This commit is contained in:
@@ -384,10 +384,17 @@ public class WatermarkSelectorDecoResolutionTests
|
||||
/// rather than missing, must fall through and return the resolvable channel logo. Together the pair
|
||||
/// shows the guard distinguishes blank from unresolvable, rather than both landing on None.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public void Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_A_Resolvable_Channel_Logo()
|
||||
/// <remarks>
|
||||
/// Parameterized over all three blank forms because the guard is <c>IsNullOrWhiteSpace</c>: testing only
|
||||
/// <c>" "</c> would let a mutation to <c>image == " "</c> pass while silently breaking fall-through
|
||||
/// for <c>null</c> and <c>""</c> — and <c>null</c> is the form the API actually persists.
|
||||
/// </remarks>
|
||||
[TestCase(null)]
|
||||
[TestCase("")]
|
||||
[TestCase(" ")]
|
||||
public void Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_A_Resolvable_Channel_Logo(string image)
|
||||
{
|
||||
ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.Custom, " ");
|
||||
ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.Custom, image);
|
||||
ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.ChannelLogo);
|
||||
channelWatermark.Id = 8;
|
||||
Channel channel = ChannelWith(LogoStoredPath, channelWatermark);
|
||||
@@ -399,6 +406,39 @@ public class WatermarkSelectorDecoResolutionTests
|
||||
result.IfNone(() => throw new InvalidOperationException()).ImagePath.ShouldBe(LogoCachePath);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Pins the <c>ImageSource is Custom</c> half of the blank-image guard, which nothing else covers.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// A <c>ChannelLogo</c> watermark's <c>Image</c> is NORMALLY blank — the API persists `Image = null` for
|
||||
/// every non-`Custom` source — so if the guard's `is Custom` discriminator were dropped, leaving only
|
||||
/// `IsNullOrWhiteSpace(Image)`, every playout-item `ChannelLogo` watermark would fall through to
|
||||
/// channel/global instead of resolving the channel's own logo. This test fails on that mutation: the
|
||||
/// playout-item watermark carries a distinguishing Id, so falling through is observable even though both
|
||||
/// levels would resolve to the same cached path.
|
||||
/// </remarks>
|
||||
[Test]
|
||||
public void Blank_Image_ChannelLogo_Playout_Item_Watermark_Should_Win_And_Not_Fall_Through()
|
||||
{
|
||||
// Image is left blank, exactly as the API stores a ChannelLogo watermark
|
||||
ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.ChannelLogo);
|
||||
playoutItemWatermark.Id = 42;
|
||||
|
||||
ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.ChannelLogo);
|
||||
channelWatermark.Id = 8;
|
||||
Channel channel = ChannelWith(LogoStoredPath, channelWatermark);
|
||||
|
||||
Option<WatermarkOptions> result = Selector(null, LogoCachePath)
|
||||
.GetWatermarkOptions(channel, playoutItemWatermark, Option<ChannelWatermark>.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
WatermarkOptions options = result.IfNone(() => throw new InvalidOperationException());
|
||||
options.ImagePath.ShouldBe(LogoCachePath);
|
||||
|
||||
// the PLAYOUT-ITEM watermark won; a fall-through would have returned the channel's (Id 8)
|
||||
options.Watermark.Id.ShouldBe(42);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// `CreateWatermarkHandler`/`UpdateWatermarkHandler` write `Image = null` for every non-`Custom`
|
||||
/// watermark, so an API-created `Resource` watermark hits `Path.Combine(folder, null)` — an
|
||||
|
||||
@@ -91,12 +91,26 @@ a neighbour's behavior.
|
||||
The parity test cases are the structural guard: they assert deco and channel-level resolution return
|
||||
identical paths case for case, so re-introducing a per-caller policy fails in CI rather than in prod.
|
||||
|
||||
**Measured mutation sensitivity: 19 of the 29 cases fail when the pre-fix resolver is restored under the
|
||||
final fixture.** The other 10 pass both ways by design — they pin behavior this change deliberately
|
||||
*preserves* (the blank-`Custom` fall-through, file-present resolution) plus the positive control, so
|
||||
passing before and after is the correct outcome for them, not a gap. This figure is worth re-measuring
|
||||
rather than trusting if the fixture changes: the review of this very change found two tests that had
|
||||
been added to close review findings yet could not fail — one whose fall-through fallback was itself
|
||||
unresolvable, and one that asserted a filtered list length while the routing claim it was cited for
|
||||
lived in a function it never called. Both are fixed; the lesson is that a test added under review
|
||||
pressure needs the same negative control as the original.
|
||||
**Measured mutation sensitivity: 19 of the 32 cases fail when the pre-fix resolver is restored under the
|
||||
final fixture.** The other 13 pass both ways by design — they pin behavior this change deliberately
|
||||
*preserves* (the blank-`Custom` fall-through, file-present resolution, the `is Custom` discriminator)
|
||||
plus the positive control, so passing before and after is the correct outcome for them, not a gap.
|
||||
|
||||
Two guard mutations were run individually, because a whole-file revert cannot show that a test aimed at
|
||||
a specific clause actually hits it:
|
||||
|
||||
| Mutation | Caught by |
|
||||
|---|---|
|
||||
| drop `ImageSource is Custom` from the blank guard | `Blank_Image_ChannelLogo_Playout_Item_Watermark_Should_Win_And_Not_Fall_Through` (1 failure, and the only test that catches it) |
|
||||
| narrow `IsNullOrWhiteSpace` to `== " "` | the `null` and `""` cases of `Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_A_Resolvable_Channel_Logo` |
|
||||
|
||||
That first mutation matters more than it looks: a `ChannelLogo` watermark's `Image` is *normally* blank,
|
||||
so dropping the discriminator would send every playout-item `ChannelLogo` watermark down the
|
||||
fall-through path instead of resolving the channel's own logo.
|
||||
|
||||
Re-measure rather than trust these figures if the fixture changes. Review of this very change found
|
||||
**two tests that had been added to close earlier review findings yet could not fail** — one whose
|
||||
fall-through fallback was itself unresolvable (so a wrongly-widened guard still yielded `None`), and one
|
||||
that asserted a filtered list length while the routing claim it was cited for lived in a function it
|
||||
never called. The lesson is that a test added under review pressure needs the same negative control as
|
||||
the original, and that a test's *name* is not evidence it pins what it claims.
|
||||
|
||||
Reference in New Issue
Block a user