diff --git a/.github/actions/ci-versioning/action.yml b/.github/actions/ci-versioning/action.yml index a7ecddd..d632d7c 100644 --- a/.github/actions/ci-versioning/action.yml +++ b/.github/actions/ci-versioning/action.yml @@ -149,6 +149,27 @@ runs: uses: BHoM/CI_Toolkit/.github/actions/discover-solution@develop with: check_title: Versioning + # --- Subject assembly bracket: opens here, closes after the alt-config builds --- + # + # The two steps between these snapshots are the only ones that build this repository, and + # everything a BHoM project builds is staged into the assembly directory by its PostBuild + # step. So the difference between the two snapshots is exactly this repository's own output. + # + # NOTHING MAY BE INSERTED BETWEEN THEM. Any step that builds, restores or copies into the + # assembly directory would be attributed to this repository and widen the subject set. + # Later steps deliberately fall outside: the Revit mocks, the upgrades recapture and the + # verification-solution build all stage assemblies that are not this repository's. + # ci-versioning-action.Tests.ps1 fails if a step is added inside the bracket. + - name: Snapshot staged assemblies (before subject build) + id: stage_before + if: steps.changed.outputs.count != '0' + shell: pwsh + run: | + $script = Join-Path $env:GITHUB_ACTION_PATH "../../scripts/Get-StagedAssemblies.ps1" + . $script + Get-AssemblyStamp -Path 'C:\ProgramData\BHoM\Assemblies' | + Set-Content -Path 'staged-before.txt' -Encoding utf8 + Write-Host "Staged before subject build: $(@(Get-Content 'staged-before.txt' -ErrorAction SilentlyContinue).Count) assembl(ies)." - name: Build primary repo id: build_subject @@ -186,6 +207,30 @@ runs: $script = Join-Path $env:GITHUB_ACTION_PATH "../../scripts/Build-AltConfigs.ps1" & $script -SlnPath "${{ steps.solution.outputs.path }}" -Configuration "Release" + # --- Subject assembly bracket: closes here --- + - name: Collect subject assemblies + id: subject_set + if: steps.changed.outputs.count != '0' + shell: pwsh + run: | + $script = Join-Path $env:GITHUB_ACTION_PATH "../../scripts/Get-StagedAssemblies.ps1" + . $script + + $before = @(Get-Content 'staged-before.txt' -ErrorAction SilentlyContinue) + $after = Get-AssemblyStamp -Path 'C:\ProgramData\BHoM\Assemblies' + $subject = @(Get-NewlyStagedAssemblies -Before $before -After $after) + + # Alt configurations are inside the bracket on purpose. A Revit repository's + # year-suffixed assemblies are its own code, so attributing failures in them to it is + # correct, and excluding them would understate what the check covers. + $subject | Set-Content -Path 'subject-assemblies.txt' -Encoding utf8 + Write-Host "Subject assemblies staged by this repository's build: $($subject.Count)." + if ($subject.Count -gt 0) { + Write-Host "::group::Subject assemblies" + $subject | ForEach-Object { Write-Host " $_" } + Write-Host "::endgroup::" + } + # There is deliberately no build-completeness fast-fail here any more. # # A "Fast-fail on missing versioning-critical DLLs" step used to download @@ -557,46 +602,40 @@ runs: } Write-Host "::notice title=Versioning::Found versioning datasets for: $($versions.Name -join ', ')" - # Precondition, not a policy choice. The runner is handed - # --subject-assemblies "\Build" and restricts attribution to the - # namespaces the assemblies there declare. If that directory is absent it falls - # back to attributing every failure across the whole dependency closure, which - # reports other repositories' defects against this one: measured at 1,056 of 1,056 - # on a real pull request. The runner does warn, but on stderr, and a warning does - # not stop the check producing a verdict it has no basis for. - # - # Asserted here rather than left to the runner because this is a property of the - # build, and the build is this action's job. Same shape as the dataset guard above. + # Precondition, not a policy choice. Attribution narrows to the namespaces this + # repository's own assemblies declare; with no assemblies there is nothing to narrow to, + # and the runner would widen to the whole dependency closure and report other + # repositories' failures against this one. Measured at 1,056 of 1,056 on a real pull + # request. # - # Not the vacuous-green question. "Nothing to check" is a legitimate green; - # "the thing to check was never built" is a broken precondition. This is the second. + # Asserted here rather than in the runner because it is a property of the build, and the + # build is this action's job. Same shape as the datasets guard above. # - # Two causes when it fires, and they need different fixes. Either the projects declare - # ..\Build\ only inside PropertyGroups conditioned on Debug or Test, so a - # Release build does not apply it; or they declare no at all and take the - # SDK default. Both send output to bin\\\ and leave Build\ absent. + # Not the vacuous-green question. "Nothing to check" is a legitimate green; "the thing to + # check was never built" is a broken precondition. This is the second. # - # Neither is a fault in those repositories. Nothing ever verified this directory: the - # convention was enforced by a linter that only rewrote lines already - # present, and no check read Build\ until this one. + # When this fires it is a build problem, not a configuration one: the solution built, but + # staged no assemblies into the shared assembly directory. Every BHoM project stages its + # output there through a PostBuild step, so a repository producing none either built + # nothing or has lost that step. - name: Validate subject build output if: steps.changed.outputs.count != '0' shell: pwsh run: | - $subjectDir = "${{ github.workspace }}\Build" + $listPath = 'subject-assemblies.txt' - if (-not (Test-Path $subjectDir)) { - Write-Host "::error title=Versioning::Subject build output missing at $subjectDir. This repository's own assemblies were not built to the directory the check attributes against, so no failure could be attributed to it. Two usual causes: the projects declare ..\Build\ only under Debug or Test conditions, which a Release build does not apply; or they declare no at all and take the SDK default. Either way the output went to bin\Release\\ instead." + if (-not (Test-Path $listPath)) { + Write-Host "::error title=Versioning::Subject assembly list missing. The step that collects this repository's build output did not run, so attribution has no subject set." exit 1 } - $dlls = @(Get-ChildItem $subjectDir -Filter *.dll -Recurse -ErrorAction SilentlyContinue) - if ($dlls.Count -eq 0) { - Write-Host "::error title=Versioning::Subject build output at $subjectDir contains no assemblies. The directory exists but nothing was built into it, so no failure could be attributed to this repository." + $subject = @(Get-Content $listPath | Where-Object { $_.Trim() }) + if ($subject.Count -eq 0) { + Write-Host "::error title=Versioning::This repository's build staged no assemblies. The solution built, but nothing reached C:\ProgramData\BHoM\Assemblies, so no failure could be attributed to this repository. Every BHoM project stages its output there through a PostBuild step; a repository producing none either built no assemblies or is missing that step." exit 1 } - Write-Host "::notice title=Versioning::Subject build output: $($dlls.Count) assembl(ies) in $subjectDir." + Write-Host "::notice title=Versioning::Subject set: $($subject.Count) assembl(ies) staged by this repository's build." - name: Prepare VersioningRunner id: runner @@ -670,7 +709,7 @@ runs: if: steps.changed.outputs.count != '0' shell: pwsh run: | - # --subject-assemblies restricts attribution to the namespaces this repo's own + # --subject-assembly-list restricts attribution to the namespaces this repo's own # assemblies declare. Without it, failures are attributed on a 3-segment # namespace prefix over the whole dependency closure, which cannot distinguish # BH.oM.Adapters.File from BH.oM.Adapters.ETABS: the v9.2 datasets name thousands @@ -690,7 +729,7 @@ runs: # annotations were produced, against two on a comparable production run. & "${{ steps.runner.outputs.runner_exe }}" ` --assemblies 'C:\ProgramData\BHoM\Assemblies' ` - --subject-assemblies "${{ github.workspace }}\Build" ` + --subject-assembly-list 'subject-assemblies.txt' ` --configuration 'Release' ` $(if ('${{ steps.vercond.outputs.file }}') { '--version-conditional', '${{ steps.vercond.outputs.file }}' }) ` --output 'versioning-result.json' | @@ -731,10 +770,19 @@ runs: $attribution = '(not reported)' $classification = '(not reported)' + # Which evidence attributed each finding. Surfaced here and not only in the log because + # the number that matters is the namespace-fallback count: that path cannot tell this + # repository's types from those of repositories extending its namespace, so a non-zero + # value means some findings may not be this repository's. The runner also emits a + # ::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)' if (Test-Path 'versioning-stdout.txt') { $out = Get-Content 'versioning-stdout.txt' -Raw - if ($out -match 'Attribution:\s*(.+)') { $attribution = $Matches[1].Trim() } - if ($out -match 'Classification:\s*(.+)') { $classification = $Matches[1].Trim() } + # '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() } } $rows = @() @@ -773,6 +821,7 @@ runs: $md += "| | |" $md += "|---|---|" $md += "| Classification | ``$classification`` |" + $md += "| Attribution basis | ``$attributionBasis`` |" $md += "| Reported unverified | $unverified |" if ($coverage) { $md += "| Surface examined | $($coverage.SubjectTypes) subject types across $($coverage.SubjectAssemblies) subject assemblies |" diff --git a/.github/scripts/Get-StagedAssemblies.ps1 b/.github/scripts/Get-StagedAssemblies.ps1 new file mode 100644 index 0000000..5c92d0c --- /dev/null +++ b/.github/scripts/Get-StagedAssemblies.ps1 @@ -0,0 +1,80 @@ +# Get-StagedAssemblies.ps1 — works out which assemblies a build staged, by comparing the +# assembly directory before and after it. +# +# Dot-sourced by .github/actions/ci-versioning/action.yml and by +# .github/scripts/tests/Get-StagedAssemblies.Tests.ps1. Defines functions and does nothing +# else, so dot-sourcing has no side effects. +# +# Why this exists. The versioning check attributes failures only to namespaces the repository +# under test declares, so it needs to know which assemblies are the repository's own. It used +# to read them from a `Build\` directory at the workspace root, on the assumption that every +# project wrote there. Nothing guaranteed that assumption and it was false for roughly a third +# of the fleet, differently under each build configuration, so the check either widened to the +# whole dependency closure and reported other repositories' failures, or attributed against a +# fraction of the repository with nothing to say so. +# +# Every BHoM project stages its output to the shared assembly directory through a PostBuild +# step, and that is the directory the runner reflects over. So the set staged during the +# subject build is both what the repository produced and what the runner can actually see. + +function Get-AssemblyStamp { + <# + .SYNOPSIS + A stable identity per assembly file: name and last-write time. + + .DESCRIPTION + The write time is part of the identity on purpose. A repository can produce an assembly + with the same file name as one already staged by a dependency, and the staging step + overwrites it in place. Comparing names alone would treat that as unchanged and drop the + repository's own assembly from its subject set, which is the failure this whole mechanism + exists to remove — silently attributing against an incomplete set. + + .PARAMETER Path + Assembly directory. A missing directory yields an empty stamp set rather than throwing, + so the caller decides what an empty result means. + #> + [CmdletBinding()] + param([Parameter(Mandatory)][string]$Path) + + if (-not (Test-Path $Path)) { return @() } + + return @( + Get-ChildItem -LiteralPath $Path -Filter *.dll -File -ErrorAction SilentlyContinue | + ForEach-Object { "$($_.Name)|$($_.LastWriteTimeUtc.Ticks)" } + ) +} + +function Get-NewlyStagedAssemblies { + <# + .SYNOPSIS + The assembly names present after a build that were not present, identically, before it. + + .DESCRIPTION + Pure over its two inputs so the comparison can be tested without a build. Returns names + rather than stamps, because the runner identifies an assembly by file name. + + An entry counts as newly staged when its name-and-time pair is absent from the before + set. That covers both shapes: an assembly that did not exist before, and one that existed + and was overwritten. + + .PARAMETER Before, After + Stamp collections from Get-AssemblyStamp. + #> + [CmdletBinding()] + param( + [string[]]$Before = @(), + [string[]]$After = @() + ) + + $seen = [System.Collections.Generic.HashSet[string]]::new( + [string[]]@($Before), [System.StringComparer]::OrdinalIgnoreCase) + + $names = foreach ($entry in @($After)) { + if (-not $seen.Contains($entry)) { ($entry -split '\|', 2)[0] } + } + + # Emitted as a sequence, not wrapped. A comma-wrap here would return an array containing + # the array, which counts as one element and silently breaks any caller that measures it. + # Callers that need a definite collection wrap with @(), which is the repository's idiom. + return @($names | Sort-Object -Unique) +} diff --git a/.github/scripts/tests/Get-StagedAssemblies.Tests.ps1 b/.github/scripts/tests/Get-StagedAssemblies.Tests.ps1 new file mode 100644 index 0000000..641cf5b --- /dev/null +++ b/.github/scripts/tests/Get-StagedAssemblies.Tests.ps1 @@ -0,0 +1,142 @@ +# Get-StagedAssemblies.Tests.ps1 — Pester tests for the staged-assembly comparison. +# +# The comparison decides which assemblies the versioning check treats as the repository's own, +# so getting it wrong reproduces the defect it replaces: a subject set that looks plausible and +# silently covers part of the repository. +# +# Get-NewlyStagedAssemblies is pure over two stamp collections, so every case below is checked +# without a build. Get-AssemblyStamp touches the filesystem and is tested against real temp +# directories, because its contract is about what it does with files that are missing, nested +# or not assemblies. +# +# Run locally: pwsh -Command "Invoke-Pester .github/scripts/tests -Output Detailed" +# Run in CI: lint-workflows.yml, the powershell-tests job. + +BeforeAll { + $repoRoot = (Resolve-Path (Join-Path $PSScriptRoot '../../..')).Path + . (Join-Path $repoRoot '.github/scripts/Get-StagedAssemblies.ps1') +} + +Describe 'Get-NewlyStagedAssemblies' { + + Context 'the ordinary case' { + + It 'returns assemblies that were not there before' { + $before = @('Dep_oM.dll|100', 'Dep_Engine.dll|100') + $after = @('Dep_oM.dll|100', 'Dep_Engine.dll|100', 'Subject_oM.dll|200') + + Get-NewlyStagedAssemblies -Before $before -After $after | Should -Be @('Subject_oM.dll') + } + + It 'returns nothing when the build staged nothing' { + $stamps = @('Dep_oM.dll|100', 'Dep_Engine.dll|100') + + @(Get-NewlyStagedAssemblies -Before $stamps -After $stamps).Count | Should -Be 0 + } + + It 'returns every new assembly, not just the first' { + $after = @('A_oM.dll|200', 'B_Engine.dll|200', 'C_Adapter.dll|200') + + Get-NewlyStagedAssemblies -Before @() -After $after | + Should -Be @('A_oM.dll', 'B_Engine.dll', 'C_Adapter.dll') + } + } + + Context 'the name-collision case, which a name-only comparison gets wrong' { + + # A repository can produce an assembly whose file name already exists in the staging + # directory from a dependency. The staging step overwrites it in place, so the name is + # present both before and after. Comparing names alone concludes nothing changed and + # drops the repository's own assembly from its subject set — which is exactly the + # partial-subject failure this mechanism replaces, reintroduced one layer down. + It 'detects an assembly overwritten in place' { + $before = @('Shared_oM.dll|100') + $after = @('Shared_oM.dll|200') + + Get-NewlyStagedAssemblies -Before $before -After $after | Should -Be @('Shared_oM.dll') + } + + It 'distinguishes an overwrite from an untouched file with the same name' { + $before = @('Untouched_oM.dll|100', 'Overwritten_oM.dll|100') + $after = @('Untouched_oM.dll|100', 'Overwritten_oM.dll|555') + + Get-NewlyStagedAssemblies -Before $before -After $after | Should -Be @('Overwritten_oM.dll') + } + + It 'reports an overwritten assembly once, not twice' { + # It appears in both inputs; the result is a set of names, not a concatenation. + $r = Get-NewlyStagedAssemblies -Before @('X_oM.dll|1') -After @('X_oM.dll|2') + @($r).Count | Should -Be 1 + } + } + + Context 'shape' { + + # The contract is a sequence of names that counts correctly when the caller wraps it + # with @(), which is what the action does before writing one line per entry. An earlier + # version comma-wrapped the return, which produced an array containing the array: it + # counted as one element regardless of content, so an empty result and a three-element + # result were indistinguishable to the caller. + It 'counts correctly for one result' { + @(Get-NewlyStagedAssemblies -Before @() -After @('Only_oM.dll|1')).Count | Should -Be 1 + } + + It 'counts correctly for no result' { + @(Get-NewlyStagedAssemblies -Before @('A.dll|1') -After @('A.dll|1')).Count | Should -Be 0 + } + + It 'counts correctly for many results' { + @(Get-NewlyStagedAssemblies -Before @() -After @('A.dll|1','B.dll|1','C.dll|1')).Count | Should -Be 3 + } + + It 'tolerates empty inputs on both sides' { + @(Get-NewlyStagedAssemblies -Before @() -After @()).Count | Should -Be 0 + } + } +} + +Describe 'Get-AssemblyStamp' { + + BeforeAll { + $script:dir = Join-Path ([IO.Path]::GetTempPath()) ("stamp-" + [Guid]::NewGuid().ToString('N')) + New-Item -ItemType Directory -Force -Path $dir | Out-Null + New-Item -ItemType Directory -Force -Path (Join-Path $dir 'nested') | Out-Null + Set-Content -Path (Join-Path $dir 'One_oM.dll') -Value 'x' + Set-Content -Path (Join-Path $dir 'Two_Engine.dll') -Value 'x' + Set-Content -Path (Join-Path $dir 'notes.txt') -Value 'x' + Set-Content -Path (Join-Path $dir 'nested\Deep_oM.dll') -Value 'x' + } + + AfterAll { Remove-Item $dir -Recurse -Force -ErrorAction SilentlyContinue } + + It 'stamps each assembly with its name and write time' { + $s = Get-AssemblyStamp -Path $dir + ($s | Where-Object { $_ -like 'One_oM.dll|*' }).Count | Should -Be 1 + } + + It 'ignores files that are not assemblies' { + Get-AssemblyStamp -Path $dir | Should -Not -Contain 'notes.txt' + (Get-AssemblyStamp -Path $dir | Where-Object { $_ -like 'notes*' }).Count | Should -Be 0 + } + + # The staging directory is flat: every project copies its own output into it. Recursing + # would pick up anything a dependency happened to nest there and attribute it to the + # repository under test. + It 'does not recurse' { + (Get-AssemblyStamp -Path $dir | Where-Object { $_ -like 'Deep_oM*' }).Count | Should -Be 0 + } + + It 'returns empty for a directory that does not exist, rather than throwing' { + { Get-AssemblyStamp -Path (Join-Path $dir 'no-such-place') } | Should -Not -Throw + @(Get-AssemblyStamp -Path (Join-Path $dir 'no-such-place')).Count | Should -Be 0 + } + + It 'round-trips through the comparison to name a genuinely new assembly' { + $before = Get-AssemblyStamp -Path $dir + Start-Sleep -Milliseconds 20 + Set-Content -Path (Join-Path $dir 'Fresh_oM.dll') -Value 'x' + $after = Get-AssemblyStamp -Path $dir + + Get-NewlyStagedAssemblies -Before $before -After $after | Should -Be @('Fresh_oM.dll') + } +} diff --git a/.github/scripts/tests/ci-versioning-action.Tests.ps1 b/.github/scripts/tests/ci-versioning-action.Tests.ps1 index 460f5ca..f04f36c 100644 --- a/.github/scripts/tests/ci-versioning-action.Tests.ps1 +++ b/.github/scripts/tests/ci-versioning-action.Tests.ps1 @@ -30,11 +30,11 @@ Describe 'ci-versioning action.yml' { } It 'guards the subject build output the same way' { - $text | Should -Match 'Subject build output missing at' -Because 'an absent subject directory makes the check attribute every failure across the whole closure' + $text | Should -Match 'Subject assembly list missing' -Because 'without a subject set the check attributes every failure across the whole closure' } - It 'also rejects a subject directory that exists but is empty' { - $text | Should -Match 'contains no assemblies' -Because 'a present but empty directory yields an empty subject set, which reports nothing and passes' + It 'also rejects a subject set that was collected but is empty' { + $text | Should -Match 'staged no assemblies' -Because 'an empty subject set attributes nothing and passes green having measured nothing' } It 'fails rather than warns, because the check cannot do its job without it' { @@ -52,13 +52,54 @@ Describe 'ci-versioning action.yml' { $guardIdx | Should -BeLessThan $runnerIdx } - # Substring rather than regex: the path contains backslashes and braces, and a guard - # that checked a different directory from the one the runner reads would pass a - # loosely-written pattern while protecting nothing. - It 'checks the same directory the runner is pointed at' { - $subjectPath = '"${{ github.workspace }}\Build"' - $text.Contains('$subjectDir = ' + $subjectPath) | Should -BeTrue -Because 'the guard must read the path the runner is given' - $text.Contains('--subject-assemblies ' + $subjectPath) | Should -BeTrue -Because 'if the runner argument changes, this guard stops protecting it' + # The guard and the runner must read the same artefact. If they diverge the guard + # passes on one thing while the runner attributes against another, which is the + # failure it exists to prevent, one level removed. + It 'checks the same artefact the runner is given' { + $artefact = 'subject-assemblies.txt' + $text.Contains("Set-Content -Path '$artefact'") | Should -BeTrue -Because 'the collection step writes it' + $text.Contains("`$listPath = '$artefact'") | Should -BeTrue -Because 'the guard reads it' + $text.Contains("--subject-assembly-list '$artefact'") | Should -BeTrue -Because 'the runner is given it' + } + } + + Context 'the subject-assembly bracket' { + + # The subject set is the difference between two snapshots of the shared assembly + # directory, so it is exactly whatever was staged between them. That makes the bracket + # an ordering assumption in a file where steps get inserted, and an inserted step that + # builds or copies would be silently attributed to the repository under test. + # + # This asserts the shape rather than today's list: it fails when anything is added + # inside the bracket, which is the point. If a step genuinely belongs there, this test + # is the place to say so deliberately. + It 'contains exactly the two steps that build this repository' { + $open = ($lines | Select-String -Pattern '- name: Snapshot staged assemblies' | Select-Object -First 1).LineNumber + $close = ($lines | Select-String -Pattern '- name: Collect subject assemblies' | Select-Object -First 1).LineNumber + + $open | Should -Not -BeNullOrEmpty + $close | Should -BeGreaterThan $open + + $inner = $lines[$open..($close - 2)] | Where-Object { $_ -match '^\s+- name: ' } + @($inner).Count | Should -Be 2 -Because "only the primary and alt-config builds may sit inside the bracket; found:`n$($inner -join "`n")" + ($inner -join ' ') | Should -Match 'Build primary repo' + ($inner -join ' ') | Should -Match 'Build alt configurations' + } + + # A nested action inside the bracket could wipe or repopulate the assembly directory, + # which would corrupt the difference without adding a step name anyone would question. + It 'contains no nested action call' { + $open = ($lines | Select-String -Pattern '- name: Snapshot staged assemblies' | Select-Object -First 1).LineNumber + $close = ($lines | Select-String -Pattern '- name: Collect subject assemblies' | Select-Object -First 1).LineNumber + + $uses = $lines[$open..($close - 2)] | Where-Object { $_ -match '^\s+uses:' } + @($uses).Count | Should -Be 0 -Because "a nested action inside the bracket can change the assembly directory: $($uses -join '; ')" + } + + It 'closes after the alt-config build, so alt configurations are included' { + $alt = ($lines | Select-String -Pattern '- name: Build alt configurations' | Select-Object -First 1).LineNumber + $close = ($lines | Select-String -Pattern '- name: Collect subject assemblies' | Select-Object -First 1).LineNumber + $close | Should -BeGreaterThan $alt -Because 'a Revit repository year-suffixed assemblies are its own code and belong in the subject set' } } } diff --git a/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs b/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs index cf7919b..791a1e0 100644 --- a/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs +++ b/tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs @@ -62,7 +62,7 @@ private static (VersioningResult Result, FailureDiagnostic Diag) Run( { var diagnostics = new List(); var result = RunCommand.ExtractFilteredResult( - tree, _ => true, new List(), + tree, (_, _) => (true, AttributionBasis.NotRecorded), new List(), (t, m, a) => probe(t, m, a), diagnostics, closure); return (result, Assert.Single(diagnostics)); } diff --git a/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs b/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs index 455cf03..4621412 100644 --- a/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs +++ b/tools/VersioningRunner/src/VersioningRunner.Tests/RunCommandTests.cs @@ -365,6 +365,11 @@ public void NonBHoMFiles_ReturnFalse(string path) } } + // IsFromSubjectNamespace is no longer the attribution mechanism. It is the fallback used + // only when a failure records no declaring assembly, because the description alone cannot + // say whether its last segment is a type or a method and so cannot separate a repository's + // own types from those of repositories extending its namespace. These cases still hold for + // what the function does; SubjectAttributionDecisionTests covers which one gets consulted. public class SubjectAttributionTests { private static HashSet Namespaces(params string[] ns) => @@ -428,76 +433,86 @@ public void EmptyDescriptionOrEmptySubjectSet_IsNotAttributed() } [Fact] - public void NoSubjectDirSupplied_FallsBackToWholeClosure() + public void NoSubjectListSupplied_FallsBackToWholeClosure() { + Assert.Null(RunCommand.ReadSubjectAssemblyList(null)); + Assert.Null(RunCommand.ReadSubjectAssemblyList(" ")); Assert.Null(RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], null)); - Assert.Null(RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], " ")); } [Fact] - public void MissingSubjectDir_FallsBackToWholeClosure() + public void MissingSubjectList_FallsBackToWholeClosure() { - string missing = Path.Combine(Path.GetTempPath(), "versioning-runner-no-such-dir-" + Guid.NewGuid()); - Assert.Null(RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], missing)); + string missing = Path.Combine(Path.GetTempPath(), "versioning-runner-no-such-list-" + Guid.NewGuid()); + Assert.Null(RunCommand.ReadSubjectAssemblyList(missing)); } [Fact] - public void SubjectDirNamingALoadedAssembly_YieldsThatAssemblysNamespaces() + public void EmptySubjectList_IsNotTheSameAsNoList() { - var asm = typeof(object).Assembly; - string dir = Path.Combine(Path.GetTempPath(), "versioning-runner-subject-" + Guid.NewGuid()); - Directory.CreateDirectory(dir); + // Null means nobody said which assemblies are the subject's, so attribution widens + // to the whole closure. Empty means the caller looked and the build staged nothing, + // which the caller fails on before this point. Collapsing the two would turn a + // build failure into a silent whole-closure report. + string list = Path.Combine(Path.GetTempPath(), "versioning-runner-list-" + Guid.NewGuid() + ".txt"); + File.WriteAllText(list, ""); try { - // Only the file name is used; the content is never loaded, because the - // namespaces come off the already-loaded assembly of the same name. - File.WriteAllText(Path.Combine(dir, Path.GetFileName(asm.Location)), ""); - - var ns = RunCommand.BuildSubjectNamespaces([asm], dir); + var names = RunCommand.ReadSubjectAssemblyList(list); - Assert.NotNull(ns); - Assert.Contains("System.Collections.Generic", ns); + Assert.NotNull(names); + Assert.Empty(names!); } - finally { Directory.Delete(dir, recursive: true); } + finally { File.Delete(list); } } [Fact] - public void SubjectAssemblyInATfmSubdirectory_IsFound() + public void SubjectListNamingALoadedAssembly_YieldsThatAssemblysNamespaces() { - // SDK-style repos append the TFM to OutputPath, so the subject's assemblies sit - // in Build\\ rather than Build\. A top-level-only scan silently produced an - // empty subject set, and an empty set means the check cannot fail. var asm = typeof(object).Assembly; - string dir = Path.Combine(Path.GetTempPath(), "versioning-runner-subject-" + Guid.NewGuid()); - string nested = Path.Combine(dir, "netstandard2.0"); - Directory.CreateDirectory(nested); - try - { - File.WriteAllText(Path.Combine(nested, Path.GetFileName(asm.Location)), ""); + var names = RunCommand.ReadSubjectAssemblyListFrom([Path.GetFileName(asm.Location)]); - var ns = RunCommand.BuildSubjectNamespaces([asm], dir); + var ns = RunCommand.BuildSubjectNamespaces([asm], names); - Assert.NotNull(ns); - Assert.Contains("System.Collections.Generic", ns); - } - finally { Directory.Delete(dir, recursive: true); } + Assert.NotNull(ns); + Assert.Contains("System.Collections.Generic", ns); } [Fact] - public void SubjectDirNamingNothingLoaded_YieldsNoNamespacesRatherThanTheClosure() + public void SubjectListEntriesMayBeFullPaths() { - string dir = Path.Combine(Path.GetTempPath(), "versioning-runner-subject-" + Guid.NewGuid()); - Directory.CreateDirectory(dir); - try - { - File.WriteAllText(Path.Combine(dir, "Nothing_Built_Here.dll"), ""); + // The caller writes names, but a full path is the obvious thing for someone to + // write later, and silently matching nothing would empty the subject set. + var asm = typeof(object).Assembly; + var names = RunCommand.ReadSubjectAssemblyListFrom([asm.Location]); - var ns = RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], dir); + var ns = RunCommand.BuildSubjectNamespaces([asm], names); - Assert.NotNull(ns); - Assert.Empty(ns); - } - finally { Directory.Delete(dir, recursive: true); } + Assert.NotNull(ns); + Assert.Contains("System.Collections.Generic", ns); + } + + [Fact] + public void SubjectListIsCaseInsensitive() + { + var asm = typeof(object).Assembly; + var names = RunCommand.ReadSubjectAssemblyListFrom([Path.GetFileName(asm.Location).ToUpperInvariant()]); + + var ns = RunCommand.BuildSubjectNamespaces([asm], names); + + Assert.NotNull(ns); + Assert.NotEmpty(ns!); + } + + [Fact] + public void SubjectListNamingNothingLoaded_YieldsNoNamespacesRatherThanTheClosure() + { + var names = RunCommand.ReadSubjectAssemblyListFrom(["Nothing_Built_Here.dll"]); + + var ns = RunCommand.BuildSubjectNamespaces([typeof(object).Assembly], names); + + Assert.NotNull(ns); + Assert.Empty(ns); } // Strings below are copied verbatim from a real run's EventMessages. @@ -554,7 +569,7 @@ public void UnresolvableLeaf_IsCollectedSeparatelyAndNotFailed() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; var skips = new List(); - var result = RunCommand.ExtractFilteredResult(outer, _ => true, skips); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), skips); Assert.Equal(VersioningStatus.Pass, result.Status); Assert.Equal(0, result.FailureCount); @@ -579,7 +594,7 @@ public void LeafNamingABHoMTypeFailure_StillFails() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; var skips = new List(); - var result = RunCommand.ExtractFilteredResult(outer, _ => true, skips); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), skips); Assert.Equal(VersioningStatus.Error, result.Status); Assert.Equal(1, result.FailureCount); @@ -637,7 +652,7 @@ public void NoTypeEventsButAnUnresolvableSignature_IsUnverifiedNotFailed() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; var skips = new List(); - var result = RunCommand.ExtractFilteredResult(outer, _ => true, skips, + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), skips, probeSignature: (_, _, _) => ("Autodesk.Revit.DB.LogicalOrFilter", ClassificationPath.SignatureBlockerOutsideBHoM, Array.Empty())); Assert.Equal(0, result.FailureCount); @@ -665,7 +680,7 @@ public void MangledDescriptionOnARealFailure_IsRelabelledFromTheMethodEvent() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; // Probe finds no blocker, so this stays a real failure — but actionable. - var result = RunCommand.ExtractFilteredResult(outer, _ => true, null, probeSignature: (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty())); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), null, probeSignature: (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty())); Assert.Equal(1, result.FailureCount); Assert.Equal("BH.Revit.Engine.MechanicalPlumbing.Create.SomethingGenuine", @@ -687,7 +702,7 @@ public void UnmangledDescriptionAlongsideAMethodEvent_IsStillLeftAlone() var versionSummary = new FakeTestResult { Status = "Error", Information = [leaf] }; var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; - var result = RunCommand.ExtractFilteredResult(outer, _ => true, null, probeSignature: (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty())); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), null, probeSignature: (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty())); Assert.Equal(1, result.FailureCount); Assert.Equal("BH.oM.Adapters.File.FileSettings", result.Failures[0].Description); @@ -706,7 +721,7 @@ public void UnmangledDescription_IsLeftAlone() var versionSummary = new FakeTestResult { Status = "Error", Information = [leaf] }; var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; - var result = RunCommand.ExtractFilteredResult(outer, _ => true); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded)); Assert.Equal(1, result.FailureCount); Assert.Equal("BH.oM.Adapters.File.FileSettings", result.Failures[0].Description); @@ -725,11 +740,11 @@ public void ExtractFilteredResult_HonoursTheSuppliedPredicate() var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; var subject = Namespaces("BH.oM.Adapters.File"); - var filtered = RunCommand.ExtractFilteredResult(outer, d => RunCommand.IsFromSubjectNamespace(d, subject)); + var filtered = RunCommand.ExtractFilteredResult(outer, (d, _) => (RunCommand.IsFromSubjectNamespace(d, subject), AttributionBasis.NamespaceFallback)); Assert.Equal(VersioningStatus.Pass, filtered.Status); Assert.Equal(0, filtered.FailureCount); - var kept = RunCommand.ExtractFilteredResult(outer, _ => true); + var kept = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded)); Assert.Equal(VersioningStatus.Error, kept.Status); Assert.Equal(1, kept.FailureCount); } @@ -763,7 +778,7 @@ public void CauseFromEvents_IsRecordedAsUnresolvableFromEvents() { var diagnostics = new List(); RunCommand.ExtractFilteredResult(Tree("BH.oM.Adapters.File.FileSettings", RevitCause), - _ => true, null, null, diagnostics); + (_, _) => (true, AttributionBasis.NotRecorded), null, null, diagnostics); var only = Assert.Single(diagnostics); Assert.False(only.CountedAsReal); @@ -778,7 +793,7 @@ public void NoMethodEvent_IsRecordedAsSuch_AndCountedReal() // real by default rather than by evidence. var diagnostics = new List(); var result = RunCommand.ExtractFilteredResult(Tree("BH.oM.Adapters.File.FileSettings"), - _ => true, null, (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty()), diagnostics); + (_, _) => (true, AttributionBasis.NotRecorded), null, (_, _, _) => (null, ClassificationPath.NoOverloadFound, Array.Empty()), diagnostics); Assert.Equal(1, result.FailureCount); var only = Assert.Single(diagnostics); @@ -792,7 +807,7 @@ public void ProbeVerdict_AndDeclaringAssembly_AreCarriedIntoTheDiagnostic() { var diagnostics = new List(); RunCommand.ExtractFilteredResult(Tree("BH.Revit.Engine.MechanicalPlumbing.Compute. }", MethodCause), - _ => true, null, (_, _, _) => (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty()), diagnostics); + (_, _) => (true, AttributionBasis.NotRecorded), null, (_, _, _) => (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty()), diagnostics); var only = Assert.Single(diagnostics); Assert.True(only.CountedAsReal); @@ -809,7 +824,7 @@ public void DeclaringAssemblyFromTheEvent_IsHandedToTheProbe() string? seen = "not called"; RunCommand.ExtractFilteredResult( Tree("BH.Revit.Engine.MechanicalPlumbing.Compute. }", MethodCause), - _ => true, null, + (_, _) => (true, AttributionBasis.NotRecorded), null, (_, _, asm) => { seen = asm; return (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty()); }, null); @@ -834,7 +849,7 @@ public void EveryAttributedFailure_GetsExactlyOneDiagnostic() var versionSummary = new FakeTestResult { Status = "Error", Information = [leafReal, leafUnverified] }; var outer = new FakeTestResult { Status = "Error", Information = [versionSummary] }; - var result = RunCommand.ExtractFilteredResult(outer, _ => true, skips, null, diagnostics); + var result = RunCommand.ExtractFilteredResult(outer, (_, _) => (true, AttributionBasis.NotRecorded), skips, null, diagnostics); Assert.Equal(1, result.FailureCount); Assert.Single(skips); diff --git a/tools/VersioningRunner/src/VersioningRunner.Tests/SubjectAttributionDecisionTests.cs b/tools/VersioningRunner/src/VersioningRunner.Tests/SubjectAttributionDecisionTests.cs new file mode 100644 index 0000000..8e6ecb4 --- /dev/null +++ b/tools/VersioningRunner/src/VersioningRunner.Tests/SubjectAttributionDecisionTests.cs @@ -0,0 +1,222 @@ +using VersioningRunner.Commands; +using VersioningRunner.Models; +using VersioningRunner.Tests.Fixtures; +using Xunit; + +namespace VersioningRunner.Tests +{ + // Attribution decides whether a failure is the subject repository's at all. It used to be + // decided on the description string, which cannot answer the question: a repository that + // declares a namespace other repositories extend was attributed all of their failures. + // Measured on a real run of a repository declaring BH.Adapter: 47 findings reported, spread + // across 28 other repositories' adapter namespaces, none of them its own. + // + // The cases below fix the shape of the fix, not just its outcome. In particular they pin + // that a declaring assembly which says "not yours" is honoured rather than falling through + // to the namespace, because falling through is what would quietly reinstate the defect. + public class SubjectAttributionDecisionTests + { + // The event shape the dataset actually produces. The "Name" field is assembly-qualified + // and is the only place the declaring assembly appears. + private static string MethodEvent(string type, string assembly) => + "Method Push from { \"_t\" : \"System.Type\", \"Name\" : \"" + type + ", " + assembly + + ", 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(params string[] subject) => + new(new HashSet(StringComparer.Ordinal), + new HashSet(StringComparer.Ordinal), + new HashSet(subject.Select(RunCommand.StripConfigSuffix), StringComparer.Ordinal)); + + // BHoM_Adapter's real subject set and namespaces, from validation run 32837506868. + private static readonly HashSet AdapterNamespaces = + new(["BH.Adapter", "BH.Engine.Adapter", "BH.oM.Adapter"], StringComparer.Ordinal); + + private static ClosureContext AdapterClosure() => + Closure("Adapter_Engine", "Adapter_oM", "BHoM_Adapter", "Structure_AdapterModules"); + + public class TheDecision + { + // The defect, stated as a test. Namespace matching accepts this because the subject + // declares BH.Adapter and the type sits below it; the declaring assembly says it + // belongs to another repository, and that is the answer that must win. + [Fact] + public void ForeignAssemblyUnderTheSubjectsOwnNamespaceRoot_IsNotAttributed() + { + var (attributable, basis) = RunCommand.AttributeToSubject( + "BH.Adapter.ETABS.ETABSAdapter..ctor", "ETABS_Adapter", + AdapterNamespaces, AdapterClosure()); + + Assert.False(attributable); + Assert.Equal(AttributionBasis.DeclaringAssembly, basis); + } + + // The same string, with no assembly recorded, still matches by namespace. This is + // the residue the runner counts and warns about: it is wrong, and it is retained + // because dropping it would lose genuine regressions that carry no Method event. + [Fact] + public void TheSameStringWithNoAssembly_FallsBackToNamespaceAndIsStillWrong() + { + var (attributable, basis) = RunCommand.AttributeToSubject( + "BH.Adapter.ETABS.ETABSAdapter..ctor", null, + AdapterNamespaces, AdapterClosure()); + + Assert.True(attributable); + Assert.Equal(AttributionBasis.NamespaceFallback, basis); + } + + [Fact] + public void SubjectsOwnAssembly_IsAttributed() + { + var (attributable, basis) = RunCommand.AttributeToSubject( + "BH.Adapter.BHoMAdapter.Push", "BHoM_Adapter", + AdapterNamespaces, AdapterClosure()); + + Assert.True(attributable); + Assert.Equal(AttributionBasis.DeclaringAssembly, basis); + } + + // Attribution must not depend on the namespace agreeing. A subject assembly may + // declare types anywhere, and the assembly is the authority. + [Fact] + public void SubjectsOwnAssemblyWithAnUnrelatedNamespace_IsStillAttributed() + { + var (attributable, _) = RunCommand.AttributeToSubject( + "BH.Something.Entirely.Else.Thing", "Structure_AdapterModules", + AdapterNamespaces, AdapterClosure()); + + Assert.True(attributable); + } + + // Revit repositories build one assembly per year. The subject staged _2022; a + // record naming _2024 is the same assembly to this repository, which is why the + // comparison strips the configuration suffix. + [Fact] + public void ConfigurationSuffixedVariantOfASubjectAssembly_IsAttributed() + { + var (attributable, _) = RunCommand.AttributeToSubject( + "BH.Revit.Engine.Core.Create.ProjectParameter", "Revit_Core_Engine_2024", + new HashSet(["BH.Revit.Engine.Core"], StringComparer.Ordinal), + Closure("Revit_Core_Engine_2022")); + + Assert.True(attributable); + } + + // No subject set was established, so nothing can be called this repository's. + // Returning true here would attribute the whole closure on no basis. + [Fact] + public void NoClosure_AttributesNothingByAssembly() + { + var (attributable, basis) = RunCommand.AttributeToSubject( + "BH.Adapter.BHoMAdapter.Push", "BHoM_Adapter", AdapterNamespaces, closure: null); + + Assert.False(attributable); + Assert.Equal(AttributionBasis.DeclaringAssembly, basis); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void BlankAssemblyName_IsNotTreatedAsASubjectMatch(string assembly) + => Assert.False(RunCommand.IsFromSubjectAssembly(assembly, AdapterClosure())); + } + + public class ThroughTheCollector + { + private static (VersioningResult Result, List Diags) Run( + FakeTestResult tree, HashSet namespaces, ClosureContext closure) + { + var diagnostics = new List(); + var result = RunCommand.ExtractFilteredResult( + tree, + (d, a) => RunCommand.AttributeToSubject(d, a, namespaces, closure), + new List(), + // Nothing answered for the type: this is the DeclaringTypeNotLoaded shape + // all 47 observed findings had, and the shape whose existing reclassification + // gate deliberately refuses to act. Attribution must therefore stop it, and + // this asserts that it does rather than relying on the classifier. + (_, _, _) => (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty()), + diagnostics, closure); + return (result, diagnostics); + } + + [Fact] + public void ForeignFinding_IsDroppedAtAttributionRatherThanClassified() + { + var (result, diags) = Run( + Tree("BH.Adapter.ETABS.ETABSAdapter..ctor", + MethodEvent("BH.Adapter.ETABS.ETABSAdapter", "ETABS_Adapter")), + AdapterNamespaces, AdapterClosure()); + + Assert.Equal(0, result.FailureCount); + // No diagnostic at all: it was never the subject's failure, so there is nothing + // to classify. A diagnostic here would mean attribution had let it through. + Assert.Empty(diags); + } + + [Fact] + public void SubjectFinding_SurvivesAndRecordsTheAssemblyAsItsBasis() + { + var (result, diags) = Run( + Tree("BH.Adapter.BHoMAdapter.Push", + MethodEvent("BH.Adapter.BHoMAdapter", "BHoM_Adapter")), + AdapterNamespaces, AdapterClosure()); + + Assert.Equal(1, result.FailureCount); + Assert.Equal(AttributionBasis.DeclaringAssembly, Assert.Single(diags).AttributedBy); + } + + // The counter the run output reports has to be able to see this path, so the basis + // is recorded on the row rather than inferred from the absence of an assembly. + [Fact] + public void FindingWithNoMethodEvent_IsRecordedAsTheFallbackBasis() + { + var (result, diags) = Run( + Tree("BH.Adapter.BHoMAdapter.Push"), + AdapterNamespaces, AdapterClosure()); + + Assert.Equal(1, result.FailureCount); + Assert.Equal(AttributionBasis.NamespaceFallback, Assert.Single(diags).AttributedBy); + } + + // The observed run in miniature: one of the subject's own failures among several + // other repositories' entries under the same namespace root. + [Fact] + public void MixedTree_KeepsOnlyTheSubjectsOwn() + { + var outer = new FakeTestResult + { + Status = "Error", + Information = + [ + Tree("BH.Adapter.ETABS.ETABSAdapter..ctor", MethodEvent("BH.Adapter.ETABS.ETABSAdapter", "ETABS_Adapter")), + Tree("BH.Adapter.Revit.RevitAdapter..ctor", MethodEvent("BH.Adapter.Revit.RevitAdapter", "Revit_Adapter")), + Tree("BH.Adapter.Mongo.MongoAdapter..ctor", MethodEvent("BH.Adapter.Mongo.MongoAdapter", "Mongo_Adapter")), + Tree("BH.Adapter.BHoMAdapter.Push", MethodEvent("BH.Adapter.BHoMAdapter", "BHoM_Adapter")) + ] + }; + + var (result, diags) = Run(outer, AdapterNamespaces, AdapterClosure()); + + Assert.Equal(1, result.FailureCount); + Assert.Equal("BH.Adapter.BHoMAdapter.Push", Assert.Single(result.Failures).Description); + Assert.Equal(AttributionBasis.DeclaringAssembly, Assert.Single(diags).AttributedBy); + } + } + } +} diff --git a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs index eb78b20..4c5f733 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs @@ -14,7 +14,7 @@ public static int Execute( string assembliesPath, string? outputPath, bool testAll, - string? subjectBuildDir = null, + string? subjectAssemblyList = null, string? configuration = null, IReadOnlyCollection? versionConditionalMethods = null) { @@ -63,30 +63,39 @@ public static int Execute( // 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. + HashSet? subjectFileNames = ReadSubjectAssemblyList(subjectAssemblyList); + ClosureContext? closure = null; - if (subjectBuildDir is not null && Directory.Exists(subjectBuildDir)) + if (subjectFileNames is { Count: > 0 }) { 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))), + subjectFileNames.Select(f => StripConfigSuffix(Path.GetFileNameWithoutExtension(f))), StringComparer.Ordinal); - if (subjectBases.Count > 0) - closure = new ClosureContext( - loadedNames, - new HashSet(loadedNames.Select(StripConfigSuffix), StringComparer.Ordinal), - subjectBases); + closure = new ClosureContext( + loadedNames, + new HashSet(loadedNames.Select(StripConfigSuffix), StringComparer.Ordinal), + subjectBases); } - var subjectNamespaces = BuildSubjectNamespaces(loaded, subjectBuildDir); - Func isAttributable; + var subjectNamespaces = BuildSubjectNamespaces(loaded, subjectFileNames); + + // Attribution answers "is this failure ours"; classification answers "is it real". + // Those are separate questions and this is the place the first one is answered, which + // is why the declaring assembly is taken into account here rather than being used to + // repair a foreign finding further down. The reclassification at the bottom of + // CollectLeafFailures deliberately refuses to touch a finding nothing answered for, + // because doing so would turn a genuine removal into a silent pass; that guard is + // correct and stays. A failure whose declaring assembly is another repository's was + // never ours to classify in the first place. + Func isAttributable; if (subjectNamespaces is null) { Console.WriteLine($"Attribution: whole closure, {nsPrefixes.Count} namespace prefix(es)"); - isAttributable = d => IsFromLoadedNamespace(d, nsPrefixes); + isAttributable = (d, _) => (IsFromLoadedNamespace(d, nsPrefixes), AttributionBasis.NotRecorded); } else { @@ -108,7 +117,8 @@ public static int Execute( ? " — nothing to attribute to this repo, so no failure can be reported" : $": {string.Join(", ", subjectNamespaces.OrderBy(x => x, StringComparer.Ordinal).Take(20))}")); var subject = subjectNamespaces; - isAttributable = d => IsFromSubjectNamespace(d, subject); + var ctx = closure; + isAttributable = (d, asm) => AttributeToSubject(d, asm, subject, ctx); } var allFailures = new List(); @@ -173,6 +183,31 @@ public static int Execute( int noAssembly = diagnostics.Count(d => d.CountedAsReal && d.DeclaringAssembly is null); if (noAssembly > 0) Console.WriteLine($"Real failures with no declaring assembly recorded: {noAssembly}"); + + // How each attributed failure was decided to be ours. Reported unconditionally + // because the interesting number is the fallback count, and a number that is only + // printed when it is non-zero cannot be distinguished from one nobody measured. + int byAssembly = diagnostics.Count(d => d.AttributedBy == AttributionBasis.DeclaringAssembly); + int byNamespace = diagnostics.Count(d => d.AttributedBy == AttributionBasis.NamespaceFallback); + if (byAssembly + byNamespace > 0) + { + Console.WriteLine( + $"Attribution basis: {byAssembly} by declaring assembly, " + + $"{byNamespace} by namespace fallback (no declaring assembly recorded)"); + + // 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. + // Warned rather than logged: it is the residue of a known defect, and the + // point of counting it is that someone notices when it stops being zero. + if (byNamespace > 0) + { + Console.Error.WriteLine( + $"::warning title=Versioning::{byNamespace} failure(s) were 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 these " + + "specific findings may not be its own."); + } + } } if (infrastructureSkips.Count > 0) @@ -390,11 +425,14 @@ private static IReadOnlyList FindAllFromJsonDatasetsMethods(IEnumera public static VersioningResult ExtractFilteredResult(object? rawResult, List loaded) { var nsPrefixes = BuildNamespacePrefixes(loaded); - return ExtractFilteredResult(rawResult, d => IsFromLoadedNamespace(d, nsPrefixes)); + return ExtractFilteredResult( + rawResult, (d, _) => (IsFromLoadedNamespace(d, nsPrefixes), AttributionBasis.NotRecorded)); } public static VersioningResult ExtractFilteredResult( - object? rawResult, Func isAttributable, List? unresolvableSkips = null, + object? rawResult, + Func isAttributable, + List? unresolvableSkips = null, Func Candidates)>? probeSignature = null, List? diagnostics = null, ClosureContext? closure = null) @@ -421,7 +459,8 @@ public static VersioningResult ExtractFilteredResult( } private static void CollectLeafFailures( - object node, Func isAttributable, List failures, + object node, Func isAttributable, + List failures, List? unresolvableSkips, Func Candidates)>? probeSignature, List? diagnostics, ClosureContext? closure, int depth) @@ -457,14 +496,29 @@ private static void CollectLeafFailures( string desc = nodeType.GetProperty("Description")?.GetValue(node)?.ToString() ?? ""; string msg = nodeType.GetProperty("Message")?.GetValue(node)?.ToString() ?? ""; - if (!isAttributable(desc)) - return; - // allChildren are all EventMessages here, since resultChildren is empty. var eventMessages = allChildren .Select(c => c.GetType().GetProperty("Message")?.GetValue(c)?.ToString() ?? "") .ToList(); + // The Method event's "Name" is assembly-qualified. It is the only handle on which + // assembly should declare the type, so it decides who gets asked when the type + // cannot be resolved, and it is also the only unambiguous answer to whether this + // failure is ours at all. + // + // Read before the attribution decision, not after. It used to be read after, so + // attribution was settled on the description alone while this sat four lines below + // unused: a repository declaring a namespace others extend was attributed all of + // their failures. The events are already in hand at this point, so reading them + // first costs nothing. + string? declaringAssembly = eventMessages + .Select(ParseMethodEventAssembly) + .FirstOrDefault(a => a is not null); + + var (attributable, attributedBy) = isAttributable(desc, declaringAssembly); + if (!attributable) + return; + // DescriptionFromJson mangles many method entries to ". }", // losing the method name. The Method event still carries both, so prefer it. var (eventType, eventMethod) = eventMessages @@ -474,13 +528,6 @@ private static void CollectLeafFailures( ? $"{eventType}.{eventMethod}" : desc; - // The Method event's "Name" is assembly-qualified. It is the only handle on which - // assembly should declare the type, so it decides who gets asked when the type - // cannot be resolved. - string? declaringAssembly = eventMessages - .Select(ParseMethodEventAssembly) - .FirstOrDefault(a => a is not null); - string? cause = ClassifyUnresolvableCause(eventMessages); var path = ClassificationPath.UnresolvableFromEvents; IReadOnlyList candidates = Array.Empty(); @@ -541,7 +588,8 @@ private static void CollectLeafFailures( // the ordinary case and carrying it would add noise to every row. DeclaringTypeCandidates: candidates.Count > 1 ? candidates : null, Configuration: s_configuration, - VersionConditional: ClassifyVersionConditional(eventType, eventMethod))); + VersionConditional: ClassifyVersionConditional(eventType, eventMethod), + AttributedBy: attributedBy)); } else { @@ -586,29 +634,56 @@ public static HashSet BuildNamespacePrefixes(IEnumerable assem // whole-closure behaviour). Names come off the build output but are resolved against // the loaded set, which is authoritative, so a stale Build\ entry cannot introduce a // namespace for an assembly that is not actually present. - internal static HashSet? BuildSubjectNamespaces(List loaded, string? subjectBuildDir) + + // The subject set is the assemblies this repository's own build staged, handed over as a + // list the caller computed rather than a directory this reads. + // + // It used to be a directory, `\Build`, which the caller assumed every project + // wrote to. Nothing guaranteed that: the convention was enforced by a linter that only + // rewrote OutputPath values already present and never inspected which configuration they + // applied to, and no check read the directory until this one. Measured across 138 projects, + // 30% did not write there under the configuration CI builds, and a different 35% would not + // under the other one. So there was no configuration that made it reliable. + // + // A list has a property the directory did not: it describes what happened rather than what + // was declared. It also cannot be partially right. A directory holding some of the + // repository's assemblies produced a subject set that looked plausible and silently covered + // a fraction of the repo, with nothing to indicate it. + // + // Null means no list was supplied and attribution falls back to the whole closure, which + // over-reports. Empty means the list was supplied and the build staged nothing, which is a + // different state and is warned about by the caller before this runs. + // Split from the file read so the name handling can be tested without a temp file. + internal static HashSet ReadSubjectAssemblyListFrom(IEnumerable entries) => + new(entries.Select(l => l.Trim()) + .Where(l => l.Length > 0) + .Select(Path.GetFileName) + .Where(n => !string.IsNullOrEmpty(n))!, + StringComparer.OrdinalIgnoreCase); + + internal static HashSet? ReadSubjectAssemblyList(string? listPath) { - if (string.IsNullOrWhiteSpace(subjectBuildDir)) + if (string.IsNullOrWhiteSpace(listPath)) return null; - if (!Directory.Exists(subjectBuildDir)) + if (!File.Exists(listPath)) { Console.Error.WriteLine( - $"::warning title=Versioning::Subject build directory '{subjectBuildDir}' not found. " + + $"::warning title=Versioning::Subject assembly list '{listPath}' not found. " + "Falling back to attributing failures across the whole dependency closure, " + "which over-reports: failures owned by dependencies will be attributed to this repo."); return null; } - // Recursive because the layout under Build\ varies by project style, measured: - // SDK-style repos append the TFM (File_Toolkit -> Build\netstandard2.0\File_oM.dll) - // while the old-style Revit projects are flat (Build\Revit_X_oM_2022.dll). A - // top-level scan finds nothing for the former, which silently empties the subject - // set. Safe to recurse: BHoM references carry False, so a - // dependency's DLL is never copied into the subject's output. - var subjectFiles = new HashSet( - Directory.GetFiles(subjectBuildDir, "*.dll", SearchOption.AllDirectories).Select(Path.GetFileName)!, - StringComparer.OrdinalIgnoreCase); + // Names, not paths. Callers may write either; the comparison downstream is by file + // name, because that is what identifies an assembly in the staged set. + return ReadSubjectAssemblyListFrom(File.ReadAllLines(listPath)); + } + + internal static HashSet? BuildSubjectNamespaces(List loaded, HashSet? subjectFiles) + { + if (subjectFiles is null) + return null; var namespaces = new HashSet(StringComparer.Ordinal); int unreadable = 0; @@ -672,7 +747,7 @@ public static HashSet BuildNamespacePrefixes(IEnumerable assem s_subjectTypeCount = subjectTypeCount; Console.WriteLine( - $"Subject assemblies: {subjectFiles.Count} name(s) in {subjectBuildDir}, " + + $"Subject assemblies: {subjectFiles.Count} name(s) staged by the subject build, " + $"{subjectFiles.Count(f => loaded.Any(a => PathName(a) == f))} present in the loaded set"); return namespaces; @@ -697,12 +772,59 @@ public static bool IsFromLoadedNamespace(string description, HashSet nsP return nsPrefixes.Contains(parts[0] + "." + parts[1] + "." + parts[2]); } - // Attribution against exact namespaces. A failure belongs to the subject when its - // declaring type sits in one of the subject's namespaces or below it. Matching on a - // segment boundary keeps BH.oM.Adapters.ETABS.Pier out when the subject only declares - // BH.oM.Adapters.File. Descriptions arrive either as a type full name or as - // "DeclaringType.MethodName" (Versioning_Toolkit's DescriptionFromJson), and both are - // covered: each is a strict prefix extension of the declaring namespace. + // The whole attribution decision for subject mode, in one place so that what the runner + // does and what the tests check cannot drift apart. + // + // The declaring assembly wins whenever there is one, including when it says no: an + // assembly that is not the subject's is a definite answer, and falling through to the + // namespace after it would reinstate the defect this exists to remove. The namespace is + // consulted only in the absence of an assembly, where it is the sole evidence available. + public static (bool Attributable, AttributionBasis Basis) AttributeToSubject( + string description, string? declaringAssembly, + HashSet subjectNamespaces, ClosureContext? closure) + { + if (declaringAssembly is not null) + return (IsFromSubjectAssembly(declaringAssembly, closure), AttributionBasis.DeclaringAssembly); + + // No assembly recorded, so the ambiguous string is all there is. Kept rather than + // dropped: a silent drop would lose a real regression. Counted by the caller so the + // size of this path is measured rather than assumed. + return (IsFromSubjectNamespace(description, subjectNamespaces), AttributionBasis.NamespaceFallback); + } + + // Attribution against the assembly the dataset record names. This is the primary test and + // the only unambiguous one: the subject either staged an assembly of that name or it did + // not, and no string parsing is involved. + // + // It works precisely where the namespace cannot. The failures this was written for are + // classified DeclaringTypeNotLoaded, meaning no loaded assembly yielded the type and the + // one the event named is outside the closure. Their namespaces are therefore absent from + // every loaded set, so no scheme that reasons about which namespace is the longest match + // can see them. The assembly name comes from the record, not from loading, so it is + // available exactly when the type is not. + // + // Compared on the config-stripped base name because Revit repositories build the same + // assembly per year (Revit_Core_Engine_2022 and _2024 are one assembly to the subject), + // reusing the closure's own normalisation so the two cannot disagree. + public static bool IsFromSubjectAssembly(string? declaringAssembly, ClosureContext? closure) + { + // No closure means no subject set was established, so nothing can be called ours. + if (string.IsNullOrWhiteSpace(declaringAssembly) || closure is null) + return false; + + return closure.SubjectBaseNames.Contains(StripConfigSuffix(declaringAssembly)); + } + + // Attribution against exact namespaces. Used only when no declaring assembly was + // recorded, because it cannot be made correct: a failure belongs to the subject when its + // declaring type sits in one of the subject's namespaces or below it, and "or below it" + // is exactly what over-attributes. Matching on a segment boundary keeps + // BH.oM.Adapters.ETABS.Pier out when the subject only declares BH.oM.Adapters.File, but + // nothing here can keep BH.Adapter.ETABS.ETABSAdapter out when the subject declares + // BH.Adapter, because the string does not say whether the last segment is a type or a + // method. Descriptions arrive either as a type full name or as "DeclaringType.MethodName" + // (Versioning_Toolkit's DescriptionFromJson), and both are strict prefix extensions of + // the declaring namespace, which is why this is a prefix test and why it is a fallback. public static bool IsFromSubjectNamespace(string description, HashSet subjectNamespaces) { if (string.IsNullOrWhiteSpace(description) || subjectNamespaces.Count == 0) diff --git a/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs b/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs index 107a7a9..df93f5b 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs @@ -48,6 +48,27 @@ public enum ClassificationPath ConfigurationNotBuilt, } +// Which evidence decided that a failure belongs to the subject repository. +// +// The two are not equally trustworthy and the difference is the whole point of recording it. +// A declaring assembly is unambiguous: the dataset record names it, and the subject either +// staged an assembly of that name or it did not. A description is ambiguous: it arrives +// either as a type full name or as "DeclaringType.MethodName" and nothing in the string says +// which, so BH.Adapter.ETABS.ETABSAdapter (another repository's type) and +// BH.Adapter.BHoMAdapter.Push (this repository's method) have the same shape. No matching +// rule over the string can separate them, which is why the assembly decides where one exists. +public enum AttributionBasis +{ + // Whole-closure attribution, where there is no subject set to compare against. + NotRecorded, + // The dataset record named a declaring assembly and the subject build staged it. + DeclaringAssembly, + // No declaring assembly was recorded, so the namespace decided it. This path still + // over-attributes a repository that owns a namespace others extend, and is counted in + // the run output for exactly that reason: its size must be measured, not assumed. + NamespaceFallback, +} + // 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 @@ -89,7 +110,10 @@ public record FailureDiagnostic( // Build configuration this run compiled, carried per row so a finding read on its // own is interpretable. Human-legible; it does not by itself explain a divergence. string? Configuration = null, - VersionConditionalState VersionConditional = VersionConditionalState.Unknown); + VersionConditionalState VersionConditional = VersionConditionalState.Unknown, + // Which evidence attributed this failure to the subject. Per row rather than only as a + // total, so a reader can tell whether any individual finding rests on the ambiguous path. + AttributionBasis AttributedBy = AttributionBasis.NotRecorded); // Coverage denominator. A verdict without one cannot be interpreted: a pass over zero // methods reads identically to a pass over seven thousand. BHoMBot reported object diff --git a/tools/VersioningRunner/src/VersioningRunner/Program.cs b/tools/VersioningRunner/src/VersioningRunner/Program.cs index f9380df..244f28f 100644 --- a/tools/VersioningRunner/src/VersioningRunner/Program.cs +++ b/tools/VersioningRunner/src/VersioningRunner/Program.cs @@ -29,7 +29,7 @@ if (args.Length == 0 || args.Contains("--help") || args.Contains("-h")) { - Console.WriteLine("Usage: VersioningRunner [--assemblies ] [--output ] [--test-all] [--subject-assemblies ] [--configuration ] [--version-conditional ]"); + Console.WriteLine("Usage: VersioningRunner [--assemblies ] [--output ] [--test-all] [--subject-assembly-list ] [--configuration ] [--version-conditional ]"); Console.WriteLine(); Console.WriteLine("Options:"); Console.WriteLine(" --assemblies BHoM assemblies folder (default: C:\\ProgramData\\BHoM\\Assemblies)"); @@ -40,15 +40,16 @@ Console.WriteLine(" conditional in the subject's source, one per line. A present file"); Console.WriteLine(" means the scan ran, so unlisted methods record No. Omit the flag"); Console.WriteLine(" only when no scan was performed: findings then record Unknown."); - Console.WriteLine(" --subject-assemblies Subject repo's build output. Failures are attributed only to"); - Console.WriteLine(" namespaces these assemblies declare (default: the whole closure)"); + Console.WriteLine(" --subject-assembly-list File listing the assemblies this repo's own build staged,"); + Console.WriteLine(" one name per line. Failures are attributed only to namespaces"); + Console.WriteLine(" those assemblies declare (default: the whole closure)."); return 1; } string assemblies = GetArg(args, "--assemblies") ?? @"C:\ProgramData\BHoM\Assemblies"; string? output = GetArg(args, "--output"); bool testAll = args.Contains("--test-all"); -string? subject = GetArg(args, "--subject-assemblies"); +string? subject = GetArg(args, "--subject-assembly-list"); string? configuration = GetArg(args, "--configuration"); string? vcFile = GetArg(args, "--version-conditional");