Uh oh!
There was an error while loading. Please reload this page.
Fix broken main: acceptance source-gen harness leaks nested bin/obj into multi-project asset builds - #9504
Conversation
The AssemblyFixtureProviderAcceptance asset has a multi-targeting ProjectReference (ProviderLibrary). In the source-gen build variant, RAR for the net8.0 leg resolves ProviderLibrary's transitive MSTest.TestFramework.Extensions.dll from the net10.0 reflection output instead of the net8.0 one, producing MSB3277 System.* version conflicts (8.0.0.0 vs 9.0.0.0). The source-gen build promotes these to errors via MSBuildTreatWarningsAsErrors, failing the fixture's ClassInitialize and breaking the main build. The test only exercises the reflection build, so opt the asset out of the source-gen build (matching FrameworkOnlyTests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a persistent main build break by preventing an unused acceptance-test asset build variant (source-generation) from compiling, avoiding MSBuild reference-resolution conflicts caused by a multi-targeting ProjectReference in the asset.
Changes:
- Opt the
AssemblyFixtureProviderAcceptanceasset out of source-generation builds by overridingSourceGenMetadataModesto an empty set. - Add an inline comment documenting the RAR/MSB3277 conflict scenario and why reflection-only is sufficient for this test.
Show a summary per file
| File | Description |
|---|---|
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AssemblyFixtureProviderTests.cs | Overrides the asset fixture’s SourceGenMetadataModes to skip source-gen builds that are not exercised by the test and currently fail due to assembly version conflicts. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Low
This comment has been minimized.
This comment has been minimized.
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Expert Review — PR #9504
Scope: Single file change in test/IntegrationTests/MSTest.Acceptance.IntegrationTests/AssemblyFixtureProviderTests.cs. The change adds a SourceGenMetadataModes => [] override and explanatory comment to TestAssetFixture, opting the AssemblyFixtureProviderAcceptance asset out of the source-gen build.
Pattern Verification
The escape hatch used here (protected override IReadOnlyList<MetadataMode> SourceGenMetadataModes => []) is the documented, established opt-out mechanism. It is applied identically in FrameworkOnlyTests.TestAssetFixture (which also carries an explanatory comment stating the reason). The TestAssetFixtureBase XML doc on SourceGenMetadataModes explicitly names this as the opt-out path for assets that "genuinely cannot build under source generation."
Confirmed that TestHost.LocateFrom(AssetFixture.TargetAssetPath, TestAssetFixture.TestProjectName, tfm) is called without a metadataMode argument — so the reflection build is the only one exercised by the test. Opting out of building the source-gen variant does not remove any test execution coverage.
22-Dimension Summary
| # | Dimension | Verdict | Notes |
|---|---|---|---|
| 1 | Algorithmic Correctness | ✅ LGTM | Fix matches root cause: source-gen build fails, test never uses source-gen variant; opting out is correct, not a symptom patch. |
| 2 | Threading & Concurrency | N/A | No new concurrent code. |
| 3 | Security & IPC Contract Safety | N/A | No security boundary touched. |
| 4 | Public API & Binary Compatibility | N/A | No public API changes. |
| 5 | Performance & Allocations | N/A | Test fixture code; not a hot path. |
| 6 | Cross-TFM Compatibility | N/A | The fix avoids the cross-TFM RAR issue by not building the failing variant. The => [] collection expression is valid on all targeted TFMs. |
| 7 | Resource & IDisposable Management | N/A | No new disposable resources. |
| 8 | Defensive Coding at Boundaries | N/A | No boundary logic changed. |
| 9 | Localization & Resources | N/A | No user-facing strings. |
| 10 | Test Isolation | ✅ LGTM | Single test method in class — no shared mutable asset across methods, so [DoNotParallelize] is not required. |
| 11 | Assertion Quality | N/A | No assertion changes. |
| 12 | Flakiness Patterns | ✅ LGTM | This PR fixes a reliability failure; no new flakiness introduced. |
| 13 | Test Completeness & Coverage | ✅ LGTM | The source-gen variant was never exercised by any test method. No coverage is lost. See observation below regarding a potential follow-up. |
| 14 | Data-Driven Test Coverage | N/A | No data rows changed. |
| 15 | Code Structure & Simplification | ✅ LGTM | Minimal, clean override consistent with FrameworkOnlyTests. |
| 16 | Naming & Conventions | ✅ LGTM | Consistent with existing pattern. |
| 17 | Documentation Accuracy | ✅ LGTM | Comment accurately explains the RAR resolution issue, the exact error (MSB3277), the promoter (MSBuildTreatWarningsAsErrors), and the justification. Matches the FrameworkOnlyTests comment style. |
| 18 | Analyzer & Code Fix Quality | N/A | No src/Analyzers/ changes. |
| 19 | IPC Wire Compatibility | N/A | No serialization changes. |
| 20 | Build Infrastructure & Dependencies | N/A | No eng/ or package reference changes. |
| 21 | Scope & PR Discipline | ✅ LGTM | Single-concern change; PR description is thorough with root-cause analysis and verification steps. |
| 22 | PowerShell Scripting Hygiene | N/A | No .ps1 changes. |
22/22 dimensions clean.
💡 Optional Follow-up Observation (non-blocking)
The underlying MSBuild/RAR issue — where the source-gen build's isolated bin/obj redirection causes the net8.0 leg of a multi-targeting ProjectReference to resolve transitive assemblies from the net10.0 output — will silently affect any other acceptance asset that has a similar multi-targeting ProjectReference. Opting this asset out is the right short-term fix to unblock main, but it may be worth filing a tracking issue to investigate whether the source-gen build infrastructure should be hardened to pass -p:TargetFrameworks=... or a comparable property to pin transitive reference resolution to the correct TFM subfolder. If such an issue already exists, linking it from the comment would help future readers.
Verdict: This is a correct, minimal, well-documented fix that follows the established pattern. Ready to merge.
…t globs Root cause of the broken main build: the acceptance source-gen build harness let a multi-project asset's leftover reflection outputs pollute the later source-gen build. TestAssetFixtureBase builds each asset twice in the same directory: first the reflection build (default bin/Release), then the source-gen build (bin/obj redirected to a *SourceGen sub-folder). For a multi-project asset (e.g. AssemblyFixtureProviderAcceptance, whose test project references ProviderLibrary), the reflection build leaves ProviderLibrary/bin/Release/<tfm>/*.dll on disk. When the source-gen build then evaluates the host project, the SDK's default item globs pulled those stray DLLs in as None items, and RAR treated a wrong-TFM copy (a net10.0 MSTest.TestFramework.Extensions.dll, assembly version 9.0.0.0) as a candidate for the net8.0 leg -> MSB3277, which MSBuildTreatWarningsAsErrors promotes to an error, failing the fixture's ClassInitialize. The injected props already re-excluded bin/obj, but only at the asset root (bin/**;obj/**), which does not match the nested ProviderLibrary/bin/**. Broaden the re-exclusion to **/obj/**;**/bin/** so a referenced project's leftover reflection outputs stay out of the source-gen build's globs. The reflection build (#1) is unaffected because at its evaluation time the referenced project's bin is still empty; only the later source-gen build (#2) runs in a directory polluted by #1. This is the proper harness fix; the AssemblyFixtureProvider opt-out is no longer needed, so the asset is restored to source-gen coverage. Verified by running the AssemblyFixtureProvider acceptance tests (reflection + source-gen builds) green across net462/net8.0/net10.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
commented
Jun 29, 2026
🧪 Test quality grade — PR #9504No new or modified test methods were identified in the changed regions The only changed file ( Re-run with
|
Uh oh!
There was an error while loading. Please reload this page.
Problem
mainhas been red across many consecutive official builds (e.g. build 3010746).The failing test is
MSTest.Acceptance.IntegrationTests.AssemblyFixtureProviderTests.AssemblyFixtureProvider_FromReferencedLibrary_RunsAssemblyInitializeAndCleanup. It fails in itsClassInitializebecause the source-gen build variant of theAssemblyFixtureProviderAcceptanceasset fails to compile withMSB3277.Root cause (a harness bug, not a product bug)
TestAssetFixtureBase.InitializeAsyncbuilds each acceptance asset twice in the same directory: first the reflection build (defaultbin/Release), then the source-gen build (withbin/objredirected to a*SourceGensub-folder).For a multi-project asset —
AssemblyFixtureProviderAcceptance's test project referencesProviderLibrary— the first (reflection) build leavesProviderLibrary/bin/Release/<tfm>/*.dllon disk. When the later source-gen build then evaluates the host project, the SDK's default item globs pull those stray DLLs in asNoneitems (confirmed in the binlog:AddItem None -> ProviderLibrary\bin\Release\net10.0\MSTest.TestFramework.Extensions.dll). RAR then treats the wrong-TFM copy (a net10.0MSTest.TestFramework.Extensions.dll, assembly version9.0.0.0) as a candidate for the net8.0 leg:MSBuildTreatWarningsAsErrors=true(passed only to the source-gen build) promotes this to an error, soClassInitializethrows.The injected props already re-excluded
bin/obj, but only at the asset root (bin/**;obj/**), which does not match the nestedProviderLibrary/bin/**. The reflection build (#1) is unaffected because at its evaluation time the referenced project'sbinis still empty — only the later source-gen build (#2) runs in a directory polluted by #1.Fix
Broaden the injected
DefaultItemExcludesinAcceptanceSourceGento also exclude nested bin/obj (**/obj/**;**/bin/**), so a referenced project's leftover reflection outputs stay out of the source-gen build's globs. This is the proper harness fix and keeps the existing redirect-based design.With the harness corrected, the
AssemblyFixtureProvideropt-out is unnecessary, so the asset is restored to source-gen coverage.Verification
MSB3277from the CI binlog (binlog-mcp).AssemblyFixtureProvideracceptance tests (which drive both the reflection and source-gen builds via the fixture) — all 3 pass acrossnet462/net8.0/net10.0.