[v5] Remove VSTest support from the test adapter and make MTP the default#9726
[v5] Remove VSTest support from the test adapter and make MTP the default#9726Evangelink wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This is the first slice of the v5 effort to drop full VSTest support from MSTest.TestAdapter and run MSTest exclusively on Microsoft.Testing.Platform (MTP). It removes the VSTest reflection surface (ITestExecutor/ITestDiscoverer implementations, extension-registration attributes, parameterless constructors, and public entry points) and the EnableMSTestRunner opt-in, making the adapter always a testing-platform application. The internal execution/discovery engine consumed by MSTestBridgedTestFramework is left intact, and the change verified that the src project still builds across all TFMs. It targets the long-lived dev/v5 branch and is deliberately a draft: VSTest-specific test projects are knowingly left broken for follow-up PRs.
Changes:
- Strip VSTest coupling from
MSTestExecutor/MSTestDiscoverer(interfaces,[ExtensionUri]/[FileExtension]/[DefaultExecutorUri], parameterless ctors, publicRunTests/DiscoverTests) and drop theEqtTracebootstrap plus theEqtTraceLoggerfile. - Remove the
[assembly: TestExtensionTypes(...)]VSTest extension registration. - Simplify
MSTest.TestAdapter.targetsto always setIsTestingPlatformApplication=trueandGenerateProgramFile=false, removing theEnableMSTestRunnercoexistence branch.
Show a summary per file
| File | Description |
|---|---|
src/Adapter/MSTest.TestAdapter/VSTestAdapter/MSTestExecutor.cs |
Drops ITestExecutor, [ExtensionUri], parameterless ctor, EqtTrace bootstrap, and public RunTests overloads; keeps internal engine used by the MTP bridge. |
src/Adapter/MSTest.TestAdapter/VSTestAdapter/MSTestDiscoverer.cs |
Drops ITestDiscoverer, file-extension/executor-uri attributes, parameterless ctor, and public DiscoverTests; keeps internal discovery path. |
src/Adapter/MSTest.TestAdapter/VSTestAdapter/EqtTraceLogger.cs |
Deleted; default NopTraceLogger + MTP's MTPTraceLogger cover logging. |
src/Adapter/MSTest.TestAdapter/Properties/AssemblyInfo.cs |
Removes VSTest extension-type registration (also inadvertently drops the file's UTF-8 BOM — flagged). |
src/Adapter/MSTest.TestAdapter/buildTransitive/common/MSTest.TestAdapter.targets |
Removes EnableMSTestRunner opt-in; adapter is now unconditionally an MTP application. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 20 | Build Infrastructure & Dependencies | 🟡 1 MODERATE (suggestion for follow-up) |
✅ 21/22 dimensions clean.
- Build Infrastructure — users with
EnableMSTestRunner=falsesilently get MTP on v5 upgrade (tracked for follow-up)
Dimension notes:
- 1–3 (Correctness, Threading, Security): Clean.
EqtTraceLoggerremoval is safe —PlatformServiceProvider.AdapterTraceLoggerdefaults toNopTraceLogger.InstanceandMSTestBridgedTestFrameworkctor setsMTPTraceLoggerbefore any execution/discovery call. No new shared state or threading concerns. - 4 (Public API): No breaking change —
MSTestDiscovererandMSTestExecutorwere alreadyinternal sealed. Thepublicmembers existed only to satisfy the VSTest interface contracts. - 5–9 (Performance, Cross-TFM, Resources, Defensive Coding, Localization): N/A or clean. Module initializer does less work now (perf improvement).
- 10–14 (Test dimensions): N/A — no test files changed. Acknowledged that CI is expected to be red until follow-up test project updates land.
- 15–17 (Structure, Naming, Docs): Clean. Minor note:
// TODO: Verify why VSTest annotates the IEnumerable as nullable.atMSTestExecutor.cs:77is now stale (VSTest no longer calls this path) and lacks an issue reference per repo policy — can be cleaned up when theisMTPflag removal follow-up lands. - 18–19 (Analyzer, IPC): N/A.
- 20 (Build Infrastructure): See inline comment on
.targetsfile — suggestion for migration warning in follow-up. - 21 (Scope & PR Discipline): Well-scoped first slice. Follow-ups are enumerated clearly. Consider tracking them as GitHub issues for discoverability.
- 22 (PowerShell): N/A.
MSBuild Authoring (supplemental): The .targets change follows correct MSBuild patterns — Condition=" '$(Prop)' == '' " for safe defaults in a transitive targets file. No anti-patterns detected.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
64edcfd to
f3f35d0
Compare
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ault Starting with v5, MSTest runs solely on Microsoft.Testing.Platform (MTP). The VSTest reflection surface is removed from MSTest.TestAdapter and the adapter is always a testing platform application (IsTestingPlatformApplication defaults to true). EnableMSTestRunner is no longer used; setting it emits an info message. Starting with VSTest 18.10, vstest.console can host an MTP test application (executable), so MSTest keeps working under 'dotnet test'/vstest in MTP mode (MTP extensions are not surfaced through VSTest). Adapter: - Remove the VSTest surface from MSTestExecutor/MSTestDiscoverer: [ExtensionUri], [DefaultExecutorUri], [FileExtension], ITestExecutor/ITestDiscoverer, the reflection parameterless ctors, and the public RunTests/DiscoverTests entry points; the internal MTP-facing async methods remain. Cancel() becomes internal. - Delete the VSTest EqtTraceLogger (and its module-initializer hook) and the [assembly: TestExtensionTypes] registration; update InternalAPI accordingly. - Default IsTestingPlatformApplication to true and key the localized-satellite copy logic off it (v5 is always MTP), replacing the removed EnableMSTestRunner. - Emit an info message (not a warning) when the obsolete EnableMSTestRunner property is set; suppress with SuppressMSTestRunnerObsoleteMessage. Packages/test infrastructure: - Bump Microsoft.NET.Test.Sdk / Microsoft.TestPlatform / Filter.Source to 18.10.0-preview-26357-05 (VSTest that can host an MTP test host). - Always include the test-tools feed in generated NuGet.config; make the shared MSTest acceptance asset use the MTP dotnet test runner; drop the addPublicFeeds parameter and Microsoft.NET.Test.Sdk from the MTP asset template (avoids a System.Memory 4.0.2.0 vs 4.0.5.0 binding-redirect crash on net462 publish). - Convert AppDomainTests to validate .NET Framework AppDomain isolation under MTP (net462 MTP exe, runsettings toggling DisableAppDomain). - Remove MSTest.VstestConsoleWrapper.IntegrationTests (drove net462 MSTest library assets through the classic vstest.console TranslationLayer, unsupported in v5; coverage is duplicated in-process by MSTest.IntegrationTests). - Skip classic-VSTest-runner acceptance tests on .NET 10+ SDK and drop the EnableMSTestRunner=false opt-out coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
f3f35d0 to
629efef
Compare
This comment has been minimized.
This comment has been minimized.
The addPublicFeeds parameter was removed from TestAsset.GenerateAssetAsync, but this call site was missed during the rebase, breaking the build with CS1739. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🧪 Test quality grade — PR #9726
This advisory comment was generated automatically. Grades are heuristic Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Summary
First slice of the v5 work to run MSTest exclusively on Microsoft.Testing.Platform (MTP). Targets the long-lived
dev/v5branch (created frommain).This PR removes the VSTest reflection surface (
ITestExecutor/ITestDiscovererand friends) fromMSTest.TestAdapterand makes MTP the only engine by removing theEnableMSTestRunneropt-in. MSTest now runs solely in MTP mode.Crucially, this does not mean MSTest stops working under
vstest.console. Starting with VSTest 18.10, VSTest knows how to host an MTP test host under VSTest, so an MTP-only test app (like MSTest v5) is still runnable throughvstest.console— it just runs as an MTP app (MTP extensions are not surfaced in that hosting mode). This PR bumps VSTest to18.10.0-preview-26357-05so that path works, which is what keeps the VSTest-based tests green.Changes
buildTransitive/common/MSTest.TestAdapter.targets— remove theEnableMSTestRunneropt-in / VSTest coexistence branch; the adapter is now always a testing platform application (IsTestingPlatformApplication=true,GenerateProgramFile=false). Adds a transitionalMSTest_WarnOnObsoleteEnableMSTestRunnerwarning when a leftoverEnableMSTestRunnervalue is detected (suppressible viaSuppressMSTestRunnerObsoleteWarning).Properties/AssemblyInfo.cs— drop[assembly: TestExtensionTypes(typeof(MSTestDiscoverer), typeof(MSTestExecutor))](the VSTest extension registration).EqtTraceLogger.cs— deleted.PlatformServiceProvider.AdapterTraceLoggerdefaults toNopTraceLogger, and MTP installsMTPTraceLogger, so no bootstrap is needed.MSTestExecutor.cs— removeITestExecutor,[ExtensionUri], the parameterless ctor, theEqtTracebootstrap, the publicRunTests(...)overloads, and the now-deadTestCase-basedRunTestsAsyncoverload +ToUnitTestElement(MTP always runs from sources). The internal source-based execution engine used byMSTestBridgedTestFrameworkis untouched.MSTestDiscoverer.cs— removeITestDiscoverer,[FileExtension]/[DefaultExecutorUri], the parameterless ctor, and the publicDiscoverTests(...)entry point. The internal discovery engine used by the MTP bridge is untouched.VSTestAdapter/andTestingPlatformAdapter/folders and move all their files (the sharedMSTestExecutor/MSTestDiscovererengine + the MTP extension files) to theMSTest.TestAdapterproject root. Namespaces unchanged.Directory.Packages.props— bump VSTest (MicrosoftNETTestSdkVersionandMicrosoft.TestPlatform.Filter.Source) to18.10.0-preview-26357-05so VSTest can host the MTP test host.MSTestAdapter.UnitTests— the removed VSTest reflection surface broke these tests: deleted the tests asserting on removed VSTest attributes and rewrote the behavioral tests to drive the internal MTP engine (DiscoverTestsAsync/RunTestsAsync(sources, ...)).FrameworkHandlerAdapter.cs(VSTestBridge) —IFrameworkHandle.EnableShutdownAfterTestRunbecame[Obsolete]("has no effect") in the new object model; suppress CS0618 while keeping the interface implementation and historical pass-through.Why this is only a first slice
The MTP path still consumes the VSTest object model through
Microsoft.Testing.Extensions.VSTestBridge(VSTestRunTestExecutionRequest,IFrameworkHandle,TestCase, …). Fully decoupling from the object model is a separate, larger effort the codebase already flags as a long-term goal ("the eventual goal should be to stop using the VSTestBridge altogether").Verification
Full repo builds cleanly (0 errors), including all
MSTest.TestAdapterTFMs (net462,net8.0,net9.0, UWP, WinUI). The rewrittenMSTestAdapter.UnitTestsclasses pass (12 tests).Follow-ups (subsequent PRs on
dev/v5)EnableMSTestRunner/ VSTest coexistence fromMicrosoft.Testing.Platform.MSBuild.targetsand every other reference.MSTest.Sdk: removeUseVSTestandSdk/VSTest/; make the Runner the only engine.VSTestBridgeobject-model coupling from the adapter (neutral request model).isMTPflag that threads throughMSTestExecutor→TestExecutionManager→UnitTestDiscoverer→AssemblyEnumeratorWrapper(alwaystruenow); requires updating the keptMSTestAdapter.PlatformServices.UnitTestscall sites.EnableMSTestRunnerfrom all.csproj.--help/--infoacceptance expectations if any option surface changes.