diff --git a/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs b/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs index 9c3f75e..3ec8840 100644 --- a/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs +++ b/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs @@ -54,108 +54,127 @@ static int Main(string[] args) continue; } - TestResult resultForThisFile; - - if (checkType == "project") + // One file must not take the run down with it. A rule that throws used to escape + // Main, which killed the process before OutputEmitter ran: the check went red with + // no annotation, no coverage line, and no name for the file that caused it, and the + // findings already gathered for every other file were lost. The run still fails + // below on accounting.Failed, so this converts a silent abort into a stated one + // rather than making the check more permissive. + try { - if (file.EndsWith(".csproj", StringComparison.OrdinalIgnoreCase)) - resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.CheckProjectFile(file, orgUrl); - else - resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.CheckAssemblyInfo(file, orgUrl); - - // Remap absolute location paths back to the relative file path so - // annotations point to the correct diff line — mirrors BHoMBot ProjectCompliance.cs. - // Note: .OfType() silently drops any ITestInformation that is not exactly - // BH.oM.Test.CodeCompliance.Error. Currently safe — the project engine only emits - // Error-typed findings — but revisit if new finding sub-types are introduced. - if (resultForThisFile?.Information != null) + TestResult resultForThisFile; + + if (checkType == "project") { - resultForThisFile.Information = resultForThisFile.Information - .OfType() - .Select(e => (ITestInformation)new BH.oM.Test.CodeCompliance.Error - { - Status = e.Status, - Message = e.Message, - Name = e.Name, - BHoM_Guid = e.BHoM_Guid, - UTCTime = e.UTCTime, - DocumentationLink = e.DocumentationLink, - Location = new BH.oM.Test.CodeCompliance.Location + if (file.EndsWith(".csproj", StringComparison.OrdinalIgnoreCase)) + resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.CheckProjectFile(file, orgUrl); + else + resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.CheckAssemblyInfo(file, orgUrl); + + // Remap absolute location paths back to the relative file path so + // annotations point to the correct diff line, mirroring BHoMBot ProjectCompliance.cs. + // Note: .OfType() silently drops any ITestInformation that is not exactly + // BH.oM.Test.CodeCompliance.Error. Currently safe, because the project engine only emits + // Error-typed findings, but revisit if new finding sub-types are introduced. + if (resultForThisFile?.Information != null) + { + resultForThisFile.Information = resultForThisFile.Information + .OfType() + .Select(e => (ITestInformation)new BH.oM.Test.CodeCompliance.Error { - FilePath = file, - Line = e.Location?.Line - } - }) - .ToList(); + Status = e.Status, + Message = e.Message, + Name = e.Name, + BHoM_Guid = e.BHoM_Guid, + UTCTime = e.UTCTime, + DocumentationLink = e.DocumentationLink, + Location = new BH.oM.Test.CodeCompliance.Location + { + FilePath = file, + Line = e.Location?.Line + } + }) + .ToList(); + } } - } - else - { - // Test_Toolkit's [Path(@"..._Engine\\..")] rules match a literal backslash; - // flip git's forward-slash paths or every rule silently no-ops. - resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.RunChecks(file.Replace('/', '\\'), checkType); - - // Group findings one-per-line and remap the path to the diff-relative file, - // mirroring BHoMBot's GroupInformation -> GroupErrors pipeline (code/copyright/ - // documentation only; project has its own remap above). GroupErrors concatenates - // each finding's FullMessage() into the grouped Error.Message, so downstream these - // annotations are built from that message directly (see the preformatted branch in - // AnnotationConvert) rather than via IFullMessage, which would append the - // documentation suffix a second time. - if (resultForThisFile?.Information != null) + else { - var errors = resultForThisFile.Information - .OfType() - .ToList(); - resultForThisFile.Information = BH.Engine.Test.CodeCompliance.Modify - .GroupErrors(errors, file) - .Select(e => (ITestInformation)e) - .ToList(); + // Test_Toolkit's [Path(@"..._Engine\\..")] rules match a literal backslash; + // flip git's forward-slash paths or every rule silently no-ops. + resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.RunChecks(file.Replace('/', '\\'), checkType); + + // Group findings one-per-line and remap the path to the diff-relative file, + // mirroring BHoMBot's GroupInformation -> GroupErrors pipeline (code/copyright/ + // documentation only; project has its own remap above). GroupErrors concatenates + // each finding's FullMessage() into the grouped Error.Message, so downstream these + // annotations are built from that message directly (see the preformatted branch in + // AnnotationConvert) rather than via IFullMessage, which would append the + // documentation suffix a second time. + if (resultForThisFile?.Information != null) + { + var errors = resultForThisFile.Information + .OfType() + .ToList(); + resultForThisFile.Information = BH.Engine.Test.CodeCompliance.Modify + .GroupErrors(errors, file) + .Select(e => (ITestInformation)e) + .ToList(); + } } - } - if (resultForThisFile == null) - { - accounting.CountNoResult(); - Console.WriteLine($" [SKIP] No result returned for: {file}"); - continue; - } + if (resultForThisFile == null) + { + accounting.CountNoResult(); + Console.WriteLine($" [SKIP] No result returned for: {file}"); + continue; + } - if (verbose) Console.WriteLine($" Result Status: {resultForThisFile.Status}"); - - accounting.CountExamined(); - mergedResult = mergedResult.Merge(resultForThisFile); - - // Code/copyright/documentation findings have been through GroupErrors, whose - // Error.Message is already the concatenated FullMessage(); take it verbatim. - // - // Project findings use IFullMessage, which appends the " - For more information - // see " documentation suffix. This is a deliberate, documented divergence from - // BHoMBot: BHoMBot's ProjectCompliance emitted the bare Error.Message with no suffix, - // even though it carried the DocumentationLink. Every other compliance check surfaces - // that link (via GroupErrors -> FullMessage), and the project engine attaches a - // DocumentationLink to every finding specifically so it can be shown, so BHoMBot's - // omission was an inconsistency. Keeping the suffix makes all five checks uniform and - // points authors at the remediation guidance. Nothing downstream parses this text - // (pass/fail is status-driven; SARIF carries the link separately). - bool preformatted = checkType != "project"; - var information = resultForThisFile.Information ?? Enumerable.Empty(); - var perFileAnnotations = information.Select(i => i.ToAnnotationEquivalent(preformatted)).ToList(); - var infoList = information.ToList(); - - for (int i = 0; i < perFileAnnotations.Count; i++) - { - var a = perFileAnnotations[i]; - var displayPath = string.IsNullOrEmpty(a.FilePath) ? file : a.FilePath; - if (verbose) + if (verbose) Console.WriteLine($" Result Status: {resultForThisFile.Status}"); + + accounting.CountExamined(); + mergedResult = mergedResult.Merge(resultForThisFile); + + // Code/copyright/documentation findings have been through GroupErrors, whose + // Error.Message is already the concatenated FullMessage(); take it verbatim. + // + // Project findings use IFullMessage, which appends the " - For more information + // see " documentation suffix. This is a deliberate, documented divergence from + // BHoMBot: BHoMBot's ProjectCompliance emitted the bare Error.Message with no suffix, + // even though it carried the DocumentationLink. Every other compliance check surfaces + // that link (via GroupErrors -> FullMessage), and the project engine attaches a + // DocumentationLink to every finding specifically so it can be shown, so BHoMBot's + // omission was an inconsistency. Keeping the suffix makes all five checks uniform and + // points authors at the remediation guidance. Nothing downstream parses this text + // (pass/fail is status-driven; SARIF carries the link separately). + bool preformatted = checkType != "project"; + var information = resultForThisFile.Information ?? Enumerable.Empty(); + var perFileAnnotations = information.Select(i => i.ToAnnotationEquivalent(preformatted)).ToList(); + var infoList = information.ToList(); + + for (int i = 0; i < perFileAnnotations.Count; i++) { - Console.WriteLine($" - [{a.Level}] {displayPath}:{a.LineStart}:{a.ColumnStart}" + - $"-{a.LineEnd}:{a.ColumnEnd} [{a.RuleName}]"); - Console.WriteLine($" {a.Message}"); - if (i < infoList.Count) - AnnotationConvert.LogDetailedFinding(infoList[i]); + var a = perFileAnnotations[i]; + var displayPath = string.IsNullOrEmpty(a.FilePath) ? file : a.FilePath; + if (verbose) + { + Console.WriteLine($" - [{a.Level}] {displayPath}:{a.LineStart}:{a.ColumnStart}" + + $"-{a.LineEnd}:{a.ColumnEnd} [{a.RuleName}]"); + Console.WriteLine($" {a.Message}"); + if (i < infoList.Count) + AnnotationConvert.LogDetailedFinding(infoList[i]); + } + allAnnotations.Add(a); } - allAnnotations.Add(a); + } + catch (Exception ex) + { + accounting.CountFailed(); + allAnnotations.Add(FailureAnnotation(file, ex)); + + // Stack trace to stderr, not into the annotation. GitHub truncates a long + // annotation message, and the one thing the author needs is the file name. + Console.Error.WriteLine($" [FAIL] {file}: {ex.GetType().Name}: {ex.Message}"); + Console.Error.WriteLine(ex.ToString()); } } @@ -164,7 +183,30 @@ static int Main(string[] args) accounting); // Exit code mirrors BHoMBot: failure only on Error; Warning and Pass are both success. - return mergedResult.Status == TestStatus.Error ? 1 : 0; + // A file the engine threw on also fails the run. It is not a finding about the code, so + // it does not reach mergedResult, and without this a crash would exit 0 and report a + // green check having examined less than it was asked to. + return mergedResult.Status == TestStatus.Error || accounting.Failed > 0 ? 1 : 0; } + /// + /// The annotation for a file the engine threw on. Carries the exception type and message so + /// the author can tell a malformed file from an environment problem, and says plainly that + /// this file was not checked, because a failure annotation that looks like a finding would + /// send them looking for a defect in their own code. + /// + static Annotation FailureAnnotation(string file, Exception ex) => new Annotation + { + FilePath = file, + LineStart = 1, + LineEnd = 1, + ColumnStart = 1, + ColumnEnd = 1, + Level = "failure", + RuleName = "FileCouldNotBeAnalysed", + Message = $"This file could not be analysed and was not checked. " + + $"{ex.GetType().Name}: {ex.Message} " + + $"Every other file in this run was checked normally.", + UTCTime = DateTime.UtcNow, + }; } diff --git a/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileAccounting.cs b/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileAccounting.cs index d167c42..08e8eb0 100644 --- a/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileAccounting.cs +++ b/tools/ComplianceRunner/src/Shared/Compliance.Shared/FileAccounting.cs @@ -4,10 +4,11 @@ /// Counts what a compliance runner did with the files it was handed, and renders that as a /// coverage denominator. /// -/// Both runners walk the same four-exit loop: a file is dropped by the filter, dropped because -/// it is not on disk, dropped because the engine returned no result, or examined. Without a -/// count of those, a run that examined every file and a run that examined none produce the same -/// output, so a green check carries no evidence that anything was checked. +/// Both runners walk the same five-exit loop: a file is dropped by the filter, dropped because +/// it is not on disk, dropped because the engine returned no result, examined, or failed because +/// the engine threw on it. Without a count of those, a run that examined every file and a run +/// that examined none produce the same output, so a green check carries no evidence that +/// anything was checked. /// /// Deliberately free of BHoM types and of any dependency on either runner, so it can be /// compiled directly into the hermetic test project and its arithmetic and wording tested @@ -37,19 +38,31 @@ public sealed class FileAccounting /// Files the runner actually examined and merged into its verdict. public int Examined { get; private set; } + /// + /// Files the engine threw on. Counted apart from the other drops because it is the only one + /// that means the check could not answer, rather than that it had nothing to answer about. + /// + public int Failed { get; private set; } + public FileAccounting(int handedIn) => HandedIn = handedIn; public void CountNotRelevant() => NotRelevant++; public void CountNotOnDisk() => NotOnDisk++; public void CountNoResult() => NoResult++; public void CountExamined() => Examined++; + public void CountFailed() => Failed++; /// /// True when files were supplied and none of them was examined. The calling workflow exits /// before invoking a runner when it has no files at all, so this is the anomalous case /// rather than the empty one: something selected these files and the runner used none. + /// + /// A run with failures is excluded deliberately. This flag exists to catch a check that + /// reports success having inspected nothing, and its warning says exactly that. A run that + /// failed on every file is already red and already carries an annotation per file, so + /// raising it here would contradict the exit code. /// - public bool ExaminedNothing => HandedIn > 0 && Examined == 0; + public bool ExaminedNothing => HandedIn > 0 && Examined == 0 && Failed == 0; /// /// The coverage line. Leads with what was examined rather than with what was handed in: @@ -75,6 +88,7 @@ private string DropSummary() if (NotRelevant > 0) parts.Add($"{NotRelevant} not relevant to this check"); if (NotOnDisk > 0) parts.Add($"{NotOnDisk} not found on disk"); if (NoResult > 0) parts.Add($"{NoResult} returned no result"); + if (Failed > 0) parts.Add($"{Failed} could not be analysed"); return parts.Count == 0 ? "none dropped" : string.Join(", ", parts); } @@ -86,5 +100,6 @@ private string DropSummary() ["notRelevant"] = NotRelevant, ["notOnDisk"] = NotOnDisk, ["noResult"] = NoResult, + ["failed"] = Failed, }; } diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/ComplianceRunnerE2ETests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/ComplianceRunnerE2ETests.cs index 4641311..2a05720 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/ComplianceRunnerE2ETests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/ComplianceRunnerE2ETests.cs @@ -115,4 +115,95 @@ public void GitHubOutput_ForPassingRun_ProducesNoFindingAnnotations() "the one annotation a passing run may produce is the report that it examined nothing"); }); } + + // ── A file the engine throws on ─────────────────────────────────────────── + + /// + /// Writes a .cs file the compliance engine cannot analyse, under a directory name the code + /// rules' path gate accepts, and returns its full path along with the directory to clean up. + /// + /// The content is malformed C#. Parsed, it leaves a parameter with no identifier, and the + /// first rule to index into a parameter name throws. Note it carries no preprocessor + /// directive: a directive is one way to reach this state and not the only one, so the test + /// does not depend on how the file got here. + /// + private static (string File, string Dir) WriteUnanalysableFile() + { + string dir = Path.Combine(Path.GetTempPath(), "bhom-ci-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(Path.Combine(dir, "Probe_Adapter")); + string file = Path.Combine(dir, "Probe_Adapter", "ProbeAdapter.cs"); + File.WriteAllText(file, + "namespace BH.Adapter.Probe\n" + + " m_value = new FileInfo(filePath).Directory.FullName;\n"); + return (file, dir); + } + + [Test] + [Category("RequiresBHoM")] + [Description("A file the engine throws on fails the run, names itself, and does not stop the other files being reported.")] + public void UnanalysableFile_FailsTheRunAndIsNamed_WithoutLosingOtherFindings() + { + var (bad, dir) = WriteUnanalysableFile(); + + // A second file with a real violation, so the test can tell "the run continued" from + // "the run died quietly". Capital parameter name breaks InputParameterStartsLower. + string good = Path.Combine(dir, "Probe_Adapter", "ProbeHelper.cs"); + File.WriteAllText(good, + "namespace BH.Adapter.Probe\n{\n public static class ProbeHelper\n {\n" + + " public static int ProbeHelperMethod(int BadParam) { return BadParam; }\n" + + " }\n}\n"); + + try + { + var (exitCode, stdout) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "github", bad, good); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(1), "a file the engine could not analyse must fail the run"); + Assert.That(stdout, Does.Contain("ProbeAdapter.cs"), + "the failing file must be named, or the author cannot act on the red"); + Assert.That(stdout, Does.Contain("could not be analysed")); + Assert.That(stdout, Does.Contain("1 of 2 file(s) examined"), + "the coverage line must survive, which it cannot if the process died"); + Assert.That(stdout, Does.Contain("InputParameterStartsLower"), + "the other file's findings must still be reported; losing them was the defect"); + }); + } + finally + { + try { Directory.Delete(dir, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + [Test] + [Category("RequiresBHoM")] + [Description("A run whose only file could not be analysed still fails, on the failure alone.")] + public void UnanalysableFileAlone_StillFailsTheRun() + { + // Separate from the test above because that one cannot see this. Its second file carries + // a real violation, so the run exits 1 on the merged Error whether or not failures count + // towards the exit code. A mutation removing the failure term from the exit code + // survived it. With nothing else in the run there is no other reason to be red, so the + // exit code here rests on the failure count and nothing else. + var (bad, dir) = WriteUnanalysableFile(); + try + { + var (exitCode, stdout) = RunnerFixture.Run("ComplianceRunner", + "code", "--output", "github", bad); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(1), + "a run that could analyse nothing must not report success"); + Assert.That(stdout, Does.Contain("0 of 1 file(s) examined")); + Assert.That(stdout, Does.Not.Contain("reports success without having inspected anything"), + "that warning is for a green that examined nothing, and this run is red"); + }); + } + finally + { + try { Directory.Delete(dir, recursive: true); } catch { /* temp dir, best effort */ } + } + } } diff --git a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileAccountingTests.cs b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileAccountingTests.cs index 10cb7cb..d37b220 100644 --- a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileAccountingTests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/FileAccountingTests.cs @@ -33,14 +33,15 @@ public void EveryFileExamined_ReportsFullCoverageAndNoDrops() [Test] public void EachDropReasonIsCountedAndNamedSeparately() { - var a = new FileAccounting(4); + var a = new FileAccounting(5); a.CountNotRelevant(); a.CountNotOnDisk(); a.CountNoResult(); + a.CountFailed(); a.CountExamined(); Assert.That(a.CoverageLine(), Is.EqualTo( - "Coverage: 1 of 4 file(s) examined; 1 not relevant to this check, 1 not found on disk, 1 returned no result.")); + "Coverage: 1 of 5 file(s) examined; 1 not relevant to this check, 1 not found on disk, 1 returned no result, 1 could not be analysed.")); } [Test] @@ -55,9 +56,67 @@ public void ADropReasonThatDidNotOccurIsNotMentioned() Assert.That(a.CoverageLine(), Does.Contain("2 not relevant to this check")); Assert.That(a.CoverageLine(), Does.Not.Contain("not found on disk")); Assert.That(a.CoverageLine(), Does.Not.Contain("returned no result")); + Assert.That(a.CoverageLine(), Does.Not.Contain("could not be analysed")); }); } + // ── Files the engine threw on ───────────────────────────────────────────────── + + [Test] + public void AFailedFileIsCountedApartFromTheOtherDrops() + { + // "could not be analysed" and "not relevant" mean opposite things: one is the check + // declining a file, the other is the check being unable to answer about it. Folding + // the second into the first would hide exactly the case this counter exists for. + var a = new FileAccounting(2); + a.CountExamined(); + a.CountFailed(); + + Assert.Multiple(() => + { + Assert.That(a.Failed, Is.EqualTo(1)); + Assert.That(a.NotRelevant, Is.EqualTo(0)); + Assert.That(a.NoResult, Is.EqualTo(0)); + Assert.That(a.CoverageLine(), Is.EqualTo( + "Coverage: 1 of 2 file(s) examined; 1 could not be analysed.")); + }); + } + + [Test] + public void AFailedFileDoesNotCountAsExamined() + { + // The whole point of the per-file catch is that the run continues. If a failure also + // counted as examined, the coverage line would claim the file was checked. + var a = new FileAccounting(1); + a.CountFailed(); + + Assert.That(a.Examined, Is.EqualTo(0)); + } + + [Test] + public void AllFilesFailed_IsNotReportedAsAGreenThatExaminedNothing() + { + // Caught by running the patched runner on a single unanalysable file. The warning + // text says the check "reports success without having inspected anything", which is + // false here: a failure exits non-zero. The flag means vacuous green specifically, + // so a red run must not raise it. + var a = new FileAccounting(1); + a.CountFailed(); + + Assert.That(a.ExaminedNothing, Is.False, + "a run that failed on every file is red, not a green that examined nothing"); + } + + [Test] + public void ExaminedNothingStillFiresWhenTheDropsAreBenign() + { + // The guard above must not silence the case this flag exists for. + var a = new FileAccounting(2); + a.CountNotRelevant(); a.CountNotOnDisk(); + + Assert.That(a.ExaminedNothing, Is.True); + } + // ── The case this exists for ────────────────────────────────────────────────── [Test] @@ -116,18 +175,19 @@ public void SomeExamined_IsNotFlagged() [Test] public void ThePayloadCarriesEveryCounter() { - var a = new FileAccounting(4); - a.CountNotRelevant(); a.CountNotOnDisk(); a.CountNoResult(); a.CountExamined(); + var a = new FileAccounting(5); + a.CountNotRelevant(); a.CountNotOnDisk(); a.CountNoResult(); a.CountFailed(); a.CountExamined(); var p = a.ToPayload(); Assert.Multiple(() => { - Assert.That(p["handedIn"], Is.EqualTo(4)); + Assert.That(p["handedIn"], Is.EqualTo(5)); Assert.That(p["examined"], Is.EqualTo(1)); Assert.That(p["notRelevant"], Is.EqualTo(1)); Assert.That(p["notOnDisk"], Is.EqualTo(1)); Assert.That(p["noResult"], Is.EqualTo(1)); + Assert.That(p["failed"], Is.EqualTo(1)); }); } @@ -140,9 +200,10 @@ public void TheCountsReconcile() for (int i = 0; i < 2; i++) a.CountNotRelevant(); for (int i = 0; i < 3; i++) a.CountNotOnDisk(); a.CountNoResult(); - for (int i = 0; i < 4; i++) a.CountExamined(); + a.CountFailed(); + for (int i = 0; i < 3; i++) a.CountExamined(); - Assert.That(a.NotRelevant + a.NotOnDisk + a.NoResult + a.Examined, + Assert.That(a.NotRelevant + a.NotOnDisk + a.NoResult + a.Failed + a.Examined, Is.EqualTo(a.HandedIn)); } }