From 4f675e594005b5fa2284f141cfcb0e414ba9d6d9 Mon Sep 17 00:00:00 2001 From: sakanni Date: Wed, 19 Aug 2026 17:26:22 +0100 Subject: [PATCH 1/2] fix: do not report a versioning finding whose declaring assembly is outside the closure --- .../ClosureReclassificationTests.cs | 196 ++++++++++++++++++ .../VersioningRunner/Commands/RunCommand.cs | 80 ++++++- .../Models/VersioningResult.cs | 18 ++ 3 files changed, 288 insertions(+), 6 deletions(-) create mode 100644 tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs diff --git a/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs b/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs new file mode 100644 index 0000000..cf7919b --- /dev/null +++ b/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs @@ -0,0 +1,196 @@ +using VersioningRunner.Commands; +using VersioningRunner.Models; +using VersioningRunner.Tests.Fixtures; +using Xunit; + +namespace VersioningRunner.Tests +{ + // The reclassification rule is easy to get subtly wrong and had no coverage at first: + // no test calls RunCommand.Execute, so the static state v1 relied on was never populated + // and the rule could not fire in any test. The three tests v1 broke therefore passed + // again under its gate incidentally, not because the gate was verified. These arrange + // the closure explicitly so the rule is actually exercised. + public class ClosureReclassificationTests + { + private const string ModelQaEvent = + "Method TryGetValueFromSource from { \"_t\" : \"System.Type\", \"Name\" : \"BH.Revit.Engine.Core.Compute, Revit_ModelQA_Engine_2022, Version=9.0.0.0, Culture=neutral, PublicKeyToken=null\", \"_bhomVersion\" : \"9.2\" } failed to deserialise."; + + private const string Config2024Event = + "Method ProjectParameter from { \"_t\" : \"System.Type\", \"Name\" : \"BH.Revit.Engine.Core.Create, Revit_Core_Engine_2024, Version=9.0.0.0, Culture=neutral, PublicKeyToken=null\", \"_bhomVersion\" : \"9.2\" } failed to deserialise."; + + private const string SubjectAsmEvent = + "Method Gone from { \"_t\" : \"System.Type\", \"Name\" : \"BH.Revit.Engine.Core.Compute, Revit_Core_Engine_2022, Version=9.0.0.0, Culture=neutral, PublicKeyToken=null\", \"_bhomVersion\" : \"9.2\" } failed to deserialise."; + + private static FakeTestResult Tree(string description, params string[] events) + { + var leaf = new FakeTestInfo + { + Status = "Error", + Description = description, + Message = "Error: Returned null from json.", + Information = events.Select(m => (object)new FakeEventMessage { Message = m }).ToList() + }; + return new FakeTestResult + { + Status = "Error", + Information = [new FakeTestResult { Status = "Error", Information = [leaf] }] + }; + } + + private static ClosureContext Closure(string[] loaded, string[] subject) + { + var l = new HashSet(loaded, StringComparer.Ordinal); + return new ClosureContext( + l, + new HashSet(l.Select(RunCommand.StripConfigSuffix), StringComparer.Ordinal), + new HashSet(subject.Select(RunCommand.StripConfigSuffix), StringComparer.Ordinal)); + } + + // A non-empty candidate list is the runner's record that some OTHER loaded assembly + // answered for the type. + private static Func)> Answered( + params string[] answering) + => (_, _, _) => (null, ClassificationPath.SignatureResolved, answering); + + private static readonly Func)> NothingAnswered = + (_, _, _) => (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty()); + + private static (VersioningResult Result, FailureDiagnostic Diag) Run( + FakeTestResult tree, + Func)> probe, + ClosureContext? closure) + { + var diagnostics = new List(); + var result = RunCommand.ExtractFilteredResult( + tree, _ => true, new List(), + (t, m, a) => probe(t, m, a), diagnostics, closure); + return (result, Assert.Single(diagnostics)); + } + + [Fact] + public void ForeignAssemblyAnsweredByAnother_IsUnverified() + { + var (result, d) = Run( + Tree("BH.Revit.Engine.Core.Compute.TryGetValueFromSource", ModelQaEvent), + Answered("Revit_Core_Engine_2022"), + Closure(loaded: ["Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022", "Revit_oM"])); + + Assert.Equal(0, result.FailureCount); + Assert.False(d.CountedAsReal); + Assert.Equal(ClassificationPath.ForeignDeclaringAssembly, d.Path); + } + + [Fact] + public void ConfigurationVariantNotBuilt_IsUnverified() + { + var (result, d) = Run( + Tree("BH.Revit.Engine.Core.Create.ProjectParameter", Config2024Event), + Answered("Revit_Core_Engine_2022"), + Closure(loaded: ["Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022"])); + + Assert.Equal(0, result.FailureCount); + Assert.False(d.CountedAsReal); + Assert.Equal(ClassificationPath.ConfigurationNotBuilt, d.Path); + } + + // The v1 defect, guarded. With no answering assembly the path is + // DeclaringTypeNotLoaded, which is how a genuinely removed type presents. v1 had no + // candidates precondition and relabelled this as foreign, turning a real removal + // into a silent pass. + [Fact] + public void AbsentAssemblyAndNothingAnswered_StaysReal() + { + var (result, d) = Run( + Tree("BH.Revit.Engine.Core.Compute.Gone", SubjectAsmEvent), + NothingAnswered, + Closure(loaded: ["Revit_oM"], subject: ["Revit_oM"])); + + Assert.Equal(1, result.FailureCount); + Assert.True(d.CountedAsReal); + Assert.Equal(ClassificationPath.DeclaringTypeNotLoaded, d.Path); + } + + [Fact] + public void DeclaringAssemblyPresent_IsUntouched() + { + var (result, d) = Run( + Tree("BH.Revit.Engine.Core.Create.ProjectParameter", Config2024Event), + Answered("Revit_Core_Engine_2024"), + Closure(loaded: ["Revit_Core_Engine_2024", "Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022"])); + + Assert.Equal(1, result.FailureCount); + Assert.True(d.CountedAsReal); + Assert.Equal(ClassificationPath.SignatureResolved, d.Path); + } + + [Fact] + public void NoClosureSupplied_IsUntouched() + { + var (result, d) = Run( + Tree("BH.Revit.Engine.Core.Compute.TryGetValueFromSource", ModelQaEvent), + Answered("Revit_Core_Engine_2022"), + closure: null); + + Assert.Equal(1, result.FailureCount); + Assert.True(d.CountedAsReal); + } + + // The whole family is gone, not just one configuration of it. Nothing distinguishes + // that from a deliberate removal, so it must not be excused. + [Fact] + public void SubjectFamilyWithNoLoadedVariant_StaysReal() + { + var (result, d) = Run( + Tree("BH.Revit.Engine.Core.Create.ProjectParameter", Config2024Event), + Answered("Revit_oM"), + Closure(loaded: ["Revit_oM"], subject: ["Revit_Core_Engine_2022", "Revit_oM"])); + + Assert.Equal(1, result.FailureCount); + Assert.True(d.CountedAsReal); + Assert.Equal(ClassificationPath.SignatureResolved, d.Path); + } + + [Fact] + public void AlreadyUnverified_IsNotRelabelled() + { + var (result, d) = Run( + Tree("BH.Revit.Engine.Core.Compute.TryGetValueFromSource", ModelQaEvent), + (_, _, _) => ("RevitAPI", ClassificationPath.SignatureBlockerOutsideBHoM, new[] { "Revit_Core_Engine_2022" }), + Closure(loaded: ["Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022"])); + + Assert.Equal(0, result.FailureCount); + Assert.False(d.CountedAsReal); + Assert.Equal(ClassificationPath.SignatureBlockerOutsideBHoM, d.Path); + Assert.Equal("RevitAPI", d.Cause); + } + + [Theory] + [InlineData("Revit_Core_Engine_2024", "Revit_Core_Engine")] + [InlineData("Revit_Core_Engine", "Revit_Core_Engine")] + [InlineData("Structure_oM", "Structure_oM")] + // Documents a known limitation: the heuristic cannot tell a Revit release year from + // any other four-digit 20xx suffix. No such assembly exists in the fleet today + // (measured: 295 of 640 match, all prefixed Revit), but nothing enforces that. + [InlineData("Eurocode_2004", "Eurocode")] + [InlineData("Foo_1999", "Foo_1999")] + public void StripConfigSuffix_CollapsesOnlyA20xxTail(string input, string expected) + => Assert.Equal(expected, RunCommand.StripConfigSuffix(input)); + + // Documents current behaviour and a residual risk: the declaring assembly is taken + // from the FIRST parsable Method event, so a finding carrying events for several + // assemblies is decided by the first. A present assembly later in the list does not + // stop the finding being excused. + [Fact] + public void MultipleMethodEvents_TheFirstNamedAssemblyDecides() + { + var (result, d) = Run( + Tree("BH.Revit.Engine.Core.Compute.TryGetValueFromSource", ModelQaEvent, SubjectAsmEvent), + Answered("Revit_Core_Engine_2022"), + Closure(loaded: ["Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022"])); + + Assert.Equal("Revit_ModelQA_Engine_2022", d.DeclaringAssembly); + Assert.Equal(0, result.FailureCount); + Assert.Equal(ClassificationPath.ForeignDeclaringAssembly, d.Path); + } + } +} diff --git a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs index bf38485..28cfb95 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs @@ -58,6 +58,29 @@ public static int Execute( // comes back as CustomObject or null, so measured on a real PR that filter // attributed 1056 of 1056 failures to a repo that owned none of them. // Attribute to the exact namespaces the subject repo's own assemblies declare. + // + // What this run built, which the classifier needs to read a missing declaring + // assembly correctly. Null when no subject build dir was supplied: with whole-closure + // attribution there is no basis for calling any assembly foreign, so the + // reclassification is disabled and the fail-safe default stands. + ClosureContext? closure = null; + if (subjectBuildDir is not null && Directory.Exists(subjectBuildDir)) + { + var loadedNames = new HashSet( + loaded.Select(a => { try { return a.GetName().Name; } catch { return null; } }) + .Where(n => !string.IsNullOrEmpty(n))!, + StringComparer.Ordinal); + var subjectBases = new HashSet( + Directory.GetFiles(subjectBuildDir, "*.dll", SearchOption.AllDirectories) + .Select(f => StripConfigSuffix(Path.GetFileNameWithoutExtension(f))), + StringComparer.Ordinal); + if (subjectBases.Count > 0) + closure = new ClosureContext( + loadedNames, + new HashSet(loadedNames.Select(StripConfigSuffix), StringComparer.Ordinal), + subjectBases); + } + var subjectNamespaces = BuildSubjectNamespaces(loaded, subjectBuildDir); Func isAttributable; if (subjectNamespaces is null) @@ -122,7 +145,7 @@ public static int Execute( return 1; } - var partial = ExtractFilteredResult(rawResult, isAttributable, unresolvableSkips, probeSignature, diagnostics); + var partial = ExtractFilteredResult(rawResult, isAttributable, unresolvableSkips, probeSignature, diagnostics, closure); allFailures.AddRange(partial.Failures); } @@ -208,7 +231,11 @@ public static int Execute( if (configuration is not null) Console.WriteLine($"Configuration: {configuration}"); - int ambiguous = diagnostics.Count(d => d.DeclaringTypeCandidates is { Count: > 1 }); + // Counted over real findings only, because the per-finding note below is printed from + // result.Failures. Counting every diagnostic made the total disagree with the detail as + // soon as a finding could be reclassified to unverified: the warning claimed N ambiguous + // findings while fewer than N were listed. + int ambiguous = diagnostics.Count(d => d.CountedAsReal && d.DeclaringTypeCandidates is { Count: > 1 }); if (ambiguous > 0) Console.Error.WriteLine( $"::warning title=Versioning::{ambiguous} finding(s) have a declaring type present in more than one loaded assembly, " + @@ -350,7 +377,8 @@ public static VersioningResult ExtractFilteredResult(object? rawResult, List isAttributable, List? unresolvableSkips = null, Func Candidates)>? probeSignature = null, - List? diagnostics = null) + List? diagnostics = null, + ClosureContext? closure = null) { if (rawResult is null) return new VersioningResult @@ -361,7 +389,7 @@ public static VersioningResult ExtractFilteredResult( }; var failures = new List(); - CollectLeafFailures(rawResult, isAttributable, failures, unresolvableSkips, probeSignature, diagnostics, depth: 0); + CollectLeafFailures(rawResult, isAttributable, failures, unresolvableSkips, probeSignature, diagnostics, closure, depth: 0); var status = failures.Count > 0 ? VersioningStatus.Error : VersioningStatus.Pass; return new VersioningResult @@ -377,7 +405,7 @@ private static void CollectLeafFailures( object node, Func isAttributable, List failures, List? unresolvableSkips, Func Candidates)>? probeSignature, - List? diagnostics, int depth) + List? diagnostics, ClosureContext? closure, int depth) { // BHoM's TestResult tree has at most 3 levels under the root (outer → per-version // summary → individual type result). Depth 5 gives headroom for unexpected nesting @@ -450,6 +478,39 @@ private static void CollectLeafFailures( path = ClassificationPath.ProbeNotSupplied; } + // Reclassify a finding whose recorded declaring assembly is not in + // this closure. + // + // candidates.Count > 0 is load-bearing and is the difference from v1. It means + // some OTHER loaded assembly answered for the type, so the probe verdict above + // describes code we were never asked about. When nothing answered, the path is + // DeclaringTypeNotLoaded, which is the signal that the type is genuinely gone; + // reclassifying that would convert a real removal into a silent pass. + if (cause is null && closure is not null && declaringAssembly is not null + && candidates.Count > 0 + && !closure.LoadedNames.Contains(declaringAssembly)) + { + string baseName = StripConfigSuffix(declaringAssembly); + if (!closure.SubjectBaseNames.Contains(baseName)) + { + // Nothing this repository builds under any configuration, and something + // else answered for the type, so the entry is another repository's. + cause = $"{declaringAssembly} (declaring assembly is not part of this repository)"; + path = ClassificationPath.ForeignDeclaringAssembly; + } + else if (closure.LoadedBaseNames.Contains(baseName)) + { + // Ours, and a sibling configuration of the same family is loaded, so the + // family exists and only this configuration was not compiled. + cause = $"{declaringAssembly} (build configuration not compiled in this run)"; + path = ClassificationPath.ConfigurationNotBuilt; + } + // Otherwise the family is ours but no configuration of it is loaded at all. + // "Not compiled" and "removed outright" are indistinguishable there, so the + // finding is left real. Ordering matters: folding this into the condition + // above makes the branch unreachable, which is how v1 lost it. + } + if (cause is not null) unresolvableSkips?.Add(new UnverifiedFailure(label, cause)); else @@ -469,7 +530,7 @@ private static void CollectLeafFailures( { string childStatus = child.GetType().GetProperty("Status")?.GetValue(child)?.ToString() ?? "Pass"; if (childStatus is "Error" or "Warning") - CollectLeafFailures(child, isAttributable, failures, unresolvableSkips, probeSignature, diagnostics, depth + 1); + CollectLeafFailures(child, isAttributable, failures, unresolvableSkips, probeSignature, diagnostics, closure, depth + 1); } } } @@ -715,6 +776,13 @@ public static (string? DeclaringType, string? MethodName) ParseMethodEvent(strin // reached through two helper layers that take no context parameter, and threading one // through both for two constant values would be a wider change than the values justify. private static string? s_configuration; + // Collapses a build-configuration suffix to the family name. Anchored and + // restricted to 20xx because that is the only config-variant convention in the fleet + // today: measured 295 of 640 assemblies match, all Revit, all with a sibling variant. + // It is a naming heuristic over an unenforced convention, not a declared relationship. + internal static string StripConfigSuffix(string assemblyName) + => Regex.Replace(assemblyName, @"_20\d{2}$", string.Empty); + private static HashSet? s_versionConditional; private static int s_subjectAssemblyCount; private static int s_subjectTypeCount; diff --git a/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs b/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs index aceb3ac..107a7a9 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs @@ -38,8 +38,26 @@ public enum ClassificationPath SignatureBlockerInsideBHoM, // No probe was supplied by the caller, which happens only in unit tests. ProbeNotSupplied, + // The dataset record names a declaring assembly that is not in this + // closure, and the type was resolved from a different assembly instead. The entry + // describes another repository's code, so no verdict on it is available here. + ForeignDeclaringAssembly, + // As above, but the absent assembly is a build-configuration variant of + // one the subject did build (Revit_Core_Engine_2024 against a Release/2022 build), so + // the code exists in the repo and simply was not compiled in this run. + ConfigurationNotBuilt, } +// What this run actually built, needed to tell "the recorded declaring +// assembly is missing because it is someone else's" from "because we did not compile that +// configuration" from "because it was genuinely removed". Passed explicitly rather than +// held in static state: the runner is a single-shot process but the tests are not, and +// static state cannot be arranged per-case. +public sealed record ClosureContext( + IReadOnlySet LoadedNames, + IReadOnlySet LoadedBaseNames, + IReadOnlySet SubjectBaseNames); + // Whether the failing method's signature is version-conditional in the subject's source. // // Three states, deliberately. An empty grep result is not the same as "not From 29ba74985fd8d91544150cb6d10289b0f9c0d671 Mon Sep 17 00:00:00 2001 From: sakanni Date: Wed, 19 Aug 2026 17:26:55 +0100 Subject: [PATCH 2/2] feat: build the calling repo's Release alternate configurations in ci-versioning --- .github/actions/ci-versioning/action.yml | 48 ++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/.github/actions/ci-versioning/action.yml b/.github/actions/ci-versioning/action.yml index 026d754..c0f35ae 100644 --- a/.github/actions/ci-versioning/action.yml +++ b/.github/actions/ci-versioning/action.yml @@ -163,6 +163,54 @@ runs: dotnet restore "${{ steps.solution.outputs.path }}" dotnet build "${{ steps.solution.outputs.path }}" --no-restore -c Release --nologo -m -clp:ErrorsOnly + # Build the repo's own Release* alternate configurations as well. + # + # The versioning datasets record the assembly that declared each method, and on Revit + # repos that is a year-suffixed assembly per configuration: Revit_Core_Engine_2022 + # through _2026. A Release-only build produces one of them, so every dataset entry + # naming another cannot be evaluated at all. BHoMBot does not have this problem + # because CloneInstaller.cs:23 runs BuildConfigs over the installer's altConfigs.txt + # before versioning, so its ProgramData carries every configuration. + # + # Measured on a sandbox Revit_Toolkit, windows-2025-vs2026, n=3: the first + # configuration takes 47-59s and carries the whole warm-up, each additional one takes + # 4-8s (median 5), and Build\ grows 1.2 MB per configuration. Total 76s against a 47s + # Release-only baseline, so 1.6x rather than the 5x a per-config estimate suggests. + # + # Debug* entries are skipped: only Release* ships, matching the installer. On + # Revit_Toolkit, Release2022 duplicates plain Release (same REVIT2022 constant, same + # assembly name). It is left in rather than special-cased, because detecting that + # requires reading DefineConstants and the saving is six seconds. + $altFile = Join-Path "${{ github.workspace }}" 'altConfigs.txt' + if (Test-Path $altFile) { + $repo = "${{ github.repository }}" + $configs = Get-Content $altFile | + ForEach-Object { $_.Trim() } | + Where-Object { $_ } | + ForEach-Object { + # Lines are 'org/repo/ConfigName'. Some files list other repos, and building + # those here would apply their configuration name to this source tree. + $parts = $_.Split('/') + if ($parts.Count -ge 3 -and "$($parts[0])/$($parts[1])" -eq $repo) { $parts[2] } + } | + Where-Object { $_ -like 'Release*' } | + Select-Object -Unique + + if ($configs) { + Write-Host "::notice title=Versioning::Building $($configs.Count) alternate configuration(s): $($configs -join ', ')." + foreach ($c in $configs) { + dotnet build "${{ steps.solution.outputs.path }}" --no-restore -c $c --nologo -m -clp:ErrorsOnly + if ($LASTEXITCODE -ne 0) { + # Loud, not soft. A configuration that will not compile is a build problem, and + # continuing silently would put us back to judging methods whose declaring + # assembly was never produced, which is the defect this exists to remove. + Write-Host "::error title=Versioning::Configuration '$c' failed to build." + exit 1 + } + } + } + } + # There is deliberately no build-completeness fast-fail here any more. # # A "Fast-fail on missing versioning-critical DLLs" step used to download