diff --git a/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs index e686f14e7..8ae3e001d 100644 --- a/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs +++ b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs @@ -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. /// - [Test] - public void Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_A_Resolvable_Channel_Logo() + /// + /// Parameterized over all three blank forms because the guard is IsNullOrWhiteSpace: testing only + /// " " would let a mutation to image == " " pass while silently breaking fall-through + /// for null and "" — and null is the form the API actually persists. + /// + [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); } + /// + /// Pins the ImageSource is Custom half of the blank-image guard, which nothing else covers. + /// + /// + /// A ChannelLogo watermark's Image 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. + /// + [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 result = Selector(null, LogoCachePath) + .GetWatermarkOptions(channel, playoutItemWatermark, Option.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); + } + /// /// `CreateWatermarkHandler`/`UpdateWatermarkHandler` write `Image = null` for every non-`Custom` /// watermark, so an API-created `Resource` watermark hits `Path.Combine(folder, null)` — an diff --git a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md index 42742ae4e..8732c32e6 100644 --- a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md +++ b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md @@ -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.