From 1d81f954aa9a9788fcf882ac7f22d090c0dd8458 Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Fri, 4 Sep 2026 20:51:49 +0100 Subject: [PATCH] fix(ci-versioning): report both unverified reasons, always, and stop the two warnings reading as one axis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two warnings both opened with "N failure(s)" — one the unverified total, the other the count attributed by namespace — with nothing saying which axis either measured. On BHoM/BHoM run 33849699768 that was read as "0 classified by resolution, 145 by attribution". The artefact says 114 UnresolvableTypeAbsent and 31 NoMethodEvent. Correct numbers, wrong conclusion, purely from presentation. Three changes, no behaviour change to classification or attribution: 1. New "Unverified basis:" line, printed unconditionally next to "Attribution basis:", splitting the unverified bucket into unresolvable (a type the record needs is absent from the closure) and unattributable (ownership inferred from a namespace prefix). Extracted as UnverifiedBasis so the arithmetic is testable. Unresolvable is counted by path; unattributable by subtraction from the unverified total, so the two always sum to it and a path added later cannot silently vanish from the report. 2. The namespace-fallback warning no longer leads with a finding count. It is a statement about the attribution basis of findings that may have been classified for a different reason, and phrasing it as "N failure(s)" is what made it read as a classification total. It now says so explicitly and reports how many of those were separately unverifiable. 3. The job summary's two breakdown rows print even when zero, and both bases are surfaced in the table. A silent zero cannot be told from a number nobody measured, which is the trap the attribution-basis line already had. Verified end to end against the local closure: "Unverified basis: 66 unresolvable ..., 32 unattributable ...", matching Classification: UnresolvableTypeAbsent=66, NoMethodEvent=32. Suite 135 passed. Verified by mutation: deriving unattributable by path instead of by subtraction fails UnverifiedBasis_AnUnknownUnverifiedPath_IsNotSilentlyDropped and nothing else. Pester 12 passed, including a contiguity check that nothing branches between the total and its two components; asserted by contiguity rather than by absence of any 'if', because #17 legitimately uses elseif ($unverified -gt 0) further down. Refs BHoM/internal-tickets#31, BHoM/internal-tickets#32 --- .github/actions/ci-versioning/action.yml | 15 +++-- .../tests/ci-versioning-action.Tests.ps1 | 24 ++++++++ .../VersioningRunner.Tests/RunCommandTests.cs | 43 ++++++++++++++ .../VersioningRunner/Commands/RunCommand.cs | 56 ++++++++++++++++--- 4 files changed, 125 insertions(+), 13 deletions(-) diff --git a/.github/actions/ci-versioning/action.yml b/.github/actions/ci-versioning/action.yml index 3259ed8..83ddfab 100644 --- a/.github/actions/ci-versioning/action.yml +++ b/.github/actions/ci-versioning/action.yml @@ -796,12 +796,17 @@ runs: # ::warning, but stderr is deliberately not teed into the file this summary reads # (see the tee step), so without this line the summary would not carry it at all. $attributionBasis = '(not reported)' + $unverifiedBasis = '(not reported)' if (Test-Path 'versioning-stdout.txt') { $out = Get-Content 'versioning-stdout.txt' -Raw # 'Attribution basis:' does not contain 'Attribution:', so these cannot cross-match. if ($out -match 'Attribution:\s*(.+)') { $attribution = $Matches[1].Trim() } if ($out -match 'Classification:\s*(.+)') { $classification = $Matches[1].Trim() } if ($out -match 'Attribution basis:\s*(.+)') { $attributionBasis = $Matches[1].Trim() } + # 'Unverified basis:' is the classification axis; 'Attribution basis:' is the + # ownership axis. Both are surfaced because reading either alone invites the + # reader to treat it as the whole picture. + if ($out -match 'Unverified basis:\s*(.+)') { $unverifiedBasis = $Matches[1].Trim() } } $rows = @() @@ -866,11 +871,13 @@ runs: $md += "|---|---|" $md += "| Classification | ``$classification`` |" $md += "| Attribution basis | ``$attributionBasis`` |" + $md += "| Unverified basis | ``$unverifiedBasis`` |" + # Both rows print even when zero. A silent zero cannot be told from a number + # nobody measured, which is the same trap as the attribution-basis line that used + # to be gated on being non-zero. $md += "| Reported unverified | $unverified |" - if ($unverified -gt 0) { - $md += "| — could not be resolved (closure gap) | $unresolved |" - $md += "| — could not be attributed (inferred ownership) | $unattributed |" - } + $md += "| — could not be resolved (closure gap) | $unresolved |" + $md += "| — could not be attributed (inferred ownership) | $unattributed |" if ($coverage) { $md += "| Surface examined | $($coverage.SubjectTypes) subject types across $($coverage.SubjectAssemblies) subject assemblies |" $md += "| Dataset versions | $(if ($coverage.DatasetVersions -eq 0) { 'all staged' } else { 'previous only' }) |" diff --git a/.github/scripts/tests/ci-versioning-action.Tests.ps1 b/.github/scripts/tests/ci-versioning-action.Tests.ps1 index 2323ee4..cb7abcc 100644 --- a/.github/scripts/tests/ci-versioning-action.Tests.ps1 +++ b/.github/scripts/tests/ci-versioning-action.Tests.ps1 @@ -81,6 +81,30 @@ Describe 'ci-versioning action.yml' { } } + Context 'the unverified breakdown is reported unconditionally' { + + # A silent zero cannot be told from a number nobody measured. Same trap as the + # attribution-basis print that used to be gated on being non-zero, and the reason + # run 33849699768's 114/31 split was read as 0/145. + It 'prints both breakdown rows without gating them on a non-zero total' { + $summary = ($text -split '- name: Write to Job Summary' | Select-Object -Last 1) + $summary | Should -Match 'could not be resolved \(closure gap\)' + $summary | Should -Match 'could not be attributed \(inferred ownership\)' + # Tested by contiguity rather than by absence of any 'if': #17 legitimately uses + # elseif ($unverified -gt 0) further down for the findings-table prose. What must + # hold is that nothing branches between the total and its two components. + $between = [regex]::Match($text, + '(?s)Reported unverified.*?could not be attributed \(inferred ownership\)').Value + $between | Should -Not -Match 'if \(' ` + -Because 'a branch between the total and its breakdown reintroduces the silent zero' + } + + It 'surfaces both axes, not just attribution' { + $text | Should -Match 'Unverified basis' -Because 'the classification axis' + $text | Should -Match 'Attribution basis' -Because 'the ownership axis' + } + } + Context 'the subject-assembly bracket' { # The subject set is the difference between two snapshots of the shared assembly diff --git a/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs b/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs index 2f88d9b..1f38105 100644 --- a/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs +++ b/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs @@ -610,6 +610,49 @@ public void EmptyIndex_DeclinesToClassify() Assert.Null(RunCommand.ClassifyUnresolvableCause([ConvertCause], LoadedTypeIndex.Empty).Cause); } + // The two reasons a finding goes unverified are different axes and must not collapse + // into one number. Measured cost of collapsing them: on BHoM/BHoM run 33849699768 two + // warnings both opened with "145 failure(s)" — one the unverified total, the other the + // count attributed by namespace — and the split was read as 0/145 when it was 114/31. + private static FailureDiagnostic Diag(string label, ClassificationPath path, bool real) => + new(label, real, path, real ? null : "cause", null, null, 0); + + [Fact] + public void UnverifiedBasis_SplitsByReasonAndAlwaysSumsToTheUnverifiedTotal() + { + var diags = new[] + { + Diag("a", ClassificationPath.UnresolvableTypeAbsent, false), + Diag("b", ClassificationPath.UnresolvableTypeAbsent, false), + Diag("c", ClassificationPath.UnresolvableFromEvents, false), + Diag("d", ClassificationPath.NoMethodEvent, false), + Diag("e", ClassificationPath.NoMethodEvent, true), // real: counts in neither + }; + + var (unresolvable, unattributable) = RunCommand.UnverifiedBasis(diags); + + Assert.Equal(3, unresolvable); + Assert.Equal(1, unattributable); + Assert.Equal(diags.Count(d => !d.CountedAsReal), unresolvable + unattributable); + } + + // A path added later must not fall out of the report entirely. Because unattributable + // is derived by subtraction it lands there and is visible, rather than in neither. + [Fact] + public void UnverifiedBasis_AnUnknownUnverifiedPath_IsNotSilentlyDropped() + { + var diags = new[] { Diag("x", ClassificationPath.ConfigurationNotBuilt, false) }; + + var (unresolvable, unattributable) = RunCommand.UnverifiedBasis(diags); + + Assert.Equal(0, unresolvable); + Assert.Equal(1, unattributable); + } + + [Fact] + public void UnverifiedBasis_NoDiagnostics_IsZeroZeroRatherThanUndefined() + => Assert.Equal((0, 0), RunCommand.UnverifiedBasis(Array.Empty())); + [Fact] public void NamedFailingType_ParsesAllThreeEventShapes() { diff --git a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs index 55f0481..0a1a324 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs @@ -208,19 +208,36 @@ public static int Execute( $"Attribution basis: {byAssembly} by declaring assembly, " + $"{byNamespace} by namespace fallback (no declaring assembly recorded)"); + // Why each finding went unverified, printed unconditionally and next to the + // attribution basis, because the two are different axes and reporting only one of + // them invites the reader to collapse them. + // + // Measured cost of not doing this: on run 33849699768 the two warnings below both + // opened with "145 failure(s)" — one the total unverified, the other the count + // attributed by namespace — and were read as "0 classified by resolution, 145 by + // attribution". The real split was 114 and 31. Correct numbers, wrong conclusion, + // because nothing said which axis either was measuring. + var (unresolvable, unattributable) = UnverifiedBasis(diagnostics); + Console.WriteLine( + $"Unverified basis: {unresolvable} unresolvable (a type the record needs is absent " + + $"from the closure), {unattributable} unattributable (ownership inferred from a namespace prefix)"); + // The fallback cannot tell this repository's namespace from a namespace it is // merely the root of, so any finding on that path may be another repository's. - // Those findings no longer gate: they are routed to the unverified bucket at - // classification, so this reports how much of the run went unverified for that - // reason rather than how much of a red was guesswork. + // + // Deliberately does NOT lead with a count of findings. It is a statement about the + // attribution basis of findings that may have been classified for an entirely + // different reason, and phrasing it as "N failure(s)" is what made it read as a + // classification total. if (byNamespace > 0) { Console.Error.WriteLine( - $"::warning title=Versioning::{byNamespace} failure(s) could only be attributed to this repository by " - + "namespace, because the dataset record named no declaring assembly. That test cannot separate " - + "this repository's types from those of repositories extending its namespace, so they are reported " - + "as unverified and do not fail this check. A genuine regression among them would not be detected " - + "this run. See BHoM/internal-tickets#31."); + $"::warning title=Versioning::Attribution basis, not a finding count: {byNamespace} of " + + $"{diagnostics.Count} finding(s) named no declaring assembly, so ownership could only be inferred " + + "from a namespace prefix, which cannot separate this repository's types from those of repositories " + + $"extending its namespace. Of those, {unresolvable} were separately unverifiable because the closure " + + $"could not resolve a type they need and are reported under that cause; {unattributable} rest on the " + + "inference alone. None of them gate. See BHoM/internal-tickets#31."); } } @@ -243,8 +260,10 @@ public static int Execute( // longer true of all of them: inferred ownership lands here too, and those types // are resolvable and may well be BHoM's. Console.Error.WriteLine( - $"::warning title=Versioning::{unresolvableSkips.Count} failure(s) attributed to this repo were not verified: " + + $"::warning title=Versioning::{unresolvableSkips.Count} finding(s) attributed to this repo were not verified, " + + $"for {causes.Count} distinct reason(s): " + $"{string.Join(", ", causes.Take(8))}{(causes.Count > 8 ? $" and {causes.Count - 8} more" : "")}. " + + "See the Unverified basis line for the split between unresolvable and unattributable. " + "A genuine versioning regression among them would not be detected this run."); } @@ -335,6 +354,25 @@ public static int Execute( return ExitCodeFor(result.Status); } + // Splits the unverified bucket by why, not by whose. The two reasons need different + // work — a closure gap is fixed by resolving more, inferred ownership by knowing more — + // so a single total is not actionable. + // + // Unresolvable is defined by the classification path rather than by subtraction, and + // unattributable by subtraction from the unverified total, so the two always sum to it + // and a new unverified path cannot silently vanish from the report: it lands in + // unattributable and shows up as a number nobody expected, rather than in neither. + internal static (int Unresolvable, int Unattributable) UnverifiedBasis( + IEnumerable diagnostics) + { + var all = diagnostics as ICollection ?? diagnostics.ToList(); + int unresolvable = all.Count(d => !d.CountedAsReal + && d.Path is ClassificationPath.UnresolvableTypeAbsent + or ClassificationPath.UnresolvableFromEvents); + int unverified = all.Count(d => !d.CountedAsReal); + return (unresolvable, unverified - unresolvable); + } + // The check's verdict, from the two counts that decide it. // // Warning, not Pass, when everything attributed was unverifiable: no genuine