diff --git a/.github/actions/ci-compliance/action.yml b/.github/actions/ci-compliance/action.yml index ecf711b..c4e15d6 100644 --- a/.github/actions/ci-compliance/action.yml +++ b/.github/actions/ci-compliance/action.yml @@ -130,19 +130,39 @@ runs: # DatasetComplianceRunner has a different CLI shape: no check-type # argument, no --org-url, and all positional args are treated as file # paths. Branch the invocation accordingly. + # Tee'd rather than piped straight through: the coverage line has to reach the job + # summary, and the runner's own stdout is inside the group above, which the UI + # collapses by default. A count nobody expands is not a count anybody reads. if ($check -eq "dataset") { - & $runnerExe --output github @fileList + $runnerOutput = & $runnerExe --output github @fileList 2>&1 } else { - & $runnerExe $check --output github --org-url "$orgUrl" @fileList + $runnerOutput = & $runnerExe $check --output github --org-url "$orgUrl" @fileList 2>&1 } $exitCode = $LASTEXITCODE + + # Re-emitted verbatim. Capturing is what lets the coverage line reach the summary below; + # the annotations and workflow commands in this output still have to reach the log, and + # Write-Host is what puts them there. + $runnerOutput | ForEach-Object { Write-Host $_ } Write-Host "::endgroup::" if ($env:GITHUB_STEP_SUMMARY) { $status = if ($exitCode -eq 0) { "passed" } else { "**failed**" } - @( - "### $check compliance $status" - ) | Out-File -FilePath $env:GITHUB_STEP_SUMMARY -Append -Encoding utf8 + + # One well-defined line the runner controls. Absent rather than fatal if the runner + # is older than this action, since the two version independently. + $coverage = $runnerOutput | + Where-Object { $_ -match '^Coverage: ' } | + Select-Object -First 1 + + $summary = @("### $check compliance $status") + if ($coverage) { + # Verbatim. Rewording it here would put the same sentence in two places and let + # them drift; the runner owns the wording and this owns where it appears. + $summary += "" + $summary += $coverage + } + $summary | Out-File -FilePath $env:GITHUB_STEP_SUMMARY -Append -Encoding utf8 } exit $exitCode diff --git a/.github/scripts/tests/ci-compliance-action.Tests.ps1 b/.github/scripts/tests/ci-compliance-action.Tests.ps1 new file mode 100644 index 0000000..760efb9 --- /dev/null +++ b/.github/scripts/tests/ci-compliance-action.Tests.ps1 @@ -0,0 +1,43 @@ +# ci-compliance-action.Tests.ps1 — structural assertions over ci-compliance's action.yml. +# +# The same shape as the sibling file for ci-serialisation: properties of the action as a whole +# that a reader cannot check from any one place in it. +# +# The property here is where the runner's output ends up. The invocation is wrapped in a +# ::group::, which the GitHub UI collapses by default, so anything the runner prints is hidden +# until someone expands it. The job summary is the surface a reader actually sees, and today it +# carries a single line saying only whether the check passed. A run that examined every file and +# a run that examined none produce the same summary. +# +# Run locally: pwsh -Command "Invoke-Pester .github/scripts/tests -Output Detailed" +# Run in CI: lint-workflows.yml, the powershell-tests job. + +BeforeAll { + $repoRoot = (Resolve-Path (Join-Path $PSScriptRoot '../../..')).Path + $script:actionPath = Join-Path $repoRoot '.github/actions/ci-compliance/action.yml' + $script:lines = Get-Content $actionPath + $script:text = $lines -join "`n" +} + +Describe 'ci-compliance action.yml' { + + Context 'the job summary says how much was examined' { + + # Stdout is not the surface. The runner's output is inside a collapsed group, so a + # count printed there is invisible to a reader who does not already suspect something. + It 'wraps the runner invocation in a collapsed group' { + $text | Should -Match '::group::' -Because 'if this stops being true the reasoning below needs revisiting' + } + + It 'carries the examined count into the summary, not only pass or fail' { + $text | Should -Match '\$summary \+= \$coverage' -Because 'a run that examined every file and one that examined none must not produce the same summary' + } + + # The count is the runner's, read back off one line it controls. If that line is ever + # renamed on one side only, the summary silently loses the count rather than breaking, + # so both ends of the contract are asserted here. + It 'reads the count off the line the runner emits' { + $text | Should -Match "match '\^Coverage: '" + } + } +} diff --git a/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs b/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs index 42e7781..9c3f75e 100644 --- a/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs +++ b/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs @@ -38,16 +38,18 @@ static int Main(string[] args) var mergedResult = new TestResult() { Status = TestStatus.Pass, Information = new List() }; var allAnnotations = new List(); + var accounting = new FileAccounting(files.Count); foreach (var file in files) { // Each check type is only relevant to certain file extensions. - if (!FileFilter.IsRelevantFile(file, checkType)) continue; + if (!FileFilter.IsRelevantFile(file, checkType)) { accounting.CountNotRelevant(); continue; } if (verbose) Console.WriteLine($"\n=== Checking: {file} ==="); if (!File.Exists(file)) { + accounting.CountNotOnDisk(); Console.WriteLine($" [SKIP] File not found: {file}"); continue; } @@ -114,12 +116,14 @@ static int Main(string[] args) if (resultForThisFile == null) { + accounting.CountNoResult(); Console.WriteLine($" [SKIP] No result returned for: {file}"); continue; } if (verbose) Console.WriteLine($" Result Status: {resultForThisFile.Status}"); + accounting.CountExamined(); mergedResult = mergedResult.Merge(resultForThisFile); // Code/copyright/documentation findings have been through GroupErrors, whose @@ -156,7 +160,8 @@ static int Main(string[] args) } OutputEmitter.Write(outputFormat, checkType, mergedResult.Status, allAnnotations, sarifFilePath, verbose, - BH.Engine.Base.Query.DocumentationURL("DevOps/Code%20Compliance%20and%20CI/Compliance%20Checks/")); + BH.Engine.Base.Query.DocumentationURL("DevOps/Code%20Compliance%20and%20CI/Compliance%20Checks/"), + accounting); // Exit code mirrors BHoMBot: failure only on Error; Warning and Pass are both success. return mergedResult.Status == TestStatus.Error ? 1 : 0; diff --git a/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs b/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs index 6422934..87a2206 100644 --- a/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs +++ b/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs @@ -37,16 +37,18 @@ static int Main(string[] args) var mergedResult = new TestResult() { Status = TestStatus.Pass, Information = new List() }; var allAnnotations = new List(); + var accounting = new FileAccounting(files.Count); foreach (var file in files) { // Only .json files under a datasets/ path are in scope. - if (!FileFilter.IsDatasetFile(file)) continue; + if (!FileFilter.IsDatasetFile(file)) { accounting.CountNotRelevant(); continue; } if (verbose) Console.WriteLine($"\n=== Checking: {file} ==="); if (!File.Exists(file)) { + accounting.CountNotOnDisk(); Console.WriteLine($" [SKIP] File not found: {file}"); continue; } @@ -55,12 +57,14 @@ static int Main(string[] args) if (resultForThisFile == null) { + accounting.CountNoResult(); Console.WriteLine($" [SKIP] No result returned for: {file}"); continue; } if (verbose) Console.WriteLine($" Result Status: {resultForThisFile.Status}"); + accounting.CountExamined(); mergedResult = mergedResult.Merge(resultForThisFile); var information = resultForThisFile.Information ?? Enumerable.Empty(); @@ -94,7 +98,8 @@ static int Main(string[] args) const string checkType = "dataset"; OutputEmitter.Write(outputFormat, checkType, mergedResult.Status, allAnnotations, sarifFilePath, verbose, - BH.Engine.Base.Query.DocumentationURL("DevOps/Code%20Compliance%20and%20CI/Compliance%20Checks/")); + BH.Engine.Base.Query.DocumentationURL("DevOps/Code%20Compliance%20and%20CI/Compliance%20Checks/"), + accounting); // Exit code mirrors BHoMBot: failure only on Error; Warning and Pass are both success. return mergedResult.Status == TestStatus.Error ? 1 : 0; diff --git a/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileAccounting.cs b/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileAccounting.cs new file mode 100644 index 0000000..d167c42 --- /dev/null +++ b/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileAccounting.cs @@ -0,0 +1,90 @@ +using System.Collections.Generic; + +/// +/// Counts what a compliance runner did with the files it was handed, and renders that as a +/// coverage denominator. +/// +/// Both runners walk the same four-exit loop: a file is dropped by the filter, dropped because +/// it is not on disk, dropped because the engine returned no result, or examined. Without a +/// count of those, a run that examined every file and a run that examined none produce the same +/// output, so a green check carries no evidence that anything was checked. +/// +/// Deliberately free of BHoM types and of any dependency on either runner, so it can be +/// compiled directly into the hermetic test project and its arithmetic and wording tested +/// without a BHoM install. It holds counters and formats strings; it decides nothing. +/// +/// The drop reasons are counted but not sub-classified. A file dropped by the filter may have +/// been excluded on purpose — project compliance deliberately skips test projects and anything +/// under .ci/ — or may be a disagreement between the pathspec that selected it and the filter +/// that rejected it. Telling those apart would require the filter to explain itself, which is a +/// change to a class both runners share. The denominator's job is to make the ratio visible; +/// diagnosing it is a separate question. +/// +public sealed class FileAccounting +{ + /// Files handed to the runner on the command line. + public int HandedIn { get; private set; } + + /// Dropped because the check's own filter did not consider them relevant. + public int NotRelevant { get; private set; } + + /// Dropped because the path did not exist on disk. + public int NotOnDisk { get; private set; } + + /// Dropped because the compliance engine returned no result for them. + public int NoResult { get; private set; } + + /// Files the runner actually examined and merged into its verdict. + public int Examined { get; private set; } + + public FileAccounting(int handedIn) => HandedIn = handedIn; + + public void CountNotRelevant() => NotRelevant++; + public void CountNotOnDisk() => NotOnDisk++; + public void CountNoResult() => NoResult++; + public void CountExamined() => Examined++; + + /// + /// True when files were supplied and none of them was examined. The calling workflow exits + /// before invoking a runner when it has no files at all, so this is the anomalous case + /// rather than the empty one: something selected these files and the runner used none. + /// + public bool ExaminedNothing => HandedIn > 0 && Examined == 0; + + /// + /// The coverage line. Leads with what was examined rather than with what was handed in: + /// the whole point is to make a green interpretable, and a count of inputs does not. + /// + public string CoverageLine() => + $"Coverage: {Examined} of {HandedIn} file(s) examined; {DropSummary()}."; + + /// + /// The warning text for a run that examined nothing. Carries the breakdown rather than the + /// bare fact, because the breakdown is what distinguishes a pull request that changed + /// nothing relevant from a selection layer handing over files this check will never accept. + /// + public string ExaminedNothingWarning() => + $"{HandedIn} file(s) were handed to this check and none was examined: {DropSummary()}. " + + "The check reports success without having inspected anything. If the files were dropped " + + "as not relevant, the pattern that selected them and the filter that rejected them " + + "disagree."; + + private string DropSummary() + { + var parts = new List(); + if (NotRelevant > 0) parts.Add($"{NotRelevant} not relevant to this check"); + if (NotOnDisk > 0) parts.Add($"{NotOnDisk} not found on disk"); + if (NoResult > 0) parts.Add($"{NoResult} returned no result"); + return parts.Count == 0 ? "none dropped" : string.Join(", ", parts); + } + + /// Shape for machine-readable output, so consumers get counts structurally. + public Dictionary ToPayload() => new() + { + ["handedIn"] = HandedIn, + ["examined"] = Examined, + ["notRelevant"] = NotRelevant, + ["notOnDisk"] = NotOnDisk, + ["noResult"] = NoResult, + }; +} diff --git a/tools/ComplianceRunner/src/Shared/Compliance.Shared/OutputEmitter.cs b/tools/ComplianceRunner/src/Shared/Compliance.Shared/OutputEmitter.cs index 9a447fb..0752e91 100644 --- a/tools/ComplianceRunner/src/Shared/Compliance.Shared/OutputEmitter.cs +++ b/tools/ComplianceRunner/src/Shared/Compliance.Shared/OutputEmitter.cs @@ -26,10 +26,32 @@ public static void Write( List annotations, string? sarifFilePath, bool verbose, - string toolUri) + string toolUri, + FileAccounting? accounting = null) { CheckMetadata.GetOutput(checkType, status, out string title, out string summary, out string text); + // Coverage goes to stdout only for the two human-facing formats. json and sarif put a + // payload on stdout and nothing else may precede it: an unconditional Console.WriteLine + // here is exactly how the [SKIP] diagnostic already breaks a caller that parses stdout + // directly. The counts still reach machine-readable consumers, structurally, below. + if (accounting is not null && (outputFormat == "console" || outputFormat == "github")) + { + Console.WriteLine(accounting.CoverageLine()); + + // Reported, never failed. Whether examining nothing should fail is a separate and + // undecided question; this only makes the state visible, and it is emitted as a + // workflow command so it survives the collapsed log group the caller wraps this in. + if (accounting.ExaminedNothing) + { + string warning = accounting.ExaminedNothingWarning(); + if (outputFormat == "github") + Console.WriteLine($"::warning title=Compliance coverage::{warning}"); + else + Console.WriteLine($"WARNING: {warning}"); + } + } + if (verbose) { if (status == TestStatus.Error || status == TestStatus.Warning) @@ -84,6 +106,7 @@ public static void Write( ["summary"] = summary, ["text"] = text, ["annotationCount"] = annotations.Count, + ["coverage"] = accounting?.ToPayload() ?? new Dictionary(), ["annotations"] = annotations.Select(a => new Dictionary { ["path"] = a.FilePath, diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/ComplianceRunnerE2ETests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/ComplianceRunnerE2ETests.cs index 41111bd..4641311 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/ComplianceRunnerE2ETests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/ComplianceRunnerE2ETests.cs @@ -97,12 +97,22 @@ public void JsonOutput_CheckType_MatchesInput() // ── GitHub Actions output format ────────────────────────────────────────── [Test] - public void GitHubOutput_ForPassingRun_ProducesNoAnnotationLines() + public void GitHubOutput_ForPassingRun_ProducesNoFindingAnnotations() { - // Filtered-out files produce no ::error/::warning lines. + // This previously asserted stdout was entirely empty. That silence was the defect: a run + // that examined nothing looked exactly like a run that examined everything. What a + // passing run must not produce is a *finding* annotation, which is what the name means + // and what is asserted now. The coverage output is not a finding and changes no verdict. var (exitCode, stdout) = RunnerFixture.Run("ComplianceRunner", "code", "--output", "github", "nonexistent.md"); - Assert.That(exitCode, Is.EqualTo(0)); - Assert.That(stdout.Trim(), Is.Empty); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(stdout, Does.Not.Contain("::error"), "no findings, so no error annotations"); + Assert.That(stdout, Does.Contain("Coverage: 0 of 1 file(s) examined")); + Assert.That(stdout, Does.Contain("::warning title=Compliance coverage::"), + "the one annotation a passing run may produce is the report that it examined nothing"); + }); } } diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/CoverageReportingTests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/CoverageReportingTests.cs new file mode 100644 index 0000000..e3490bc --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/CoverageReportingTests.cs @@ -0,0 +1,183 @@ +using NUnit.Framework; +using System.Text.Json; + +/// +/// What the compliance runners report about how much they examined. +/// +/// Both runners walk the same four-exit loop: a file is dropped by the filter, dropped because +/// it is not on disk, dropped because the engine returned no result, or examined. Only the last +/// contributes to the verdict. Without a count of the four, a run that examined every file and a +/// run that examined none produce the same output and the same green check. +/// +/// These assert the counts reach each output surface, and — as importantly — that they reach +/// only the surfaces where they belong. The machine-readable formats put a payload on stdout and +/// nothing may precede it, so the coverage line is gated to the human formats and the counts +/// travel structurally instead. +/// +/// Process invocation via RunnerFixture, like the other tests here: the accounting is fed from +/// each runner's entry point, which compiles against BHoM types and cannot be reached in-process. +/// The arithmetic and the wording are tested directly in the hermetic project; these tests are +/// about whether the runner actually wires it up. +/// +/// Reporting only. None of this changes an exit code, which is asserted rather than assumed. +/// +[TestFixture] +[Category("Integration")] +public class CoverageReportingTests +{ + // ── ComplianceRunner ────────────────────────────────────────────────────────────── + + [Test] + [Description("A run that examines nothing says so, with the breakdown.")] + public void ComplianceRunner_ExaminesNothing_ReportsTheDenominatorAndWarns() + { + // Three files relevant to a code check, none of them on disk. Every one is dropped. + var (exitCode, stdout) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "github", "a.cs", "b.cs", "c.cs"); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0), + "unchanged: reporting a count decides nothing, and whether examining nothing " + + "should fail is a separate and open question"); + + Assert.That(stdout, Does.Contain("Coverage: 0 of 3 file(s) examined")); + Assert.That(stdout, Does.Contain("3 not found on disk")); + + // The warning is what a reader sees. It carries the breakdown, because the bare + // fact does not distinguish an empty pull request from a selection layer handing + // over files this check will never accept. + Assert.That(stdout, Does.Contain("::warning title=Compliance coverage::")); + Assert.That(stdout, Does.Contain("none was examined")); + }); + } + + [Test] + [Description("A file the filter discards is counted, and the discard is visible.")] + public void ComplianceRunner_FileDroppedByFilter_IsCountedAndWarned() + { + // Ends with AssemblyInfo.cs, so a '*AssemblyInfo.cs' pathspec selects it, but the + // project filter requires the name to equal AssemblyInfo.cs exactly. This is the + // disagreement that produces a green check having examined nothing, and before this + // change the file was dropped without appearing anywhere in the output. + var (exitCode, stdout) = RunnerFixture.Run("ComplianceRunner", + "project", "--output", "github", "Properties/NotAssemblyInfo.cs"); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(stdout, Does.Contain("Coverage: 0 of 1 file(s) examined")); + Assert.That(stdout, Does.Contain("1 not relevant to this check")); + + // The reader is told what a drop-as-not-relevant implies rather than left to infer it. + Assert.That(stdout, Does.Contain("disagree")); + }); + } + + [Test] + [Description("A run that examines something reports the count and does not warn.")] + public void ComplianceRunner_ExaminesSomething_ReportsCoverageWithoutWarning() + { + // RunnerFixture's own source file: a real .cs file that exists on disk, so the code + // check examines it rather than dropping it. + string self = typeof(RunnerFixture).Assembly.Location; + string dir = Path.GetDirectoryName(self)!; + string file = Path.Combine(dir, "coverage-probe.cs"); + File.WriteAllText(file, "// nothing to find here\n"); + try + { + var (exitCode, stdout) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "github", file); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(stdout, Does.Contain("Coverage: 1 of 1 file(s) examined")); + Assert.That(stdout, Does.Contain("none dropped")); + Assert.That(stdout, Does.Not.Contain("::warning title=Compliance coverage::"), + "the warning is for the zero case only, or it becomes noise and stops being read"); + }); + } + finally + { + File.Delete(file); + } + } + + // ── Machine-readable output must stay machine-readable ──────────────────────────── + + [Test] + [Description("The json payload carries the counts, and the coverage line stays off stdout.")] + public void ComplianceRunner_JsonOutput_ParsesCleanlyAndCarriesCounts() + { + // Dropped by the filter, which is silent, so nothing precedes the payload. A file that + // is relevant but absent would print [SKIP] and break the parse for an unrelated + // reason, which is a known separate defect; this test measures one thing. + var (_, stdout) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "json", "not-a-code-file.txt"); + + // The property being pinned: adding coverage output must not put anything on stdout + // ahead of the payload. If the coverage line ever stops being gated by output format, + // this is what fails. + using var doc = JsonDocument.Parse(stdout); + + Assert.That(doc.RootElement.TryGetProperty("coverage", out var coverage), Is.True, + "machine-readable consumers get the counts structurally, not by parsing a console line"); + + Assert.Multiple(() => + { + Assert.That(coverage.GetProperty("handedIn").GetInt32(), Is.EqualTo(1)); + Assert.That(coverage.GetProperty("examined").GetInt32(), Is.EqualTo(0)); + Assert.That(coverage.GetProperty("notRelevant").GetInt32(), Is.EqualTo(1)); + }); + + Assert.That(stdout, Does.Not.Contain("Coverage: "), + "the human-facing line belongs to the console and github formats only"); + } + + [Test] + [Description("The sarif payload is likewise not preceded by a coverage line.")] + public void ComplianceRunner_SarifOutput_IsNotPrecededByCoverage() + { + var (_, stdout) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "sarif", "not-a-code-file.txt"); + + Assert.That(stdout.TrimStart(), Does.StartWith("{"), + "sarif goes to stdout as a payload and nothing may precede it"); + Assert.That(stdout, Does.Not.Contain("Coverage: ")); + } + + // ── DatasetComplianceRunner: the same loop, the same instrumentation ────────────── + + [Test] + [Description("The dataset runner reports the same denominator from the same loop shape.")] + public void DatasetComplianceRunner_ExaminesNothing_ReportsTheDenominatorAndWarns() + { + // Relevant to a dataset check by path and extension, absent from disk. + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", + "--output", "github", "datasets/a.json", "datasets/b.json"); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(stdout, Does.Contain("Coverage: 0 of 2 file(s) examined")); + Assert.That(stdout, Does.Contain("2 not found on disk")); + Assert.That(stdout, Does.Contain("::warning title=Compliance coverage::")); + }); + } + + [Test] + [Description("A non-dataset file is counted as not relevant rather than dropped silently.")] + public void DatasetComplianceRunner_FileDroppedByFilter_IsCounted() + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", + "--output", "github", "src/NotADataset.json"); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(stdout, Does.Contain("Coverage: 0 of 1 file(s) examined")); + Assert.That(stdout, Does.Contain("1 not relevant to this check")); + }); + } +} diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs index c6f4ff2..9646503 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs @@ -80,11 +80,21 @@ public void JsonOutput_CheckType_IsDataset() // ── GitHub Actions output format ────────────────────────────────────────── [Test] - public void GitHubOutput_ForPassingRun_ProducesNoAnnotationLines() + public void GitHubOutput_ForPassingRun_ProducesNoFindingAnnotations() { + // This previously asserted stdout was entirely empty. That silence was the defect: a run + // that examined nothing looked exactly like a run that examined everything. What a + // passing run must not produce is a *finding* annotation, which is what the name means + // and what is asserted now. The coverage output is not a finding and changes no verdict. var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", "--output", "github", "notadataset/foo.json"); - Assert.That(exitCode, Is.EqualTo(0)); - Assert.That(stdout.Trim(), Is.Empty); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(stdout, Does.Not.Contain("::error")); + Assert.That(stdout, Does.Contain("Coverage: 0 of 1 file(s) examined")); + Assert.That(stdout, Does.Contain("::warning title=Compliance coverage::")); + }); } } diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs index 236ca72..2622e4d 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs @@ -69,20 +69,22 @@ public void AllRelevantFilesMissing_ExitsZeroHavingExaminedNothing() } [Test] - [Description("The count of files actually examined is not reported anywhere.")] - public void ExaminedCount_IsNotReported() + [Description("The count of files actually examined is reported.")] + public void ExaminedCount_IsReported() { - // Contrast with VersioningRunner, which prints a Coverage line precisely so that a - // pass over zero and a pass over thousands are distinguishable (RunCommand.cs:226-230). - // ComplianceRunner has no equivalent, so a pass that examined nothing is - // indistinguishable in the log from one that examined everything. + // This was the inverse assertion: it recorded that no count existed, and said adding one + // was the cheapest partial mitigation and did not depend on the pass-versus-fail question + // being settled. The count now exists, so the assertion is inverted. The pass-versus-fail + // question is still open and this still does not touch it. + // + // Matches VersioningRunner, which prints a Coverage line precisely so that a pass over + // zero and a pass over thousands are distinguishable. var (_, stdout) = RunnerFixture.Run("ComplianceRunner", "code", "--output", "github", "a.cs", "b.cs", "c.cs"); - Assert.That(stdout, Does.Not.Contain("examined"), - "There is no coverage line. Adding one is the cheapest partial mitigation and does " - + "not depend on how the pass-versus-fail question is settled, because reporting the " - + "number changes no verdict."); + Assert.That(stdout, Does.Contain("0 of 3 file(s) examined"), + "A pass that examined nothing must be distinguishable from one that examined " + + "everything, which is what the denominator is for."); } // ── Machine-readable output and the [SKIP] diagnostic ───────────────────────────── diff --git a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Compliance.Unit.Tests.csproj b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Compliance.Unit.Tests.csproj index d40cb78..ec13e3e 100644 --- a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Compliance.Unit.Tests.csproj +++ b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Compliance.Unit.Tests.csproj @@ -50,6 +50,7 @@ + diff --git a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileAccountingTests.cs b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileAccountingTests.cs new file mode 100644 index 0000000..10cb7cb --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileAccountingTests.cs @@ -0,0 +1,149 @@ +using NUnit.Framework; + +namespace Compliance.Tests +{ + /// + /// The coverage denominator's arithmetic and its wording, tested without a BHoM install. + /// + /// The accounting was extracted from the two runners' entry points precisely so it could be + /// tested here: those entry points compile against BHoM types, so with the counting inlined + /// the only way to check a total would be a full CI run against a real dependency closure. + /// + /// The wording is asserted as well as the arithmetic. This text is the whole deliverable — + /// it is what a reader sees when a check reports success having examined nothing — so a + /// change to it should be a deliberate edit to a test, not a silent drift. + /// + [TestFixture] + public class FileAccountingTests + { + [Test] + public void EveryFileExamined_ReportsFullCoverageAndNoDrops() + { + var a = new FileAccounting(3); + a.CountExamined(); a.CountExamined(); a.CountExamined(); + + Assert.Multiple(() => + { + Assert.That(a.Examined, Is.EqualTo(3)); + Assert.That(a.ExaminedNothing, Is.False); + Assert.That(a.CoverageLine(), Is.EqualTo("Coverage: 3 of 3 file(s) examined; none dropped.")); + }); + } + + [Test] + public void EachDropReasonIsCountedAndNamedSeparately() + { + var a = new FileAccounting(4); + a.CountNotRelevant(); + a.CountNotOnDisk(); + a.CountNoResult(); + a.CountExamined(); + + Assert.That(a.CoverageLine(), Is.EqualTo( + "Coverage: 1 of 4 file(s) examined; 1 not relevant to this check, 1 not found on disk, 1 returned no result.")); + } + + [Test] + public void ADropReasonThatDidNotOccurIsNotMentioned() + { + // Listing every reason at zero would bury the one that fired. + var a = new FileAccounting(2); + a.CountNotRelevant(); a.CountNotRelevant(); + + Assert.Multiple(() => + { + Assert.That(a.CoverageLine(), Does.Contain("2 not relevant to this check")); + Assert.That(a.CoverageLine(), Does.Not.Contain("not found on disk")); + Assert.That(a.CoverageLine(), Does.Not.Contain("returned no result")); + }); + } + + // ── The case this exists for ────────────────────────────────────────────────── + + [Test] + public void FilesHandedInAndNoneExamined_IsFlagged() + { + var a = new FileAccounting(3); + a.CountNotRelevant(); a.CountNotRelevant(); a.CountNotRelevant(); + + Assert.That(a.ExaminedNothing, Is.True); + } + + [Test] + public void TheWarningCarriesTheBreakdown_NotJustTheFact() + { + // A bare "nothing was examined" does not tell a reader whether the pull request + // changed nothing relevant or the selection layer handed over files this check + // will never accept. The breakdown is what separates those two. + var a = new FileAccounting(3); + a.CountNotRelevant(); a.CountNotRelevant(); a.CountNotRelevant(); + + string w = a.ExaminedNothingWarning(); + + Assert.Multiple(() => + { + Assert.That(w, Does.Contain("3 file(s) were handed to this check and none was examined")); + Assert.That(w, Does.Contain("3 not relevant to this check")); + Assert.That(w, Does.Contain("reports success without having inspected anything")); + Assert.That(w, Does.Contain("disagree"), + "the reader needs to be told what a drop-as-not-relevant implies, not left to infer it"); + }); + } + + [Test] + public void NoFilesAtAll_IsNotFlagged() + { + // The calling workflow exits before invoking a runner when it has no files, so this + // is not a state production reaches. Asserted so the flag means "selected but + // unused" rather than "empty", which is the distinction that makes it worth raising. + var a = new FileAccounting(0); + + Assert.That(a.ExaminedNothing, Is.False); + } + + [Test] + public void SomeExamined_IsNotFlagged() + { + var a = new FileAccounting(2); + a.CountNotRelevant(); + a.CountExamined(); + + Assert.That(a.ExaminedNothing, Is.False); + } + + // ── Machine-readable shape ──────────────────────────────────────────────────── + + [Test] + public void ThePayloadCarriesEveryCounter() + { + var a = new FileAccounting(4); + a.CountNotRelevant(); a.CountNotOnDisk(); a.CountNoResult(); a.CountExamined(); + + var p = a.ToPayload(); + + Assert.Multiple(() => + { + Assert.That(p["handedIn"], Is.EqualTo(4)); + Assert.That(p["examined"], Is.EqualTo(1)); + Assert.That(p["notRelevant"], Is.EqualTo(1)); + Assert.That(p["notOnDisk"], Is.EqualTo(1)); + Assert.That(p["noResult"], Is.EqualTo(1)); + }); + } + + [Test] + public void TheCountsReconcile() + { + // If they ever stop adding up, the denominator is lying and every number above it + // is unreadable. + var a = new FileAccounting(10); + for (int i = 0; i < 2; i++) a.CountNotRelevant(); + for (int i = 0; i < 3; i++) a.CountNotOnDisk(); + a.CountNoResult(); + for (int i = 0; i < 4; i++) a.CountExamined(); + + Assert.That(a.NotRelevant + a.NotOnDisk + a.NoResult + a.Examined, + Is.EqualTo(a.HandedIn)); + } + } +}