diff --git a/.github/workflows/test-tools.yml b/.github/workflows/test-tools.yml index a80d060..c792dc7 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,59 @@ 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 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 + 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..236ca72 --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/MainAccountingTests.cs @@ -0,0 +1,130 @@ +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: 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, +/// 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 with it the only signal that a file went unexamined."); + } + + [Test] + [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", + "code", "--output", "github", "a.cs", "b.cs", "c.cs"); + + Assert.Multiple(() => + { + // 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), + "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("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, 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"), + "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 ───────────────────────────── + + [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..d27bd03 --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/PathspecFilterPairingTests.cs @@ -0,0 +1,144 @@ +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 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. 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")] +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("The pathspec selects a file the filter then discards.")] + public void PathspecAndFilter_DisagreeOnAStraddlingName() + { + 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. 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, + "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."); + }); + } + + [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; + } +}