From a1f9d40ca29a066c446cceaf4bb1adb8f351f541 Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Sun, 25 Oct 2026 16:46:32 +0000 Subject: [PATCH] feat(ci): report Confidence as a warning for unit-test dataset fixtures The Confidence rule errors on any dataset whose source information leaves the field Undefined. That is right for a dataset a repository distributes and wrong for a fixture that only drives its own tests, where blocking a merge on it costs more than it buys. A dataset under a .ci directory now reports the finding as a warning; everything else, including a dataset that does not deserialise, is unchanged. The demotion sits in the runner rather than in IsValidDataset because the engine has no concept of a unit-test fixture. It runs before the merge because Modify.Merge combines only the two top-level statuses, so demoting the annotation alone would leave the check red with nothing to account for it. Scoped to any .ci descendant rather than .ci/Datasets alone, because the repository convention puts test projects under .ci/unit-tests/ and a fixture beside one is test data by the same argument. Measured across 340 clones and 1694 selector-matching dataset files: the two spellings pick the same 865 files in the same 15 repositories, so this closes a class without changing an outcome. A run that demoted anything says so, for the same reason a self-skipping job names its pathspec: a green that examined nothing and a green that downgraded fifty-five findings should not look alike. 48 tests added. The demotion keys on a rule name owned by Test_Toolkit, which is the one fragility in the design, and it fails safe: a rename upstream stops the match, every fixture returns to Error and the check gets stricter rather than quieter. The pin test catches the rename on the next pull request here that touches tools/, which is when this suite runs, rather than when Test_Toolkit changes. --- .../DatasetComplianceRunner.cs | 23 ++ .../Compliance.Shared/DatasetSeverity.cs | 132 +++++++++ .../Shared/Compliance.Shared/FileFilter.cs | 41 +++ .../Compliance.Tests/Compliance.Tests.csproj | 5 + .../DatasetComplianceRunnerE2ETests.cs | 267 ++++++++++++++++++ .../Unit/DatasetSeverityTests.cs | 204 +++++++++++++ .../Unit/FileFilterTests.cs | 45 +++ 7 files changed, 717 insertions(+) create mode 100644 tools/ComplianceRunner/src/Shared/Compliance.Shared/DatasetSeverity.cs create mode 100644 tools/ComplianceRunner/tests/Compliance.Tests/Unit/DatasetSeverityTests.cs diff --git a/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs b/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs index d781e6f..79c15b1 100644 --- a/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs +++ b/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs @@ -38,6 +38,7 @@ 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); + var demotedTotal = 0; foreach (var file in files) { @@ -75,6 +76,18 @@ static int Main(string[] args) continue; } + // The Confidence rule errors on a dataset that leaves the field Undefined, which + // is right for a dataset the repository ships and wrong for a fixture driving + // its own tests. Demoted here rather than in the engine, and before the merge + // rather than at the annotation, because Modify.Merge combines only the two + // top-level statuses and the exit code reads the merged one. + if (FileFilter.IsUnitTestFixture(file)) + { + resultForThisFile = DatasetSeverity.DemoteToWarning( + resultForThisFile, DatasetSeverity.ConfidenceRule, out int demotedHere); + demotedTotal += demotedHere; + } + if (verbose) Console.WriteLine($" Result Status: {resultForThisFile.Status}"); accounting.CountExamined(); @@ -121,6 +134,16 @@ static int Main(string[] args) } } + // Guarded on the format for the same reason OutputEmitter guards the coverage line: + // json and sarif put a payload on stdout and nothing may precede it. + if (demotedTotal > 0 && (outputFormat == "console" || outputFormat == "github")) + { + string notice = DatasetSeverity.DemotionNotice(demotedTotal); + Console.WriteLine(outputFormat == "github" + ? $"::notice title=Dataset confidence::{notice}" + : $"NOTICE: {notice}"); + } + 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/"), diff --git a/tools/ComplianceRunner/src/Shared/Compliance.Shared/DatasetSeverity.cs b/tools/ComplianceRunner/src/Shared/Compliance.Shared/DatasetSeverity.cs new file mode 100644 index 0000000..2a3f582 --- /dev/null +++ b/tools/ComplianceRunner/src/Shared/Compliance.Shared/DatasetSeverity.cs @@ -0,0 +1,132 @@ +using System; +using System.Collections.Generic; +using BH.Engine.Base; // ShallowClone +using BH.oM.Test; // TestStatus +using BH.oM.Test.Results; // TestResult, ITestInformation +using Error = BH.oM.Test.CodeCompliance.Error; + +/// +/// Reports one dataset rule at a lower severity for one kind of file, without changing what the +/// rule looks for or whether it runs. +/// +/// The Confidence rule errors on any dataset whose source information leaves Confidence +/// Undefined. That is the right answer for a dataset a repository distributes and the wrong one +/// for a fixture that exists to drive the repository's own tests, where the field says nothing +/// useful and blocking a merge on it costs more than it buys. The field is still expected on +/// both, which is why this demotes the finding rather than suppressing it or dropping the file. +/// +/// It lives here and not in the compliance engine deliberately. The engine judges a dataset; it +/// has no concept of a unit-test fixture and should not acquire one for this. The convention +/// that a .ci directory holds test data belongs to CI, so the severity decision belongs to CI too. +/// +public static class DatasetSeverity +{ + /// + /// The engine's name for the Confidence finding, as it appears on the Error and therefore on + /// Annotation.RuleName. + /// + /// + /// This is a string owned by another repository, which is the one fragile part of this. The + /// name is still the most stable handle available: matching on message text breaks on the + /// first reword, and matching on position in the information list breaks on the first + /// reorder. The guard is a test that runs the real engine over a dataset with Confidence + /// unset and asserts this exact value comes back, so a rename upstream fails a build here + /// instead of silently restoring the error for every fixture. + /// + public const string ConfidenceRule = "Dataset confidence error"; + + /// + /// Returns a result in which every Error carrying is reported as + /// a Warning, with the top-level status recomputed from what is left. Returns the input + /// unchanged, by reference, when nothing matches. + /// + /// How many findings were demoted, for the caller to report. + /// + /// The top-level status has to be recomputed, not just the finding's. Modify.Merge combines + /// the two top-level statuses and never looks at Information, and the runner's exit code + /// reads the merged top-level status. Demoting only the finding would leave the check red + /// with no error annotation to account for it, which is worse than leaving it alone. + /// + /// Recomputed as the most severe status remaining rather than set to Warning, so a fixture + /// that also fails to deserialise stays an Error. Those two cannot co-occur today, because + /// IsValidDataset returns early on a deserialisation failure, but that is the engine's + /// current shape rather than a guarantee, and assuming it here would be assuming it twice. + /// + /// The input is cloned rather than mutated. Nothing in the runner reads the original + /// afterwards, so this buys nothing today; it costs one allocation per fixture and means a + /// caller that does read it later is not surprised. + /// + public static TestResult DemoteToWarning(TestResult result, string ruleName, out int demoted) + { + demoted = 0; + + // Not null-tolerant on the result itself. The only caller returns early on a null + // result and counts it, so accepting one here would hide that path rather than guard + // it. A null Information is different: the runner defends against it two lines later + // and so does this. + if (result.Information == null) + return result; + + var rewritten = new List(result.Information.Count); + + foreach (var information in result.Information) + { + if (information is Error error + && error.Status == TestStatus.Error + && string.Equals(error.Name, ruleName, StringComparison.Ordinal)) + { + var demotedError = error.ShallowClone(); + demotedError.Status = TestStatus.Warning; + rewritten.Add(demotedError); + demoted++; + } + else + { + rewritten.Add(information); + } + } + + if (demoted == 0) + return result; + + var updated = result.ShallowClone(); + updated.Information = rewritten; + updated.Status = MostSevere(rewritten); + return updated; + } + + /// The most severe status across a set of findings, Pass when there are none. + private static TestStatus MostSevere(IEnumerable information) + { + var worst = TestStatus.Pass; + + foreach (var entry in information) + { + if (entry.Status == TestStatus.Error) + return TestStatus.Error; + + if (entry.Status == TestStatus.Warning) + worst = TestStatus.Warning; + } + + return worst; + } + + /// + /// The line reported when findings were demoted, so a green check states that it downgraded + /// something rather than implying there was nothing to downgrade. + /// + /// + /// Same argument as the self-skip summary line: a check that goes green for a reason the + /// reader cannot see is indistinguishable from one that found nothing, and this change makes + /// two repositories go green at once. + /// + /// The scope named here must match FileFilter.IsUnitTestFixture. They were allowed to + /// drift apart once, when the predicate broadened from the pair .ci/Datasets to any .ci + /// descendant and this string did not, which left the log describing a narrower rule than the + /// one that ran. A test asserts the wording for that reason. + /// + public static string DemotionNotice(int demoted) => + $"{demoted} Confidence finding(s) in unit-test fixtures under a .ci directory reported as " + + "warnings rather than errors. The field is still expected on those datasets."; +} diff --git a/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileFilter.cs b/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileFilter.cs index 4b7ddab..a1dff8a 100644 --- a/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileFilter.cs +++ b/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileFilter.cs @@ -89,6 +89,47 @@ private static bool IsUnderDatasetsDirectory(string file) return false; } + /// + /// True when a file sits anywhere under a ".ci" directory, case-insensitive, which is where + /// a repository keeps the inputs to its own tests rather than anything it ships. + /// + /// + /// Separates a dataset a repository ships from one that exists only to drive its own tests. + /// The engine has no concept of that difference and should not: it is a convention about + /// where CI puts things, so it is decided here rather than in the compliance rule. + /// + /// Anchored on a whole segment, not on a substring. A substring test matches a directory + /// named My.ci or .cix, which is the same fault IsUnderDatasetsDirectory above was + /// changed to avoid. + /// + /// Not root-anchored, so a nested SomeProject/.ci counts. That directory is a .ci directory + /// wherever it sits, and the answer must not change because a caller passed an absolute path + /// rather than a repository-relative one. + /// + /// Any .ci descendant rather than .ci/Datasets specifically, which is the broader of the two + /// readings and matches what IsRelevantFile already does for the project check. The narrower + /// one left a gap: the repository convention puts test projects under .ci/unit-tests/, and a + /// fixture under one of those would have been examined and not demoted. Both spellings give + /// the same answer on the fleet as it stands, so this closes a class rather than changing an + /// outcome. + /// + public static bool IsUnitTestFixture(string file) + { + var normalized = file.Replace("\\", "/"); + + int lastSlash = normalized.LastIndexOf('/'); + if (lastSlash < 0) + return false; + + foreach (var segment in normalized.Substring(0, lastSlash).Split('/')) + { + if (segment.Equals(".ci", StringComparison.OrdinalIgnoreCase)) + return true; + } + + return false; + } + /// /// True for a versioning upgrade map, named Versioning_<digits>.json. /// diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Compliance.Tests.csproj b/tools/ComplianceRunner/tests/Compliance.Tests/Compliance.Tests.csproj index a5bd949..ce28087 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Compliance.Tests.csproj +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Compliance.Tests.csproj @@ -32,6 +32,11 @@ Unit/OutputEmitterTests.cs OutputEmitter.Write takes a BH.oM.Test.TestStatus. Unit/AnnotationConvertTests.cs Constructs BH.oM.Test.CodeCompliance.Error and exercises the ITestInformation conversion path. + Unit/DatasetSeverityTests.cs DatasetSeverity rewrites a BH.oM.Test.Results.TestResult + and reads BH.oM.Test.CodeCompliance.Error, so neither it + nor its tests can be linked into the hermetic project. + Its path predicate is BHoM-free and does live there, in + FileFilter, which is the reason the two are split apart. Integration/* Invoke the built runner executables against real engines. Note the boundary is NOT unit-versus-integration: three of the six original unit test diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs index a445f2f..0835783 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs @@ -246,4 +246,271 @@ public void UnanalysableFileAlone_StillFailsTheRun() try { Directory.Delete(dir, recursive: true); } catch { /* temp dir, best effort */ } } } + + // ── Confidence severity by dataset kind ────────────────────────────────── + + /// + /// Writes one dataset under and returns it with the + /// root to clean up. The content is the same every time, so the only variable across these + /// tests is where the file sits. + /// + private static (string File, string Root) WriteDataset(string relativeDirectory, string name, string json) + { + string root = Path.Combine(Path.GetTempPath(), "bhom-ci-" + Guid.NewGuid().ToString("N")); + string directory = Path.Combine(root, relativeDirectory.Replace('/', Path.DirectorySeparatorChar)); + Directory.CreateDirectory(directory); + + string file = Path.Combine(directory, name); + File.WriteAllText(file, json); + return (file, root); + } + + /// A dataset that deserialises and leaves Confidence unset, which is the rule under test. + private const string ConfidenceUnsetDataset = + "{ \"_t\" : \"BH.oM.Data.Library.Dataset\", \"Name\" : \"Probe\", " + + "\"SourceInformation\" : { \"_t\" : \"BH.oM.Data.Library.Source\", " + + "\"Title\" : \"Probe title\", \"Author\" : \"Probe author\", \"Confidence\" : \"Undefined\" }, " + + "\"Data\" : [] }"; + + [Test] + [Category("RequiresBHoM")] + [Description("The same dataset blocks under Datasets/ and only warns under .ci/Datasets/. One behaviour, both halves.")] + [TestCase("Datasets", 1, "failure", TestName = "ConfidenceUnset_InAProductionDataset_FailsTheRun")] + [TestCase(".ci/Datasets", 0, "warning", TestName = "ConfidenceUnset_InAUnitTestFixture_OnlyWarns")] + public void ConfidenceSeverity_DependsOnWhereTheDatasetSits( + string directory, int expectedExitCode, string expectedLevel) + { + var (file, root) = WriteDataset(directory, "Conf.json", ConfidenceUnsetDataset); + try + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", "--output", "json", file); + var payload = JsonDocument.Parse(stdout).RootElement; + + var confidence = payload.GetProperty("annotations").EnumerateArray() + .Single(a => a.GetProperty("ruleName").GetString() == "Dataset confidence error"); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(expectedExitCode)); + Assert.That(confidence.GetProperty("level").GetString(), Is.EqualTo(expectedLevel)); + Assert.That(payload.GetProperty("coverage").GetProperty("examined").GetInt32(), Is.EqualTo(1), + "the demotion must not change what is selected or examined"); + }); + } + finally + { + try { Directory.Delete(root, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + [Test] + [Category("RequiresBHoM")] + [Description("A fixture that does not deserialise still fails. Only the Confidence rule is demoted, and only its severity.")] + public void AFixtureThatDoesNotDeserialise_StillFailsTheRun() + { + var (file, root) = WriteDataset(".ci/Datasets", "Bad.json", "not a dataset"); + try + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", "--output", "json", file); + var payload = JsonDocument.Parse(stdout).RootElement; + + var deserialisation = payload.GetProperty("annotations").EnumerateArray() + .Single(a => a.GetProperty("ruleName").GetString() == "Dataset deserialisation error"); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(1)); + Assert.That(payload.GetProperty("status").GetString(), Is.EqualTo("Error")); + Assert.That(deserialisation.GetProperty("level").GetString(), Is.EqualTo("failure")); + }); + } + finally + { + try { Directory.Delete(root, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + [Test] + [Category("RequiresBHoM")] + [Description("A fixture's missing title is still a warning. The other source rules are untouched by the split.")] + public void AFixturesOtherSourceWarnings_AreUnchanged() + { + const string noTitle = + "{ \"_t\" : \"BH.oM.Data.Library.Dataset\", \"Name\" : \"Probe\", " + + "\"SourceInformation\" : { \"_t\" : \"BH.oM.Data.Library.Source\", " + + "\"Author\" : \"a\", \"Confidence\" : \"High\" }, \"Data\" : [] }"; + + var (file, root) = WriteDataset(".ci/Datasets", "NoTitle.json", noTitle); + try + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", "--output", "json", file); + var payload = JsonDocument.Parse(stdout).RootElement; + + var title = payload.GetProperty("annotations").EnumerateArray() + .Single(a => a.GetProperty("ruleName").GetString() == "Dataset source title error"); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(title.GetProperty("level").GetString(), Is.EqualTo("warning")); + }); + } + finally + { + try { Directory.Delete(root, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + [Test] + [Category("RequiresBHoM")] + [Description("A demoted finding and a real one in the same run: the run is red and the demotion did not swallow either.")] + public void ADemotedFixtureAlongsideAFailingOne_StillFailsTheRun() + { + string root = Path.Combine(Path.GetTempPath(), "bhom-ci-" + Guid.NewGuid().ToString("N")); + string fixtures = Path.Combine(root, ".ci", "Datasets"); + Directory.CreateDirectory(fixtures); + + string demoted = Path.Combine(fixtures, "Conf.json"); + File.WriteAllText(demoted, ConfidenceUnsetDataset); + string broken = Path.Combine(fixtures, "Bad.json"); + File.WriteAllText(broken, "not a dataset"); + + try + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", "--output", "json", demoted, broken); + var payload = JsonDocument.Parse(stdout).RootElement; + + var levels = payload.GetProperty("annotations").EnumerateArray() + .Where(a => a.GetProperty("ruleName").GetString()!.Length > 0) + .ToDictionary(a => a.GetProperty("ruleName").GetString()!, + a => a.GetProperty("level").GetString()); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(1)); + Assert.That(levels["Dataset confidence error"], Is.EqualTo("warning")); + Assert.That(levels["Dataset deserialisation error"], Is.EqualTo("failure")); + }); + } + finally + { + try { Directory.Delete(root, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + [Test] + [Category("RequiresBHoM")] + [Description("A green that demoted something says so, rather than looking like a run that found nothing.")] + public void ADemotedRun_ReportsWhatItDowngraded() + { + var (file, root) = WriteDataset(".ci/Datasets", "Conf.json", ConfidenceUnsetDataset); + try + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", "--output", "github", file); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(stdout, Does.Contain("::notice title=Dataset confidence::1 Confidence finding")); + Assert.That(stdout, Does.Contain("The field is still expected on those datasets.")); + Assert.That(stdout, Does.Not.Contain("under .ci/Datasets"), + "the notice must describe the scope the predicate applies, which is any .ci descendant"); + }); + } + finally + { + try { Directory.Delete(root, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + [Test] + [Category("RequiresBHoM")] + [Description("json and sarif carry a payload on stdout, so the demotion notice must not precede it.")] + [TestCase("json")] + [TestCase("sarif")] + public void TheDemotionNotice_DoesNotContaminateAMachineReadablePayload(string format) + { + var (file, root) = WriteDataset(".ci/Datasets", "Conf.json", ConfidenceUnsetDataset); + try + { + var (_, stdout) = RunnerFixture.Run("DatasetComplianceRunner", "--output", format, file); + + Assert.Multiple(() => + { + Assert.That(stdout, Does.Not.Contain("Confidence finding(s) in unit-test fixtures")); + Assert.That(() => JsonDocument.Parse(stdout), Throws.Nothing, + "stdout must be the payload and nothing else, which is the property at risk here"); + }); + } + finally + { + try { Directory.Delete(root, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + [Test] + [Category("RequiresBHoM")] + [Description("SARIF carries the demoted finding at warning too. The level is mapped in a second place.")] + public void ADemotedFinding_IsAWarningInSarifAsWell() + { + var (file, root) = WriteDataset(".ci/Datasets", "Conf.json", ConfidenceUnsetDataset); + try + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", "--output", "sarif", file); + var results = JsonDocument.Parse(stdout).RootElement + .GetProperty("runs")[0].GetProperty("results"); + + var confidence = results.EnumerateArray() + .Single(r => r.GetProperty("ruleId").GetString()!.Contains("Dataset confidence error")); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(confidence.GetProperty("level").GetString(), Is.EqualTo("warning"), + "SarifBuilder maps the level separately from the GitHub emitter, so it needs its own assertion"); + }); + } + finally + { + try { Directory.Delete(root, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + /// + /// Pins the rule name the demotion keys on against the engine that actually produces it. + /// + /// DatasetSeverity matches a string owned by Test_Toolkit. If that name is reworded upstream + /// the demotion silently stops matching, every fixture goes back to blocking, and nothing + /// points at the cause. This fails the build here instead. It deliberately uses the literal + /// rather than the constant on one side, so a change to the constant cannot make it pass. + /// + [Test] + [Category("RequiresBHoM")] + [Description("The engine still calls the Confidence finding what DatasetSeverity expects.")] + public void TheEngineStillNamesTheConfidenceRuleWhatTheDemotionExpects() + { + var (file, root) = WriteDataset("Datasets", "Conf.json", ConfidenceUnsetDataset); + try + { + var (_, stdout) = RunnerFixture.Run("DatasetComplianceRunner", "--output", "json", file); + var payload = JsonDocument.Parse(stdout).RootElement; + + var ruleNames = payload.GetProperty("annotations").EnumerateArray() + .Select(a => a.GetProperty("ruleName").GetString()) + .ToList(); + + Assert.Multiple(() => + { + Assert.That(ruleNames, Does.Contain("Dataset confidence error"), + "the engine's name for this finding has changed; DatasetSeverity.ConfidenceRule no longer matches it"); + Assert.That(DatasetSeverity.ConfidenceRule, Is.EqualTo("Dataset confidence error"), + "the constant has been edited away from the name the engine emits"); + }); + } + finally + { + try { Directory.Delete(root, recursive: true); } catch { /* temp dir, best effort */ } + } + } + } diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Unit/DatasetSeverityTests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Unit/DatasetSeverityTests.cs new file mode 100644 index 0000000..a6ea375 --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Unit/DatasetSeverityTests.cs @@ -0,0 +1,204 @@ +using NUnit.Framework; +using System.Collections.Generic; +using BH.oM.Test; +using BH.oM.Test.Results; +using Error = BH.oM.Test.CodeCompliance.Error; +using DatasetSource = BH.oM.Test.CodeCompliance.DatasetSource; + +/// +/// The demotion itself, in isolation from the engine and the runner. +/// +/// These construct the TestResult shape IsValidDataset returns rather than calling it, so a +/// failure here points at the demotion and not at the rule. The engine-backed half, including +/// the pin on the rule name, is in DatasetComplianceRunnerE2ETests. +/// +[TestFixture] +public class DatasetSeverityTests +{ + private static Error Finding(string name, TestStatus status) => + new Error { Name = name, Status = status, Message = name }; + + private static TestResult Result(TestStatus status, params ITestInformation[] information) => + new TestResult { Status = status, Information = new List(information) }; + + [Test] + [Description("The rule being demoted drops to Warning, and so does the top-level status the exit code reads.")] + public void ConfidenceError_BecomesAWarning_AndTheResultStopsBeingAnError() + { + var result = Result(TestStatus.Error, Finding(DatasetSeverity.ConfidenceRule, TestStatus.Error)); + + var demotedResult = DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out int demoted); + + Assert.Multiple(() => + { + Assert.That(demoted, Is.EqualTo(1)); + Assert.That(demotedResult.Status, Is.EqualTo(TestStatus.Warning), + "Modify.Merge reads only the top-level status, so leaving it Error would keep the check red with no error annotation"); + Assert.That(demotedResult.Information[0].Status, Is.EqualTo(TestStatus.Warning)); + }); + } + + [Test] + [Description("A deserialisation failure is untouched. This is the case the split must not weaken.")] + public void ADifferentRule_IsLeftAlone() + { + var result = Result(TestStatus.Error, Finding("Dataset deserialisation error", TestStatus.Error)); + + var demotedResult = DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out int demoted); + + Assert.Multiple(() => + { + Assert.That(demoted, Is.EqualTo(0)); + Assert.That(demotedResult.Status, Is.EqualTo(TestStatus.Error)); + Assert.That(demotedResult, Is.SameAs(result), "nothing matched, so nothing should have been rebuilt"); + }); + } + + [Test] + [Description("A fixture carrying both keeps its Error. The status is recomputed from what is left, not set to Warning.")] + public void ConfidenceAlongsideAnotherError_StaysAnError() + { + var result = Result(TestStatus.Error, + Finding(DatasetSeverity.ConfidenceRule, TestStatus.Error), + Finding("Dataset deserialisation error", TestStatus.Error)); + + var demotedResult = DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out int demoted); + + Assert.Multiple(() => + { + Assert.That(demoted, Is.EqualTo(1)); + Assert.That(demotedResult.Status, Is.EqualTo(TestStatus.Error), + "the surviving error must still fail the run"); + Assert.That(demotedResult.Information[0].Status, Is.EqualTo(TestStatus.Warning)); + Assert.That(demotedResult.Information[1].Status, Is.EqualTo(TestStatus.Error)); + }); + } + + [Test] + [Description("The Pass-level DatasetSource notice is carried through, not promoted or dropped.")] + public void TheProvenanceNotice_SurvivesUntouched() + { + var notice = new DatasetSource { Status = TestStatus.Pass, Message = "Confidence: Undefined" }; + var result = Result(TestStatus.Error, notice, Finding(DatasetSeverity.ConfidenceRule, TestStatus.Error)); + + var demotedResult = DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out _); + + Assert.Multiple(() => + { + Assert.That(demotedResult.Information, Has.Count.EqualTo(2)); + Assert.That(demotedResult.Information[0], Is.SameAs(notice)); + Assert.That(demotedResult.Information[0].Status, Is.EqualTo(TestStatus.Pass)); + Assert.That(demotedResult.Status, Is.EqualTo(TestStatus.Warning)); + }); + } + + [Test] + [Description("A finding already at Warning is not counted as demoted, so the reported figure stays honest.")] + public void AConfidenceFindingAlreadyAtWarning_IsNotCountedAgain() + { + var result = Result(TestStatus.Warning, Finding(DatasetSeverity.ConfidenceRule, TestStatus.Warning)); + + DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out int demoted); + + Assert.That(demoted, Is.EqualTo(0)); + } + + [Test] + [Description("Matching is exact. A near-miss on the rule name must not silently demote something else.")] + [TestCase("dataset confidence error")] + [TestCase("Dataset Confidence Error")] + [TestCase("Dataset confidence")] + [TestCase("Dataset confidence error ")] + public void TheRuleNameIsMatchedExactly(string name) + { + var result = Result(TestStatus.Error, Finding(name, TestStatus.Error)); + + DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out int demoted); + + Assert.That(demoted, Is.EqualTo(0)); + } + + [Test] + [Description("The caller's result is not mutated, so a later reader sees what the engine returned.")] + public void TheInputIsNotMutated() + { + var original = Finding(DatasetSeverity.ConfidenceRule, TestStatus.Error); + var result = Result(TestStatus.Error, original); + + DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out _); + + Assert.Multiple(() => + { + Assert.That(result.Status, Is.EqualTo(TestStatus.Error)); + Assert.That(original.Status, Is.EqualTo(TestStatus.Error)); + }); + } + + [Test] + [Description("Every finding demoted leaves a Warning, not a Pass, because the findings are still there.")] + public void TwoConfidenceErrors_BothDemoted_LeaveAWarning() + { + var result = Result(TestStatus.Error, + Finding(DatasetSeverity.ConfidenceRule, TestStatus.Error), + Finding(DatasetSeverity.ConfidenceRule, TestStatus.Error)); + + var demotedResult = DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out int demoted); + + Assert.Multiple(() => + { + Assert.That(demoted, Is.EqualTo(2)); + Assert.That(demotedResult.Status, Is.EqualTo(TestStatus.Warning)); + }); + } + + [Test] + [Description("A null Information does not throw. The runner defends against it, and so does this.")] + public void ANullInformation_IsReturnedUnchanged() + { + var result = new TestResult { Status = TestStatus.Pass, Information = null }; + + var demotedResult = DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out int demoted); + + Assert.Multiple(() => + { + Assert.That(demotedResult, Is.SameAs(result)); + Assert.That(demoted, Is.EqualTo(0)); + }); + } + + [Test] + [Description("An empty Information does not throw and reports nothing demoted.")] + public void AnEmptyInformation_IsReturnedUnchanged() + { + var result = Result(TestStatus.Pass); + + var demotedResult = DatasetSeverity.DemoteToWarning(result, DatasetSeverity.ConfidenceRule, out int demoted); + + Assert.Multiple(() => + { + Assert.That(demotedResult, Is.SameAs(result)); + Assert.That(demoted, Is.EqualTo(0)); + }); + } + + [Test] + [Description("The notice names the count, so a green check states what it downgraded.")] + public void TheDemotionNotice_CarriesTheCount() + { + Assert.That(DatasetSeverity.DemotionNotice(55), Does.StartWith("55 Confidence finding")); + } + + [Test] + [Description("The notice describes the scope the predicate actually applies. These drifted apart once.")] + public void TheDemotionNotice_NamesTheSameScopeAsThePredicate() + { + string notice = DatasetSeverity.DemotionNotice(1); + + Assert.Multiple(() => + { + Assert.That(notice, Does.Contain(".ci")); + Assert.That(notice, Does.Not.Contain(".ci/Datasets"), + "the predicate matches any .ci descendant, so the notice must not claim the narrower pair"); + }); + } +} diff --git a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileFilterTests.cs b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileFilterTests.cs index 1b154d0..781ff4f 100644 --- a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileFilterTests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileFilterTests.cs @@ -109,4 +109,49 @@ public bool IsDatasetFile(string file) public bool IsVersioningUpgradeMap(string file) => FileFilter.IsVersioningUpgradeMap(file); } + + [TestFixture] + public class IsUnitTestFixtureTests + { + // The shape that exists in practice. + [TestCase(".ci/Datasets/Compute/Split.json", ExpectedResult = true)] + [TestCase(".ci/Datasets/foo.json", ExpectedResult = true)] + [TestCase(@".ci\Datasets\foo.json", ExpectedResult = true)] // backslash separators + [TestCase(".CI/Datasets/foo.json", ExpectedResult = true)] // segment case-insensitive + [TestCase(".Ci/DataSets/foo.json", ExpectedResult = true)] + + // Not root-anchored. A nested .ci directory is still a .ci directory, and the answer + // must not change because a caller passed an absolute path. + [TestCase("SomeProject/.ci/Datasets/foo.json", ExpectedResult = true)] + [TestCase("a/b/c/.ci/Datasets/deep/nested/x.json", ExpectedResult = true)] + [TestCase("C:/work/repo/.ci/Datasets/foo.json", ExpectedResult = true)] + + // Any .ci descendant, not only .ci/Datasets. The repository convention puts test + // projects under .ci/unit-tests/, and a fixture dropped beside one is test data by the + // same argument. The narrower pair test left exactly this gap. + [TestCase(".ci/unit-tests/Datasets/foo.json", ExpectedResult = true)] + [TestCase(".ci/unit-tests/Foo_Tests/FooTests/Datasets/x.json", ExpectedResult = true)] + [TestCase(".ci/DatasetsOld/foo.json", ExpectedResult = true)] + [TestCase(".ci/foo.json", ExpectedResult = true)] + [TestCase("Datasets/.ci/foo.json", ExpectedResult = true)] + + // A production dataset, which is the case the whole split exists to keep blocking. + [TestCase("Datasets/Structure/EPD.json", ExpectedResult = false)] + [TestCase("DataSets/Structure/DesignData/x.json", ExpectedResult = false)] + [TestCase("Revit_Engine/Datasets/UK/Regional.json", ExpectedResult = false)] + + // The anchoring, which is the reason this is not a substring test. Each of these + // contains ".ci" in the path and must not match, because no whole segment is ".ci". + [TestCase("My.ci/Datasets/foo.json", ExpectedResult = false)] + [TestCase("x.ci/Datasets/foo.json", ExpectedResult = false)] + [TestCase(".cix/Datasets/foo.json", ExpectedResult = false)] + [TestCase("ci/Datasets/foo.json", ExpectedResult = false)] // no leading dot + + // Only the directory portion is read, so a file named .ci does not take itself out. + [TestCase("a/b/.ci", ExpectedResult = false)] + [TestCase("Datasets.json", ExpectedResult = false)] + [TestCase("foo.json", ExpectedResult = false)] // no directory at all + public bool IsUnitTestFixture(string file) + => FileFilter.IsUnitTestFixture(file); + } }