From 11167f95bc7638d2f0cc3546781e1ba99a7e16ab Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Tue, 29 Sep 2026 14:44:04 +0100 Subject: [PATCH] fix(ci-compliance): exclude versioning upgrade maps in the runner, not per repo ci-dataset-compliance selects files by a substring test over the whole path, then asserts each one deserialises into a BH.oM.Data.Library.Dataset. Versioning upgrade maps, named Versioning_.json, are never Dataset documents, so any that the selector reaches fails. The selector reaches them when a directory supplies the "datasets" substring, which in practice is a project directory named *_Datasets. Measured: BuroHappold_Datasets#163 carries one such file and it is the single failure annotation on that check, against six real datasets read correctly. The exclusion was being added to each repository's pathspec instead. That is a general rule about a file type, so review asked for it centrally, and a per-repository pathspec cannot be the place: a repository that never receives the edit stays wrong, and nothing propagates a template change to an onboarded repository. IsDatasetFile is the layer that is correct however the file was selected, whether from a pathspec, a refreshed template, or a direct invocation. It is also the only one that arrives without a distribution wave: prepare-runner keys its cache on a content hash of tools/ComplianceRunner/src when the action is referenced by branch, which every consumer does, so the change lands on the first run after merge. The rule is anchored on the file name and gated on a digit, so Versioning_Rules.json stays in scope and a directory named Versioning_93 does not take its contents out of scope. Discriminating on content instead, by requiring a top-level _t, was rejected: it would also skip a real Dataset that had lost its _t, which is a defect this check exists to catch. This diverges from BHoMBot's DatasetCompliance, which applies the bare path test and still holds the required context. The docstring no longer claims they agree. 88 unit tests and 66 integration tests pass, 17 and 2 of them new. Five mutation checks ran against the new rule, covering the exclusion itself, the digit gate, the file-name anchor, the separator handling and the prefix anchor. Each failed tests and reverted clean. --- .../Shared/Compliance.Shared/FileFilter.cs | 47 +++++++++++++++++-- .../DatasetComplianceRunnerE2ETests.cs | 34 ++++++++++++++ .../Unit/FileFilterTests.cs | 41 ++++++++++++++++ 3 files changed, 119 insertions(+), 3 deletions(-) diff --git a/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileFilter.cs b/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileFilter.cs index 8df6ee2..3043935 100644 --- a/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileFilter.cs +++ b/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileFilter.cs @@ -30,14 +30,55 @@ public static bool IsRelevantFile(string file, string checkType) /// /// Returns true for .json files whose path contains "datasets" (case-insensitive), - /// matching BHoMBot's DatasetCompliance filter exactly: - /// x.ToLower().Contains("datasets") && x.EndsWith(".json") + /// except versioning upgrade maps, which are never Dataset documents. /// + /// + /// The path test is BHoMBot's DatasetCompliance filter, + /// x.ToLower().Contains("datasets") && x.EndsWith(".json"). The upgrade-map + /// exclusion is a deliberate divergence from it, so the two no longer agree. + /// public static bool IsDatasetFile(string file) { if (!file.EndsWith(".json", StringComparison.OrdinalIgnoreCase)) return false; - return file.Contains("datasets", StringComparison.OrdinalIgnoreCase); + if (!file.Contains("datasets", StringComparison.OrdinalIgnoreCase)) + return false; + + return !IsVersioningUpgradeMap(file); + } + + /// + /// True for a versioning upgrade map, named Versioning_<digits>.json. + /// + /// + /// These hold {"Dataset":{"ToNew":..,"ToOld":..}}, carry no _t, and can never + /// deserialise into a BH.oM.Data.Library.Dataset. They are caught by the path test above + /// only when some directory supplies the "datasets" substring, which in practice is a + /// project directory named *_Datasets. The rule lives here rather than in a per-repository + /// pathspec because a repository carrying no pathspec must still get the right answer. + /// + /// Anchored on the file name, not the path, so a directory named Versioning_93 does not + /// take every file under it out of scope. + /// + /// The digit is what separates a generated upgrade map from a hand-authored dataset that + /// happens to start with the same word: Versioning_Rules.json stays in scope. Discriminating + /// on content instead, by requiring a top-level _t, was rejected: it would also skip a real + /// Dataset document that had lost its _t, which is a defect this check exists to catch. + /// + public static bool IsVersioningUpgradeMap(string file) + { + // Both separators are split by hand rather than through Path.GetFileName, which treats + // '\' as one only on Windows. Paths arrive from git with '/' and from a caller on + // Windows with '\', and the answer must not depend on which platform is asking. + var normalized = file.Replace("\\", "/"); + int lastSlash = normalized.LastIndexOf('/'); + var name = lastSlash >= 0 ? normalized[(lastSlash + 1)..] : normalized; + + const string prefix = "Versioning_"; + if (!name.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) + return false; + + return name.Length > prefix.Length && char.IsAsciiDigit(name[prefix.Length]); } } diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs index 9646503..dfacac7 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs @@ -49,6 +49,40 @@ public void NonJsonFile_ExitsWithCode0() Assert.That(exitCode, Is.EqualTo(0)); } + [Test] + [Description("A versioning upgrade map under a *_Datasets project directory is filtered before BHoM is called, and counts as not relevant rather than as an examined file.")] + public void VersioningUpgradeMap_UnderADatasetsDirectory_IsNotExamined() + { + // The shape that fails today: the file name is the only signal, and the "datasets" + // substring comes from the project directory rather than from a data directory. + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", + "--output", "json", "BHoM_Datasets/Versioning_93.json"); + + var root = JsonDocument.Parse(stdout).RootElement; + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(root.GetProperty("status").GetString(), Is.EqualTo("Pass")); + Assert.That(root.GetProperty("annotationCount").GetInt32(), Is.EqualTo(0)); + }); + } + + [Test] + [Description("The digit gate holds end to end: Versioning_Rules.json is still selected, so it reaches the BHoM call and is reported as missing on disk rather than filtered.")] + public void VersioningRulesJson_IsStillInScope() + { + // Distinguishes "filtered out" from "examined": a filtered file never reaches the + // File.Exists branch, so the [SKIP] line is what proves this one was not filtered. + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", + "--output", "json", "BHoM_Datasets/Versioning_Rules.json"); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(0)); + Assert.That(stdout, Does.Contain("[SKIP] File not found")); + }); + } + // ── JSON output structure ───────────────────────────────────────────────── [Test] diff --git a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileFilterTests.cs b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileFilterTests.cs index 69ab273..9e47289 100644 --- a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileFilterTests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileFilterTests.cs @@ -40,7 +40,48 @@ public class IsDatasetFileTests [TestCase("datasets/foo.json", ExpectedResult = true)] // root-level [TestCase("DataSets/foo.json", ExpectedResult = true)] // root-level, mixed case [TestCase("DataSets/LCA/deep/x.json", ExpectedResult = true)] // root-level, nested + + // Versioning upgrade maps. The live case is a project directory supplying the + // "datasets" substring, which is how BuroHappold_Datasets/Versioning_93.json and + // BHoM_Datasets/Versioning_100.json get selected at all. + [TestCase("BHoM_Datasets/Versioning_93.json", ExpectedResult = false)] + [TestCase("BHoM_Datasets/Versioning_100.json", ExpectedResult = false)] + [TestCase(@"BHoM_Datasets\Versioning_93.json", ExpectedResult = false)] // backslash separators + [TestCase("Datasets/Versioning_9.json", ExpectedResult = false)] // single digit + [TestCase("a/Datasets/deep/Versioning_93.json", ExpectedResult = false)] // at depth + [TestCase("BHoM_Datasets/versioning_93.json", ExpectedResult = false)] // name case-insensitive + [TestCase("BHoM_Datasets/Versioning_93.JSON", ExpectedResult = false)] // extension case-insensitive + + // The digit gate. A hand-authored dataset starting with the same word stays in scope, + // which is the whole reason the rule is not a bare Versioning_* match. + [TestCase("Datasets/Versioning_Rules.json", ExpectedResult = true)] + [TestCase("BHoM_Datasets/Versioning_.json", ExpectedResult = true)] + [TestCase("BHoM_Datasets/Versioning.json", ExpectedResult = true)] + + // Anchored on the file name, so a directory named for a version does not take the real + // datasets under it out of scope. + [TestCase("Datasets/Versioning_93/RealDataset.json", ExpectedResult = true)] + + // The prefix has to start the name. A dataset merely containing the word is unaffected. + [TestCase("Datasets/MyVersioning_93.json", ExpectedResult = true)] + + // Pins the anchoring itself rather than the digit test. The name is contrived on + // purpose: the prefix has to appear at an offset AND a digit has to sit at the index + // the digit test reads, which is the only way the two can disagree. Without it, a + // relaxed prefix match still passes every realistic fixture above, because reading a + // fixed index lands inside the prefix whenever the prefix is not at the start. + [TestCase("Datasets/012345678901Versioning_5.json", ExpectedResult = true)] public bool IsDatasetFile(string file) => FileFilter.IsDatasetFile(file); + + // The upgrade-map rule on its own, so a failure says which of the two predicates moved. + // These paths carry no "datasets" substring, so IsDatasetFile rejects them anyway and + // could not distinguish the two. + [TestCase("Tagging_oM/Versioning_93.json", ExpectedResult = true)] + [TestCase("Tagging_oM/Versioning_Rules.json", ExpectedResult = false)] + [TestCase("Versioning_93.json", ExpectedResult = true)] // no directory at all + [TestCase("Structure_oM/Versioning_93.txt", ExpectedResult = true)] // extension is IsDatasetFile's job + public bool IsVersioningUpgradeMap(string file) + => FileFilter.IsVersioningUpgradeMap(file); } }