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