Skip to content

Native MTP integration for MSTest — RFC 018 + Phases 1–5 (opt-in native path) - #9706

Merged
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
dev/amauryleve/remove-vstest-bridge-mstest
Jul 8, 2026
Merged

Amaury Levé (Evangelink) merged 4 commits into
mainfrom
dev/amauryleve/remove-vstest-bridge-mstest

Conversation

@Evangelink

@Evangelink Amaury Levé (Evangelink) commented Jul 7, 2026 •

Copy link
Copy Markdown
Member

What

Works toward removing the Microsoft.Testing.Extensions.VSTestBridge dependency from MSTest's Microsoft.Testing.Platform (MTP) code path, so MSTest plugs into MTP natively. Delivered as a reviewable, phased series — all behind the opt-in MSTEST_EXPERIMENTAL_NATIVE_MTP switch, so the shipping default (the bridge) is unchanged.

  • RFC 018 — the roadmap/design (docs/RFCs/018-...md).
  • Phase 1 — native TestNode production seam (MSTestTestNodeConverter, MtpUnitTestElementSink, MtpTestResultRecorder).
  • Phase 2 — proved the seams by wiring them behind the flag (now superseded by the native framework).
  • Phases 3–5 — a native MSTestTestFramework that handles the MTP request directly: native filtering, runsettings/config, message logging, and session lifecycle. No VSTest bridge object-model adapters on the native request path.
  • Review feedback addressed (async neutral seams, UTF-8 BOM, doc style).

Why

On the MTP path, MSTest round-trips through the VSTest object model (context/filter/runsettings/handle/sink adapters) even though its engine already runs on neutral models. Going native removes that plumbing.

Phases 3–5 in the latest commit

A native MSTestTestFramework : ITestFramework, IDataProducer replaces the bridge framework on the flag path and reuses MSTest's existing MSTestDiscoverer/MSTestExecutor engine. New (all #if !WINDOWS_UWP):

  • MSTestFilterContext (MSTestRunContext/MSTestDiscoveryContext) — native IRunContext/IDiscoveryContext that builds the VSTest ITestCaseFilterExpression from the MTP ITestExecutionFilter + --filter + runsettings <TestCaseFilter> (reusing Microsoft.TestPlatform.Filter.Source, now referenced directly). MSTest's existing TestMethodFilter matching is unchanged.
  • MSTestRunSettings — native IRunSettings: reads runsettings, patches MTP defaults (DesignMode/ResultsDirectory/--test-parameter), and warns on unsupported entries (reusing the bridge's already-localized ExtensionResources).
  • MSTestFrameworkHandle — an IFrameworkHandle that only forwards messages to IOutputDevice (results flow through the native recorder).
  • AddMSTest branches 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).

Validation

  • Full .\build.cmd — 0 warnings, 0 errors across all TFMs.
  • Unit tests: 45 (MSTestAdapter.UnitTests) + 898 (MSTestAdapter.PlatformServices.UnitTests), all green.
  • Native path (flag on, acceptance): 185 tests green — FilterTests, RunsettingsTests (incl. localization), OutputTests, TrxReportTests, ThreadingTests (STA), ServerModeTests, DeploymentItem, DataSourceTests, Inconclusive/Ignore, TestRunParameters, TimeoutTests.
  • Default bridge path (flag off, acceptance): FilterTests + RunsettingsTests (45) — unchanged.

Remaining — Phase 6 (separate, gated)

Flip the default to native and drop the VSTestBridge dependency from MSTest. This is gated on:

  1. Full MSTest acceptance-suite parity on the native path (hundreds of tests — needs CI).
  2. Native replacements for the bridge's remaining shared registrations (the --filter/--settings/--test-parameter option 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. Only MSTest stops depending on it.

…bridge)

Proposes a staged roadmap to plug MSTest directly into Microsoft.Testing.Platform as a native ITestFramework, replacing the Microsoft.Testing.Extensions.VSTestBridge indirection on the MTP code path. Documents the existing neutral seams (IUnitTestElementSink, ITestResultRecorder, IAdapterMessageLogger, ITestElementFilterProvider, DeploymentContext), a 6-phase plan, capability parity checklist, testing strategy, and risks.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 7, 2026 12:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds RFC 018, a design/roadmap document proposing that MSTest plug directly into Microsoft.Testing.Platform (MTP) as a first-class ITestFramework, retiring the Microsoft.Testing.Extensions.VSTestBridge indirection on the MTP code path. It is a documentation-only artifact (status "Under discussion") with no production code changes, intended to let maintainers steer the approach before implementation. The RFC documents the current double-conversion architecture, the existing neutral seams the engine already uses, a 6-phase independently-shippable roadmap, a capability/feature-parity checklist, and the testing strategy, risks, and open questions.

Changes:

  • Documents current vs. target MTP architecture, highlighting the wasteful neutral model → VSTest TestCase/TestResult → TestNode double conversion and the resulting information loss (empty AssemblyFullName/ReturnTypeFullName).
  • Lays out a 6-phase roadmap that keeps the bridge as default until the native path reaches parity, with a feature-parity checklist and dual-run testing strategy.
  • Follows the established docs/RFCs/ convention; the bridge package is not removed (NUnit/Expecto/third-party adapters still use it) — only MSTest stops depending on it.
Show a summary per file
File Description
docs/RFCs/018-Native-MTP-Integration-For-MSTest.md New RFC describing the native MTP integration design, phasing, parity checklist, testing strategy, risks, and open questions. All referenced types, seams, file paths, and the quoted in-code comment were verified as accurate against the codebase.

Review details

  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Medium

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
17 Documentation Accuracy 🟢 2 NIT
21 Scope & PR Discipline 🟢 1 NIT

✅ 20/22 dimensions N/A (no code changes). 2 dimensions reviewed — only minor nits found.

Summary: This is a well-structured, thoroughly researched RFC. The claims about existing neutral seams (IUnitTestElementSink, ITestResultRecorder, IAdapterMessageLogger, ITestElementFilterProvider) are verified against the codebase, the MSTestBridgedTestFramework information-loss quote is accurate (line 100–106 in that file), and the 6-phase roadmap with the capability parity checklist covers all bridge features I can identify. The design principle of "boundary replacement, not engine rewrite" is well-supported by the architecture diagrams.

  • Path convention: use forward slashes for cross-platform consistency (line 84)
  • Consider linking a tracking issue for the 6-phase roadmap

Adds the first, unwired building block for plugging MSTest directly into Microsoft.Testing.Platform instead of routing through the VSTest bridge:

- MSTestTestNodeConverter: builds MTP TestNodes directly from MSTest's neutral models (UnitTestElement + framework TestResult), faithfully porting the mapping the bridge does today via ObjectModelConverters/TestResultExtensions/UnitTestElementExtensions (outcome, timing, std out/err, TRX categories/messages/exception/type-name, traits, file location, TestMethodIdentifierProperty).
- MtpUnitTestElementSink / MtpTestResultRecorder: MTP-native IUnitTestElementSink / ITestResultRecorder that publish TestNodeUpdateMessage on the message bus.
- MSTestTestNodeException: native message+stacktrace carrier (replaces the bridge's VSTestException on the MSTest path).
- UnitTestElementExtensions.GetTestId: exposes the stable test id neutrally, without materializing a VSTest TestCase.

The bridge remains the default; these seams are covered by 24 unit tests and are wired in a later phase. No behavior change.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@Evangelink Amaury Levé (Evangelink) changed the title docs: RFC 018 — native MTP integration for MSTest (retire the VSTest bridge) Native MTP integration for MSTest — RFC 018 + Phase 1 (TestNode production seam) Jul 7, 2026
@github-actions

This comment has been minimized.

When the MSTEST_EXPERIMENTAL_NATIVE_MTP opt-in is set, MSTest publishes discovery and result TestNodes directly via the Phase 1 native converter+seams, bypassing the bridge's ObjectModelConverters / FrameworkHandlerAdapter / TestCaseDiscoverySinkAdapter. The bridge remains the default; only context/filter/config/command-line parsing still flows through it.

Changes:
- VSTestBridge base: expose the session UID as an internal auto-property (bridge already has InternalsVisibleTo MSTest.TestAdapter; no public API change).
- MSTestDiscoverer: add an IUnitTestElementSink overload and share a discovery core so the native sink is used directly instead of discoverySink.ToUnitTestElementSink().
- MSTestExecutor: add a Func<MSTestSettings,ITestResultRecorder> overload and share a run core so a native recorder is used instead of frameworkHandle.ToTestResultRecorder(); the framework handle is still used for message logging and apartment-state handling.
- MSTestBridgedTestFramework: flag-gated native wiring.

Validated: full build 0/0; MSTestAdapter.UnitTests 45/45; native-path acceptance OutputTests(6)+TrxReportTests(3)+Inconclusive/Ignore/DataSource(38) all green; default bridge-path OutputTests(6) unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 7, 2026 14:04
@Evangelink Amaury Levé (Evangelink) changed the title Native MTP integration for MSTest — RFC 018 + Phase 1 (TestNode production seam) Native MTP integration for MSTest — RFC 018 + Phase 1 & 2 (opt-in native TestNode path) Jul 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 11/11 changed files
  • Comments generated: 5
  • Review effort level: Medium

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

🧪 Test quality grade — PR #9706

24 new test methods in MSTestTestNodeConverterTests cover MSTestTestNodeConverter, MtpUnitTestElementSink, and MtpTestResultRecorder. All 24 earn A (90–100): clear Arrange-Act-Assert structure, focused single-behavior design, and rich AwesomeAssertions throughout. Two minor opportunities: some tests pass DateTimeOffset.Now as a non-timing parameter (a named constant would be cleaner), and one negative test omits an absence assertion for symmetry with its enabled counterpart.

GradeTestNotes
A (90–100) new MSTestTestNodeConverterTests.
MtpTestResultRecorder_
RecordEmptyResult_
PublishesNothing
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
MtpTestResultRecorder_
RecordResult_
PublishesResultNodeAndReturnsFailedFlag
Tests both return value and published state; consider splitting into two focused tests for sharper isolation.
A (90–100) new MSTestTestNodeConverterTests.
MtpTestResultRecorder_
RecordResult_
ReturnsFalse_
ForPassedTest
Return value correctly checked; consider also asserting the published node carries PassedTestNodeStateProperty for symmetry.
A (90–100) new MSTestTestNodeConverterTests.
MtpTestResultRecorder_
RecordStart_
PublishesInProgressNode
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
MtpUnitTestElementSink_
PublishesDiscoveredTestNode
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToDiscoveredTestNode_
AddsCategoriesAndTraitsAsMetadata
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToDiscoveredTestNode_
AddsFileLocation_
WhenDeclaringFileKnown
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToDiscoveredTestNode_
AddsTestMethodIdentifier_
FromManagedNames
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToDiscoveredTestNode_
AddsTrxCategories_
OnlyWhenTrxEnabled
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToDiscoveredTestNode_
DoesNotAddFileLocation_
WhenDeclaringFileUnknown
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToDiscoveredTestNode_
DoesNotAddTestMethodIdentifier_
WhenNoManagedMethodName
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToDiscoveredTestNode_
SetsUidDisplayNameAndDiscoveredState
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToDiscoveredTestNode_
UsesExplicitDisplayName_
WhenProvided
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToInProgressTestNode_
AddsInProgressState
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
AddsTimingProperty
Uses explicit deterministic timestamps; duration equality verified — no issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
AddsTrxProperties_
WhenTrxEnabled
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
DoesNotAddTrxProperties_
WhenTrxDisabled
Consider also asserting TrxFullyQualifiedTypeNameProperty absence for symmetry with the enabled test.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
IncludesDebugTraceAndTestContextBannersInStandardOutput
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
MapsFailedOutcome_
WithMessageAndStackTrace
Both exception fields verified; replace DateTimeOffset.Now in non-timing args with a fixed constant for clarity.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
MapsIgnoredOutcomeToSkipped
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
MapsInconclusiveToSkipped_
ByDefault
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
MapsLogOutputAndLogErrorToStandardStreams
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
MapsNotFoundOutcomeToError
No issues found.
A (90–100) new MSTestTestNodeConverterTests.
ToResultTestNode_
MapsPassedOutcome
No issues found.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · 145.8 AIC · ⌖ 10.8 AIC · ⊞ 9.5K · [◷]( · ◷)

@Evangelink Amaury Levé (Evangelink) added the state/needs-review Awaiting review from the team. label Jul 7, 2026
- Make the neutral discovery/result seams asynchronous end-to-end (per review): ITestResultRecorder (RecordStartAsync/RecordEmptyResultAsync/RecordResultAsync) and IUnitTestElementSink (SendTestElementAsync). Thread async through UnitTestDiscoverer (DiscoverTestsAsync/DiscoverTestsInSourceAsync/SendTestCasesAsync) and TestExecutionManager.SendTestResultsAsync. The MTP implementations now await the message-bus publish instead of blocking with GetAwaiter().GetResult(); the VSTest implementations return completed tasks (sync fast-path preserves STA behavior).
- Add the UTF-8 BOM required by .editorconfig to the five new .cs files.
- Use forward slashes for the seam path in RFC 018.

Validated: full build 0/0; unit tests MSTestAdapter.UnitTests 45/45 and MSTestAdapter.PlatformServices.UnitTests 898/898; acceptance default-path threading/output/trx 41 green; native-path threading/output/trx/outcome/data-driven 75 green.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@Evangelink
Amaury Levé (Evangelink) enabled auto-merge (squash) July 8, 2026 10:16
@Evangelink
Amaury Levé (Evangelink) merged commit 641c192 into main Jul 8, 2026
101 of 102 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/remove-vstest-bridge-mstest branch July 8, 2026 12:02
github-actions Bot added a commit that referenced this pull request Jul 8, 2026
Apply two project-convention improvements to files added in recent PRs:

- MSTestTestNodeConverter.cs (added in #9706): replace Substring calls
  with range operators per csharp_style_prefer_range_operator = true
- TestMethodRunner.DataSource.cs (added in #9729): replace == null with
  is null per the project's 'always use is null / is not null' rule

No behavioral change; builds verified to succeed with 0 errors.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink Amaury Levé (Evangelink) changed the title Native MTP integration for MSTest — RFC 018 + Phase 1 & 2 (opt-in native TestNode path) Native MTP integration for MSTest — RFC 018 + Phases 1–5 (opt-in native path) Jul 8, 2026
github-actions Bot added a commit that referenced this pull request Jul 9, 2026
- 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>
Amaury Levé (Evangelink) pushed a commit that referenced this pull request Jul 9, 2026
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state/needs-review Awaiting review from the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants