Uh oh!
There was an error while loading. Please reload this page.
Native MTP framework for MSTest — Phases 3–5 (native filter/runsettings/context, opt-in) - #9743
Conversation
…t, opt-in) Replaces the VSTest bridge framework on the opt-in native path (MSTEST_EXPERIMENTAL_NATIVE_MTP) with a native MSTestTestFramework that handles the MTP discovery/run requests directly — no VSTest bridge object-model adapters (RunContextAdapter/DiscoveryContextAdapter/FrameworkHandlerAdapter/RunSettingsAdapter/TestCaseDiscoverySinkAdapter/MessageLoggerAdapter) on the request path. New (all #if !WINDOWS_UWP, reusing MSTest's existing MSTestDiscoverer/MSTestExecutor engine): - MSTestTestFramework: ITestFramework + IDataProducer + single-session lifecycle; builds native context/runsettings/handle and drives discovery/execution via the Phase 1 seams (MtpUnitTestElementSink/MtpTestResultRecorder). - MSTestFilterContext (MSTestRunContext/MSTestDiscoveryContext): native IRunContext/IDiscoveryContext building the VSTest ITestCaseFilterExpression from the MTP ITestExecutionFilter + --filter + runsettings TestCaseFilter (reuses Microsoft.TestPlatform.Filter.Source, now referenced directly). - MSTestRunSettings: native IRunSettings (runsettings read + MTP patching: DesignMode/ResultsDirectory/--test-parameter + unsupported-entry warnings), reusing the bridge's already-localized ExtensionResources. - MSTestFrameworkHandle: IFrameworkHandle that only forwards messages to IOutputDevice (results go through the recorder). AddMSTest branches the framework factory on the flag; the bridge's command-line/config/env-var registrations are kept for now (retired with the dependency in the final step). MSTestBridgedTestFramework reverts to pure-bridge (native production now lives in the native framework). Validated: full build 0/0; unit tests 45 + 898; native-path acceptance 185 (filter, runsettings incl. localization, output, trx, threading/STA, server mode, deployment, data-driven, outcomes, test-parameters, timeouts); default bridge-path acceptance 45 unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to #9706 that continues the phased migration of MSTest's Microsoft.Testing.Platform (MTP) integration away from the Microsoft.Testing.Extensions.VSTestBridge object model. It introduces a nativeMSTestTestFramework (implementing ITestFramework/IDataProducer directly) that reuses MSTest's existing MSTestDiscoverer/MSTestExecutor engine and builds filter/runsettings/framework-handle context natively. Everything remains behind the opt-in MSTEST_EXPERIMENTAL_NATIVE_MTP environment switch, so the shipping default (the bridged framework) is unchanged. The new native classes are faithful ports of the corresponding bridge adapters (ContextAdapterBase, RunSettingsAdapter/RunSettingsPatcher/RunSettingsHelpers, MessageLoggerAdapter), and the previously-added Phase 2 SessionUid helper is removed from the bridge base now that native production lives in the native framework.
Changes:
- Adds native
MSTestTestFramework,MSTestRunSettings,MSTestFrameworkHandle, andMSTestFilterContext(MSTestRunContext/MSTestDiscoveryContext) that mirror the bridge adapters without depending on the bridge request path. AddMSTestbranches the framework factory on the flag;MSTestBridgedTestFrameworkreverts to pure-bridge and the unusedSessionUidproperty is dropped from the bridge base.- References
Microsoft.TestPlatform.Filter.Sourcedirectly inMSTest.TestAdapter.csproj(it flowed only privately via the bridge before).
Show a summary per file
| File | Description |
|---|---|
TestApplicationBuilderExtensions.cs | Adds the MSTEST_EXPERIMENTAL_NATIVE_MTP flag and branches framework construction between native and bridged frameworks. |
MSTestTestFramework.cs | New native MTP framework: session lifecycle, request routing, discovery/run via the existing engine and native seams. |
MSTestRunSettings.cs | New native IRunSettings: reads/patches runsettings and warns on unsupported entries (port of bridge helpers). |
MSTestFrameworkHandle.cs | New native IFrameworkHandle forwarding only diagnostic messages to IOutputDevice; VSTest recording members are no-ops. |
MSTestFilterContext.cs | New native IRunContext/IDiscoveryContext building the VSTest filter expression from MTP filter/--filter/runsettings. |
MSTestBridgedTestFramework.cs | Reverts to pure-bridge production, removing the Phase 2 native-production branches. |
SynchronizedSingleSessionVSTestAndTestAnywhereAdapter.cs | Removes the now-unused internal SessionUid property, using a private _sessionUid field. |
MSTest.TestAdapter.csproj | Adds an explicit Microsoft.TestPlatform.Filter.Source reference for the native filter-parsing path. |
The native classes are faithful copies of already-reviewed bridge logic, the SessionUid removal has no dangling references, and the package reference is correct. The one concrete issue found is that all four new .cs files are missing the UTF-8 BOM required by .editorconfig (every sibling file in the folder has one).
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Medium
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Review Summary — Phases 3–5 Native MTP Framework
Verdict: COMMENT (no blocking issues)
Overall this is a well-structured introduction of the native MSTestTestFramework. The architecture cleanly separates the native MTP path from the bridge, the CountdownEvent lifecycle is correct, and the opt-in gating via MSTEST_EXPERIMENTAL_NATIVE_MTP keeps shipping behavior untouched.
Dimension Results
| # | Dimension | Result |
|---|---|---|
| 1 | Algorithmic Correctness | BuildFilter escape logic uses wrong loop variable (i vs k). Pre-existing bridge parity; latent. |
| 2 | Concurrency / Thread Safety | ✅ LGTM — CountdownEvent pattern mirrors bridge; protocol guarantees prevent race. |
| 3 | Resource Management | ✅ LGTM — IDisposable on CountdownEvent; no leaked handles. |
| 4 | Defensive Coding | default switch case (see inline). |
| 5 | Error Handling | ✅ LGTM — Exceptions propagate correctly; finally ensures Signal()+Complete(). |
| 6 | API Surface | ✅ LGTM — All new types are internal. No public API additions. |
| 7 | Backward Compatibility | ✅ LGTM — Opt-in only; bridge path unchanged. SessionUid visibility downgrade is internal. |
| 8 | Performance | ✅ LGTM — No unnecessary allocations on hot path; XDocument parsing acceptable for settings. |
| 9 | Naming & Style | ✅ LGTM |
| 10 | Documentation | ✅ LGTM — XML docs are thorough. |
| 11-22 | Remaining dims | N/A or LGTM |
Notes
- The
sync-over-asyncinMSTestFrameworkHandle.SendMessage(.GetAwaiter().GetResult()) mirrors the bridge'sFrameworkHandlerAdapter— acceptable for this opt-in path. PatchTestRunParametersis safe against missing=because the upstreamValidateOptionArgumentsAsyncrejects such input.- The
IRunSettings.RunSettingsproperty getter onMSTestFilterContextBaseis a nullableIRunSettings?but the constructor always assigns it — this is fine.
Solid work. The three inline suggestions are non-blocking quality improvements.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…warning - Re-save the four new TestingPlatformAdapter files as UTF-8 with BOM (per .editorconfig charset=utf-8-bom). - MSTestRunSettings.WarnOnUnsupportedEntries: filter the settings sequence explicitly with .Where(...) instead of a foreach + if. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
- MSTestTestFramework.CreateTestSessionAsync: throw InvalidOperationException if a session was already created (mirrors the bridge base), instead of silently overwriting. - MSTestTestFramework.ExecuteRequestAsync: throw NotSupportedException for unrecognized request types instead of silently completing. - MSTestFilterContext.BuildFilter: fix the escape-already-present check to index the previous character with the inner loop index k (was i, the test-node index) — avoids a potential IndexOutOfRangeException and corrects the escaping. The bridge's ContextAdapterBase has the same latent bug; a follow-up can fix it there too. Validated: full build 0/0 (warnings-as-errors); native-path acceptance FilterTests+ServerModeTests+RunsettingsTests 51/51 green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ureAwait - MSTestRunSettings.ReadRunSettings: use the first --settings argument when one or more are provided (Length > 0), matching the runsettings environment-variable provider, instead of requiring exactly one and silently ignoring an otherwise-valid value. - MSTestTestFramework.CreateTestSessionAsync: use the shared localized ExtensionResources.VSTestBridgedTestFrameworkSessionAlreadyCreatedErrorMessage instead of a hard-coded English string. - MSTestFrameworkHandle.SendMessage: add ConfigureAwait(false) before GetAwaiter().GetResult() to reduce deadlock risk on context-bound threads. Validated: full build 0/0 (warnings-as-errors). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
- AzureFoundry: add DefaultAzureCredential/managed identity auth details and required environment variables (PR #9707) - MSTestTestFramework: new entry documenting the native MTP ITestFramework for MSTest introduced by RFC 018 (PRs #9706, #9743, #9748, #9755) - VSTestBridge: note MSTest no longer depends on it on the MTP path as of MSTest 4.3 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- AzureFoundry: add DefaultAzureCredential/managed identity auth details and required environment variables (PR #9707) - MSTestTestFramework: new entry documenting the native MTP ITestFramework for MSTest introduced by RFC 018 (PRs #9706, #9743, #9748, #9755) - VSTestBridge: note MSTest no longer depends on it on the MTP path as of MSTest 4.3 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow-up to #9706 (RFC 018 + Phases 1–2, merged). Continues retiring the
Microsoft.Testing.Extensions.VSTestBridgedependency from MSTest's Microsoft.Testing.Platform (MTP) code path. Everything is behind the opt-inMSTEST_EXPERIMENTAL_NATIVE_MTPswitch, so the shipping default (the bridge) is unchanged.What
A native
MSTestTestFramework : ITestFramework, IDataProducerthat handles the MTP discovery/run request directly and reuses MSTest's existingMSTestDiscoverer/MSTestExecutorengine — removing all VSTest bridge object-model adapters (RunContextAdapter,DiscoveryContextAdapter,FrameworkHandlerAdapter,RunSettingsAdapter,TestCaseDiscoverySinkAdapter,MessageLoggerAdapter) from the native request path.New (all
#if !WINDOWS_UWP):MSTestFilterContext(MSTestRunContext/MSTestDiscoveryContext) — nativeIRunContext/IDiscoveryContextthat builds the VSTestITestCaseFilterExpressionfrom the MTPITestExecutionFilter+--filter+ runsettings<TestCaseFilter>(reusingMicrosoft.TestPlatform.Filter.Source, now referenced directly). MSTest's existingTestMethodFiltermatching is unchanged.MSTestRunSettings— nativeIRunSettings: reads runsettings, patches MTP defaults (DesignMode / ResultsDirectory /--test-parameter), and warns on unsupported entries (reusing the bridge's already-localizedExtensionResources).MSTestFrameworkHandle— anIFrameworkHandlethat only forwards messages toIOutputDevice(results flow through the nativeMtpTestResultRecorder).MSTestTestFramework— session lifecycle + request handling; drives discovery/execution through the native seams from Native MTP integration for MSTest — RFC 018 + Phases 1–5 (opt-in native path) #9706.AddMSTestbranches the framework factory on the flag. The bridge's shared command-line/config/env-var registrations are kept for now (retired with the dependency in the final step).MSTestBridgedTestFrameworkreverts to pure-bridge (native production now lives in the native framework), and the now-unused Phase 2SessionUidhelper is removed from the bridge base.Validation
.\build.cmd— 0 warnings, 0 errors across all TFMs (rebased onto currentmain).MSTestAdapter.UnitTests) + 898 (MSTestAdapter.PlatformServices.UnitTests), all green.FilterTests,RunsettingsTests(incl. localization),OutputTests,TrxReportTests,ThreadingTests(STA),ServerModeTests,DeploymentItem,DataSourceTests,Inconclusive/Ignore,TestRunParameters,TimeoutTests.FilterTests+RunsettingsTests(45) — unchanged.Remaining — Phase 6 (separate, gated)
Flip the default to native and drop the
VSTestBridgedependency from MSTest. Gated on (1) full MSTest acceptance-suite parity on the native path (needs CI) and (2) native replacements for the bridge's remaining shared registrations (the--filter/--settings/--test-parameteroption providers and the runsettings config/env-var providers) so the package reference can be removed.The bridge package itself is not removed — NUnit/Expecto/3rd-party adapters still use it.