Fix main build broken by csproj-based MSTest packaging (#9259)#9344
Conversation
PR #9259 (csproj packing, removing nuspec) introduced two regressions on MSTest.TestAdapter's project references that broke the in-repo build once the daily build number rolled past the dogfood pin: 1. NU1605 (restore): the adapter now ProjectReferences Microsoft.Testing.Platform.MSBuild with PrivateAssets="none", flowing the locally built platform-MSBuild (current build version) transitively to in-repo consumers. Test projects with UseMSTestFromSource also PackageReference the darc-pinned version, producing a package downgrade. Reference the platform MSBuild from source when UseMSTestFromSource is set so both paths use the same (project) version. 2. CS0234 (compile): the adapter now suppresses MSTestAdapter.PlatformServices as a flowed dependency (PrivateAssets="all", correct for the produced package), so in-repo consumers no longer see its (internals-visible) types. Reference PlatformServices directly from MSTest.IntegrationTests and MSTestAdapter.UnitTests. Validated by building the affected projects with /p:OfficialBuildId=20260622.12 (reproducing the CI build version > pin) -- restore and compile now succeed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes the main branch build break caused by the csproj-based MSTest.TestAdapter packaging changes (#9259) by aligning in-repo test project references with the new PrivateAssets behavior and avoiding a restore-time NU1605 downgrade scenario when UseMSTestFromSource=true.
Changes:
- Add direct
ProjectReferencetoMSTestAdapter.PlatformServicesfor test projects that compile against its types, since it no longer flows transitively fromMSTest.TestAdapterdue toPrivateAssets="all". - Replace a
Microsoft.Testing.Platform.MSBuildPackageReferencewith a sourceProjectReferencewhenUseMSTestFromSource=trueto eliminate restore downgrades against the locally-built MSBuild integration. - Document the rationale inline in the affected project files / targets.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/MSTestAdapter.UnitTests/MSTestAdapter.UnitTests.csproj | Adds direct reference to MSTestAdapter.PlatformServices so unit tests can compile against its APIs despite PrivateAssets="all" on the adapter’s reference. |
| test/IntegrationTests/MSTest.IntegrationTests/MSTest.IntegrationTests.csproj | Adds direct reference to MSTestAdapter.PlatformServices to restore compile-time access for integration test code using those types. |
| test/Directory.Build.targets | Switches Microsoft.Testing.Platform.MSBuild from PackageReference to ProjectReference under UseMSTestFromSource=true to prevent NU1605 downgrade vs the transitively-referenced source-built project. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 0
This comment has been minimized.
This comment has been minimized.
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.
✅ 22/22 dimensions clean — no findings.
Review notes (all dimensions)
Scope: 3 MSBuild project files changed (test/Directory.Build.targets, MSTest.IntegrationTests.csproj, MSTestAdapter.UnitTests.csproj). No C# runtime code touched.
| Dimension | Result | Notes |
|---|---|---|
| 1. Algorithmic Correctness | ✅ LGTM | Fix correctly targets the root cause: PrivateAssets="all" on the MSTestAdapter.PlatformServices ProjectReference in MSTest.TestAdapter prevents the assembly from flowing as a compile-time reference to consumers. Adding direct ProjectReference entries in both consuming test projects restores the reference. The PackageReference→ProjectReference change in Directory.Build.targets correctly resolves the NU1605 downgrade by ensuring both the direct reference and the transitively-flowed one resolve to the same locally-built project version. |
| 2. Threading & Concurrency | N/A | No C# code changed. |
| 3. Security & IPC Contract Safety | N/A | Build config changes only. |
| 4. Public API & Binary Compatibility | N/A | No public API touched. |
| 5. Performance & Allocations | N/A | No runtime code changed. |
| 6. Cross-TFM Compatibility | ✅ LGTM | MSTestAdapter.PlatformServices is built for all TFMs that MSTestAdapter.UnitTests targets (net462;net48;net8.0;net9.0;WinUiMinimum), because MSTest.TestAdapter already includes them all. The single-TFM MSTest.IntegrationTests ($(NetFrameworkMinimum)) is also covered. |
| 7. Resource & IDisposable Management | N/A | No code changed. |
| 8. Defensive Coding at Boundaries | N/A | No code changed. |
| 9. Localization & Resources | N/A | No strings changed. |
| 10. Test Isolation | N/A | No test code changed. |
| 11. Assertion Quality | N/A | No test code changed. |
| 12. Flakiness Patterns | N/A | No test code changed. |
| 13. Test Completeness & Coverage | ✅ LGTM | This is a build/restore fix with no new code paths; the author validated by reproducing the failure with /p:OfficialBuildId=20260622.12 and confirming all 6 affected projects restore and build clean. No additional tests needed. |
| 14. Data-Driven Test Coverage | N/A | No test code changed. |
| 15. Code Structure & Simplification | N/A | No C# code changed. |
| 16. Naming & Conventions | N/A | No C# code changed. |
| 17. Documentation Accuracy | ✅ LGTM | All three added XML comments accurately explain why the reference is needed (link to #9259, explain the PrivateAssets="all" mechanism). Comments describe the intent, not just what the element does. |
| 18. Analyzer & Code Fix Quality | N/A | No src/Analyzers/ files changed. |
| 19. IPC Wire Compatibility | N/A | No serialization code changed. |
| 20. Build Infrastructure & Dependencies | ✅ LGTM | The PackageReference→ProjectReference swap is gated on UseMSTestFromSource='true' (matching the old condition), sits inside the existing EnableMSTestRunner OR UseInternalTestFramework outer ItemGroup, and references a first-party in-repo project that is always resolvable. The direct ProjectReference entries in the two test projects do not duplicate the assembly in output since MSBuild deduplicates project builds. InternalsVisibleTo coverage confirmed: MSTestAdapter.PlatformServices.csproj already declares InternalsVisibleTo for both Microsoft.VisualStudio.TestPlatform.MSTestAdapter.UnitTests and MSTest.IntegrationTests. The four other UseMSTestFromSource=true projects omitted from the fix (MSTest.VstestConsoleWrapper.IntegrationTests, MSTest.Analyzers.UnitTests, MSTest.AotReflection.SourceGeneration.UnitTests, MSTest.SelfRealExamples.UnitTests) do not consume MSTestAdapter.PlatformServices internals directly and are not listed in its InternalsVisibleTo; they build fine without an explicit reference. |
| 21. Scope & PR Discipline | ✅ LGTM | Single, tightly-scoped concern. Clear description with root-cause analysis, precise reproduction steps, and explicit validation. Refs #9259 appropriately. |
| 22. PowerShell Scripting Hygiene | N/A | No .ps1 files changed. |
| MSBuild Authoring (supplemental) | ✅ LGTM | Condition quoting is consistent with the repo style ('$(Prop)' == 'value'). The new ProjectReference has no PrivateAssets (defaults to all-assets-flow), which is the correct behavior for a test project reference. No DependsOn append issues, no stray whitespace, no condition ordering problems. |
…ounts After the build progresses past the restore/compile fixes, verify-nupkgs.ps1 fails because MSTest.AotReflection.SourceGeneration (made packable as a non-shipping PoC in #9338) is a 4.3.0 package not present in the expected file-count table, so it is validated with an empty expected value. Add it with its actual content count (6: [Content_Types].xml, Icon.png, .nuspec, the analyzer dll, .psmdcp, .rels). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ency (#9259) After the move to csproj-based packing (#9259), MSTest.TestFramework no longer flowed the MSTest analyzers to consumers: the package was produced with empty dependency groups, so projects referencing MSTest.TestFramework built with the analyzers silently disabled (acceptance tests AnalyzersShouldBeEnabled*, MSTEST0001/0014, AnalyzerMessagesShouldBeLocalized, VerifyMSTestAnalysisMode* all failed). Root cause: the ProjectReference to MSTest.Analyzers.Package used ReferenceOutputAssembly="false". NuGet pack omits the generated package dependency for project references that do not reference the output assembly, so the MSTest.Analyzers dependency was dropped entirely. Removing the attribute makes pack emit `<dependency id="MSTest.Analyzers" include="...Analyzers, BuildTransitive" />` at the co-built version while ExcludeAssets="compile;runtime" still keeps the analyzer assembly out of this project's compile/runtime graph (the analyzer package has IncludeBuildOutput=false, so there is no real assembly). Verified by packing MSTest.TestFramework + MSTest.Analyzers locally and building a consumer that references the package: it now reports `warning MSTEST0014` again. net462 framework build remains clean (0 warnings) with the analyzers active. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Once MSTest.TestFramework correctly flows its MSTest.Analyzers dependency, the analyzer projects' Microsoft.CodeAnalysis.* package references leaked transitively through the framework's ProjectReference to in-repo consumers. That forced Microsoft.CodeAnalysis.Common/CSharp 4.8.0 onto the source-generation test projects (MSTest.SourceGeneration.UnitTests, MSTest.AotReflection.SourceGeneration.UnitTests), which pin Roslyn 3.11.0, producing NU1608. Roslyn is provided by the compiler host at analysis time and must never flow to consumers, so mark Microsoft.CodeAnalysis.Analyzers/Common (MSTest.Analyzers) and Microsoft.CodeAnalysis.CSharp.Workspaces (MSTest.Analyzers.CodeFixes) as PrivateAssets="all". The produced MSTest.Analyzers package is unaffected (SuppressDependenciesWhenPacking already yields zero dependencies and still ships all analyzer assemblies). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
…ositive The previous commit also marked Microsoft.CodeAnalysis.Analyzers PrivateAssets="all", which changed the analyzer-authoring analyzer version that MSTest.Analyzers.CodeFixes resolves and surfaced a known RS1024 false positive on FlowTestContextCancellationTokenFixer.cs (the code already uses SymbolEqualityComparer.Default). Only the Roslyn API package (Microsoft.CodeAnalysis.Common) needs PrivateAssets to fix NU1608; revert the attribute on Microsoft.CodeAnalysis.Analyzers so its pinned version keeps flowing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rift MSTest.Analyzers.CodeFixes resolved Microsoft.CodeAnalysis.Analyzers transitively, so a newer version (with a known RS1024 false positive on new HashSet<ISymbol>(SymbolEqualityComparer.Default)) could win depending on the restore graph. Add an explicit PrivateAssets="all" reference so it is always the repo-pinned 5.3.0. Verified: CodeFixes restores 5.3.0 and builds clean (0 warnings). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
RunWithCooperativeCancellationAsync relied on new CancellationTokenSource(0) having already cancelled its token at the IsCancellationRequested check, but the 0ms cancellation is scheduled on a timer rather than applied synchronously. On fast machines (seen on net48 Release) the check ran before the timer fired, so the action was invoked instead of failing immediately -- intermittently breaking CancellationTimeoutHelperTests.RunWithCooperativeCancellationAsyncWhenTimeoutIsZeroReturnsFailureWithoutCallingAction. Treat timeout == 0 as an immediate timeout explicitly. Production callers always pass a positive configured timeout (Timeout.Infinite is -1, not 0), so behavior is unchanged for them. Verified: the full MSTestAdapter.PlatformServices.UnitTests net48 Release suite (896 tests) passes, and the previously-failing test passes 6/6 runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧪 Test quality grade — PR #9344No new or modified test methods were identified in the changed regions Re-run with
|
Problem
mainis broken: the latest CI builds fail in the Build/Test tasks (Windows/Linux/macOS). The failures came in layers — fixing each revealed the next. This PR fixes all of them.NU1605downgrade ofMicrosoft.Testing.Platform.MSBuildPackageReferenceCS0234missingMSTestAdapter.PlatformServicestypesPrivateAssets="all"on PlatformServices, hiding it from in-repo consumersverify-nupkgs.ps1file-count mismatch forMSTest.AotReflection.SourceGenerationAnalyzersShouldBeEnabled*,MSTEST0001/0014,AnalyzerMessagesShouldBeLocalized,VerifyMSTestAnalysisMode*MSTest.TestFrameworkwas packed with empty dependency groups — theMSTest.Analyzersdependency was dropped, so analyzers no longer ran for consumersNU1608Roslyn version conflict in the source-generation unit testsMicrosoft.CodeAnalysis.*refs leak transitively through the framework, clashing with the pinned Roslyn 3.11.0Fixes
test/Directory.Build.targets— underUseMSTestFromSource, referenceMicrosoft.Testing.Platform.MSBuildfrom source so both references resolve to the same version. (NU1605)MSTest.IntegrationTests/MSTestAdapter.UnitTests— add a directProjectReferencetoMSTestAdapter.PlatformServices. (CS0234)eng/verify-nupkgs.ps1— addMSTest.AotReflection.SourceGeneration = 6. (verify-nupkgs)TestFramework.Extensions.csproj— removeReferenceOutputAssembly="false"from theMSTest.Analyzers.Packagereference so pack emits<dependency id="MSTest.Analyzers" include="...Analyzers,BuildTransitive" />at the co-built version. (analyzers)MSTest.Analyzers/MSTest.Analyzers.CodeFixes— markMicrosoft.CodeAnalysis.*referencesPrivateAssets="all"so Roslyn (a compiler-host-provided dependency) never flows to consumers. (NU1608)The adapter's intentional
PrivateAssetssettings are unchanged — the produced packages keep their historical dependency graphs.Validation
main(/p:OfficialBuildId=20260622.12, so the build version exceeds the darc pin) and confirmed each fix resolves it.NU1608.MSTest.TestFramework+MSTest.Analyzers, built a consumer referencing the package →warning MSTEST0014fires again. TheMSTest.Analyzerspackage still ships all analyzer assemblies with zero dependencies; the frameworknet462build stays clean with analyzers active.Refs #9259, #9338.