Report Confidence as a warning for unit-test dataset fixtures

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<ITestInformation>() };
         var allAnnotations = new List<Annotation>();
         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/"),

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;
+
+/// <summary>
+/// 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.
+/// </summary>
+public static class DatasetSeverity
+{
+    /// <summary>
+    /// The engine's name for the Confidence finding, as it appears on the Error and therefore on
+    /// Annotation.RuleName.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    public const string ConfidenceRule = "Dataset confidence error";
+
+    /// <summary>
+    /// Returns a result in which every Error carrying <paramref name="ruleName"/> is reported as
+    /// a Warning, with the top-level status recomputed from what is left. Returns the input
+    /// unchanged, by reference, when nothing matches.
+    /// </summary>
+    /// <param name="demoted">How many findings were demoted, for the caller to report.</param>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    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<ITestInformation>(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;
+    }
+
+    /// <summary>The most severe status across a set of findings, Pass when there are none.</summary>
+    private static TestStatus MostSevere(IEnumerable<ITestInformation> 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;
+    }
+
+    /// <summary>
+    /// The line reported when findings were demoted, so a green check states that it downgraded
+    /// something rather than implying there was nothing to downgrade.
+    /// </summary>
+    /// <remarks>
+    /// 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 <c>FileFilter.IsUnitTestFixture</c>. 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.
+    /// </remarks>
+    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.";
+}

tools/ComplianceRunner/src/Shared/Compliance.Shared/FileFilter.cs

@@ -89,6 +89,47 @@ private static bool IsUnderDatasetsDirectory(string file)
         return false;
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    /// <remarks>
+    /// 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 <c>IsUnderDatasetsDirectory</c> 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.
+    /// </remarks>
+    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;
+    }
+
     /// <summary>
     /// True for a versioning upgrade map, named Versioning_&lt;digits&gt;.json.
     /// </summary>

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