test(ci): cover the missing-upload-step branch with a synthetic job set (#303)

FormCraft.UnitTests/Ci/TestReportingTests.cs

@@ -295,10 +295,176 @@ private static List<string> PromisedGlobs(string build)
     /// silence this file guards against everywhere else (see <see cref="JobsThatRunTests" />). Absence
     /// offends every claim here — a step that is not there has no <c>if: always()</c>, and equally
     /// does not "avoid the stale glob" in any sense worth being green about.
+    /// <para>
+    /// That <c>&amp;&amp;</c> inversion is no longer a hazard a reader has to hold in mind (#303):
+    /// <see cref="Every_Upload_Claim_Should_Report_A_Job_That_Has_No_Upload_Step" /> 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 <c>ci.yml</c> and
+    /// reverting, which proved the behaviour once and could not prove it again.
+    /// </para>
     /// </remarks>
     private static bool UploadStepFails(TestRunningJob job, Func<string, bool> claim) =>
         UploadStep(job) is not { } step || !claim(step);
 
+    /// <summary>
+    /// The jobs in <paramref name="jobs" /> whose upload step fails <paramref name="claim" /> —
+    /// including by not existing.
+    /// </summary>
+    /// <remarks>
+    /// Takes the job set rather than reading <see cref="JobsThatRunTests" /> 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 <see cref="JobsThatRunTests" />; the coverage test passes a synthetic pair.
+    /// <para>
+    /// The vacuity guard stays in <see cref="JobsThatRunTests" /> 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.
+    /// </para>
+    /// </remarks>
+    private static List<TestRunningJob> UploadOffenders(
+        IReadOnlyList<TestRunningJob> jobs,
+        Func<string, bool> claim) =>
+        jobs.Where(j => UploadStepFails(j, claim)).ToList();
+
+    /// <summary>
+    /// The jobs with no upload step at all — <see cref="UploadOffenders" /> under a claim every present
+    /// step satisfies.
+    /// </summary>
+    /// <remarks>
+    /// Named rather than spelled `_ =&gt; 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".
+    /// </remarks>
+    private static List<TestRunningJob> UploadMissing(IReadOnlyList<TestRunningJob> 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.
+
+    /// <summary>
+    /// `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.
+    /// </summary>
+    private static readonly Func<string, bool> RunsOnFailure =
+        s => s.Contains("if: always()", StringComparison.Ordinal);
+
+    /// <summary>
+    /// Asserted as the whole <c>path:</c> value, not as a substring: <c>name: test-results</c> names the
+    /// artifact and would otherwise satisfy a claim about where it is read from.
+    /// </summary>
+    private static readonly Func<string, bool> PointsAtTheDirectoryTheBuildFills =
+        s => s.Split('\n').Any(line => line.Trim() == "path: test-results");
+
+    /// <summary>
+    /// Until #231 the per-assembly logs landed under <c>**/TestResults/</c>; <c>--results-directory</c>
+    /// moved them, so that glob now matches nothing on every run.
+    /// </summary>
+    private static readonly Func<string, bool> AvoidsThePathTheBuildNoLongerFills =
+        s => !s.Contains("**/TestResults/", StringComparison.Ordinal);
+
+    /// <summary>
+    /// These steps run under <c>if: always()</c>, which includes runs that failed before any test
+    /// executed and therefore before <c>test-results/</c> existed.
+    /// </summary>
+    private static readonly Func<string, bool> ToleratesAMissingPath =
+        s => s.Contains("if-no-files-found: ignore", StringComparison.Ordinal);
+
+    /// <summary>The four content claims by name, for the coverage theories below.</summary>
+    private static readonly IReadOnlyDictionary<string, Func<string, bool>> UploadClaims =
+        new Dictionary<string, Func<string, bool>>(StringComparer.Ordinal)
+        {
+            ["if: always()"] = RunsOnFailure,
+            ["path: test-results"] = PointsAtTheDirectoryTheBuildFills,
+            ["avoids **/TestResults/"] = AvoidsThePathTheBuildNoLongerFills,
+            ["if-no-files-found: ignore"] = ToleratesAMissingPath,
+        };
+
+    /// <summary>
+    /// The upload step as all three workflows really write it — copied from <c>ci.yml</c> rather than
+    /// simplified.
+    /// </summary>
+    /// <remarks>
+    /// The <c>name: test-results</c> line matters: it is the exact key
+    /// <see cref="PointsAtTheDirectoryTheBuildFills" /> exists to tell apart from <c>path:</c>, so a
+    /// fixture that dropped it could not exercise the one discrimination that claim makes.
+    /// </remarks>
+    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}");
+
+    /// <summary>A step satisfying every content claim except the named one.</summary>
+    public static TheoryData<string, string> 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<string> 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();
     }