From 6b22aaa9e819700594f5e642f867b3d68702b4b2 Mon Sep 17 00:00:00 2001 From: Philippe Matray Date: Thu, 13 Aug 2026 14:59:29 +0200 Subject: [PATCH 1/3] chore(#303): scaffold draft PR for guard-suite testability From 5250333658d65de36f41018f186a3d5fcf4c567c Mon Sep 17 00:00:00 2001 From: Philippe Matray Date: Thu, 13 Aug 2026 15:02:26 +0200 Subject: [PATCH 2/3] test(ci): cover the missing-upload-step branch with a synthetic job set --- FormCraft.UnitTests/Ci/TestReportingTests.cs | 105 ++++++++++++++++--- 1 file changed, 90 insertions(+), 15 deletions(-) diff --git a/FormCraft.UnitTests/Ci/TestReportingTests.cs b/FormCraft.UnitTests/Ci/TestReportingTests.cs index a3ac816..848e768 100644 --- a/FormCraft.UnitTests/Ci/TestReportingTests.cs +++ b/FormCraft.UnitTests/Ci/TestReportingTests.cs @@ -221,10 +221,87 @@ private static List PromisedGlobs(string build) /// silence this file guards against everywhere else (see ). Absence /// offends every claim here — a step that is not there has no if: always(), and equally /// does not "avoid the stale glob" in any sense worth being green about. + /// + /// That && inversion is no longer a hazard a reader has to hold in mind (#303): + /// drives every claim + /// over a synthetic job that lacks the step and fails if any of them lets it pass. Before that, the + /// branch was reachable only by hand — #267 verified it by deleting the step from ci.yml and + /// reverting, which proved the behaviour once and could not prove it again. + /// /// private static bool UploadStepFails(TestRunningJob job, Func claim) => UploadStep(job) is not { } step || !claim(step); + /// + /// The jobs in whose upload step fails — + /// including by not existing. + /// + /// + /// Takes the job set rather than reading itself (#303), and that + /// parameter is the whole point: every real test-running job has the upload step, so a test that + /// cannot supply its own jobs cannot reach the "the step is missing" branch at all. The five + /// assertions below pass ; the coverage test passes a synthetic pair. + /// + /// The vacuity guard stays in rather than moving here — a hand-built + /// list cannot be accidentally empty, and asserting non-emptiness of the *caller's* argument would + /// make the synthetic case impossible to express. + /// + /// + private static List UploadOffenders( + IReadOnlyList jobs, + Func claim) => + jobs.Where(j => UploadStepFails(j, claim)).ToList(); + + /// + /// The five claims this file makes about the upload step, keyed by a short name for each — so the + /// absence coverage below cannot drift out of step with the assertions themselves. + /// + private static Dictionary> UploadClaims => new(StringComparer.Ordinal) + { + ["exists"] = _ => true, + ["if: always()"] = s => s.Contains("if: always()", StringComparison.Ordinal), + ["path: test-results"] = s => s.Split('\n').Any(line => line.Trim() == "path: test-results"), + ["avoids **/TestResults/"] = s => !s.Contains("**/TestResults/", StringComparison.Ordinal), + ["if-no-files-found: ignore"] = s => s.Contains("if-no-files-found: ignore", StringComparison.Ordinal), + }; + + [Fact] + public void Every_Upload_Claim_Should_Report_A_Job_That_Has_No_Upload_Step() + { + // The branch this pins (#267, PR #287) shipped verified only by hand — deleting the step from + // ci.yml, watching all five tests name the job, then reverting. It cannot fire in a normal run, + // because every real test-running job HAS the step: that is what this file exists to keep true. + // So writing `is { } step &&` instead of `is not { } step ||` would ship green, and a missing + // upload would read as *satisfying* all five claims — the vacuity this suite guards against + // everywhere else, sitting in its own helper. + var missing = new TestRunningJob("fixture.yml", "build", """ + steps: + - name: 'Run: Test' + run: ./build.sh Test + """); + + // The conforming job is not decoration: without it this test would pass just as happily against + // a helper that reported EVERY job as an offender. + var conforming = new TestRunningJob("fixture.yml", "good", $""" + steps: + - name: '{UploadStepName}' + if: always() + uses: actions/upload-artifact@v4 + with: + path: test-results + if-no-files-found: ignore + """); + + foreach (var claim in UploadClaims) + { + UploadOffenders([missing], claim.Value) + .ShouldBe([missing], $"the '{claim.Key}' claim does not report a job with no upload step"); + + UploadOffenders([conforming], claim.Value) + .ShouldBeEmpty($"the '{claim.Key}' claim reports a conforming upload step as an offender"); + } + } + [Fact] public void BuildScript_Should_Not_Set_The_VSTest_Properties_That_Mtp_Ignores() { @@ -360,9 +437,7 @@ public void Every_Job_That_Runs_Tests_Should_Upload_The_TestResults_Artifact() // Asked through the same primitive as the four tests below rather than by a raw substring // search, so "present" means the one thing here and there — a step the scan can actually // isolate, not merely the text of a `- name:` line appearing somewhere in the job. - var missing = JobsThatRunTests() - .Where(j => UploadStep(j) is null) - .ToList(); + var missing = UploadOffenders(JobsThatRunTests(), _ => true); missing.ShouldBeEmpty(); } @@ -374,9 +449,9 @@ public void Every_TestResults_Upload_Should_Run_On_Failure() // one worth preserving, and it is exactly the path a bare step skips. Microsoft.Testing. // Platform prints only a summary line to stdout, so without this a red CI run leaves no // record of *which* assertion failed — a cost that was paid for real during #200. - var offenders = JobsThatRunTests() - .Where(j => UploadStepFails(j, s => s.Contains("if: always()", StringComparison.Ordinal))) - .ToList(); + var offenders = UploadOffenders( + JobsThatRunTests(), + s => s.Contains("if: always()", StringComparison.Ordinal)); offenders.ShouldBeEmpty(); } @@ -389,9 +464,9 @@ public void Every_TestResults_Upload_Should_Point_At_The_Directory_The_Build_Fil // block listing test-results among other globs would satisfy a laxer line-wise check while // re-introducing the very globs the next test rejects. All three steps upload exactly one // directory, so that is what is pinned. - var offenders = JobsThatRunTests() - .Where(j => UploadStepFails(j, s => s.Split('\n').Any(line => line.Trim() == "path: test-results"))) - .ToList(); + var offenders = UploadOffenders( + JobsThatRunTests(), + s => s.Split('\n').Any(line => line.Trim() == "path: test-results")); offenders.ShouldBeEmpty(); } @@ -408,9 +483,9 @@ public void No_TestResults_Upload_Should_Point_At_A_Path_The_Build_No_Longer_Fil // offends it, even though a step that is not there points at nothing: a missing step makes // this test red today by throwing, so letting it read as vacuously satisfied would trade a // loud failure for a quieter suite — the opposite of the point. - var offenders = JobsThatRunTests() - .Where(j => UploadStepFails(j, s => !s.Contains("**/TestResults/", StringComparison.Ordinal))) - .ToList(); + var offenders = UploadOffenders( + JobsThatRunTests(), + s => !s.Contains("**/TestResults/", StringComparison.Ordinal)); offenders.ShouldBeEmpty(); } @@ -423,9 +498,9 @@ public void Every_TestResults_Upload_Should_Tolerate_A_Missing_Path() // warns on a condition that is entirely expected, and a warning nobody can act on is how a // real one gets missed. The opposite risk — an *empty* directory passing unremarked — is // covered in the build rather than here, by the Assert.NotEmpty on the trx. - var offenders = JobsThatRunTests() - .Where(j => UploadStepFails(j, s => s.Contains("if-no-files-found: ignore", StringComparison.Ordinal))) - .ToList(); + var offenders = UploadOffenders( + JobsThatRunTests(), + s => s.Contains("if-no-files-found: ignore", StringComparison.Ordinal)); offenders.ShouldBeEmpty(); } From e3418b556bd7164230e0f46908d1bda51fbf7814 Mon Sep 17 00:00:00 2001 From: Philippe Matray Date: Thu, 13 Aug 2026 15:14:00 +0200 Subject: [PATCH 3/3] fix: address code-review findings --- FormCraft.UnitTests/Ci/TestReportingTests.cs | 177 ++++++++++++++----- 1 file changed, 129 insertions(+), 48 deletions(-) diff --git a/FormCraft.UnitTests/Ci/TestReportingTests.cs b/FormCraft.UnitTests/Ci/TestReportingTests.cs index 848e768..a0c1af8 100644 --- a/FormCraft.UnitTests/Ci/TestReportingTests.cs +++ b/FormCraft.UnitTests/Ci/TestReportingTests.cs @@ -253,50 +253,139 @@ private static List UploadOffenders( jobs.Where(j => UploadStepFails(j, claim)).ToList(); /// - /// The five claims this file makes about the upload step, keyed by a short name for each — so the - /// absence coverage below cannot drift out of step with the assertions themselves. + /// The jobs with no upload step at all — under a claim every present + /// step satisfies. /// - private static Dictionary> UploadClaims => new(StringComparer.Ordinal) + /// + /// Named rather than spelled `_ => true` at the call site: "jobs that fail an always-true claim" + /// is a double negative the reader has to unfold before it reads as "jobs with no step". + /// + private static List UploadMissing(IReadOnlyList jobs) => + UploadOffenders(jobs, _ => true); + + // The four content claims, each defined ONCE. The `[Fact]` that holds a claim against the real + // workflows and the coverage `[Theory]` that proves the claim reports an offender both reference + // the same delegate — a dictionary of re-typed copies would let the two drift apart silently, which + // is the failure this file exists to prevent rather than to commit. + + /// + /// `if: always()` is the entire point of the artifact (#225): the failure path is the only one worth + /// preserving, and it is exactly the path a bare step skips. + /// + private static readonly Func RunsOnFailure = + s => s.Contains("if: always()", StringComparison.Ordinal); + + /// + /// Asserted as the whole path: value, not as a substring: name: test-results names the + /// artifact and would otherwise satisfy a claim about where it is read from. + /// + private static readonly Func PointsAtTheDirectoryTheBuildFills = + s => s.Split('\n').Any(line => line.Trim() == "path: test-results"); + + /// + /// Until #231 the per-assembly logs landed under **/TestResults/; --results-directory + /// moved them, so that glob now matches nothing on every run. + /// + private static readonly Func AvoidsThePathTheBuildNoLongerFills = + s => !s.Contains("**/TestResults/", StringComparison.Ordinal); + + /// + /// These steps run under if: always(), which includes runs that failed before any test + /// executed and therefore before test-results/ existed. + /// + private static readonly Func ToleratesAMissingPath = + s => s.Contains("if-no-files-found: ignore", StringComparison.Ordinal); + + /// The four content claims by name, for the coverage theories below. + private static readonly IReadOnlyDictionary> UploadClaims = + new Dictionary>(StringComparer.Ordinal) + { + ["if: always()"] = RunsOnFailure, + ["path: test-results"] = PointsAtTheDirectoryTheBuildFills, + ["avoids **/TestResults/"] = AvoidsThePathTheBuildNoLongerFills, + ["if-no-files-found: ignore"] = ToleratesAMissingPath, + }; + + /// + /// The upload step as all three workflows really write it — copied from ci.yml rather than + /// simplified. + /// + /// + /// The name: test-results line matters: it is the exact key + /// exists to tell apart from path:, so a + /// fixture that dropped it could not exercise the one discrimination that claim makes. + /// + private const string ConformingUploadStep = """ + - name: 'Publish: test-results' + if: always() + uses: actions/upload-artifact@v7 + with: + name: test-results + path: test-results + if-no-files-found: ignore + """; + + private static TestRunningJob Job(string name, string steps) => + new("fixture.yml", name, $"steps:\n{steps}"); + + /// A step satisfying every content claim except the named one. + public static TheoryData StepsViolatingOneClaim() => new() { - ["exists"] = _ => true, - ["if: always()"] = s => s.Contains("if: always()", StringComparison.Ordinal), - ["path: test-results"] = s => s.Split('\n').Any(line => line.Trim() == "path: test-results"), - ["avoids **/TestResults/"] = s => !s.Contains("**/TestResults/", StringComparison.Ordinal), - ["if-no-files-found: ignore"] = s => s.Contains("if-no-files-found: ignore", StringComparison.Ordinal), + { "if: always()", WithoutLine(ConformingUploadStep, "if: always()") }, + { "path: test-results", ConformingUploadStep.Replace("path: test-results", "path: test-results/**", StringComparison.Ordinal) }, + // Adds the stale glob rather than replacing the good path, so this violates that claim ALONE. + { "avoids **/TestResults/", ConformingUploadStep.Replace(" path: test-results\n", " path: test-results\n extra: '**/TestResults/'\n", StringComparison.Ordinal) }, + { "if-no-files-found: ignore", WithoutLine(ConformingUploadStep, "if-no-files-found: ignore") }, }; - [Fact] - public void Every_Upload_Claim_Should_Report_A_Job_That_Has_No_Upload_Step() + private static string WithoutLine(string step, string marker) => + string.Join('\n', step.Split('\n').Where(l => !l.Contains(marker, StringComparison.Ordinal))); + + public static TheoryData UploadClaimNames() => [.. UploadClaims.Keys]; + + [Theory] + [MemberData(nameof(UploadClaimNames))] + public void Every_Upload_Claim_Should_Report_A_Job_With_No_Upload_Step(string claimName) { // The branch this pins (#267, PR #287) shipped verified only by hand — deleting the step from - // ci.yml, watching all five tests name the job, then reverting. It cannot fire in a normal run, - // because every real test-running job HAS the step: that is what this file exists to keep true. - // So writing `is { } step &&` instead of `is not { } step ||` would ship green, and a missing - // upload would read as *satisfying* all five claims — the vacuity this suite guards against - // everywhere else, sitting in its own helper. - var missing = new TestRunningJob("fixture.yml", "build", """ - steps: - - name: 'Run: Test' - run: ./build.sh Test - """); - - // The conforming job is not decoration: without it this test would pass just as happily against - // a helper that reported EVERY job as an offender. - var conforming = new TestRunningJob("fixture.yml", "good", $""" - steps: - - name: '{UploadStepName}' - if: always() - uses: actions/upload-artifact@v4 - with: - path: test-results - if-no-files-found: ignore - """); + // ci.yml, watching the tests name the job, then reverting. It fires exactly when this file does + // its job, but it cannot fire in a *green* run, because every real test-running job has the + // step. So writing `is { } step &&` instead of `is not { } step ||` would ship green, and a + // missing upload would read as *satisfying* every claim. + // + // The conforming job is in the same call on purpose: passed one job at a time, this would pass + // just as happily against a helper that returned the whole list whenever any member offended — + // which in production (three jobs) would destroy the "names the offending job" property. + var missing = Job("build", " - name: 'Run: Test'\n run: ./build.sh Test"); + + UploadOffenders([Job("good", ConformingUploadStep), missing], UploadClaims[claimName]) + .ShouldBe([missing]); + } + + [Theory] + [MemberData(nameof(StepsViolatingOneClaim))] + public void Every_Upload_Claim_Should_Report_A_Step_That_Violates_It(string claimName, string violatingStep) + { + // The other half, and the one the absence coverage cannot reach: a step that is PRESENT and + // wrong. Without it, a `UploadStepFails` that ignored `claim` entirely — reducing all four + // content assertions to "the step exists" — would pass every test in this file. + var violating = Job("violates", violatingStep); + + UploadOffenders([Job("good", ConformingUploadStep), violating], UploadClaims[claimName]) + .ShouldBe([violating]); + } + + [Fact] + public void A_Conforming_Upload_Step_Should_Offend_No_Claim() + { + // The fixture the two theories above lean on has to be conforming, or both of them prove + // nothing: "reports the offender" is trivially satisfied by a helper that reports everyone. + var conforming = Job("good", ConformingUploadStep); + + UploadMissing([conforming]).ShouldBeEmpty(); foreach (var claim in UploadClaims) { - UploadOffenders([missing], claim.Value) - .ShouldBe([missing], $"the '{claim.Key}' claim does not report a job with no upload step"); - UploadOffenders([conforming], claim.Value) .ShouldBeEmpty($"the '{claim.Key}' claim reports a conforming upload step as an offender"); } @@ -437,7 +526,7 @@ public void Every_Job_That_Runs_Tests_Should_Upload_The_TestResults_Artifact() // Asked through the same primitive as the four tests below rather than by a raw substring // search, so "present" means the one thing here and there — a step the scan can actually // isolate, not merely the text of a `- name:` line appearing somewhere in the job. - var missing = UploadOffenders(JobsThatRunTests(), _ => true); + var missing = UploadMissing(JobsThatRunTests()); missing.ShouldBeEmpty(); } @@ -449,9 +538,7 @@ public void Every_TestResults_Upload_Should_Run_On_Failure() // one worth preserving, and it is exactly the path a bare step skips. Microsoft.Testing. // Platform prints only a summary line to stdout, so without this a red CI run leaves no // record of *which* assertion failed — a cost that was paid for real during #200. - var offenders = UploadOffenders( - JobsThatRunTests(), - s => s.Contains("if: always()", StringComparison.Ordinal)); + var offenders = UploadOffenders(JobsThatRunTests(), RunsOnFailure); offenders.ShouldBeEmpty(); } @@ -464,9 +551,7 @@ public void Every_TestResults_Upload_Should_Point_At_The_Directory_The_Build_Fil // block listing test-results among other globs would satisfy a laxer line-wise check while // re-introducing the very globs the next test rejects. All three steps upload exactly one // directory, so that is what is pinned. - var offenders = UploadOffenders( - JobsThatRunTests(), - s => s.Split('\n').Any(line => line.Trim() == "path: test-results")); + var offenders = UploadOffenders(JobsThatRunTests(), PointsAtTheDirectoryTheBuildFills); offenders.ShouldBeEmpty(); } @@ -483,9 +568,7 @@ public void No_TestResults_Upload_Should_Point_At_A_Path_The_Build_No_Longer_Fil // offends it, even though a step that is not there points at nothing: a missing step makes // this test red today by throwing, so letting it read as vacuously satisfied would trade a // loud failure for a quieter suite — the opposite of the point. - var offenders = UploadOffenders( - JobsThatRunTests(), - s => !s.Contains("**/TestResults/", StringComparison.Ordinal)); + var offenders = UploadOffenders(JobsThatRunTests(), AvoidsThePathTheBuildNoLongerFills); offenders.ShouldBeEmpty(); } @@ -498,9 +581,7 @@ public void Every_TestResults_Upload_Should_Tolerate_A_Missing_Path() // warns on a condition that is entirely expected, and a warning nobody can act on is how a // real one gets missed. The opposite risk — an *empty* directory passing unremarked — is // covered in the build rather than here, by the Assert.NotEmpty on the trx. - var offenders = UploadOffenders( - JobsThatRunTests(), - s => s.Contains("if-no-files-found: ignore", StringComparison.Ordinal)); + var offenders = UploadOffenders(JobsThatRunTests(), ToleratesAMissingPath); offenders.ShouldBeEmpty(); }