diff --git a/FormCraft.UnitTests/Ci/TestReportingTests.cs b/FormCraft.UnitTests/Ci/TestReportingTests.cs index 6df452a..4f3372e 100644 --- a/FormCraft.UnitTests/Ci/TestReportingTests.cs +++ b/FormCraft.UnitTests/Ci/TestReportingTests.cs @@ -295,10 +295,176 @@ 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 jobs with no upload step at all — under a claim every present + /// step satisfies. + /// + /// + /// 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() + { + { "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") }, + }; + + 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 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([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() { @@ -522,9 +688,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 = UploadMissing(JobsThatRunTests()); missing.ShouldBeEmpty(); } @@ -536,9 +700,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 = JobsThatRunTests() - .Where(j => UploadStepFails(j, s => s.Contains("if: always()", StringComparison.Ordinal))) - .ToList(); + var offenders = UploadOffenders(JobsThatRunTests(), RunsOnFailure); offenders.ShouldBeEmpty(); } @@ -551,9 +713,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 = JobsThatRunTests() - .Where(j => UploadStepFails(j, s => s.Split('\n').Any(line => line.Trim() == "path: test-results"))) - .ToList(); + var offenders = UploadOffenders(JobsThatRunTests(), PointsAtTheDirectoryTheBuildFills); offenders.ShouldBeEmpty(); } @@ -570,9 +730,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 = JobsThatRunTests() - .Where(j => UploadStepFails(j, s => !s.Contains("**/TestResults/", StringComparison.Ordinal))) - .ToList(); + var offenders = UploadOffenders(JobsThatRunTests(), AvoidsThePathTheBuildNoLongerFills); offenders.ShouldBeEmpty(); } @@ -585,9 +743,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 = JobsThatRunTests() - .Where(j => UploadStepFails(j, s => s.Contains("if-no-files-found: ignore", StringComparison.Ordinal))) - .ToList(); + var offenders = UploadOffenders(JobsThatRunTests(), ToleratesAMissingPath); offenders.ShouldBeEmpty(); }