From d4e99a5a1118883656ca1c9768f42ecf3fcb9d9f Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Sun, 23 Aug 2026 21:27:31 +0100 Subject: [PATCH 1/2] test(compliance): make ComplianceRunner.Main observable The entry point had no coverage reachable from the hermetic suite. Every branch of Main compiles against BHoM types, so it cannot be unit-tested in-process without restructuring the runner. Process invocation via the existing RunnerFixture observes it without touching production code. MainAccountingTests characterises the file accounting and the exit code, including the path where every relevant file is absent and the runner still exits 0 with status Pass. PathspecFilterPairingTests pairs a template pathspec against FileFilter in one test, which nothing else does: the pathspec matches any name ending in AssemblyInfo.cs, the filter requires the name to equal it. Both assert current behaviour on purpose, and record what each assertion should become if that behaviour is changed. A second workflow job resolves the Test_Toolkit dependency graph so Compliance.Tests compiles, which also un-inerts the six test files already there. resolve-dependencies in mode: seeds is the same mechanism ci-compliance uses, and it does produce CodeComplianceTest_oM.dll. Kept as a separate job so a resolve-dependencies breakage cannot suppress the runner tests that would catch it. The job also builds the runners before testing. RunnerFixture uses 'dotnet run --no-build' but Compliance.Tests references neither executable project, so dotnet test never built them; the first validation run failed 14 tests with exit 1 and empty stdout for that reason alone. 114 tests pass on a bare runner with no BHoM install. No runner behaviour changed. --- .github/workflows/test-tools.yml | 82 +++++++++- .../Integration/MainAccountingTests.cs | 127 ++++++++++++++++ .../Integration/PathspecFilterPairingTests.cs | 141 ++++++++++++++++++ 3 files changed, 345 insertions(+), 5 deletions(-) create mode 100644 tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs create mode 100644 tools/ComplianceRunner/tests/Compliance.Tests/Integration/PathspecFilterPairingTests.cs diff --git a/.github/workflows/test-tools.yml b/.github/workflows/test-tools.yml index a80d060..eec4da1 100644 --- a/.github/workflows/test-tools.yml +++ b/.github/workflows/test-tools.yml @@ -6,11 +6,28 @@ # all: PR #131 reported zero. That left every runner regression test inert, including the # one added in #131 to stop the System.Drawing.Common reference being dropped as unused. # -# Compliance.Tests is deliberately NOT run here. Compliance.Shared references BHoM.dll, -# BHoM_Engine.dll, CodeComplianceTest_oM.dll, Test_Engine.dll and Test_oM.dll by HintPath -# out of C:\ProgramData\BHoM\Assemblies, so it cannot even compile without a BHoM install -# plus a built Test_Toolkit. Adding it needs the dependency-resolution step and belongs in -# a separate change; a job that fails for environmental reasons is worse than no job. +# Two jobs, deliberately separate. +# +# dotnet-tests Hermetic. Checkout, SDK, run. No network beyond NuGet, no BHoM. +# compliance-tests Resolves the Test_Toolkit dependency graph first, so Compliance.Tests +# can compile and its entry-point tests can run. +# +# Why Compliance.Tests needs the second job. Compliance.Shared HintPaths five assemblies out +# of C:\ProgramData\BHoM\Assemblies. Measured on a machine carrying the BHoM installer, four +# resolve and only CodeComplianceTest_oM.dll is missing, because it is built by +# BHoM/Test_Toolkit (CodeCompliance_oM/CodeComplianceTest_oM.csproj) rather than shipped in +# the installer payload. AnnotationConvert.cs then fails CS0234 on BH.oM.Test.CodeCompliance +# and takes the whole project down, which is why all nine of its test files were inert. +# +# resolve-dependencies in mode: seeds with seed BHoM/Test_Toolkit is exactly what ci-compliance +# already does before prepare-runner publishes ComplianceRunner, and ComplianceRunner.csproj +# HintPaths the same assembly. That path demonstrably works in production, so it works here. +# +# Kept as a separate job rather than extra steps on the first for two reasons. The hermetic +# suite stays fast and stays independent: a resolve-dependencies breakage cannot also suppress +# the runner tests that would catch it, which matters because resolve-dependencies is itself +# one of the actions this repository ships. And a dependency-graph build is minutes where the +# hermetic suite is seconds, so failures stay attributable. name: Test Tools on: @@ -51,3 +68,58 @@ jobs: - name: VersioningRunner tests run: dotnet test tools/VersioningRunner/src/VersioningRunner.Tests --nologo + + # ComplianceRunner's own suite, including the entry-point characterisation tests. Needs + # CodeComplianceTest_oM.dll, so it builds the Test_Toolkit graph first. See the header. + compliance-tests: + name: ComplianceRunner tests + runs-on: windows-latest + # The hermetic job is 15. This one clones and builds Test_Toolkit's graph before it can + # compile anything, and the assembly cache is cold on a fresh branch (register item 28). + timeout-minutes: 45 + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + # Test_Toolkit is public, so github.token suffices and no App credentials are needed. + # resolve-dependencies builds with the SDK it is given; ComplianceRunner targets + # net10.0 and is published by the test host, so both versions are installed below. + - name: Resolve Test_Toolkit dependency graph + uses: ./.github/actions/resolve-dependencies + with: + mode: seeds + seeds: BHoM/Test_Toolkit + dotnet_version: '8.0' + configuration: Release + token: ${{ github.token }} + + - name: Set up .NET + uses: actions/setup-dotnet@c2fa09f4bde5ebb9d1777cf28262a3eb3db3ced7 # v5 + with: + dotnet-version: '10.x' + + # Fail loudly and specifically if the graph build did not produce the one assembly this + # job exists to obtain, rather than letting it surface as a bare CS0234 later. + - name: Assert CodeComplianceTest_oM is present + shell: pwsh + run: | + $dll = 'C:\ProgramData\BHoM\Assemblies\CodeComplianceTest_oM.dll' + if (-not (Test-Path $dll)) { + Write-Host "::error title=Test Tools::CodeComplianceTest_oM.dll absent after resolving BHoM/Test_Toolkit. Compliance.Tests cannot compile without it." + exit 1 + } + Write-Host "::notice title=Test Tools::CodeComplianceTest_oM.dll present ($([int](Get-Item $dll).Length) bytes)." + + # RunnerFixture invokes the runners with `dotnet run --no-build`, so their output must + # already exist. `dotnet test` does not produce it: Compliance.Tests has no + # ProjectReference to either executable project, so only Compliance.Shared and the test + # assembly get built. RunnerFixture.cs:9-11 claims the solution "is built automatically + # when running via the test project", which is not true and is why every process-invoking + # test failed with exit 1 and empty stdout the first time this job ran. Building the + # solution here satisfies the prerequisite the fixture documents without changing the + # fixture. Debug, because that is what `dotnet run --no-build` resolves to by default. + - name: Build the runners (RunnerFixture uses --no-build) + run: dotnet build tools/ComplianceRunner/Platform.slnx -c Debug --nologo + + - name: ComplianceRunner tests + run: dotnet test tools/ComplianceRunner/tests/Compliance.Tests --no-build --nologo diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs new file mode 100644 index 0000000..e6efc5d --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs @@ -0,0 +1,127 @@ +using NUnit.Framework; +using System.Text.Json; + +/// +/// Characterisation tests for ComplianceRunner.Main's file accounting and exit code. +/// +/// These pin down what the entry point currently does when it examines nothing, which is +/// findings-register item 3: the runner can report success having inspected no file at all. +/// They are written to PASS against today's behaviour on purpose. The register item is +/// Critical and blocked on decision Q3 (fail, warn, or fail only once a repo is gated), so +/// this file records the behaviour rather than asserting a preferred one. Each assertion that +/// is expected to invert once Q3 is answered is marked INVERTS-ON-Q3 with what it should +/// become. +/// +/// Main cannot be unit-tested in-process: every branch of it compiles against BHoM types +/// (TestResult, TestStatus, ITestInformation, BH.Engine.Test.CodeCompliance.Compute, +/// BH.Engine.Base.Query), so even the usage path at :18-31, which touches no BHoM type at +/// runtime, cannot be reached without them. Process invocation via RunnerFixture is therefore +/// the only way to observe it without restructuring the runner, and it is the convention the +/// existing E2E tests already use. +/// +[TestFixture] +[Category("Integration")] +public class MainAccountingTests +{ + // ── The [SKIP] path: relevant extension, file absent (ComplianceRunner.cs:49-53) ── + // + // Distinct from the filter path already covered by ComplianceRunnerE2ETests. There, a + // file is rejected by FileFilter and `continue`d silently. Here the file IS relevant, so + // it passes the filter, and is then dropped because it is not on disk. That second drop + // prints a line but changes nothing else: no annotation, no status change, no exit code. + + [Test] + [Description("A relevant file that is absent from disk is announced as [SKIP] on stdout.")] + public void MissingRelevantFile_AnnouncesSkipOnStdout() + { + var (_, stdout) = RunnerFixture.Run("ComplianceRunner", "code", "definitely-absent.cs"); + + Assert.That(stdout, Does.Contain("[SKIP]"), + "ComplianceRunner.cs:51 prints ' [SKIP] File not found: ' for a relevant " + + "file that is not on disk. If this assertion fails the diagnostic has been removed " + + "or reworded, and item 3's only current signal has gone with it."); + } + + [Test] + [Description("ITEM 3: every relevant file being absent still exits 0 with status Pass.")] + public void AllRelevantFilesMissing_ExitsZeroWithPassStatus_ITEM3() + { + // Three files, all relevant to a code check, none on disk. Nothing is examined. + var (exitCode, stdout) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "github", "a.cs", "b.cs", "c.cs"); + + Assert.Multiple(() => + { + // INVERTS-ON-Q3: should become Is.EqualTo(1) if Q3 decides that examining + // nothing is a failure, or stay 0 with a ::warning if Q3 decides it warns. + Assert.That(exitCode, Is.EqualTo(0), + "ITEM 3 (register, Critical). mergedResult.Status is initialised to Pass at " + + "ComplianceRunner.cs:39 and only ever changes via Merge inside the per-file " + + "loop. Every file skipping means the loop body never runs, so :162 returns 0. " + + "A compliance check therefore reports success having inspected nothing."); + + // No annotation is emitted either, so nothing in the GitHub log distinguishes + // this from a genuine clean pass except the [SKIP] lines. + Assert.That(stdout, Does.Not.Contain("::error"), + "No annotation is produced when nothing was examined."); + }); + } + + [Test] + [Description("ITEM 3: the count of files actually examined is not reported anywhere.")] + public void ExaminedCount_IsNotReported_ITEM3() + { + // 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, which is why item 3 is invisible in the log. + var (_, stdout) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "github", "a.cs", "b.cs", "c.cs"); + + Assert.That(stdout, Does.Not.Contain("examined"), + "ITEM 3. There is no coverage line. Adding one is the cheapest partial mitigation " + + "and does not need Q3 answered, because reporting the number changes no verdict."); + } + + // ── Machine-readable output and the [SKIP] diagnostic ───────────────────────────── + + [Test] + [Description("The [SKIP] diagnostic is written to stdout even when the output format is json.")] + public void MissingRelevantFile_JsonOutput_SkipLinePrecedesTheJson() + { + var (exitCode, stdout) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "json", "definitely-absent.cs"); + + Assert.That(exitCode, Is.EqualTo(0)); + + // ComplianceRunner.cs:51 is an unconditional Console.WriteLine, not gated on the + // `verbose` flag that the console format sets. So in json and sarif modes the + // diagnostic lands on the same stream as the payload, ahead of it. + Assert.That(stdout.TrimStart(), Does.StartWith("[SKIP]").Or.StartWith(" [SKIP]"), + "If this fails, the skip diagnostic has been moved off stdout or behind the " + + "verbose flag, which would resolve the stream-mixing issue."); + + // The consequence, asserted rather than described: the raw stdout is not valid JSON. + // Assert.Catch rather than Assert.Throws because the concrete type is + // JsonReaderException, a subclass, and Assert.Throws matches the exact type only. + Assert.Catch(() => JsonDocument.Parse(stdout), + "Raw stdout does not parse as JSON once a skip line is present. Callers using " + + "--output json must strip leading diagnostics. The existing E2E tests parse " + + "stdout directly and only pass because the filter path prints nothing."); + } + + // ── Exit code mapping at :162 ───────────────────────────────────────────────────── + + [Test] + [Description("A run with no findings maps to exit 0 (the Pass and Warning half of :162).")] + public void NoFindings_MapsToExitZero() + { + var (exitCode, _) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "json", "definitely-absent.cs"); + + // :162 is `return mergedResult.Status == TestStatus.Error ? 1 : 0`, so Pass and + // Warning both map to 0. That mirrors BHoMBot deliberately (ComplianceRunner.cs:161). + // The Error half needs a real finding from the BHoM engine and so belongs with the + // RequiresBHoM tests, not here. + Assert.That(exitCode, Is.EqualTo(0)); + } +} diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/PathspecFilterPairingTests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/PathspecFilterPairingTests.cs new file mode 100644 index 0000000..a556da4 --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/PathspecFilterPairingTests.cs @@ -0,0 +1,141 @@ +using NUnit.Framework; +using System.Diagnostics; + +/// +/// Pairs the two halves of file selection against each other, which nothing else does. +/// +/// Selection happens twice, in two languages, in two repositories' worth of convention: +/// 1. compute-changed-files runs `git diff --name-only --diff-filter=ACMRT HEAD^1 HEAD -- +/// <pathspec>` with the pathspec taken from the calling template, e.g. +/// `patterns: '*AssemblyInfo.cs *.csproj'` for a project-compliance job. +/// 2. ComplianceRunner then asks FileFilter.IsRelevantFile of every file it was handed. +/// +/// Each half has tests. `.github/scripts/tests/test-changed-file-patterns.sh` asserts the +/// pathspec behaviour against real git, and FileFilterTests asserts the predicate. Neither +/// asserts that the two agree, and findings-register item 34b is that they do not: the +/// pathspec token `*AssemblyInfo.cs` selects any file whose name ENDS with that string, while +/// FileFilter.cs:24 requires the name to EQUAL it. A file in between is selected, counted into +/// the skip decision, handed to the runner, and then silently discarded. +/// +/// These tests are written to PASS against today's behaviour. They record the disagreement so +/// it is visible in the suite. Whether the fix narrows the pathspec or widens the filter is +/// open: BHoMBot used EndsWith("AssemblyInfo.cs") (ProjectCompliance.cs:33), so widening the +/// filter restores the older semantics, and FileFilterTests.cs:20 currently asserts the +/// narrower one deliberately. Marked INVERTS-ON-34b where a decision would change them. +/// +[TestFixture] +[Category("Integration")] +public class PathspecFilterPairingTests +{ + // The token shipped by every project-compliance job in templates/{BHoM,BHE}/ci-*.yml. + private const string ProjectPathspec = "*AssemblyInfo.cs"; + + // Ends with "AssemblyInfo.cs" but is not "AssemblyInfo.cs". The pathspec suite's fixture + // has "Engine/AssemblyInfoHelper.cs", which does NOT end with the token and so does not + // probe this gap; nothing in either suite currently uses a name of this shape. + private const string StraddlingFile = "Properties/NotAssemblyInfo.cs"; + + [Test] + [Description("ITEM 34b: the pathspec selects a file the filter then discards.")] + public void PathspecAndFilter_DisagreeOnAStraddlingName_ITEM34B() + { + bool selectedByGit = GitDiffSelects(ProjectPathspec, StraddlingFile); + bool acceptedByFilter = FileFilter.IsRelevantFile(StraddlingFile, "project"); + + Assert.Multiple(() => + { + // Half 1: git selects it, so compute-changed-files counts it and writes it into + // changed_files.txt, and the check does not self-skip. + Assert.That(selectedByGit, Is.True, + "git pathspec '*AssemblyInfo.cs' matches any path ending in that string. " + + "'*' also matches '/', which is why the leading directory is no obstacle."); + + // Half 2: the runner then drops it. INVERTS-ON-34b if the filter is widened to + // BHoMBot's EndsWith semantics. + Assert.That(acceptedByFilter, Is.False, + "FileFilter.cs:24 requires Path.GetFileName(file).Equals(\"AssemblyInfo.cs\"), " + + "so the file is discarded with no message."); + + // The pairing, stated as the thing that is actually wrong. + Assert.That(selectedByGit && !acceptedByFilter, Is.True, + "ITEM 34b (register). Both halves are individually tested and individually " + + "defensible; they disagree. A pull request changing only a file of this shape " + + "produces a green project-compliance check that examined nothing, which is " + + "item 3 reached by a route no single-layer test can see."); + }); + } + + [Test] + [Description("Control: the exact name agrees across both halves, so the gap is specific.")] + public void PathspecAndFilter_AgreeOnTheExactName() + { + const string exact = "Properties/AssemblyInfo.cs"; + + Assert.Multiple(() => + { + Assert.That(GitDiffSelects(ProjectPathspec, exact), Is.True); + Assert.That(FileFilter.IsRelevantFile(exact, "project"), Is.True); + }); + } + + // ── Fixture ─────────────────────────────────────────────────────────────────────── + // + // Mirrors compute-changed-files' invocation exactly: same --diff-filter, same HEAD^1..HEAD + // range, pathspec passed as a trailing -- argument. A throwaway repo rather than the real + // one so the fixture is explicit and the test cannot be perturbed by the working tree. + + private static bool GitDiffSelects(string pathspec, string relativePath) + { + string repo = Path.Combine(Path.GetTempPath(), "pairing-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(repo); + try + { + Git(repo, "init", "-q"); + Git(repo, "-c", "user.email=t@t", "-c", "user.name=t", "commit", "-q", + "--allow-empty", "-m", "base"); + + string full = Path.Combine(repo, relativePath.Replace('/', Path.DirectorySeparatorChar)); + Directory.CreateDirectory(Path.GetDirectoryName(full)!); + File.WriteAllText(full, "// fixture\n"); + + Git(repo, "add", "-A"); + Git(repo, "-c", "user.email=t@t", "-c", "user.name=t", "commit", "-q", "-m", "change"); + + string selected = Git(repo, "diff", "--name-only", "--diff-filter=ACMRT", + "HEAD^1", "HEAD", "--", pathspec); + + return selected + .Split('\n', StringSplitOptions.RemoveEmptyEntries) + .Select(l => l.Trim()) + .Contains(relativePath); + } + finally + { + try { Directory.Delete(repo, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + private static string Git(string workingDir, params string[] args) + { + using var proc = new Process(); + proc.StartInfo = new ProcessStartInfo("git") + { + WorkingDirectory = workingDir, + UseShellExecute = false, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + foreach (var a in args) proc.StartInfo.ArgumentList.Add(a); + + proc.Start(); + string stdout = proc.StandardOutput.ReadToEnd(); + string stderr = proc.StandardError.ReadToEnd(); + proc.WaitForExit(); + + if (proc.ExitCode != 0) + throw new InvalidOperationException( + $"git {string.Join(' ', args)} failed with {proc.ExitCode}: {stderr}"); + + return stdout; + } +} From 997f5fbbf885bb704c68b0d7bb47babd08ad6d8b Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Mon, 24 Aug 2026 14:23:55 +0100 Subject: [PATCH 2/2] docs(compliance): make the test names and comments self-explaining The characterisation tests were named and annotated against notes kept outside this repository, so a reader here could not tell what an assertion was pinning down or what would change it. Method names now state what they assert. Comments on assertions that are expected to change state the condition and the resulting assertion in their own terms, rather than pointing at a decision recorded elsewhere. The workflow comment explaining the longer timeout now gives the reason: the assembly cache is keyed per dependency SHA set, so a new set builds the whole graph from source. Comments explaining behaviour are unchanged. No assertion, fixture or workflow step changed, so the suite runs exactly as before. --- .github/workflows/test-tools.yml | 3 +- .../Integration/MainAccountingTests.cs | 45 ++++++++++--------- .../Integration/PathspecFilterPairingTests.cs | 29 ++++++------ 3 files changed, 42 insertions(+), 35 deletions(-) diff --git a/.github/workflows/test-tools.yml b/.github/workflows/test-tools.yml index eec4da1..c792dc7 100644 --- a/.github/workflows/test-tools.yml +++ b/.github/workflows/test-tools.yml @@ -75,7 +75,8 @@ jobs: name: ComplianceRunner tests runs-on: windows-latest # The hermetic job is 15. This one clones and builds Test_Toolkit's graph before it can - # compile anything, and the assembly cache is cold on a fresh branch (register item 28). + # compile anything, and the assembly cache is keyed per dependency SHA set, so it is cold + # the first time a branch resolves a new set and the whole graph is built from source. timeout-minutes: 45 steps: - name: Checkout diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs index e6efc5d..236ca72 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs @@ -4,13 +4,14 @@ /// /// Characterisation tests for ComplianceRunner.Main's file accounting and exit code. /// -/// These pin down what the entry point currently does when it examines nothing, which is -/// findings-register item 3: the runner can report success having inspected no file at all. -/// They are written to PASS against today's behaviour on purpose. The register item is -/// Critical and blocked on decision Q3 (fail, warn, or fail only once a repo is gated), so -/// this file records the behaviour rather than asserting a preferred one. Each assertion that -/// is expected to invert once Q3 is answered is marked INVERTS-ON-Q3 with what it should -/// become. +/// These pin down what the entry point currently does when it examines nothing: the runner +/// reports success having inspected no file at all. They are written to PASS against today's +/// behaviour on purpose, so that a change to it shows up as a failing test rather than as a +/// silent change in what a compliance check means. +/// +/// Whether examining nothing should fail, warn, or stay as it is has not been decided. Where +/// an assertion would change under a different answer, the comment says what it should become +/// and under which answer, so nothing here has to be reverse-engineered later. /// /// Main cannot be unit-tested in-process: every branch of it compiles against BHoM types /// (TestResult, TestStatus, ITestInformation, BH.Engine.Test.CodeCompliance.Compute, @@ -39,12 +40,12 @@ public void MissingRelevantFile_AnnouncesSkipOnStdout() Assert.That(stdout, Does.Contain("[SKIP]"), "ComplianceRunner.cs:51 prints ' [SKIP] File not found: ' for a relevant " + "file that is not on disk. If this assertion fails the diagnostic has been removed " - + "or reworded, and item 3's only current signal has gone with it."); + + "or reworded, and with it the only signal that a file went unexamined."); } [Test] - [Description("ITEM 3: every relevant file being absent still exits 0 with status Pass.")] - public void AllRelevantFilesMissing_ExitsZeroWithPassStatus_ITEM3() + [Description("Every relevant file being absent still exits 0 with status Pass.")] + public void AllRelevantFilesMissing_ExitsZeroHavingExaminedNothing() { // Three files, all relevant to a code check, none on disk. Nothing is examined. var (exitCode, stdout) = RunnerFixture.Run("ComplianceRunner", @@ -52,13 +53,13 @@ public void AllRelevantFilesMissing_ExitsZeroWithPassStatus_ITEM3() Assert.Multiple(() => { - // INVERTS-ON-Q3: should become Is.EqualTo(1) if Q3 decides that examining - // nothing is a failure, or stay 0 with a ::warning if Q3 decides it warns. + // Should become Is.EqualTo(1) if examining nothing is later treated as a failure, + // or stay 0 with an added ::warning if it is treated as a warning instead. Assert.That(exitCode, Is.EqualTo(0), - "ITEM 3 (register, Critical). mergedResult.Status is initialised to Pass at " - + "ComplianceRunner.cs:39 and only ever changes via Merge inside the per-file " - + "loop. Every file skipping means the loop body never runs, so :162 returns 0. " - + "A compliance check therefore reports success having inspected nothing."); + "mergedResult.Status is initialised to Pass at ComplianceRunner.cs:39 and only " + + "ever changes via Merge inside the per-file loop. Every file skipping means the " + + "loop body never runs, so :162 returns 0. A compliance check therefore reports " + + "success having inspected nothing."); // No annotation is emitted either, so nothing in the GitHub log distinguishes // this from a genuine clean pass except the [SKIP] lines. @@ -68,18 +69,20 @@ public void AllRelevantFilesMissing_ExitsZeroWithPassStatus_ITEM3() } [Test] - [Description("ITEM 3: the count of files actually examined is not reported anywhere.")] - public void ExaminedCount_IsNotReported_ITEM3() + [Description("The count of files actually examined is not reported anywhere.")] + public void ExaminedCount_IsNotReported() { // 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, which is why item 3 is invisible in the log. + // ComplianceRunner has no equivalent, so a pass that examined nothing is + // indistinguishable in the log from one that examined everything. var (_, stdout) = RunnerFixture.Run("ComplianceRunner", "code", "--output", "github", "a.cs", "b.cs", "c.cs"); Assert.That(stdout, Does.Not.Contain("examined"), - "ITEM 3. There is no coverage line. Adding one is the cheapest partial mitigation " - + "and does not need Q3 answered, because reporting the number changes no verdict."); + "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."); } // ── Machine-readable output and the [SKIP] diagnostic ───────────────────────────── diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/PathspecFilterPairingTests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/PathspecFilterPairingTests.cs index a556da4..d27bd03 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/PathspecFilterPairingTests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/PathspecFilterPairingTests.cs @@ -12,16 +12,17 @@ /// /// Each half has tests. `.github/scripts/tests/test-changed-file-patterns.sh` asserts the /// pathspec behaviour against real git, and FileFilterTests asserts the predicate. Neither -/// asserts that the two agree, and findings-register item 34b is that they do not: the -/// pathspec token `*AssemblyInfo.cs` selects any file whose name ENDS with that string, while -/// FileFilter.cs:24 requires the name to EQUAL it. A file in between is selected, counted into -/// the skip decision, handed to the runner, and then silently discarded. +/// asserts that the two agree, and they do not: the pathspec token `*AssemblyInfo.cs` selects +/// any file whose name ENDS with that string, while FileFilter.cs:24 requires the name to +/// EQUAL it. A file in between is selected, counted into the skip decision, handed to the +/// runner, and then silently discarded. /// /// These tests are written to PASS against today's behaviour. They record the disagreement so /// it is visible in the suite. Whether the fix narrows the pathspec or widens the filter is /// open: BHoMBot used EndsWith("AssemblyInfo.cs") (ProjectCompliance.cs:33), so widening the /// filter restores the older semantics, and FileFilterTests.cs:20 currently asserts the -/// narrower one deliberately. Marked INVERTS-ON-34b where a decision would change them. +/// narrower one deliberately. Where an assertion would change under one of those two answers, +/// the comment says what it should become and under which answer. /// [TestFixture] [Category("Integration")] @@ -36,8 +37,8 @@ public class PathspecFilterPairingTests private const string StraddlingFile = "Properties/NotAssemblyInfo.cs"; [Test] - [Description("ITEM 34b: the pathspec selects a file the filter then discards.")] - public void PathspecAndFilter_DisagreeOnAStraddlingName_ITEM34B() + [Description("The pathspec selects a file the filter then discards.")] + public void PathspecAndFilter_DisagreeOnAStraddlingName() { bool selectedByGit = GitDiffSelects(ProjectPathspec, StraddlingFile); bool acceptedByFilter = FileFilter.IsRelevantFile(StraddlingFile, "project"); @@ -50,18 +51,20 @@ public void PathspecAndFilter_DisagreeOnAStraddlingName_ITEM34B() "git pathspec '*AssemblyInfo.cs' matches any path ending in that string. " + "'*' also matches '/', which is why the leading directory is no obstacle."); - // Half 2: the runner then drops it. INVERTS-ON-34b if the filter is widened to - // BHoMBot's EndsWith semantics. + // Half 2: the runner then drops it. Should become Is.True if the filter is widened + // to BHoMBot's EndsWith semantics; stays Is.False if the pathspec is narrowed + // instead, since then git would not select the file in the first place and the + // assertion above is the one that changes. Assert.That(acceptedByFilter, Is.False, "FileFilter.cs:24 requires Path.GetFileName(file).Equals(\"AssemblyInfo.cs\"), " + "so the file is discarded with no message."); // The pairing, stated as the thing that is actually wrong. Assert.That(selectedByGit && !acceptedByFilter, Is.True, - "ITEM 34b (register). Both halves are individually tested and individually " - + "defensible; they disagree. A pull request changing only a file of this shape " - + "produces a green project-compliance check that examined nothing, which is " - + "item 3 reached by a route no single-layer test can see."); + "Both halves are individually tested and individually defensible; they " + + "disagree. A pull request changing only a file of this shape produces a green " + + "project-compliance check that examined nothing, reached by a route no " + + "single-layer test can see."); }); }