fix(823,824): a scheduling NULL collection reads as UNRESTRICTED and is guarded at both read sites; the Elastic indexer gets its own mutation proof
Build ErsatzTV Image / CI toolchain image resolves (pull_request) Successful in 6s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 18s
Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 20s
PR Gates / decisions lifecycle (pull_request) Successful in 20s
PR Gates / Docs update reminder (pull_request) Successful in 21s
PR Gates / Fix proofs (Proves trailers) (pull_request) Successful in 11s
review-verdict/h10 Review-verdict: MERGEABLE @ 95b2700 (base: main)
Review verdict / Set review-verdict status (pull_request_target) Successful in 23s
PR Gates / Script lint and tests (ruff + pytest) (pull_request) Successful in 8m6s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m55s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 6m35s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Canceled after 2m56s
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
Build ErsatzTV Image / CI toolchain image resolves (pull_request) Successful in 6s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 18s
Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 20s
PR Gates / decisions lifecycle (pull_request) Successful in 20s
PR Gates / Docs update reminder (pull_request) Successful in 21s
PR Gates / Fix proofs (Proves trailers) (pull_request) Successful in 11s
review-verdict/h10 Review-verdict: MERGEABLE @ 95b2700 (base: main)
Review verdict / Set review-verdict status (pull_request_target) Successful in 23s
PR Gates / Script lint and tests (ruff + pytest) (pull_request) Successful in 8m6s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m55s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 6m35s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Canceled after 2m56s
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
Both issues are #701 deferrals, and they land together because both rewrite the same decision record. #823 -- can a null reach one of the six collection-valued scalar columns? MEASURED against a real TvContext on BOTH providers (SQLite, and MySQL 8.4 on an ephemeral server), because the reasoning available beforehand pointed the wrong way. The two converters differ on their read side -- IntCollectionValueConverter maps null-or-blank to Array.Empty<int>(), while EnumCollectionJsonValueConverter would dereference the result of JsonConvert.DeserializeObject -- so the expectation was that a NULL row behaves differently per column. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all. All six materialize as CLR null, the int converter's null-to-empty branch is dead on this path, and unguarded each .Contains in AlternateScheduleSelector throws NullReferenceException. A NULL reads as UNRESTRICTED -- the All*() sets -- not as empty. This is the whole semantic question and the first draft got it backwards. It is decided by the one NULL reachable WITHOUT any code writing one: Sqlite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth adds the column with nullable:true and NO defaultValue, so a PlayoutTemplate row inserted before it holds NULL and by construction had no day-of-month restriction. Reading that as empty INVERTS the row's meaning and silently stops the template applying at all. All*() preserves it, and is how "no restriction recorded" is already represented (GetPlayoutAlternateSchedulesHandler, PreviewBlockPlayoutHandler). What does NOT decide it, and was wrongly cited in the first draft: the API request records normalize an omitted field with `?? []`, but that is a client omitting a field on a WRITE and says nothing about what a legacy database NULL meant. Two read sites, not one. Guarding only the selector would have left the entity->DTO mappers unguarded, and those feed the SPA: PlayoutScheduleEditors spreads the collection (`[...template.daysOfMonth]` -> TypeError on a JSON null) and playoutTemplateCalendar's appliesToDate -- an exact port of GetScheduleForDate -- calls .includes on it. Both mappers now substitute the SAME defaults, so the preview agrees with what is actually scheduled. Neither guard is assigned back onto the entity, which is the media.nullable-primitive-collection-mutation mechanism. Reachability, stated precisely rather than overclaimed. All six are nullable:true on both providers, but a nullable column does not produce a NULL row: five of the six were present at CreateTable, so a NULL there still needs code to write one, and on MySQL there is NO code-path-free NULL for any of the six. The write path ACCEPTS a null (SaveChanges succeeds, stores SQL NULL) but no caller supplies one today -- every production construction of the two commands goes through the request records. That is a property of the code, not a live caller; claiming otherwise would be the banned "it's AsNoTracking today" argument pointed the other way. #824 -- ElasticSearchIndex.UpdateSong had no regression test Issue option 1 (a non-network transport) shipped, and needed no new package: Elastic.Transport.InMemoryRequestInvoker is public in the pinned version and ElasticsearchClientSettings(NodePool, IRequestInvoker) accepts it, 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. Two traps there are load-bearing, both measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client's product check throws UnsupportedProductException INTO UpdateSong's catch, and an empty body fails to deserialize the same way. Either turns the fixture into a green measurement of the error path -- which is how it first failed here, caught by the ThrowOnWarningLogger. The document id is asserted as the LAST PATH SEGMENT, not by substring: the index name carries digits, so ShouldContain would stop discriminating for a song whose id collided with one. Six mutations executed, each disarming ITS OWN clause alone: - `??=` restored in ElasticSearchIndex only -> the Elastic fixture reddens on "metadata.Artists should be null but was []" while the LUCENE fixture stays GREEN. The #824 hole demonstrated, not described. - DaysOfWeek guard disarmed in the selector -> 4 red, 3 green (DaysOfMonth and MonthsOfYear unaffected). Each clause is independently load-bearing. - DaysOfMonth guard disarmed in Playouts.Mapper -> 1 red, 2 green. - Elastic dropped from the covered set / mapped to the SAME fixture as Lucene / mapped to a class with no [Test] -> SearchIndexMutationCoverageTests reddens on each. That coverage guard is the boundary fix the issue asked for: the covered set is compared against an ISearchIndex population DERIVED FROM THE ASSEMBLY. Its claim stops where the check does -- no static check can establish that a named fixture actually DRIVES its indexer, so it forces a human to look rather than proving coverage. ThrowOnWarningLogger moved to ErsatzTV.Tests/Support so both fixtures share it; the Lucene fixture's assertions are otherwise untouched, since it is a witnessed proof artifact. No production change in ElasticSearchIndex.cs -- #824 is coverage only. Docs: testing.md gains a "Provider-parity fixtures" section naming all THREE opt-in-MySQL fixtures and recording that CI runs none of them (#627); docs/README.md gains the matching task signal; guard-inventory.md's hand-written C# guard list goes from five files to six. Scheduling/Mapper.cs loses the UTF-8 BOM it inherited, per #311 fix-as-you-touch. Local gate (with the MySQL lane armed): ErsatzTV.Tests 2096 passed / 0 skipped, Core.Tests 693/1, Infrastructure.Tests 114, Architecture.Tests 7, Scanner.Tests 1504 -- 0 failures in each. scripts/tests 1228 passed / 2 skipped. dotnet format whitespace --verify-no-changes clean; BOM check over the touched set with the population COUNT asserted, because a bare zsh loop silently checks one concatenated filename. decisions_validate OK. Fixes #823 Fixes #824 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Scheduling;
|
||||
using ErsatzTV.Core.Scheduling;
|
||||
|
||||
namespace ErsatzTV.Application.Playouts;
|
||||
|
||||
@@ -40,9 +41,15 @@ internal static class Mapper
|
||||
programScheduleAlternate.Id,
|
||||
programScheduleAlternate.Index,
|
||||
programScheduleAlternate.ProgramScheduleId,
|
||||
programScheduleAlternate.DaysOfWeek,
|
||||
programScheduleAlternate.DaysOfMonth,
|
||||
programScheduleAlternate.MonthsOfYear,
|
||||
// ersatztv#823: these three are NULLABLE columns and a legacy row can hold NULL. Substitute the
|
||||
// SAME unrestricted defaults AlternateScheduleSelector.GetScheduleForDate reads, so the DTO the
|
||||
// SPA renders agrees with what actually gets scheduled -- web/src/screens/playoutTemplateCalendar.ts
|
||||
// `appliesToDate` is an exact port of that method, and it would otherwise both mispreview and
|
||||
// throw (`[...template.daysOfMonth]` on a null is a TypeError). Never assigned back onto the
|
||||
// entity (`media.nullable-primitive-collection-mutation`).
|
||||
programScheduleAlternate.DaysOfWeek ?? AlternateScheduleSelector.AllDaysOfWeek(),
|
||||
programScheduleAlternate.DaysOfMonth ?? AlternateScheduleSelector.AllDaysOfMonth(),
|
||||
programScheduleAlternate.MonthsOfYear ?? AlternateScheduleSelector.AllMonthsOfYear(),
|
||||
programScheduleAlternate.LimitToDateRange,
|
||||
programScheduleAlternate.StartMonth,
|
||||
programScheduleAlternate.StartDay,
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
using ErsatzTV.Application.Tree;
|
||||
using ErsatzTV.Application.Tree;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Scheduling;
|
||||
using ErsatzTV.Core.Scheduling;
|
||||
|
||||
namespace ErsatzTV.Application.Scheduling;
|
||||
|
||||
@@ -190,9 +191,15 @@ internal static class Mapper
|
||||
ProjectToViewModel(playoutTemplate.Template),
|
||||
ProjectToViewModel(playoutTemplate.DecoTemplate),
|
||||
playoutTemplate.Index,
|
||||
playoutTemplate.DaysOfWeek,
|
||||
playoutTemplate.DaysOfMonth,
|
||||
playoutTemplate.MonthsOfYear,
|
||||
// ersatztv#823: these three are NULLABLE columns and a legacy row can hold NULL. Substitute the
|
||||
// SAME unrestricted defaults AlternateScheduleSelector.GetScheduleForDate reads, so the DTO the
|
||||
// SPA renders agrees with what actually gets scheduled -- web/src/screens/playoutTemplateCalendar.ts
|
||||
// `appliesToDate` is an exact port of that method, and it would otherwise both mispreview and
|
||||
// throw (`[...template.daysOfMonth]` on a null is a TypeError). Never assigned back onto the
|
||||
// entity (`media.nullable-primitive-collection-mutation`).
|
||||
playoutTemplate.DaysOfWeek ?? AlternateScheduleSelector.AllDaysOfWeek(),
|
||||
playoutTemplate.DaysOfMonth ?? AlternateScheduleSelector.AllDaysOfMonth(),
|
||||
playoutTemplate.MonthsOfYear ?? AlternateScheduleSelector.AllMonthsOfYear(),
|
||||
playoutTemplate.LimitToDateRange,
|
||||
playoutTemplate.StartMonth,
|
||||
playoutTemplate.StartDay,
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Scheduling;
|
||||
using ErsatzTV.Core.Scheduling;
|
||||
using NUnit.Framework;
|
||||
@@ -864,4 +865,148 @@ public static class AlternateScheduleSelectorTests
|
||||
result.IsNone.ShouldBeFalse();
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// ersatztv#823. <c>DaysOfWeek</c>, <c>DaysOfMonth</c> and <c>MonthsOfYear</c> on
|
||||
/// <see cref="PlayoutTemplate" /> and <see cref="ProgramScheduleAlternate" /> are six
|
||||
/// single-column primitive collections whose columns are <c>nullable: true</c> on both providers.
|
||||
/// A NULL column materializes as CLR <c>null</c> — EF does not invoke the value converter for a
|
||||
/// NULL at all — so unguarded, each <c>.Contains</c> in
|
||||
/// <see cref="AlternateScheduleSelector.GetScheduleForDate{T}" /> throws
|
||||
/// <see cref="NullReferenceException" />. These tests are RED without the read-site guard.
|
||||
/// <para>
|
||||
/// A null reads as UNRESTRICTED (the <c>All*()</c> sets), not as empty. The deciding case is
|
||||
/// SQLite's <c>20240113140741_Add_PlayoutTemplate_DaysOfMonth</c>, which adds the column
|
||||
/// <c>nullable: true</c> with NO default: a row inserted before it had no day-of-month
|
||||
/// restriction, so reading its NULL as empty would INVERT its meaning and silently stop the
|
||||
/// template applying. That is the one NULL reachable without any code writing one.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Reachability itself is pinned by
|
||||
/// <c>ErsatzTV.Tests.Integration.SchedulingCollectionColumnNullTests</c> against a real
|
||||
/// <c>TvContext</c>; these tests pin what the selector does once the null is there.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[TestFixture]
|
||||
public class GetScheduleForDate_NullCollections
|
||||
{
|
||||
private static readonly TimeSpan Offset = TimeSpan.FromHours(-5);
|
||||
|
||||
// A Wednesday in March, so no All*() member is coincidentally excluded.
|
||||
private static readonly DateTimeOffset AnyDate = new(2024, 3, 6, 0, 0, 0, Offset);
|
||||
|
||||
private static PlayoutTemplate Unrestricted() =>
|
||||
new()
|
||||
{
|
||||
DaysOfWeek = AlternateScheduleSelector.AllDaysOfWeek(),
|
||||
DaysOfMonth = AlternateScheduleSelector.AllDaysOfMonth(),
|
||||
MonthsOfYear = AlternateScheduleSelector.AllMonthsOfYear()
|
||||
};
|
||||
|
||||
private static Option<PlayoutTemplate> Select(params PlayoutTemplate[] templates) =>
|
||||
AlternateScheduleSelector.GetScheduleForDate(templates.ToList(), AnyDate);
|
||||
|
||||
[Test]
|
||||
public void Null_DaysOfWeek_Reads_As_Unrestricted()
|
||||
{
|
||||
PlayoutTemplate template = Unrestricted();
|
||||
template.DaysOfWeek = null!;
|
||||
|
||||
Select(template).IsSome.ShouldBeTrue(
|
||||
"a NULL DaysOfWeek means no weekday restriction was recorded, so the template still applies");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Null_DaysOfMonth_Reads_As_Unrestricted()
|
||||
{
|
||||
PlayoutTemplate template = Unrestricted();
|
||||
template.DaysOfMonth = null!;
|
||||
|
||||
Select(template).IsSome.ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Null_MonthsOfYear_Reads_As_Unrestricted()
|
||||
{
|
||||
PlayoutTemplate template = Unrestricted();
|
||||
template.MonthsOfYear = null!;
|
||||
|
||||
Select(template).IsSome.ShouldBeTrue();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void All_Three_Null_On_ProgramScheduleAlternate_Reads_As_Unrestricted()
|
||||
{
|
||||
var alternate = new ProgramScheduleAlternate
|
||||
{
|
||||
DaysOfWeek = null!,
|
||||
DaysOfMonth = null!,
|
||||
MonthsOfYear = null!
|
||||
};
|
||||
|
||||
AlternateScheduleSelector.GetScheduleForDate(
|
||||
new List<ProgramScheduleAlternate> { alternate },
|
||||
AnyDate)
|
||||
.IsSome.ShouldBeTrue();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A null must not be confused with an explicitly EMPTY collection. Empty is a legal, reachable
|
||||
/// state meaning "matches no day", and it keeps that meaning — which is exactly why a NULL
|
||||
/// cannot be normalized to it.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public void An_Explicitly_Empty_Collection_Still_Matches_Nothing()
|
||||
{
|
||||
PlayoutTemplate template = Unrestricted();
|
||||
template.DaysOfWeek = [];
|
||||
|
||||
Select(template).IsNone.ShouldBeTrue(
|
||||
"an empty DaysOfWeek is a recorded restriction of NO days, unlike a NULL");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The guard resolves per item, so a null on the FIRST item must not decide the second.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public void A_Null_Item_Does_Not_Disturb_Selection_Of_A_Later_Item()
|
||||
{
|
||||
PlayoutTemplate nulled = Unrestricted();
|
||||
nulled.DaysOfWeek = null!;
|
||||
nulled.Index = 0;
|
||||
|
||||
PlayoutTemplate second = Unrestricted();
|
||||
second.Index = 1;
|
||||
|
||||
foreach (PlayoutTemplate selected in Select(nulled, second))
|
||||
{
|
||||
// index 0 is unrestricted once its NULL is read, so it legitimately wins on ordering
|
||||
selected.ShouldBeSameAs(nulled);
|
||||
return;
|
||||
}
|
||||
|
||||
Assert.Fail("the null-collection item was skipped instead of read as unrestricted");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The read-site guard must not be written BACK onto the item. These are single-column
|
||||
/// primitive collections, so assigning the guard would flip a tracked entity to
|
||||
/// <c>Modified</c> and the next <c>SaveChanges</c> would persist the substituted collection
|
||||
/// over the NULL — the mechanism recorded as <c>media.nullable-primitive-collection-mutation</c>.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public void Guard_Must_Not_Be_Written_Back_Onto_The_Item()
|
||||
{
|
||||
PlayoutTemplate template = Unrestricted();
|
||||
template.DaysOfWeek = null!;
|
||||
template.DaysOfMonth = null!;
|
||||
template.MonthsOfYear = null!;
|
||||
|
||||
Select(template);
|
||||
|
||||
template.DaysOfWeek.ShouldBeNull();
|
||||
template.DaysOfMonth.ShouldBeNull();
|
||||
template.MonthsOfYear.ShouldBeNull();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -85,19 +85,46 @@ public static class AlternateScheduleSelector
|
||||
}
|
||||
}
|
||||
|
||||
bool daysOfWeek = item.DaysOfWeek.Contains(date.DayOfWeek);
|
||||
// These three are NULLABLE single-column primitive collections, and a runtime null IS
|
||||
// reachable (ersatztv#823, measured against a real TvContext on SQLite and MySQL 8.4): EF does
|
||||
// NOT invoke the value converter for a NULL column, so it materializes as CLR null rather than
|
||||
// through IntCollectionValueConverter's null-to-empty branch, which never runs on this path.
|
||||
// Unguarded, each .Contains below throws NullReferenceException.
|
||||
//
|
||||
// A NULL reads as UNRESTRICTED -- the All*() sets -- NOT as empty. This is the whole semantic
|
||||
// question and it is decided by the one NULL that is reachable WITHOUT any code writing one:
|
||||
// Sqlite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth adds DaysOfMonth with
|
||||
// `nullable: true` and NO defaultValue, so a PlayoutTemplate row inserted before it holds NULL
|
||||
// and, by construction, had NO day-of-month restriction. Reading that as empty would INVERT
|
||||
// the row's meaning and silently stop the template applying at all. All*() preserves it, and
|
||||
// it is how "no restriction recorded" is already represented elsewhere in this domain
|
||||
// (GetPlayoutAlternateSchedulesHandler, PreviewBlockPlayoutHandler). Note what does NOT decide
|
||||
// it: the API request records normalize an omitted field with `?? []`, but that is a client
|
||||
// omitting a field on a WRITE and says nothing about what a legacy database NULL meant.
|
||||
//
|
||||
// Guarded at the READ SITE, into locals, and NEVER assigned back onto `item`: the property IS
|
||||
// the column value, so writing the guard back would flip a tracked entry to Modified and
|
||||
// persist the substituted collection over the NULL
|
||||
// (`media.nullable-primitive-collection-mutation`). The matching substitution happens at the
|
||||
// entity->DTO boundary in the two Mapper.ProjectToViewModel overloads, so the SPA's
|
||||
// appliesToDate -- an exact port of this method -- previews what this actually schedules.
|
||||
ICollection<DayOfWeek> itemDaysOfWeek = item.DaysOfWeek ?? AllDaysOfWeek();
|
||||
ICollection<int> itemDaysOfMonth = item.DaysOfMonth ?? AllDaysOfMonth();
|
||||
ICollection<int> itemMonthsOfYear = item.MonthsOfYear ?? AllMonthsOfYear();
|
||||
|
||||
bool daysOfWeek = itemDaysOfWeek.Contains(date.DayOfWeek);
|
||||
if (!daysOfWeek)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
bool daysOfMonth = item.DaysOfMonth.Contains(date.Day);
|
||||
bool daysOfMonth = itemDaysOfMonth.Contains(date.Day);
|
||||
if (!daysOfMonth)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
bool monthOfYear = item.MonthsOfYear.Contains(date.Month);
|
||||
bool monthOfYear = itemMonthsOfYear.Contains(date.Month);
|
||||
if (monthOfYear)
|
||||
{
|
||||
return item;
|
||||
|
||||
@@ -0,0 +1,120 @@
|
||||
using ErsatzTV.Application.Playouts;
|
||||
using ErsatzTV.Application.Scheduling;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Scheduling;
|
||||
using ErsatzTV.Core.Scheduling;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
using PlayoutsMapper = ErsatzTV.Application.Playouts.Mapper;
|
||||
using SchedulingMapper = ErsatzTV.Application.Scheduling.Mapper;
|
||||
|
||||
namespace ErsatzTV.Tests.Application.Scheduling;
|
||||
|
||||
/// <summary>
|
||||
/// ersatztv#823. Guarding <see cref="AlternateScheduleSelector" /> alone would have left the OTHER read
|
||||
/// of the same six columns unguarded — the entity→view-model mappers, which feed
|
||||
/// <c>PlayoutController</c>'s response models and therefore the SPA.
|
||||
/// <para>
|
||||
/// Two things break without the guard, and neither is a C# exception, which is why the selector
|
||||
/// tests cannot see them. <c>web/src/screens/PlayoutScheduleEditors.tsx</c> spreads the collection
|
||||
/// (<c>daysOfMonth: [...template.daysOfMonth]</c>) and throws <c>TypeError: not iterable</c> on a
|
||||
/// JSON <c>null</c>; and <c>web/src/screens/playoutTemplateCalendar.ts</c>'s <c>appliesToDate</c> —
|
||||
/// an exact TypeScript port of <see cref="AlternateScheduleSelector.GetScheduleForDate{T}" /> —
|
||||
/// calls <c>.includes</c> on it.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// So the mappers substitute the SAME unrestricted defaults the selector reads. That agreement is
|
||||
/// the point: a DTO that said "empty" while the selector scheduled "unrestricted" would make the
|
||||
/// preview calendar disagree with the playout it is previewing.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[TestFixture]
|
||||
public class RecurrenceLimitsMapperNullTests
|
||||
{
|
||||
private static Template MinimalTemplate() =>
|
||||
new()
|
||||
{
|
||||
Id = 7,
|
||||
Name = "T",
|
||||
TemplateGroupId = 1,
|
||||
TemplateGroup = new TemplateGroup { Name = "G" },
|
||||
Items = []
|
||||
};
|
||||
|
||||
[Test]
|
||||
public void ProgramScheduleAlternate_Null_Collections_Map_To_Unrestricted()
|
||||
{
|
||||
var alternate = new ProgramScheduleAlternate
|
||||
{
|
||||
Id = 1,
|
||||
Index = 0,
|
||||
ProgramScheduleId = 2,
|
||||
DaysOfWeek = null!,
|
||||
DaysOfMonth = null!,
|
||||
MonthsOfYear = null!
|
||||
};
|
||||
|
||||
PlayoutAlternateScheduleViewModel vm = PlayoutsMapper.ProjectToViewModel(alternate);
|
||||
|
||||
vm.DaysOfWeek.ShouldBe(AlternateScheduleSelector.AllDaysOfWeek());
|
||||
vm.DaysOfMonth.ShouldBe(AlternateScheduleSelector.AllDaysOfMonth());
|
||||
vm.MonthsOfYear.ShouldBe(AlternateScheduleSelector.AllMonthsOfYear());
|
||||
|
||||
// Never assigned back: these are single-column primitive collections, so writing the guard onto a
|
||||
// tracked entity would persist the substituted set over the NULL
|
||||
// (media.nullable-primitive-collection-mutation).
|
||||
alternate.DaysOfWeek.ShouldBeNull();
|
||||
alternate.DaysOfMonth.ShouldBeNull();
|
||||
alternate.MonthsOfYear.ShouldBeNull();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void PlayoutTemplate_Null_Collections_Map_To_Unrestricted()
|
||||
{
|
||||
var template = new PlayoutTemplate
|
||||
{
|
||||
Id = 1,
|
||||
Index = 0,
|
||||
Template = MinimalTemplate(),
|
||||
DecoTemplate = null,
|
||||
DaysOfWeek = null!,
|
||||
DaysOfMonth = null!,
|
||||
MonthsOfYear = null!
|
||||
};
|
||||
|
||||
PlayoutTemplateViewModel vm = SchedulingMapper.ProjectToViewModel(template);
|
||||
|
||||
vm.DaysOfWeek.ShouldBe(AlternateScheduleSelector.AllDaysOfWeek());
|
||||
vm.DaysOfMonth.ShouldBe(AlternateScheduleSelector.AllDaysOfMonth());
|
||||
vm.MonthsOfYear.ShouldBe(AlternateScheduleSelector.AllMonthsOfYear());
|
||||
|
||||
template.DaysOfWeek.ShouldBeNull();
|
||||
template.DaysOfMonth.ShouldBeNull();
|
||||
template.MonthsOfYear.ShouldBeNull();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// An explicitly EMPTY collection is a recorded restriction of no days and must survive the mapper
|
||||
/// unchanged. Without this, a guard written as "empty or null becomes All*" would pass the two tests
|
||||
/// above while silently rewriting real user data on the way out.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public void An_Explicitly_Empty_Collection_Is_Not_Rewritten()
|
||||
{
|
||||
var alternate = new ProgramScheduleAlternate
|
||||
{
|
||||
Id = 1,
|
||||
Index = 0,
|
||||
ProgramScheduleId = 2,
|
||||
DaysOfWeek = [],
|
||||
DaysOfMonth = [],
|
||||
MonthsOfYear = []
|
||||
};
|
||||
|
||||
PlayoutAlternateScheduleViewModel vm = PlayoutsMapper.ProjectToViewModel(alternate);
|
||||
|
||||
vm.DaysOfWeek.ShouldBeEmpty();
|
||||
vm.DaysOfMonth.ShouldBeEmpty();
|
||||
vm.MonthsOfYear.ShouldBeEmpty();
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,299 @@
|
||||
using System.Reflection;
|
||||
using System.Text;
|
||||
using Elastic.Clients.Elasticsearch;
|
||||
using Elastic.Transport;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Metadata;
|
||||
using ErsatzTV.Core.Interfaces.Repositories;
|
||||
using ErsatzTV.Core.Search;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Infrastructure.Extensions;
|
||||
using ErsatzTV.Infrastructure.Search;
|
||||
using ErsatzTV.Tests.Support;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using NSubstitute;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Tests.Integration;
|
||||
|
||||
/// <summary>
|
||||
/// ersatztv#824 — the gap ersatztv#701 named rather than papered over.
|
||||
/// <para>
|
||||
/// <c>ElasticSearchIndex.UpdateSong</c> holds an INDEPENDENT copy of the logic
|
||||
/// <see cref="SongIndexerMetadataMutationTests" /> pins on <c>LuceneSearchIndex</c>. #701 removed
|
||||
/// <c>metadata.AlbumArtists ??= []; metadata.Artists ??= [];</c> from both, but only Lucene gained
|
||||
/// a regression test — so reintroducing the mutation in the Elastic copy ALONE left the whole
|
||||
/// suite green. This fixture closes that: the assertions are the same three, driven through the
|
||||
/// real <c>ElasticSearchIndex</c> against a real <see cref="TvContext" />.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Why a stubbed transport rather than a live server.</b> #824 listed "inject a non-network
|
||||
/// <c>ElasticsearchClient</c> transport" as option 1 and it is what shipped:
|
||||
/// <c>Elastic.Transport.InMemoryRequestInvoker</c> is public in the pinned Elastic.Transport, and
|
||||
/// <c>ElasticsearchClientSettings(NodePool, IRequestInvoker)</c> accepts it. No server, no socket,
|
||||
/// no new package.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>The canned response body is load-bearing, not decoration.</b> A bare
|
||||
/// <c>InMemoryRequestInvoker()</c> answers with an EMPTY body, which the client cannot deserialize
|
||||
/// into an <c>IndexResponse</c>. That throw lands in <c>UpdateSong</c>'s catch, which logs a
|
||||
/// warning and assigns <c>metadata.Song = null</c> — so the fixture would measure the ERROR path
|
||||
/// while every "did not mutate" assertion below still passed, vacuously. The
|
||||
/// <see cref="ThrowOnWarningLogger{T}" /> is the belt to that brace: it fails the test if the
|
||||
/// catch ran at all.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The client is injected into the private <c>_client</c> field rather than obtained normally,
|
||||
/// because <c>CreateClient</c> reads the process-wide static <c>ElasticSearchIndex.Uri</c> and
|
||||
/// would open a real socket. <c>UpdateItems</c> — unlike <c>IndexExists</c> and
|
||||
/// <c>Initialize</c> — never runs <c>_client ??= CreateClient()</c>, so the injected instance is
|
||||
/// the one used and an uninjected one would simply be null.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[TestFixture]
|
||||
[NonParallelizable]
|
||||
public class ElasticSongIndexerMetadataMutationTests
|
||||
{
|
||||
private const string TestIndexName = "etv-824-test";
|
||||
|
||||
private string? _originalIndexName;
|
||||
|
||||
/// <summary>
|
||||
/// <c>ElasticSearchIndex.IndexName</c> is a process-wide static. Only <c>Startup</c> reads it today,
|
||||
/// so leaving it set leaks nothing that currently runs — but a static this fixture writes and never
|
||||
/// restores is a cross-test hazard waiting for the first test that does read it.
|
||||
/// </summary>
|
||||
[SetUp]
|
||||
public void SetUp() => _originalIndexName = ElasticSearchIndex.IndexName;
|
||||
|
||||
[TearDown]
|
||||
public void TearDown() => ElasticSearchIndex.IndexName = _originalIndexName;
|
||||
|
||||
/// <summary>
|
||||
/// A well-formed <c>IndexResponse</c>. See the fixture docstring: an empty body diverts the run
|
||||
/// into <c>UpdateSong</c>'s catch and makes every assertion below vacuous.
|
||||
/// </summary>
|
||||
private const string IndexResponseBody =
|
||||
"""
|
||||
{"_index":"etv-824-test","_id":"1","_version":1,"result":"created",
|
||||
"_shards":{"total":1,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1}
|
||||
""";
|
||||
|
||||
[Test]
|
||||
public async Task UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity()
|
||||
{
|
||||
await using var harness = await InMemoryTvContext.CreateAsync();
|
||||
|
||||
int metadataId;
|
||||
int songId;
|
||||
await using (TvContext context = harness.CreateContext())
|
||||
{
|
||||
var library = new LocalLibrary { Name = "Music", MediaKind = LibraryMediaKind.Songs };
|
||||
context.Add(library);
|
||||
await context.SaveChangesAsync();
|
||||
|
||||
var libraryPath = new LibraryPath { Path = "/music", LibraryId = library.Id };
|
||||
context.Add(libraryPath);
|
||||
await context.SaveChangesAsync();
|
||||
|
||||
var song = new Song
|
||||
{
|
||||
LibraryPathId = libraryPath.Id,
|
||||
MediaVersions = [],
|
||||
SongMetadata =
|
||||
[
|
||||
new SongMetadata
|
||||
{
|
||||
MetadataKind = MetadataKind.Fallback,
|
||||
Title = "Untagged Track",
|
||||
SortTitle = "untagged track",
|
||||
DateAdded = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc),
|
||||
|
||||
// The shape FallbackMetadataProvider.GetSongMetadata leaves behind: it never
|
||||
// assigns either primitive collection, so both columns persist as NULL.
|
||||
Artists = null!,
|
||||
AlbumArtists = null!,
|
||||
|
||||
Genres = [],
|
||||
Tags = [],
|
||||
Studios = [],
|
||||
Actors = [],
|
||||
Artwork = [],
|
||||
Guids = []
|
||||
}
|
||||
]
|
||||
};
|
||||
|
||||
context.Add(song);
|
||||
await context.SaveChangesAsync();
|
||||
metadataId = song.SongMetadata[0].Id;
|
||||
songId = song.Id;
|
||||
}
|
||||
|
||||
// The seed must actually have produced NULL columns, or every assertion below is vacuous.
|
||||
(await ReadRawArtists(harness, metadataId)).ShouldBeNull();
|
||||
|
||||
await using (TvContext context = harness.CreateContext())
|
||||
{
|
||||
// Deliberately TRACKED -- the indexer's own contract is what is being pinned, not the
|
||||
// AsNoTracking() habit of today's two callers. See SongIndexerMetadataMutationTests.
|
||||
Song tracked = await context.Songs
|
||||
.IncludeForSearch()
|
||||
.AsSplitQuery()
|
||||
.SingleAsync();
|
||||
|
||||
SongMetadata metadata = tracked.SongMetadata[0];
|
||||
metadata.Artists.ShouldBeNull("EF must materialize the NULL column as null, not as an empty list");
|
||||
|
||||
var logger = new ThrowOnWarningLogger<ElasticSearchIndex>();
|
||||
var index = new ElasticSearchIndex(
|
||||
new SearchQueryParser(
|
||||
Substitute.For<ISmartCollectionCache>(),
|
||||
Substitute.For<ILogger<SearchQueryParser>>()),
|
||||
logger);
|
||||
|
||||
var invoker = new CapturingRequestInvoker(
|
||||
new InMemoryRequestInvoker(
|
||||
Encoding.UTF8.GetBytes(IndexResponseBody),
|
||||
200,
|
||||
exception: null,
|
||||
contentType: "application/json",
|
||||
// The X-Elastic-Product header is REQUIRED, not cosmetic. The client runs a product
|
||||
// check on its first response and throws UnsupportedProductException ("the server is
|
||||
// not a supported distribution of Elasticsearch") without it -- which lands in
|
||||
// UpdateSong's catch and makes the fixture measure the error path. Measured: this is
|
||||
// exactly how this fixture first failed.
|
||||
headers: ProductCheckHeaders()));
|
||||
|
||||
var settings = new ElasticsearchClientSettings(
|
||||
new SingleNodePool(new Uri("http://localhost:9200")),
|
||||
invoker)
|
||||
.DefaultIndex(TestIndexName);
|
||||
|
||||
ElasticSearchIndex.IndexName = TestIndexName;
|
||||
|
||||
typeof(ElasticSearchIndex)
|
||||
.GetField("_client", BindingFlags.NonPublic | BindingFlags.Instance)!
|
||||
.SetValue(index, new ElasticsearchClient(settings));
|
||||
|
||||
// A bare substitute returns null from GetAllLanguageCodes, which NPEs inside AddLanguages and
|
||||
// would divert the run into UpdateSong's catch.
|
||||
var languageCodeService = Substitute.For<ILanguageCodeService>();
|
||||
languageCodeService.GetAllLanguageCodes(Arg.Any<List<string>>()).Returns([]);
|
||||
languageCodeService.GetAllLanguageCodes(Arg.Any<string>()).Returns([]);
|
||||
|
||||
await index.UpdateItems(
|
||||
Substitute.For<ISearchRepository>(),
|
||||
Substitute.For<IFallbackMetadataProvider>(),
|
||||
languageCodeService,
|
||||
[tracked]);
|
||||
|
||||
// Surfacing the exception rather than asserting ShouldBeNull: the catch is the fixture's
|
||||
// most likely failure mode (see the canned-response note above), and "expected null but was
|
||||
// <Exception>" without the message sends the next reader hunting for a cause the fixture
|
||||
// already had in its hand.
|
||||
if (logger.Failure is not null)
|
||||
{
|
||||
Assert.Fail("UpdateSong threw and its catch ran, so this probe measured the error path "
|
||||
+ $"rather than the indexing path: {logger.Failure}");
|
||||
}
|
||||
|
||||
// POSITIVE CONTROL, and it is not optional: every assertion below says something did NOT
|
||||
// happen, so all of them hold vacuously if UpdateSong never ran. The Lucene fixture uses
|
||||
// `writer.NumDocs == 1` for exactly this; the transport-level equivalent is that the indexer
|
||||
// actually issued the index request for THIS song. Like NumDocs, it proves the song-indexing
|
||||
// path ran -- it does NOT prove the artist reads specifically ran.
|
||||
invoker.Requests.Count.ShouldBe(
|
||||
1,
|
||||
"UpdateSong did not issue exactly one index request, so the assertions below would pass "
|
||||
+ $"without exercising the code under test. Captured: [{string.Join(", ", invoker.Requests)}]");
|
||||
|
||||
// The document id is compared as the LAST PATH SEGMENT, not with ShouldContain. A substring
|
||||
// test is a false-pass vector here: the index name itself carries digits ("etv-824-test"), so
|
||||
// ShouldContain("2") or ShouldContain("4") would be satisfied by the index name alone for a
|
||||
// song whose id happened to be 2 or 4, and the assertion would stop discriminating without
|
||||
// ever failing.
|
||||
string path = invoker.Requests[0].Split(' ')[^1].Split('?')[0];
|
||||
path.Split('/')[^1].ShouldBe(
|
||||
songId.ToString(),
|
||||
$"the index request was not for the seeded song. Captured: {invoker.Requests[0]}");
|
||||
|
||||
// 1. The indexer left the entity alone.
|
||||
metadata.Artists.ShouldBeNull();
|
||||
metadata.AlbumArtists.ShouldBeNull();
|
||||
|
||||
// 2. ...so EF has nothing to persist. This is the assertion that fails loudly the day the
|
||||
// mutation returns, even if a later refactor stopped the value from being observable above.
|
||||
context.Entry(metadata).State.ShouldBe(EntityState.Unchanged);
|
||||
|
||||
// 3. And the save that a real caller would go on to make does not rewrite the column.
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
(await ReadRawArtists(harness, metadataId)).ShouldBeNull();
|
||||
}
|
||||
|
||||
private static Dictionary<string, IEnumerable<string>> ProductCheckHeaders() =>
|
||||
new(StringComparer.OrdinalIgnoreCase) { ["x-elastic-product"] = ["Elasticsearch"] };
|
||||
|
||||
/// <summary>
|
||||
/// Records every request the client actually issues, so the fixture can prove the code under test
|
||||
/// ran. Delegates the answering to a real <see cref="InMemoryRequestInvoker" /> rather than
|
||||
/// hand-building a response.
|
||||
/// </summary>
|
||||
private sealed class CapturingRequestInvoker(InMemoryRequestInvoker inner) : IRequestInvoker
|
||||
{
|
||||
public List<string> Requests { get; } = [];
|
||||
|
||||
public ResponseFactory ResponseFactory => inner.ResponseFactory;
|
||||
|
||||
public TResponse Request<TResponse>(
|
||||
Endpoint endpoint,
|
||||
BoundConfiguration boundConfiguration,
|
||||
PostData? postData)
|
||||
where TResponse : TransportResponse, new()
|
||||
{
|
||||
Requests.Add($"{endpoint.Method} {endpoint.PathAndQuery}");
|
||||
return inner.Request<TResponse>(endpoint, boundConfiguration, postData);
|
||||
}
|
||||
|
||||
public Task<TResponse> RequestAsync<TResponse>(
|
||||
Endpoint endpoint,
|
||||
BoundConfiguration boundConfiguration,
|
||||
PostData? postData,
|
||||
CancellationToken cancellationToken)
|
||||
where TResponse : TransportResponse, new()
|
||||
{
|
||||
Requests.Add($"{endpoint.Method} {endpoint.PathAndQuery}");
|
||||
return inner.RequestAsync<TResponse>(endpoint, boundConfiguration, postData, cancellationToken);
|
||||
}
|
||||
|
||||
// IRequestInvoker extends IDisposable, but InMemoryRequestInvoker holds no disposable state and
|
||||
// exposes no Dispose of its own -- there is nothing to forward to.
|
||||
public void Dispose()
|
||||
{
|
||||
}
|
||||
}
|
||||
|
||||
private static async Task<object?> ReadRawArtists(InMemoryTvContext harness, int metadataId)
|
||||
{
|
||||
await using TvContext context = harness.CreateContext();
|
||||
await using var command = context.Database.GetDbConnection().CreateCommand();
|
||||
command.CommandText = $"SELECT Artists FROM SongMetadata WHERE Id = {metadataId}";
|
||||
object? value = await command.ExecuteScalarAsync();
|
||||
|
||||
// ExecuteScalar returns CLR null both for "the column is NULL" and for "there is no such row",
|
||||
// and the second is reachable: UpdateSong's catch assigns metadata.Song = null, which severs a
|
||||
// required relationship and cascades the row to Deleted, so a SaveChanges on the error path
|
||||
// DELETES it and a plain null check would pass for the wrong reason.
|
||||
if (value is null)
|
||||
{
|
||||
Assert.Fail($"SongMetadata row {metadataId} no longer exists, so its Artists column cannot "
|
||||
+ "be read -- the probe measured a deleted row rather than a preserved NULL.");
|
||||
}
|
||||
|
||||
return value is DBNull ? null : value;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,388 @@
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain.Scheduling;
|
||||
using ErsatzTV.Infrastructure;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Infrastructure.MySql.Data;
|
||||
using ErsatzTV.Infrastructure.Sqlite.Data;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using MySqlConnector;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Tests.Integration;
|
||||
|
||||
/// <summary>
|
||||
/// ersatztv#823 — the question ersatztv#701 split out rather than answered: can a runtime null reach
|
||||
/// one of the SIX collection-valued scalar columns (<c>DaysOfMonth</c>, <c>MonthsOfYear</c>,
|
||||
/// <c>DaysOfWeek</c> on <see cref="ProgramScheduleAlternate" /> and <see cref="PlayoutTemplate" />)?
|
||||
/// <para>
|
||||
/// <b>Measured, not reasoned about</b>, because the reasoning available beforehand pointed the
|
||||
/// wrong way. The two converters differ on their read side —
|
||||
/// <c>IntCollectionValueConverter</c> maps null-or-blank to <c>Array.Empty<int>()</c> while
|
||||
/// <c>EnumCollectionJsonValueConverter</c> would dereference the result of
|
||||
/// <c>JsonConvert.DeserializeObject</c> — so the expectation was that the two behave differently
|
||||
/// on a NULL row. They do not: <b>EF does not invoke a value converter for a NULL column at
|
||||
/// all</b>, so all six materialize as CLR <c>null</c> and the int converter's null-to-empty
|
||||
/// branch is dead on this path. That is the measurement this fixture pins.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// It also pins the WRITE half: assigning <c>null</c> to one of the six and calling
|
||||
/// <c>SaveChanges</c> SUCCEEDS and stores SQL NULL — the converter is skipped on the way out too.
|
||||
/// State that precisely, because the overclaim is tempting: this is a property of the CODE, not a
|
||||
/// live caller. NO caller supplies a null today — every production construction of
|
||||
/// <c>ReplacePlayoutAlternateSchedule</c> / <c>ReplacePlayoutTemplate</c> goes through the HTTP
|
||||
/// request records, which normalize with <c>?? []</c>. What makes it a latent gun is that
|
||||
/// <c>ReplacePlayoutAlternateScheduleItemsHandler</c> / <c>ReplacePlayoutTemplateItemsHandler</c>
|
||||
/// assign the command value straight onto the entity, so nothing in the write path itself refuses.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The LEGACY route is narrower than "the columns are nullable", and conflating the two is the easy
|
||||
/// error: all six are <c>nullable: true</c> on both providers, but five of the six were present at
|
||||
/// <c>CreateTable</c>, so a NULL there still needs code to write one. EXACTLY ONE case is
|
||||
/// code-path-free — SQLite's <c>20240113140741_Add_PlayoutTemplate_DaysOfMonth</c> is an
|
||||
/// <c>AddColumn</c> with <c>nullable: true</c> and NO <c>defaultValue</c>, so <c>PlayoutTemplate</c>
|
||||
/// rows inserted before it hold NULL, and by construction those rows had no day-of-month
|
||||
/// restriction. On MySQL <c>PlayoutTemplate</c> arrived whole in
|
||||
/// <c>20240114034944_Add_BlockScheduling</c>, so there is no code-path-free NULL for any of the six
|
||||
/// there. This fixture manufactures its NULL with a raw <c>UPDATE</c>, which is a code path — it
|
||||
/// measures MATERIALIZATION, and the legacy route above is established by reading the migrations.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// What the selector then does with it is pinned by
|
||||
/// <c>ErsatzTV.Core.Tests.Scheduling.AlternateScheduleSelectorTests.GetScheduleForDate_NullCollections</c>:
|
||||
/// unguarded, <c>.Contains</c> throws <see cref="NullReferenceException" />.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Runs against BOTH providers from ONE fixture body, because the question is about provider
|
||||
/// materialization and a SQLite-only answer would not have settled it. MySQL needs a live server,
|
||||
/// supplied via <c>ETV_TEST_MYSQL_CONNECTION</c>; without it the MySQL fixture <b>ignores</b> — a
|
||||
/// visible skip, never a silent pass. <c>ETV_REQUIRE_MYSQL_TESTS</c> turns that skip into a hard
|
||||
/// failure for a runner that is supposed to have one. This mirrors
|
||||
/// <see cref="LibraryFolderDedupeMigrationTests" />, which established the pattern.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Cost, stated.</b> Each test migrates a FRESH database (per-test isolation by construction —
|
||||
/// ersatztv#491 measured what a shared database and a wipe-that-must-succeed cost). Replaying every
|
||||
/// migration is the expensive part and it is deliberate: a reachability fixture should stand on the
|
||||
/// SHIPPED schema, not on one <c>EnsureCreated</c> builds from the current model.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[TestFixture(TestProvider.Sqlite)]
|
||||
[TestFixture(TestProvider.MySql)]
|
||||
[NonParallelizable]
|
||||
public class SchedulingCollectionColumnNullTests(TestProvider provider)
|
||||
{
|
||||
private const string MySqlConnectionVariable = "ETV_TEST_MYSQL_CONNECTION";
|
||||
private const string MySqlRequiredVariable = "ETV_REQUIRE_MYSQL_TESTS";
|
||||
|
||||
private string _databasePath = null!;
|
||||
private string? _mySqlConnectionString;
|
||||
private DbContextOptions<TvContext> _options = null!;
|
||||
|
||||
private string _originalLastInsertedRowId = null!;
|
||||
private string _originalCollation = null!;
|
||||
private bool _originalIsSqlite;
|
||||
private Func<DbUpdateException, bool> _originalUniqueViolation = null!;
|
||||
|
||||
/// <summary>
|
||||
/// <c>TvContext</c>'s provider statics are process-wide, and the MySQL arm sets them to MySQL
|
||||
/// values. Restoring them is NOT belt-and-braces: <see cref="InMemoryTvContext" /> — the harness
|
||||
/// most of this suite uses — resets only three of the five (<c>IsSqlite</c>,
|
||||
/// <c>IsUniqueConstraintViolation</c>, <c>RegisterUnicodeCaseFunctions</c>) and leaves
|
||||
/// <c>LastInsertedRowId</c> and <c>CaseInsensitiveCollation</c> alone. So without this, a MySQL arm
|
||||
/// running before a SQLite test leaves <c>last_insert_id()</c> in place for a SQLite connection,
|
||||
/// which is an order-dependent failure in a fixture that never mentions MySQL.
|
||||
/// <c>[NonParallelizable]</c> serialises execution; it does not restore state.
|
||||
/// </summary>
|
||||
[SetUp]
|
||||
public async Task SetUp()
|
||||
{
|
||||
_originalLastInsertedRowId = TvContext.LastInsertedRowId;
|
||||
_originalCollation = TvContext.CaseInsensitiveCollation;
|
||||
_originalIsSqlite = TvContext.IsSqlite;
|
||||
_originalUniqueViolation = TvContext.IsUniqueConstraintViolation;
|
||||
|
||||
if (provider is TestProvider.Sqlite)
|
||||
{
|
||||
TvContext.IsSqlite = true;
|
||||
TvContext.LastInsertedRowId = "last_insert_rowid()";
|
||||
TvContext.CaseInsensitiveCollation = "NOCASE";
|
||||
TvContext.IsUniqueConstraintViolation = SqliteErrorClassifier.IsUniqueConstraintViolation;
|
||||
|
||||
_databasePath = Path.Combine(Path.GetTempPath(), $"etv823-{Guid.NewGuid():N}.sqlite3");
|
||||
_options = new DbContextOptionsBuilder<TvContext>()
|
||||
.UseSqlite(
|
||||
$"Data Source={_databasePath};Foreign Keys=False",
|
||||
o => o.MigrationsAssembly("ErsatzTV.Infrastructure.Sqlite"))
|
||||
.Options;
|
||||
|
||||
await using TvContext sqlite = Create(_options);
|
||||
await sqlite.Database.MigrateAsync();
|
||||
return;
|
||||
}
|
||||
|
||||
string? baseConnectionString = Environment.GetEnvironmentVariable(MySqlConnectionVariable);
|
||||
if (string.IsNullOrWhiteSpace(baseConnectionString))
|
||||
{
|
||||
string message =
|
||||
$"{MySqlConnectionVariable} is not set, so the MySql half of the #823 measurement cannot "
|
||||
+ "run. Whether a NULL column materializes as CLR null is a provider question, so a "
|
||||
+ "SQLite-only answer does not settle it.";
|
||||
|
||||
if (IsTrue(Environment.GetEnvironmentVariable(MySqlRequiredVariable)))
|
||||
{
|
||||
Assert.Fail($"{message} {MySqlRequiredVariable} is set, so this is a failure, not a skip.");
|
||||
}
|
||||
|
||||
Assert.Ignore($"{message} Set it to run this locally.");
|
||||
}
|
||||
|
||||
// A database of our own, and a FRESH one per test: isolation by construction, per #491's finding
|
||||
// that a shared name trades isolation for a wipe that has to succeed.
|
||||
_mySqlConnectionString =
|
||||
new MySqlConnectionStringBuilder(baseConnectionString) { Database = $"etv823_{Guid.NewGuid():N}" }
|
||||
.ConnectionString;
|
||||
|
||||
TvContext.IsSqlite = false;
|
||||
TvContext.LastInsertedRowId = "last_insert_id()";
|
||||
TvContext.CaseInsensitiveCollation = "utf8mb4_general_ci";
|
||||
TvContext.IsUniqueConstraintViolation = MySqlErrorClassifier.IsUniqueConstraintViolation;
|
||||
|
||||
ServerVersion serverVersion = ServerVersion.AutoDetect(_mySqlConnectionString);
|
||||
_options = new DbContextOptionsBuilder<TvContext>()
|
||||
.UseMySql(
|
||||
_mySqlConnectionString,
|
||||
serverVersion,
|
||||
o => o.MigrationsAssembly("ErsatzTV.Infrastructure.MySql"))
|
||||
.Options;
|
||||
|
||||
// NUnit does not run [TearDown] when [SetUp] throws, and by this point the database exists — the
|
||||
// migration itself created it. Without this, a migration that fails part way strands a database
|
||||
// and its connection pool on a SHARED server, once per attempt.
|
||||
try
|
||||
{
|
||||
await using TvContext mysql = Create(_options);
|
||||
await mysql.Database.MigrateAsync();
|
||||
}
|
||||
catch
|
||||
{
|
||||
await DropMySqlDatabase();
|
||||
_mySqlConnectionString = null;
|
||||
throw;
|
||||
}
|
||||
}
|
||||
|
||||
private async Task DropMySqlDatabase()
|
||||
{
|
||||
try
|
||||
{
|
||||
await using (TvContext context = Create(_options))
|
||||
{
|
||||
await context.Database.EnsureDeletedAsync();
|
||||
}
|
||||
|
||||
await using var probe = new MySqlConnection(_mySqlConnectionString);
|
||||
await MySqlConnection.ClearPoolAsync(probe);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
// Never mask the original failure with a cleanup failure, and never fail a PASSING test in
|
||||
// teardown because the server went away -- but say so, because a silent cleanup failure is how
|
||||
// a leak becomes invisible.
|
||||
await TestContext.Out.WriteLineAsync(
|
||||
$"WARNING: could not drop the MySql test database {_mySqlConnectionString}: {ex.Message}");
|
||||
}
|
||||
}
|
||||
|
||||
[TearDown]
|
||||
public async Task TearDown()
|
||||
{
|
||||
TvContext.LastInsertedRowId = _originalLastInsertedRowId;
|
||||
TvContext.CaseInsensitiveCollation = _originalCollation;
|
||||
TvContext.IsSqlite = _originalIsSqlite;
|
||||
TvContext.IsUniqueConstraintViolation = _originalUniqueViolation;
|
||||
|
||||
if (provider is TestProvider.Sqlite)
|
||||
{
|
||||
Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools();
|
||||
foreach (string path in new[] { _databasePath, $"{_databasePath}-wal", $"{_databasePath}-shm" })
|
||||
{
|
||||
if (File.Exists(path))
|
||||
{
|
||||
File.Delete(path);
|
||||
}
|
||||
}
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
if (_mySqlConnectionString is not null)
|
||||
{
|
||||
await DropMySqlDatabase();
|
||||
_mySqlConnectionString = null;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A row whose three columns are NULL materializes as CLR null on every one of them — including
|
||||
/// the two <c>IntCollectionValueConverter</c> columns, whose converter would have produced
|
||||
/// <c>Array.Empty<int>()</c> had it been invoked.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public async Task A_Null_Column_Materializes_As_Clr_Null_On_All_Six()
|
||||
{
|
||||
await using (TvContext context = Create(_options))
|
||||
{
|
||||
await DisableForeignKeys(context);
|
||||
|
||||
context.ProgramScheduleAlternates.Add(NewAlternate(playoutId: 1));
|
||||
context.PlayoutTemplates.Add(NewTemplate(playoutId: 1));
|
||||
await context.SaveChangesAsync();
|
||||
|
||||
// Out of band, so the row is byte-identical to a legacy one. This UPDATE is itself a code
|
||||
// path and does NOT demonstrate the legacy route -- see the class docstring for which single
|
||||
// (column, provider) case is genuinely code-path-free. What is being measured here is what EF
|
||||
// MATERIALIZES from such a row, which is the same regardless of how the NULL got there.
|
||||
await context.Database.ExecuteSqlRawAsync(
|
||||
"UPDATE ProgramScheduleAlternate SET DaysOfWeek = NULL, DaysOfMonth = NULL, MonthsOfYear = NULL");
|
||||
await context.Database.ExecuteSqlRawAsync(
|
||||
"UPDATE PlayoutTemplate SET DaysOfWeek = NULL, DaysOfMonth = NULL, MonthsOfYear = NULL");
|
||||
}
|
||||
|
||||
// ANTI-VACUITY: a read that found no row would leave every ShouldBeNull below trivially true,
|
||||
// so both reads are Single and would throw on an empty table.
|
||||
await using (TvContext context = Create(_options))
|
||||
{
|
||||
ProgramScheduleAlternate alternate =
|
||||
await context.ProgramScheduleAlternates.AsNoTracking().SingleAsync();
|
||||
|
||||
alternate.DaysOfWeek.ShouldBeNull(
|
||||
"EnumCollectionJsonValueConverter must not be invoked for a NULL column");
|
||||
alternate.DaysOfMonth.ShouldBeNull(
|
||||
"IntCollectionValueConverter's null-to-empty branch must not run — EF skips the "
|
||||
+ "converter for a NULL column, so this is null rather than an empty array");
|
||||
alternate.MonthsOfYear.ShouldBeNull();
|
||||
|
||||
PlayoutTemplate template = await context.PlayoutTemplates.AsNoTracking().SingleAsync();
|
||||
|
||||
template.DaysOfWeek.ShouldBeNull();
|
||||
template.DaysOfMonth.ShouldBeNull();
|
||||
template.MonthsOfYear.ShouldBeNull();
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The write half. A caller that hands one of the six a null — which the two Replace* handlers do
|
||||
/// verbatim from their command, and only the HTTP request records guard against — gets a stored
|
||||
/// SQL NULL and no error at all.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public async Task Assigning_Null_Persists_Sql_Null_Rather_Than_Throwing()
|
||||
{
|
||||
await using (TvContext context = Create(_options))
|
||||
{
|
||||
await DisableForeignKeys(context);
|
||||
|
||||
ProgramScheduleAlternate alternate = NewAlternate(playoutId: 2);
|
||||
alternate.DaysOfWeek = null!;
|
||||
alternate.DaysOfMonth = null!;
|
||||
alternate.MonthsOfYear = null!;
|
||||
|
||||
context.ProgramScheduleAlternates.Add(alternate);
|
||||
|
||||
// Not Should.NotThrow: the point is that this is the SHIPPED behaviour of the write path, so
|
||||
// the assertion is that the round-trip below finds NULL, not merely that nothing blew up.
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
await using (TvContext context = Create(_options))
|
||||
{
|
||||
ProgramScheduleAlternate reloaded =
|
||||
await context.ProgramScheduleAlternates.AsNoTracking().SingleAsync();
|
||||
|
||||
reloaded.DaysOfWeek.ShouldBeNull();
|
||||
reloaded.DaysOfMonth.ShouldBeNull();
|
||||
reloaded.MonthsOfYear.ShouldBeNull();
|
||||
}
|
||||
|
||||
// ...and it really is SQL NULL in the column, not an empty string the converter round-trips.
|
||||
// ExecuteScalar hands back CLR null both for a NULL column and for NO SUCH ROW, so the row is
|
||||
// counted first — otherwise a fixture that silently deleted its row would report the same thing.
|
||||
await using (TvContext context = Create(_options))
|
||||
{
|
||||
(await ScalarAsync(context, "SELECT COUNT(*) FROM ProgramScheduleAlternate"))
|
||||
.ShouldNotBeNull();
|
||||
Convert.ToInt32(await ScalarAsync(context, "SELECT COUNT(*) FROM ProgramScheduleAlternate"))
|
||||
.ShouldBe(1, "the row is gone, so a null read below would prove nothing");
|
||||
|
||||
(await ScalarAsync(context, "SELECT DaysOfWeek FROM ProgramScheduleAlternate")).ShouldBeNull();
|
||||
(await ScalarAsync(context, "SELECT DaysOfMonth FROM ProgramScheduleAlternate")).ShouldBeNull();
|
||||
(await ScalarAsync(context, "SELECT MonthsOfYear FROM ProgramScheduleAlternate")).ShouldBeNull();
|
||||
}
|
||||
}
|
||||
|
||||
private static ProgramScheduleAlternate NewAlternate(int playoutId) =>
|
||||
new()
|
||||
{
|
||||
PlayoutId = playoutId,
|
||||
ProgramScheduleId = 1,
|
||||
Index = 0,
|
||||
DaysOfWeek = [],
|
||||
DaysOfMonth = [],
|
||||
MonthsOfYear = [],
|
||||
StartMonth = 1,
|
||||
StartDay = 1,
|
||||
EndMonth = 12,
|
||||
EndDay = 31
|
||||
};
|
||||
|
||||
private static PlayoutTemplate NewTemplate(int playoutId) =>
|
||||
new()
|
||||
{
|
||||
PlayoutId = playoutId,
|
||||
TemplateId = 1,
|
||||
Index = 0,
|
||||
DaysOfWeek = [],
|
||||
DaysOfMonth = [],
|
||||
MonthsOfYear = [],
|
||||
StartMonth = 1,
|
||||
StartDay = 1,
|
||||
EndMonth = 12,
|
||||
EndDay = 31
|
||||
};
|
||||
|
||||
/// <summary>
|
||||
/// The rows here are deliberately partial graphs (a <c>PlayoutId</c> pointing at no Playout), so
|
||||
/// foreign keys are off. SQLite takes it as a connection-string keyword; MySQL's
|
||||
/// <c>foreign_key_checks</c> is a SESSION variable, so it is set on the context's own connection
|
||||
/// and lives as long as that context does.
|
||||
/// </summary>
|
||||
private async Task DisableForeignKeys(TvContext context)
|
||||
{
|
||||
if (provider is TestProvider.MySql)
|
||||
{
|
||||
await context.Database.OpenConnectionAsync();
|
||||
await context.Database.ExecuteSqlRawAsync("SET SESSION foreign_key_checks = 0");
|
||||
}
|
||||
}
|
||||
|
||||
private static async Task<object?> ScalarAsync(TvContext context, string sql)
|
||||
{
|
||||
await context.Database.OpenConnectionAsync();
|
||||
await using System.Data.Common.DbCommand command = context.Database.GetDbConnection().CreateCommand();
|
||||
command.CommandText = sql;
|
||||
object? value = await command.ExecuteScalarAsync();
|
||||
return value is DBNull ? null : value;
|
||||
}
|
||||
|
||||
private static bool IsTrue(string? value) =>
|
||||
!string.IsNullOrWhiteSpace(value)
|
||||
&& (value == "1" || value.Equals("true", StringComparison.OrdinalIgnoreCase));
|
||||
|
||||
private static TvContext Create(DbContextOptions<TvContext> options) =>
|
||||
new(
|
||||
options,
|
||||
NullLoggerFactory.Instance,
|
||||
new SlowQueryInterceptor(NullLogger<SlowQueryInterceptor>.Instance));
|
||||
}
|
||||
@@ -0,0 +1,89 @@
|
||||
using ErsatzTV.Core.Interfaces.Search;
|
||||
using ErsatzTV.Infrastructure.Search;
|
||||
using NUnit.Framework;
|
||||
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)
|
||||
{
|
||||
fixture.GetMethods()
|
||||
.Any(m => m.GetCustomAttributes(typeof(TestAttribute), inherit: true).Length > 0)
|
||||
.ShouldBeTrue(
|
||||
$"{fixture.Name} is named as the mutation fixture for {indexer.Name} but declares no "
|
||||
+ "[Test] method, so it proves nothing");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -186,28 +186,6 @@ public class SongIndexerMetadataMutationTests
|
||||
(await ReadRawArtists(harness, metadataId)).ShouldBeNull();
|
||||
}
|
||||
|
||||
private sealed class ThrowOnWarningLogger<T> : ILogger<T>
|
||||
{
|
||||
public Exception? Failure { get; private set; }
|
||||
|
||||
public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
|
||||
|
||||
public bool IsEnabled(LogLevel logLevel) => true;
|
||||
|
||||
public void Log<TState>(
|
||||
LogLevel logLevel,
|
||||
EventId eventId,
|
||||
TState state,
|
||||
Exception? exception,
|
||||
Func<TState, Exception?, string> formatter)
|
||||
{
|
||||
if (logLevel >= LogLevel.Warning)
|
||||
{
|
||||
Failure ??= exception ?? new InvalidOperationException(formatter(state, exception));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private static async Task<object?> ReadRawArtists(InMemoryTvContext harness, int metadataId)
|
||||
{
|
||||
await using TvContext context = harness.CreateContext();
|
||||
|
||||
@@ -0,0 +1,37 @@
|
||||
using Microsoft.Extensions.Logging;
|
||||
|
||||
namespace ErsatzTV.Tests.Support;
|
||||
|
||||
/// <summary>
|
||||
/// Captures the first warning-or-worse a component logs, so a fixture can fail on it instead of
|
||||
/// silently measuring an error path.
|
||||
/// <para>
|
||||
/// Both search indexers wrap each <c>Update*</c> body in a <c>catch</c> that logs a warning and
|
||||
/// assigns <c>metadata.Song = null</c> — which severs a required relationship and cascades the
|
||||
/// metadata row to <see cref="Microsoft.EntityFrameworkCore.EntityState.Deleted" />. A fixture
|
||||
/// that let that catch run quietly would report the wrong cause for every assertion after it, and
|
||||
/// on the first run of the ersatztv#701 probe it did exactly that (a bare
|
||||
/// <c>ILanguageCodeService</c> substitute NPEs inside <c>AddLanguages</c>).
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public sealed class ThrowOnWarningLogger<T> : ILogger<T>
|
||||
{
|
||||
public Exception? Failure { get; private set; }
|
||||
|
||||
public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
|
||||
|
||||
public bool IsEnabled(LogLevel logLevel) => true;
|
||||
|
||||
public void Log<TState>(
|
||||
LogLevel logLevel,
|
||||
EventId eventId,
|
||||
TState state,
|
||||
Exception? exception,
|
||||
Func<TState, Exception?, string> formatter)
|
||||
{
|
||||
if (logLevel >= LogLevel.Warning)
|
||||
{
|
||||
Failure ??= exception ?? new InvalidOperationException(formatter(state, exception));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -39,6 +39,7 @@ doc below, or that changes which sections a task signal points to.**
|
||||
| Live local run / Playwright-MCP verification | `docs/e2e-local.md` + `scripts/e2e-local.sh` |
|
||||
| Adding/changing a UI-E2E browser flow | `docs/e2e-local.md` → "UI-E2E harness" + `scripts/e2e-ui.sh` |
|
||||
| What does a test suite cover | `docs/testing.md` |
|
||||
| Writing a test whose behaviour is PROVIDER-SPECIFIC (collation, a value converter, data-migration DML) | `docs/testing.md` → "Provider-parity fixtures (opt-in MySQL)" — run one fixture body against both providers via `ETV_TEST_MYSQL_CONNECTION`; without it the MySQL arm `Assert.Ignore`s visibly, and CI does not currently run it (ersatztv#627) |
|
||||
| Legacy Blazor route lookup | `docs/blazor-route-parity.md` (historical #91 phase (b) inventory) |
|
||||
| "Why do we do X this way" / challenging a convention | **Catalog-first**: `docs/decisions/README.md` (active rows) → follow the row's link to `docs/decisions/records/<area>/<topic>.md` for full rationale. `docs/decisions/archive/<area>/` only for "what did the rule used to be." |
|
||||
|
||||
|
||||
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
@@ -64,7 +64,10 @@ with have sat in these gaps. Treat anything not named here as unassessed, not as
|
||||
the check proves each job was classified, never that the classification is CORRECT; and an
|
||||
`inline` assertion cannot be proven to work without running the job, so those rows carry
|
||||
`Proof: NONE` honestly instead of borrowing a neighbouring file's proof.
|
||||
2. **C# and TypeScript guards** — five files, none with a row:
|
||||
2. **C# and TypeScript guards** — six files, none with a row:
|
||||
`ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs` (ersatztv#824 — derives the
|
||||
`ISearchIndex` population from the declaring assembly and asserts set equality against a
|
||||
hand-written covered set),
|
||||
`ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `web/src/api/pageSizeCallSites.guard.test.ts`,
|
||||
`web/src/api/completeRequest.guard.test.ts`, and the pair
|
||||
`web/vite-plugins/trackedSourceFiles.test.ts` + its `.realgit.test.ts` sibling, which together
|
||||
|
||||
@@ -116,6 +116,33 @@ UI-E2E (needs a built solution + built SPA; boots and tears down its own instanc
|
||||
scripts/e2e-ui.sh # from the repo root, NOT web/
|
||||
```
|
||||
|
||||
## Provider-parity fixtures (opt-in MySQL)
|
||||
|
||||
Most of `ErsatzTV.Tests` runs on the in-memory SQLite harness described above. A few fixtures in
|
||||
`ErsatzTV.Tests/Integration/` instead drive a **real, migrated** database, and run the SAME body against
|
||||
**both** providers because the behaviour they pin is provider-specific:
|
||||
|
||||
| Fixture | What is provider-specific about it |
|
||||
| --- | --- |
|
||||
| `LibraryFolderDedupeMigrationTests` | the #491 dedupe DML — two MySQL-only collation defects (case-insensitive grouping, then `PAD SPACE`) were unreachable from SQLite |
|
||||
| `SchedulingCollectionColumnNullTests` | what a NULL column materializes as through a value converter (ersatztv#823) |
|
||||
| `SearchFieldValuesProviderTests` | the search-field-values query shape, which differs per provider |
|
||||
|
||||
The MySQL half needs a live server, supplied as `ETV_TEST_MYSQL_CONNECTION`. **Without it these
|
||||
fixtures `Assert.Ignore` — a visible skip, never a silent pass**, so an ordinary local run needs no
|
||||
MySQL. Setting `ETV_REQUIRE_MYSQL_TESTS=1` turns that skip into a hard failure, for a runner that is
|
||||
supposed to have one.
|
||||
|
||||
```bash
|
||||
ETV_TEST_MYSQL_CONNECTION='Server=<host>;Port=3306;Uid=root;Pwd=<pw>;DefaultCommandTimeout=300;' \
|
||||
dotnet test ErsatzTV.Tests --filter FullyQualifiedName~SchedulingCollectionColumnNullTests
|
||||
```
|
||||
|
||||
Each test uses a database name it generates per run, so isolation does not depend on a wipe
|
||||
succeeding, and drops it in teardown. **CI does not currently run any of these MySQL halves** — the
|
||||
`migrations` job spins a `mysql:8.4` service but only applies migrations to a fresh EMPTY database,
|
||||
so it executes no data rows; re-arming these fixtures there is tracked by ersatztv#627.
|
||||
|
||||
## Per-PR verification gate
|
||||
|
||||
Before opening a PR: build the solution, run `ErsatzTV.Tests` + `ErsatzTV.Core.Tests` (plus
|
||||
|
||||
Reference in New Issue
Block a user