diff --git a/.github/actions/ci-compliance/action.yml b/.github/actions/ci-compliance/action.yml index c4e15d6..8a2f81c 100644 --- a/.github/actions/ci-compliance/action.yml +++ b/.github/actions/ci-compliance/action.yml @@ -52,6 +52,37 @@ runs: patterns: ${{ inputs.patterns }} output_file: changed_files.txt + # A skipped job writes nothing to the summary, so a green with no files to check and a + # green that checked everything read the same there. The runner's coverage line solves + # this only once the runner runs, and on a skip it never does. Says which pathspec was + # searched, because the one way a skip is wrong is the pathspec missing a file that did + # change, and that is not diagnosable from "skipped" alone. + - name: Report the skip + if: steps.changed.outputs.count == '0' + shell: bash + env: + CHECK_TYPE: ${{ inputs.check_type }} + PATTERNS: ${{ inputs.patterns }} + REASON: ${{ steps.changed.outputs.reason }} + run: | + set -euo pipefail + + if [ "$REASON" = "not-a-pull-request" ]; then + detail="This run was not triggered by a pull request, so there was no diff to examine." + else + detail="No files matching \`${PATTERNS}\` changed in this pull request, so no file was examined." + fi + + echo "::notice title=${CHECK_TYPE} compliance::Skipped. ${detail}" + + if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then + { + echo "### ${CHECK_TYPE} compliance skipped" + echo "" + echo "${detail}" + } >> "$GITHUB_STEP_SUMMARY" + fi + - name: Mint dependency token id: dep-token if: steps.changed.outputs.count != '0' diff --git a/.github/actions/compute-changed-files/action.yml b/.github/actions/compute-changed-files/action.yml index 5725d54..df12423 100644 --- a/.github/actions/compute-changed-files/action.yml +++ b/.github/actions/compute-changed-files/action.yml @@ -31,6 +31,14 @@ outputs: '0' signals callers to skip; any other value signals proceed. May exceed the output_file line count, which excludes deletions. value: ${{ steps.compute.outputs.count }} + reason: + description: > + Why the count came out as it did: 'files-changed', 'no-matching-files' + or 'not-a-pull-request'. A caller reporting its own skip needs this to + say which one happened; the count alone collapses the last two, and a + summary line claiming no files changed when the run was never a pull + request would be wrong. + value: ${{ steps.compute.outputs.reason }} runs: using: composite @@ -58,6 +66,7 @@ runs: # failure mode for this action, because every consumer treats count=0 # as "nothing to do". if [ -z "$BASE_SHA" ]; then + echo "reason=not-a-pull-request" >> "$GITHUB_OUTPUT" if [ "$SKIP_IF_NOT_PR" = "true" ]; then echo "count=0" >> "$GITHUB_OUTPUT" echo "::notice::Not triggered from a pull request. Check skipped." @@ -127,7 +136,9 @@ runs: echo "count=$count" >> "$GITHUB_OUTPUT" if [ "$count" -gt 0 ]; then + echo "reason=files-changed" >> "$GITHUB_OUTPUT" echo "::notice::$count relevant file(s) changed." else + echo "reason=no-matching-files" >> "$GITHUB_OUTPUT" echo "::notice::No relevant files changed. Check skipped." fi diff --git a/.github/scripts/tests/ci-compliance-action.Tests.ps1 b/.github/scripts/tests/ci-compliance-action.Tests.ps1 index 760efb9..f1a421b 100644 --- a/.github/scripts/tests/ci-compliance-action.Tests.ps1 +++ b/.github/scripts/tests/ci-compliance-action.Tests.ps1 @@ -17,6 +17,7 @@ BeforeAll { $script:actionPath = Join-Path $repoRoot '.github/actions/ci-compliance/action.yml' $script:lines = Get-Content $actionPath $script:text = $lines -join "`n" + $script:changedFiles = Get-Content (Join-Path $repoRoot '.github/actions/compute-changed-files/action.yml') -Raw } Describe 'ci-compliance action.yml' { @@ -40,4 +41,40 @@ Describe 'ci-compliance action.yml' { $text | Should -Match "match '\^Coverage: '" } } + + Context 'a self-skip says so' { + + # The coverage line only exists once the runner runs. On a skip it never does, so + # without this the summary is empty and a green that had nothing to check reads the + # same as a green that checked everything. + It 'writes to the summary when the check is skipped' { + $text | Should -Match 'compliance skipped' + } + + It 'names the pathspec it searched' { + $text | Should -Match 'PATTERNS' -Because 'the one way a skip is wrong is the pathspec missing a file that did change, and that is not diagnosable from "skipped" alone' + } + + # Two different states both produce count=0. Saying "no files changed" when the run was + # never a pull request would be a wrong statement, not just a vague one. + It 'distinguishes an empty diff from a run that was never a pull request' { + $text | Should -Match 'not-a-pull-request' + } + + It 'takes that reason from the action that knows it' { + $text | Should -Match 'steps\.changed\.outputs\.reason' -Because 'compute-changed-files is the only place the two cases are told apart' + } + + # The other end of that contract. A consumer reading an output no producer sets gets an + # empty string and falls through to the wrong branch, silently. + It 'is an output compute-changed-files actually declares' { + $changedFiles | Should -Match '(?m)^\s{2}reason:' + } + + It 'sets a reason on every path out of compute-changed-files' { + foreach ($r in 'not-a-pull-request', 'no-matching-files', 'files-changed') { + $changedFiles | Should -Match "reason=$r" + } + } + } } diff --git a/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs b/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs index 3ec8840..915e571 100644 --- a/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs +++ b/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs @@ -169,7 +169,7 @@ static int Main(string[] args) catch (Exception ex) { accounting.CountFailed(); - allAnnotations.Add(FailureAnnotation(file, ex)); + allAnnotations.Add(Annotation.ForUnanalysableFile(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. @@ -188,25 +188,4 @@ static int Main(string[] args) // 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/DatasetComplianceRunner/DatasetComplianceRunner.cs b/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs index 87a2206..97db7fe 100644 --- a/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs +++ b/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs @@ -53,46 +53,71 @@ static int Main(string[] args) continue; } - var resultForThisFile = file.IsValidDataset(); - - if (resultForThisFile == null) + // One file must not take the run down with it, for the same reason it must not in + // ComplianceRunner: an escaping exception kills the process before OutputEmitter + // runs, so the check goes 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 + // are 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. + // + // IsValidDataset opens the file with an unguarded StreamReader before it parses + // anything, so the throw does not need a malformed dataset to reach it. Reproduced + // by holding the file open with no sharing, which is what an antivirus scan or a + // concurrent step does. + try { - accounting.CountNoResult(); - Console.WriteLine($" [SKIP] No result returned for: {file}"); - continue; - } + var resultForThisFile = file.IsValidDataset(); + + if (resultForThisFile == null) + { + accounting.CountNoResult(); + Console.WriteLine($" [SKIP] No result returned for: {file}"); + continue; + } - if (verbose) Console.WriteLine($" Result Status: {resultForThisFile.Status}"); + if (verbose) Console.WriteLine($" Result Status: {resultForThisFile.Status}"); - accounting.CountExamined(); - mergedResult = mergedResult.Merge(resultForThisFile); + accounting.CountExamined(); + mergedResult = mergedResult.Merge(resultForThisFile); - var information = resultForThisFile.Information ?? Enumerable.Empty(); - var perFileAnnotations = information - .Select(i => i.ToAnnotationEquivalent()) - .ToList(); - var infoList = information.ToList(); + var information = resultForThisFile.Information ?? Enumerable.Empty(); + var perFileAnnotations = information + .Select(i => i.ToAnnotationEquivalent()) + .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) + 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]); + } + // Dataset checks don't populate Location.FilePath, so anchor to the JSON file + // so GitHub can produce an inline PR annotation instead of falling back to + // "unknown". + if (string.IsNullOrEmpty(a.FilePath)) + a.FilePath = file; + if (a.LineStart <= 0) + a.LineStart = 1; + + allAnnotations.Add(a); } - // Dataset checks don't populate Location.FilePath — anchor to the JSON file so - // GitHub can produce an inline PR annotation instead of falling back to "unknown". - if (string.IsNullOrEmpty(a.FilePath)) - a.FilePath = file; - if (a.LineStart <= 0) - a.LineStart = 1; - - allAnnotations.Add(a); + } + catch (Exception ex) + { + accounting.CountFailed(); + allAnnotations.Add(Annotation.ForUnanalysableFile(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()); } } @@ -102,7 +127,10 @@ 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 dataset, + // 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; } } diff --git a/tools/ComplianceRunner/src/Shared/Compliance.Shared/Annotation.cs b/tools/ComplianceRunner/src/Shared/Compliance.Shared/Annotation.cs index 40004eb..e1e5a04 100644 --- a/tools/ComplianceRunner/src/Shared/Compliance.Shared/Annotation.cs +++ b/tools/ComplianceRunner/src/Shared/Compliance.Shared/Annotation.cs @@ -23,4 +23,28 @@ public class Annotation public string BHoMGuid { get; set; } = ""; /// UTC timestamp at which the engine produced this finding. public DateTime UTCTime { get; set; } + + /// + /// 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. + /// + /// Shared by both runners rather than written out in each, so a reword cannot land in one + /// and not the other. It names no check and no engine, which is what lets it be shared. + /// + public static Annotation ForUnanalysableFile(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/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs index c09eb42..a445f2f 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs +++ b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs @@ -152,4 +152,98 @@ public void GitHubOutput_ForPassingRun_ProducesNoFindingAnnotations() Assert.That(stdout, Does.Contain("::warning title=Compliance coverage::")); }); } + + // ── A file the engine throws on ─────────────────────────────────────────── + + /// + /// Writes a dataset file and a sibling under a real Datasets directory, so both pass the + /// filter and reach the engine, and returns the pair with the directory to clean up. + /// + /// The sibling carries a genuine finding. Without it a test cannot tell "the run continued" + /// from "the run died quietly". + /// + private static (string Target, string Sibling, string Dir) WriteDatasetPair() + { + string dir = Path.Combine(Path.GetTempPath(), "bhom-ci-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(Path.Combine(dir, "Datasets")); + + string target = Path.Combine(dir, "Datasets", "Target.json"); + File.WriteAllText(target, "{ \"_t\" : \"BH.oM.Data.Library.Dataset\" }"); + + string sibling = Path.Combine(dir, "Datasets", "Sibling.json"); + File.WriteAllText(sibling, "not a dataset"); + + return (target, sibling, dir); + } + + [Test] + [Category("RequiresBHoM")] + [Platform("Win", Reason = "FileShare.None only blocks another process on Windows; on Unix .NET's sharing mode is advisory, so the file would open and nothing would throw.")] + [Description("A dataset 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 (target, sibling, dir) = WriteDatasetPair(); + try + { + // Held open with no sharing, which is what an antivirus scan or a concurrent step + // does. IsValidDataset opens the file with an unguarded StreamReader before it + // parses anything, so this reaches the throw without needing a malformed dataset, + // and the file stays a valid one so the test is not really about its content. + using (File.Open(target, FileMode.Open, FileAccess.Read, FileShare.None)) + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", + "--output", "github", target, sibling); + + Assert.Multiple(() => + { + Assert.That(exitCode, Is.EqualTo(1), "a file the engine could not analyse must fail the run"); + Assert.That(stdout, Does.Contain("Target.json"), + "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("did not deserialise"), + "the other file's finding must still be reported; losing them was the defect"); + }); + } + } + finally + { + try { Directory.Delete(dir, recursive: true); } catch { /* temp dir, best effort */ } + } + } + + [Test] + [Category("RequiresBHoM")] + [Platform("Win", Reason = "FileShare.None only blocks another process on Windows; on Unix .NET's sharing mode is advisory, so the file would open and nothing would throw.")] + [Description("A run whose only dataset 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 sibling carries a + // real finding, so the run exits 1 on the merged Error whether or not failures count + // towards the exit code. With nothing else in the run the exit code here rests on the + // failure count and nothing else. + var (target, _, dir) = WriteDatasetPair(); + try + { + using (File.Open(target, FileMode.Open, FileAccess.Read, FileShare.None)) + { + var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner", + "--output", "github", target); + + 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/AnnotationTests.cs b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/AnnotationTests.cs new file mode 100644 index 0000000..723c525 --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/AnnotationTests.cs @@ -0,0 +1,83 @@ +using NUnit.Framework; +using System; +using System.IO; + +namespace Compliance.Tests +{ + /// + /// The annotation both runners emit for a file the engine threw on, tested without a BHoM + /// install. + /// + /// It was a private method on ComplianceRunner until DatasetComplianceRunner needed the same + /// thing. Moving it here rather than writing it out twice is what these tests hold in place: + /// the wording is the whole deliverable, it is the only thing an author sees when a file + /// cannot be checked, and two copies of it would drift. + /// + [TestFixture] + public class AnnotationTests + { + [Test] + public void ItNamesTheFileAndSaysTheFileWasNotChecked() + { + var a = Annotation.ForUnanalysableFile( + "Datasets/Regional.json", new IOException("The process cannot access the file.")); + + Assert.Multiple(() => + { + Assert.That(a.FilePath, Is.EqualTo("Datasets/Regional.json"), + "without the file name the author cannot act on the red"); + Assert.That(a.Message, Does.Contain("could not be analysed and was not checked")); + Assert.That(a.Message, Does.Contain("Every other file in this run was checked normally."), + "says the run continued, so the author does not assume the rest went unchecked"); + }); + } + + [Test] + public void ItCarriesTheExceptionTypeAndMessage() + { + // The distinction the author needs is between their file being malformed and the + // environment being wrong. The type alone makes that call: an IOException is not a + // defect in the dataset. + var a = Annotation.ForUnanalysableFile( + "Datasets/Regional.json", new IOException("used by another process")); + + Assert.Multiple(() => + { + Assert.That(a.Message, Does.Contain("IOException")); + Assert.That(a.Message, Does.Contain("used by another process")); + }); + } + + [Test] + public void ItIsAFailureAndNotAFinding() + { + // Level drives the GitHub annotation level, so a warning here would let a run that + // could not read a file look like one that read it and had a minor comment. + // RuleName is the SARIF ruleId and is deliberately not a check name: no check ran. + var a = Annotation.ForUnanalysableFile("Datasets/Regional.json", new Exception("x")); + + Assert.Multiple(() => + { + Assert.That(a.Level, Is.EqualTo("failure")); + Assert.That(a.RuleName, Is.EqualTo("FileCouldNotBeAnalysed")); + }); + } + + [Test] + public void ItAnchorsToLineOne() + { + // GitHub drops the "#L" suffix when LineStart is 0 and renders the annotation + // against the file rather than the diff. There is no meaningful line for a file that + // was never parsed, and line 1 is what puts it in the pull request's file view. + var a = Annotation.ForUnanalysableFile("Datasets/Regional.json", new Exception("x")); + + Assert.Multiple(() => + { + Assert.That(a.LineStart, Is.EqualTo(1)); + Assert.That(a.LineEnd, Is.EqualTo(1)); + Assert.That(a.ColumnStart, Is.EqualTo(1)); + Assert.That(a.ColumnEnd, Is.EqualTo(1)); + }); + } + } +}