test(ci): shard every test class, and guard against silent omission

[renemadsen]

The problem

dotnet test here is fanned out across a shard matrix, each shard a --filter listing the classes it owns, duplicated across dotnet-core-pr.yml and dotnet-core-master.yml. A class named in no filter is never executed — vstest reports nothing, the shard is green, the gate job is green, the PR merges. There is no error, no warning, no skipped count. The failure mode is total silence.

14 test classes were in that state. Not hypothetical harm: CorruptedPauseIdRepairTests sat unsharded for months and, when finally added, turned out to contain a test that could never have passed (it seeded three active rows for one site on one day, which a UNIQUE index forbids). Invisible tests rot.

Root-cause evidence: 5 of the 14 arrived in a single commit — 861ea89e "test(timeplanning): comprehensive coverage for non-5-min stamp calculations" — a PR that merged green having executed none of its own tests.

WHAT: the 14 classes

Derived twice independently — once by parsing sources, once by reflecting over the compiled assembly. 76 test classes exist; 62 were sharded. Zero stale entries (no filter names a class that no longer exists).

class	tests	cost
PauseIdSelfHealGuardTests	6	DB (TestBaseSetup)
PlanningServiceAdminEditNonRoundMinutesTests	2	DB (TestBaseSetup)
WorkingHoursGrpcKioskNonRoundMinutesTests	2	DB (TestBaseSetup)
PlanTextHelperTests	85	pure
GrpcServices.TimePlanningPlanningsGrpcServiceMapTests	81	pure
ComputeShiftPauseSecondsTests	16	pure
Helpers.PauseMinutesCalculatorTests	11	pure
PauseOverrideInferenceTests	9	pure
CalculatePayLinesForDayTests	8	pure
ResolveShiftSecondsTests	8	pure
EnumerateShiftSegmentsTests	6	pure
FirstUnlockedDateTests	5	pure
PauseIdCorrectionTests	4	pure
TimePlanningWorkingHoursServiceNullUserTests	1	[Ignore], zero

PauseOverrideInferenceTests deserves a callout: it is a second [TestFixture] living inside ComputeShiftPauseSecondsTests.cs (line 278). It was invisible to the shard filters and to a filename-based scan, which is presumably how it escaped the manual audit that found the other 13.

TimePlanningWorkingHoursServiceNullUserTests is [Ignore]d — the file explains that instantiating the real TimePlanningWorkingHoursService needs a constructor graph the harness cannot seed. It is sharded anyway: an ignored test that is also invisible is doubly lost, and once sharded it at least reports as skipped. (Worth knowing: even un-ignored its body is Assert.Pass("Shape-asserting placeholder") with the real assertions in a comment.)

After this change both workflows name exactly 77 classes — the 76 discovered plus the guard — with no duplicates, and the set is equal to what reflection discovers.

To be precise about the win: 13 of the 14 will now execute; the 14th reports as skipped. TimePlanningWorkingHoursServiceNullUserTests carries a class-level [Ignore] and stays ignored. That is still worth sharding — a skipped test is visible in the run summary, an unsharded one does not exist — but this PR does not claim 14 new executions.

THE GUARD

ShardCoverageTests reflects over the test assembly for every class owning or inheriting a method with [Test], [TestCase] or [TestCaseSource], reads both workflow files, parses the FullyQualifiedName= entries, and fails unless every discovered class is named in BOTH files — a class in one and not the other runs on master but not on PRs, which is the same bug half-fixed. It needs no database and does not derive from TestBaseSetup.

Four things it deliberately does not do the easy way:

• It matches NUnit's builder interfaces, not a list of attribute types. TestAttribute is an ISimpleTestBuilder; TestCase, TestCaseSource, Theory, Combinatorial, Pairwise and Sequential are all ITestBuilders, as is any custom builder attribute. Naming three concrete attributes would have silently missed a [Theory]-only fixture — a class NUnit runs happily and the guard would have declared absent. None of those shapes exists in the assembly today, but the guard's entire purpose is the class that arrives tomorrow, and that was the shape it would have let through. Generic fixtures, whose closed instantiations it cannot name, are asserted absent rather than silently skipped, so that exclusion is enforced rather than remembered.
• It identifies our workflows by content, not filename. eform-angular-frontend — the host app this plugin is copied into in Base/Full dev mode — ships workflows with these exact same two filenames and zero shard filters. A filename test therefore stops at the host and misdiagnoses. The guard requires the file to contain FullyQualifiedName=TimePlanning.Pn.Test. and keeps climbing otherwise.
• It strips YAML comment lines before parsing, so a commented-out shard entry cannot count as coverage. That was the only way found to fool it without editing the C#.
• It fails, loudly, when it cannot find or parse its input. A guard that passes when it cannot find what it is guarding is worse than no guard. The not-found message lists every .github/workflows it rejected on the way up.

The failure message names the missing classes, says which file each is absent from, gives the job anchor (test-dotnet, strategy.matrix.shard[].filter), both absolute paths, a copy-pasteable |FullyQualifiedName=… fragment, and the cost rule.

Verified by reflection in seven states (no dotnet test run locally — CI-only in this repo):

1. Real repo, workflows as updated → passes.
2. Dev-mode: base directory pointed at the real eform-angular-frontend/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/bin/... — the guard walks past the host's identically-named workflows and fails with the accurate not-found message, listing both rejected directories (the host's, plus a third .github/workflows at the workspace root that this turned up). Under a filename-only check it would have stopped at the first and reported a bogus "parser is stale". This is the case a reviewer is most likely to doubt, so it was tested against the real host tree rather than a mock.
3. Two classes deleted from dotnet-core-pr.yml only → fails, names both, reports absent from: dotnet-core-pr.yml — the both-files half is live, not just the union.
4. Shard e's filter: line commented out → fails, reporting all 8 of e's classes. This was a silent pass before the comment-stripping.
5. Orphan directory with no repo above → not-found failure, does not pass.
6. A temporary [Theory]-only fixture with zero [Test] attributes → discovered and reported missing. Invisible under the previous predicate.
7. A temporary [TestFixture(typeof(int))] class …<T> → the generic assertion fires, naming TempGenericProbeTests`1.

(Both probes were deleted and the tree rebuilt; state 1 re-verified green afterwards.)

The guard is in shard f. A guard that is itself unsharded would be invisible too.

SHARD ASSIGNMENT

shard	added
a	PlanTextHelperTests, PauseIdCorrectionTests
b	ComputeShiftPauseSecondsTests, PauseOverrideInferenceTests
c	nothing
d	PlanningServiceAdminEditNonRoundMinutesTests (DB), CalculatePayLinesForDayTests
e	PauseIdSelfHealGuardTests (DB), FirstUnlockedDateTests
f	WorkingHoursGrpcKioskNonRoundMinutesTests (DB), ResolveShiftSecondsTests, ShardCoverageTests
g	TimePlanningPlanningsGrpcServiceMapTests, PauseMinutesCalculatorTests
h	EnumerateShiftSegmentsTests, TimePlanningWorkingHoursServiceNullUserTests

The three DB fixtures went one each into light shards, so no shard takes more than one new container. Nothing was added to c.

On cost — deliberately no minutes anywhere in this PR. Two independent samples of recent master runs disagreed about which shards are slow: in one, shard e ranged 3–68 min and c ranged 6–18; in the other, e sat at ~4 and c at ~16. Run-to-run variance (runner contention, Testcontainers image pulls) dominates, so any figure written into a comment is misleading by the time someone reads it. The durable, checkable rule is used instead: every fixture deriving from TestBaseSetup starts its own MariaDB Testcontainer and dominates shard wall-clock; shard c is already the DB-dense one (11 of its 12 classes derive TestBaseSetup), so keep new DB-backed fixtures out of it; check the last green master run's per-shard durations before choosing.

That rule is now a comment above shard: in both workflows — where the person making the choice is actually looking — and it names ShardCoverageTests so that someone who hits the guard knows to add the class it names rather than delete a "weird failing test".

EXPECT RED — that is the point

These 14 classes have not run in months, or ever. A failure here is information, not an obstacle. No assertion was weakened, no [Ignore] added, no production code touched in this PR. If something goes red, the question to answer is which is wrong, the test or the production code — and that decision is deliberately left to review, not made here.

Rather than a vague "expect red", here is a falsifiable prediction, so that review can score it against what CI actually does.

The three DB fixtures are predicted GREEN, not red — including on the day-lock question, which is the obvious worry given they were written 2026-05-15 / 2026-06-16, before ReconciledDayLockInterceptor.Instance was wired into TestBaseSetup's DbContext (2026-09-12…15). The mechanism: the lock boundary is derived, not stored. Reconciled appears zero times in those three fixtures, and TimePlanningPluginSeed creates no PlanRegistration rows, so no boundary row can exist; LockedThroughForSitesAsync returns null; the interceptor costs one extra SELECT per save and changes nothing else. If a DayLockedException does appear, it is the interceptor talking, not a broken assertion — and it would falsify this prediction, which is the point of stating it.

Residual risks, in order:

1. TimePlanningPlanningsGrpcServiceMapTests — 81 tests, last touched 2026-04-28, the widest drift window in the set, against a mapping surface that changes often. It still compiles, which bounds the damage to behavioural drift.
2. ComputeShiftPauseSecondsTests / PauseOverrideInferenceTests — last touched 2026-09-02 by the flex-chain refactor, so someone kept them compiling. Compiling is not passing.
3. Shard d and f wall-clock, from container concurrency on a 4-vCPU runner now hosting one more Testcontainer each.

On shard d specifically, a timeout is not the expected failure. PlanningServiceAdminEditNonRoundMinutesTests does call GetCore() in SetUp, but the expensive ~45 s SDK Migrate is memoised once per fixture behind a if (MicrotingDbContext == null) null check in EnsureSdkDbProvisionedAsync, not paid per test. What the [SetUp] doubling actually doubles is the plugin DB EnsureDeleted + Migrate + seed, and the ~7 s SDK data replay — across a fixture with two tests. There is also no timeout-minutes in either workflow, so a job would have to run past GitHub's 6-hour default to time out.

So: if something goes red here, expect an assertion failure or an exception — not a timeout.

Checked statically and found clean: none of the three DB fixtures repeats the CorruptedPauseIdRepairTests arrange-impossible pattern — each seeds exactly one PlanRegistration per (site, date). PlanTextHelper.cs has not changed since the commit that introduced PlanTextHelperTests, so that one has no drift window; and FirstUnlockedDateTests / PauseIdCorrectionTests were hand-checked against the current helpers.

FOLLOW-UPS (not in this PR)

• workflow_call reusable workflow for the shared test-dotnet block. It would make drift between the two files structurally impossible rather than guarded, which is genuinely more attractive than this guard — but it trades away the "both files" property the guard is built around, and it is a CI restructure that does not belong inside this change.
• [SetUp] doubling. 38 fixtures re-declare [SetUp] and call base.Setup(), on top of TestBaseSetup's own [SetUp]. NUnit runs the base setup and then the derived one calls it again, so container bootstrap + EnsureDeleted / Migrate / seed happens twice per test across most of the DB suite. Pre-existing and repo-wide; almost certainly a large share of why the DB-dense shards run long.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The guard can be silently omitted itself and does not detect duplicate shard assignments.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds comprehensive test sharding and a reflection-based guard to prevent unsharded test classes.

Changes:

• Adds ShardCoverageTests to validate both workflow shard filters.
• Assigns 14 previously unsharded fixtures across shards.
• Documents shard cost considerations in both workflows.

File summaries

File	Description
ShardCoverageTests.cs	Adds shard coverage validation.
.github/workflows/dotnet-core-pr.yml	Updates PR test shard assignments.
.github/workflows/dotnet-core-master.yml	Updates master test shard assignments.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.