From 5dcf46f4e55d5c5fc28a45f7fe5535d5837788eb Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Wed, 26 Aug 2026 10:49:35 +0100 Subject: [PATCH 1/2] fix(dataset-tests): route DatasetTestRunner through the shared output emitter The runner carried its own copy of the emitter. The copy had drifted: no title= or location prefix on the message, every non-failure level mapped to warning, and newlines flattened to spaces. The first of those is why a failing fixture named nothing in the job log even though its annotation anchored correctly. Routing through OutputEmitter corrects all three and, because Write already takes an optional FileAccounting, adds the coverage denominator the other runners report. Accounting is wired at the three exits the loop can take. Adds the project to Platform.slnx so a solution build compiles it and the suite can invoke it; it was previously built only by prepare-runner. Verdict is unchanged: the exit code and merged status are untouched. --- tools/ComplianceRunner/Platform.slnx | 1 + .../DatasetTestRunner/DatasetTestRunner.cs | 97 ++++++------------- .../Integration/DatasetTestRunnerE2ETests.cs | 95 ++++++++++++++++++ 3 files changed, 126 insertions(+), 67 deletions(-) create mode 100644 tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetTestRunnerE2ETests.cs diff --git a/tools/ComplianceRunner/Platform.slnx b/tools/ComplianceRunner/Platform.slnx index 2470a6a..bc16749 100644 --- a/tools/ComplianceRunner/Platform.slnx +++ b/tools/ComplianceRunner/Platform.slnx @@ -1,6 +1,7 @@ + diff --git a/tools/ComplianceRunner/src/DatasetTestRunner/DatasetTestRunner.cs b/tools/ComplianceRunner/src/DatasetTestRunner/DatasetTestRunner.cs index 0c8cd24..37360a9 100644 --- a/tools/ComplianceRunner/src/DatasetTestRunner/DatasetTestRunner.cs +++ b/tools/ComplianceRunner/src/DatasetTestRunner/DatasetTestRunner.cs @@ -2,7 +2,6 @@ using System.Collections.Generic; using System.IO; using System.Linq; -using System.Text.Json; using BH.Engine.Test; // Modify.Merge using BH.Engine.UnitTest; // CheckTest extension method using BH.oM.Test; // TestStatus @@ -37,12 +36,18 @@ static int Main(string[] args) var mergedResult = new TestResult() { Status = TestStatus.Pass, Information = new List() }; var allAnnotations = new List(); + // Three of FileAccounting's four exits. This runner applies no relevance filter — the + // action hands it the fixtures it already selected — so NotRelevant stays zero and the + // denominator reads as examined / handed in. + var accounting = new FileAccounting(files.Count); + foreach (var file in files) { if (verbose) Console.WriteLine($"\n=== Running: {file} ==="); if (!File.Exists(file)) { + accounting.CountNotOnDisk(); Console.WriteLine($" [SKIP] File not found: {file}"); continue; } @@ -51,12 +56,14 @@ static int Main(string[] args) if (result == null) { + accounting.CountNoResult(); Console.WriteLine($" [SKIP] No result returned for: {file}"); continue; } if (verbose) Console.WriteLine($" Result Status: {result.Status}"); + accounting.CountExamined(); mergedResult = mergedResult.Merge(result); var information = (result.Information ?? Enumerable.Empty()) @@ -86,73 +93,29 @@ static int Main(string[] args) } const string checkType = "dataset-tests"; - CheckMetadata.GetOutput(checkType, mergedResult.Status, - out string title, out string summary, out string text); - if (verbose) - { - if (mergedResult.Status == TestStatus.Error || mergedResult.Status == TestStatus.Warning) - { - Console.WriteLine("\n--- Check output ---"); - Console.WriteLine($"Title: {title}"); - Console.WriteLine($"Summary: {summary}"); - if (!string.IsNullOrEmpty(text)) Console.WriteLine($"Text: {text}"); - } - Console.WriteLine("\n==============================="); - Console.WriteLine($"FINAL RESULT: {mergedResult.Status} (Annotations: {allAnnotations.Count})"); - Console.WriteLine("==============================="); - } - - if (outputFormat == "github") - { - foreach (var a in allAnnotations) - { - var path = PathHelper.NormaliseAnnotationPath(a.FilePath); - var level = a.Level == "failure" ? "error" : "warning"; - var msg = a.Message.Replace("\r", "").Replace("\n", " "); - var col = a.ColumnStart > 0 ? $",col={a.ColumnStart}" : ""; - Console.WriteLine($"::{level} file={path},line={a.LineStart}{col}::{msg}"); - } - } - else if (outputFormat == "json") - { - var payload = new Dictionary - { - ["status"] = mergedResult.Status.ToString(), - ["checkType"] = checkType, - ["title"] = title, - ["summary"] = summary, - ["text"] = text, - ["annotationCount"] = allAnnotations.Count, - ["annotations"] = allAnnotations.Select(a => new Dictionary - { - ["path"] = a.FilePath, - ["lineStart"] = a.LineStart, - ["lineEnd"] = a.LineEnd, - ["columnStart"] = a.ColumnStart, - ["columnEnd"] = a.ColumnEnd, - ["level"] = a.Level, - ["message"] = a.Message, - ["ruleName"] = a.RuleName, - ["documentationUrl"] = a.DocumentationUrl, - ["bhomGuid"] = a.BHoMGuid, - ["utcTime"] = a.UTCTime.ToString("o") - }).ToList() - }; - Console.WriteLine(JsonSerializer.Serialize(payload, new JsonSerializerOptions { WriteIndented = false })); - } - else if (outputFormat == "sarif" || outputFormat == "sarif-file") - { - var sarif = SarifBuilder.Build(checkType, title, allAnnotations, - BH.Engine.Base.Query.DocumentationURL("DevOps/Code%20Compliance%20and%20CI/Compliance%20Checks/")); - if (outputFormat == "sarif-file" && !string.IsNullOrEmpty(sarifFilePath)) - { - File.WriteAllText(sarifFilePath, sarif); - if (verbose) Console.WriteLine($"SARIF written to {sarifFilePath}"); - } - else - Console.WriteLine(sarif); - } + // Routed through the shared emitter rather than a local copy. The copy that stood here + // had drifted from OutputEmitter in three ways, all of them silent: + // - it omitted title= and the location prefix on the message, which is the workaround + // OutputEmitter documents for GitHub stripping file= out of the rendered log line. + // Measured: the annotation anchored correctly and the log line carried no path at + // all, so a failing fixture named nothing a reader could act on. + // - it mapped every non-failure level to warning, so a notice was reported as a + // warning. + // - it flattened newlines to spaces instead of %0A, so a nested result hierarchy + // arrived as one long line. + // Passing accounting also gives this runner the coverage denominator the other four + // already report. Verdict is unchanged: the exit code below is untouched, and Write + // reports rather than decides. + OutputEmitter.Write( + outputFormat, + checkType, + mergedResult.Status, + allAnnotations, + sarifFilePath, + verbose, + BH.Engine.Base.Query.DocumentationURL("DevOps/Code%20Compliance%20and%20CI/Compliance%20Checks/"), + accounting); return mergedResult.Status == TestStatus.Error ? 1 : 0; } diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetTestRunnerE2ETests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetTestRunnerE2ETests.cs new file mode 100644 index 0000000..9c5e930 --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetTestRunnerE2ETests.cs @@ -0,0 +1,95 @@ +using NUnit.Framework; +using System.Text.Json; + +/// +/// End-to-end tests for DatasetTestRunner (dataset unit-test fixtures). +/// +/// Scope note. The emitted annotation shape — title=, the location prefix, the notice mapping, +/// the %0A escaping — is already covered by OutputEmitterTests against the shared emitter. These +/// tests deliberately do not restate it. What they cover is what is specific to this runner: +/// that it routes through that emitter at all rather than through a local copy, and that its +/// file accounting is wired at each of the three loop exits it can take. +/// +/// Almost everything here is [Category("RequiresBHoM")]. Unlike the two compliance runners, this +/// one calls LoadAllAssemblies() before it looks at its arguments, so there is no path past the +/// usage message that returns before BHoM is touched. +/// +[TestFixture] +[Category("Integration")] +public class DatasetTestRunnerE2ETests +{ + // ── Usage / bad args — no BHoM call made ────────────────────────────────── + + [Test] + public void NoArgs_ExitsWithCode1() + { + var (exitCode, _) = RunnerFixture.Run("DatasetTestRunner"); + Assert.That(exitCode, Is.EqualTo(1)); + } + + // ── Coverage denominator ────────────────────────────────────────────────── + + [Test] + [Category("RequiresBHoM")] + [Description("A path that is not on disk takes the NotOnDisk exit and is counted, not silently dropped.")] + public void MissingFile_JsonOutput_CountsTheFileAsNotOnDisk() + { + var (_, stdout) = RunnerFixture.Run("DatasetTestRunner", + "--output", "json", "no/such/fixture.json"); + + var coverage = JsonDocument.Parse(stdout).RootElement.GetProperty("coverage"); + Assert.Multiple(() => + { + Assert.That(coverage.GetProperty("handedIn").GetInt32(), Is.EqualTo(1)); + Assert.That(coverage.GetProperty("examined").GetInt32(), Is.EqualTo(0)); + Assert.That(coverage.GetProperty("notOnDisk").GetInt32(), Is.EqualTo(1)); + // No relevance filter in this runner, so this exit can never be taken. + Assert.That(coverage.GetProperty("notRelevant").GetInt32(), Is.EqualTo(0)); + }); + } + + [Test] + [Category("RequiresBHoM")] + [Description("The denominator reaches machine-readable output structurally, not by parsing stdout.")] + public void JsonOutput_ContainsCoverageKey() + { + var (_, stdout) = RunnerFixture.Run("DatasetTestRunner", + "--output", "json", "no/such/fixture.json"); + + Assert.That(JsonDocument.Parse(stdout).RootElement.TryGetProperty("coverage", out _), + "json output carries no coverage key, so the runner is not passing FileAccounting to OutputEmitter"); + } + + [Test] + [Category("RequiresBHoM")] + [Description("github output carries the coverage line and, when nothing was examined, the warning that says so.")] + public void MissingFile_GithubOutput_ReportsCoverageAndExaminedNothing() + { + var (_, stdout) = RunnerFixture.Run("DatasetTestRunner", + "--output", "github", "no/such/fixture.json"); + + Assert.Multiple(() => + { + Assert.That(stdout, Does.Contain("Coverage: 0 of 1 file(s) examined; 1 not found on disk.")); + Assert.That(stdout, Does.Contain("::warning title=Compliance coverage::")); + }); + } + + // ── Verdict is unchanged by any of the above ────────────────────────────── + + [Test] + [Category("RequiresBHoM")] + [Description("Reporting a denominator must not move the verdict. Examining nothing still exits 0 and reports Pass, exactly as before this runner reported coverage at all.")] + public void MissingFile_ExaminedNothing_StillPassesAndExitsZero() + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetTestRunner", + "--output", "json", "no/such/fixture.json"); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(JsonDocument.Parse(stdout).RootElement.GetProperty("status").GetString(), + Is.EqualTo("Pass")); + }); + } +} From 9b62fc2efd779c05108586bf182472c1d5f2ca50 Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Wed, 26 Aug 2026 10:55:57 +0100 Subject: [PATCH 2/2] fix(dataset-tests): send the [SKIP] diagnostics to stderr They were written unconditionally to stdout, so a single missing fixture made --output json and --output sarif unparseable from their first character. The shared emitter already cites this diagnostic as the reason it guards its own coverage line; routing round it stopped being sufficient once the counts became part of the json payload. stderr rather than deletion, so the diagnostic still reaches the job log everywhere it was visible before. Found by the new coverage tests. --- .../src/DatasetTestRunner/DatasetTestRunner.cs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tools/ComplianceRunner/src/DatasetTestRunner/DatasetTestRunner.cs b/tools/ComplianceRunner/src/DatasetTestRunner/DatasetTestRunner.cs index 37360a9..276eab1 100644 --- a/tools/ComplianceRunner/src/DatasetTestRunner/DatasetTestRunner.cs +++ b/tools/ComplianceRunner/src/DatasetTestRunner/DatasetTestRunner.cs @@ -45,10 +45,21 @@ static int Main(string[] args) { if (verbose) Console.WriteLine($"\n=== Running: {file} ==="); + // The two [SKIP] diagnostics go to stderr, not stdout. They were unconditional + // Console.WriteLine, which corrupts the json and sarif formats: those put a single + // payload on stdout and nothing may precede it, so one missing fixture made the + // output unparseable from its first character. Pre-existing, and OutputEmitter.cs + // already cites this exact diagnostic as the reason it guards its own coverage + // line. Routing round it was enough while coverage was console-only; it is not now + // that the counts are part of the json payload. + // + // stderr rather than deleting them: both streams reach the job log, so the + // diagnostic is preserved everywhere it was visible before, and the counts now + // carry the same fact structurally. if (!File.Exists(file)) { accounting.CountNotOnDisk(); - Console.WriteLine($" [SKIP] File not found: {file}"); + Console.Error.WriteLine($" [SKIP] File not found: {file}"); continue; } @@ -57,7 +68,7 @@ static int Main(string[] args) if (result == null) { accounting.CountNoResult(); - Console.WriteLine($" [SKIP] No result returned for: {file}"); + Console.Error.WriteLine($" [SKIP] No result returned for: {file}"); continue; }