Disable the MTP testhost by default - #16337
Merged
Jakub Jareš (nohwnd) merged 3 commits intoAug 4, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the “MTP under vstest” execution path explicitly opt-in (via VSTEST_OPTIN_MTP), so Microsoft.Testing.Platform-marked sources no longer select the MTP runtime provider by default. This aligns the implementation with the stated “experimental” status by gating provider selection and ensuring test coverage and documentation reflect the opt-in contract.
Changes:
- Gate
MtpTestRuntimeProvidersource-aware selection behindFeatureFlag.VSTEST_OPTIN_MTP. - Update MTP acceptance tests to explicitly opt in for their runs.
- Document the new opt-in environment variable and add unit tests for the provider gating behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.TestHostProvider.UnitTests/Hosting/MtpTestRuntimeProviderTests.cs | Adds unit tests validating opt-in gating and short-circuiting of MTP detection when disabled. |
| test/Microsoft.TestPlatform.Acceptance.IntegrationTests/MtpUnderVstestTests.cs | Ensures MTP acceptance scenarios set VSTEST_OPTIN_MTP for all vstest invocations. |
| src/Microsoft.TestPlatform.TestHostProvider/Hosting/MtpTestRuntimeProvider.cs | Implements the opt-in gate in provider selection and improves testability via injected detector/feature flag. |
| src/Microsoft.TestPlatform.CoreUtilities/FeatureFlag/FeatureFlag.cs | Introduces VSTEST_OPTIN_MTP constant and clarifies feature-flag usage guidance for experimental opt-ins. |
| docs/Overview.md | Updates overview text to reflect opt-in requirement for the experimental MTP path. |
| docs/environment-variables.md | Documents VSTEST_OPTIN_MTP semantics and default behavior. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Microsoft.TestPlatform.CoreUtilities/FeatureFlag/FeatureFlag.cs:82
- This introduces an opt-in enable flag, but the header comment in this file strongly recommends never using flags to enable functionality. Without an explicit rationale here, future maintainers may treat this as accidental/inconsistent and rename/remove it incorrectly. Consider clarifying that this is a deliberate exception to keep the MTP path off-by-default while experimental.
// Opt-in to running Microsoft.Testing.Platform applications under vstest while the integration is experimental.
public const string VSTEST_ENABLE_MTP_TESTHOST = nameof(VSTEST_ENABLE_MTP_TESTHOST);
src/Microsoft.TestPlatform.TestHostProvider/Hosting/MtpTestRuntimeProvider.cs:83
- With the feature flag off, this provider declines MTP sources and the run falls back to generic host resolution, which appears to surface a generic "Could not find testhost for test source" error for MTP apps. That message is misleading in this scenario; consider emitting an actionable error that explicitly mentions
VSTEST_ENABLE_MTP_TESTHOSTwhen MTP sources are detected but the feature is disabled.
bool ISourceAwareTestRuntimeProvider.CanExecuteCurrentRunConfiguration(string? runsettingsXml, IEnumerable<string> sources)
=> FeatureFlag.Instance.IsSet(FeatureFlag.VSTEST_ENABLE_MTP_TESTHOST) && AllSourcesAreMicrosoftTestingPlatform(sources);
Jakub Jareš (nohwnd)
changed the title
Jakub Jareš (nohwnd)
enabled auto-merge (squash)
August 3, 2026 15:45
Azat Mukhametshin (azat-msft)
approved these changes
Aug 4, 2026
Jakub Jareš (nohwnd)
added a commit
that referenced
this pull request
Aug 4, 2026
Jakub Jareš (nohwnd)
added a commit
to nohwnd/vstest
that referenced
this pull request
Aug 10, 2026
RunMtpApplicationPreservesNonAsciiTestNames drove the MTP app with a plain InvokeVsTest, which stopped detecting the app after main merged microsoft#16337 (MTP testhost disabled by default). Align it with every other MTP-driving test by using InvokeVsTestWithMtpTestHostEnabled, so the net11.0 runner finds the testhost again. net11.0 is back to a full pass; the remaining net481 /logger:trx failures are the pre-existing environmental logger-load issue on the desktop runner, unrelated to this change. 🤖
Jakub Jareš (nohwnd)
added a commit
that referenced
this pull request
Aug 14, 2026
…ient.Sources (#16300) * Retarget the MTP client onto Microsoft.Testing.Platform.ServerClient.Source testfx now ships vstest's MTP server-mode JSON-RPC client as a source-only package built from the MTP server's own protocol and serialization source, so the wire format cannot drift from the server. Delete vstest's transport core (MtpServerConnection, MtpJson, MtpConstants, MtpClientHelpers) and retarget the glue onto the package's IMtpServerClient: launch via MtpServerClient.Launch, drive Initialize/Discover/Run/Exit, read node updates from the TestNodesUpdated event with typed MtpTestNodeUpdate accessors, and bridge EqtTrace through DelegateMtpClientLogger. MtpClientOptionsFactory centralizes option construction and log-level mapping. The package is a compile-time source dependency (PrivateAssets=all), so no runtime dependency and no public API are added. Blocked on testfx publishing the package (microsoft/testfx#10085); references an interim local feed, so CI cannot restore it yet. * Commit the interim local MTP client feed so restore works everywhere NuGet.config pointed local-mtp at the absolute path Q:\q\local-mtp-feed, which is machine-local and does not exist in CI, so restore failed with an incorrect path. Move the feed under the repo at eng/local-mtp-feed, point NuGet.config at that repo-relative path, and commit the package into the feed. .gitignore keeps ignoring *.nupkg but adds a negation for eng/local-mtp-feed/*.nupkg so the feed package is tracked. The package is the fresh Design-A drop of Microsoft.Testing.Platform.ServerClient.Source 2.4.0-dev, which builds CrossPlatEngine clean on net462, netstandard2.0, and net8.0 (0 errors, 0 warnings) with the retargeted glue. Interim only; remove the feed once microsoft/testfx#10085 ships the package to a public feed. 🤖 * Order remote NuGet feeds before the interim local feed in test asset restore The acceptance tests restore the TestAssets solution, which transitively restores product projects like CrossPlatEngine that now reference the interim local-mtp feed. Passing that local-folder feed to dotnet restore alongside the remote https feeds triggered two NuGet quirks, both surfacing as NU1301: a relative --source path is rooted at each restored project's directory, and a local-folder source placed before the remote sources mis-normalizes the https URLs into per-project relative paths. Resolve relative local-folder sources to absolute paths and emit the remote sources first so all local-folder sources come last; remote feeds keep their configured order. Only needed while the MTP client package lives on the interim local feed, and harmless once testfx#10085 ships it to a public feed. 🤖 * Key MTP environment variable dictionary case-insensitively on Windows Both places that collect environment variables for the MTP application launch now share one comparer: case-insensitive on Windows, case-sensitive elsewhere. Before, the runsettings path used that comparer but the data-collector-only path used a plain ordinal dictionary, so a run with no runsettings variables but with data-collector variables lost the case-folding the classic testhost path applied on Windows. The package options dictionary is ordinal, so deduping here preserves the classic Windows semantics before the values reach it. 🤖 * Consume official B-fixed MTP client source drop (testfx#10085) Replaces the interim 2.4.0-dev pack with the official drop that fixes the STJ number-decode bug: untyped JSON numbers were hard-cast to Int32, so node bags carrying doubles (durations) or longs (timestamps) threw FormatException and faulted the MTP read loop on the net8 client. The fix decodes numbers generically (ReadNumber: TryGetInt32 -> TryGetInt64 -> TryGetUInt64 -> double). Pinned to the unique version 2.4.0-dev.20260721161520 to avoid NuGet same-version cache collisions while the package is served from the committed local feed. MtpUnderVstestTests: net11.0 (STJ) axis now 7/7 (was 0/7); net481 (Jsonite) axis 5/7. The 2 remaining failures are a pre-existing net462 TRX-logger load issue that also breaks classic non-MTP trx tests, unrelated to this retarget. 🤖 * Align interim MTP client pin to the coordinator's canonical numberfix drop Swaps the interim feed pack and pin from the timestamped unique 2.4.0-dev.20260721161520 to the coordinator's canonical uniquely-named drop 2.4.0-dev.numberfix (MD5 FC7F7A9F68EF482718B61DC9DA5F38B4). Byte-equivalent fixed content -- the packed net8 Json.Deserializers.cs decodes untyped JSON numbers via ReadNumber at both sinks (L55/L97, helper L344), same as the prior drop -- this only adopts the stable canonical interim identity the package owner is standardizing on across consumers. Validation unchanged: MtpUnderVstestTests net11.0 (STJ) axis 7/7, full suite 12/14 (the 2 remaining failures are the pre-existing net462 TRX-logger load issue, unrelated to this retarget). 🤖 * Add MTP converter/options unit tests and fix numeric and trait coercion The retarget onto Microsoft.Testing.Platform.ServerClient.Source left the MTP glue with no unit coverage at all - the only tests were the end-to-end MtpUnderVstestTests. The conversion code is now pure and dependency-free, so cover it directly. Add MtpTestNodeConverterTests and MtpClientOptionsFactoryTests (55 tests) covering the normalized-Node contract, per-formatter number boxing, outcome mapping, the action-node filter, vstest bridge properties, standard output/error, traits, duration and log-level mapping. Three fixes fall out of writing them: - TryGetRawInt wrapped out-of-range values with unchecked((int)l), turning a bad line number into a plausible-looking wrong answer. Range-check instead so the property stays at its visibly-unset default. - AddTraits collapsed every non-string trait value to an empty string. The two formatters box JSON scalars differently, so a numeric or boolean trait was silently dropped on one formatter and kept on the other. Format invariantly. - MtpClientOptionsFactory re-read VSTEST_CONNECTION_TIMEOUT and hardcoded the 90-second default instead of calling EnvironmentHelper.GetConnectionTimeout, which seven other vstest call sites already use and which also traces the override. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Fix MTP client shutdown and fail loudly on a missing node uid Retargeting onto the source package changed exit from a fire-and-forget notification into an awaited request/response call, which introduced two regressions: - Exit was awaited on the run's own cancellation token. Cancelling or aborting a run is exactly when that token is already cancelled, so ExitAsync threw immediately and the graceful shutdown handshake was skipped in the one case it matters most. - The await was unbounded, so a test application that never acknowledges exit would hang discovery or execution indefinitely. The notification it replaced could not block at all. Route both proxy managers through MtpServerClientFactory: TryExit runs on its own bounded token, swallows failures (the caller disposes the client next, which tears the process down regardless), and is called from a finally block so a failed or cancelled run still shuts the application down. The factory also exposes a replaceable Launch delegate so the managers can be driven against a fake server in unit tests; production always uses MtpServerClient.Launch. Separately, BuildUids substituted FullyQualifiedName when a TestCase carried no MTP.TestNode.Uid. The server projects node.Uid alone when building a run filter and never reads any other field, so that substitution produced a filter matching nothing: the run reported success having executed zero of the tests the user selected, with no error anywhere. Throw instead, with a comment explaining why no fallback is correct. Adds 15 tests covering the shutdown paths, the uid filter, and both manager flows against a fake MTP server. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Add non-ASCII MTP acceptance coverage for UTF-8 frame length MTP frames declare Content-Length in UTF-8 bytes, but the transport shipped by Microsoft.Testing.Platform.ServerClient.Source reads that number of characters: it rents a char buffer of Content-Length and calls StreamReader.ReadBlockAsync. For any frame carrying multi-byte UTF-8 the two disagree, so the reader under-reads and leaves the body's tail to be parsed as the next frame's headers - the connection desynchronizes from the following message onward. vstest's deleted MtpServerConnection was byte-correct here (it read Content-Length bytes into a byte[] and then UTF-8-decoded), so the retarget is a regression, not an inherited defect. Client-to-server traffic is ASCII in practice, which is why it has not surfaced; node updates flow the other way and carry user-authored test names. Give MtpMSTestProject a test whose display name mixes German umlauts (2 bytes each), Japanese (3 bytes each) and an emoji (4 bytes, 2 chars), and mirror it in MtpPureProject. Because the corruption lands on the message *after* the offending one, its mere presence makes the whole run fail rather than just that test, so every existing MTP scenario now exercises the transport with multi-byte content. Adds a dedicated test asserting the name survives into the TRX. These fail until the fix lands upstream in testfx. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Narrow the non-ASCII MTP test name to the BMP and fix the collector count Running the acceptance test revealed two things worth recording. First, an end-to-end MTP run cannot reproduce the Content-Length byte-vs-char framing bug: the .NET MTP server serializes with System.Text.Json, whose default encoder escapes every non-ASCII character to \\uXXXX, so the bytes on the wire are ASCII and the byte count coincidentally equals the character count. The framing bug is real but has to be proved at the unit level against the transport directly, which is what the companion testfx change does. This test is therefore a name-integrity guard, and its comments now say so rather than overclaiming. Second, the emoji originally in the name exposed a separate defect: astral-plane characters are escaped by System.Text.Json as a surrogate pair and arrive in the TRX as the literal text \\ud83c\\udf89 instead of the character. BMP characters decode correctly. That is its own bug, tracked separately, so the name is narrowed to BMP multi-byte characters (umlauts 2 bytes, Japanese 3 bytes) which still exercise the byte-denominated length without tripping over it. Also updates the out-of-proc data collector's expected per-test-case attachment count, which follows the test count. MtpUnderVstestTests: 16/16 on both console axes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Consume the MTP client drop with the Content-Length framing fix Replaces the interim local-feed pack with a build of microsoft/testfx#10297, which stacks the Content-Length byte/char fix onto #10085. The transport now reads exactly Content-Length bytes and UTF-8-decodes them, symmetric with the write path, and reads the headers through the same byte-level buffer so no StreamReader can buffer part of the body across the boundary. That drop also carries #10085's ServerRequestHandler signature change (the result is now constrained to a serializable dictionary), so FakeMtpServerClient is updated to match. Verification on this drop: - CrossPlatEngine builds clean on net462, netstandard2.0 and net8.0. - MTP unit tests 140/140 (70 per axis, net11.0 and net481). - MtpUnderVstestTests 16/16 on both console axes. Note the 16/16: the two /logger:trx failures reported against the earlier drop do not reproduce here, so they look like a local deployment issue rather than anything in the retarget. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Repin the interim MTP client to the uniquely-named utf8fix1 drop Swaps the interim feed pack from the plain 2.4.0-dev build output to the coordinator's canonical 2.4.0-dev.utf8fix1 drop of microsoft/testfx#10297. Byte-equivalent content: all 184 contentFiles are identical between the two packs, including TcpMessageHandler.cs with both ReadExactlyAsync and the TrimPreamble BOM tolerance. Only the version metadata differs. The rename is the point. While the package is served from a committed local folder, NuGet caches by version, so a plain 2.4.0-dev risks silently resolving a stale cache entry from an earlier drop of the same name. The unique suffix makes that impossible, matching the convention the branch already used for 2.4.0-dev.numberfix. Re-verified from a cleared package cache: CrossPlatEngine clean on all three TFMs, MTP unit tests 140/140, MtpUnderVstestTests 16/16. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Address expert review feedback on the MTP hardening Localize the missing-uid error. The message reaches the user verbatim - StartTestRun funnels ex.Message into HandleLogMessage(Error) - and every other user-facing TestPlatformException in this assembly is resourced, so a hardcoded English string formatted with CurrentCulture was self-contradictory. Adds MtpTestCaseMissingNodeUid to Resources.resx, the generated designer property, and a trans-unit to all 13 xlf files. The text now also states the remedy (re-run discovery, or run without a selection) rather than only naming the failure, and the comment records that aborting the whole source is deliberate: silently running the addressable subset would recreate the same class of bug in a smaller form. Mark the three new test classes DoNotParallelize. MSTest parallelizes across classes at MethodLevel by default here, and these classes mutate process-global state - the MtpServerClientFactory.Launch seam and VSTEST_CONNECTION_TIMEOUT - so a save/restore in TestInitialize/TestCleanup could restore one class's value while another class's test was still relying on its own. That would have flaked in CI looking like a product bug. Close a hole in the float range guard. (float)int.MaxValue rounds *up* to 2147483648f, so comparing a float directly against int.MaxValue let that value through and the cast then saturated - precisely the plausible-looking wrong answer the guard exists to reject. Widen to double before comparing, and extend the regression test to cover it. Capture ProcessId before the exit handshake instead of reading it afterwards, when the process may already be gone. Test fixes: TryExitDoesNotUseAnAlreadyCancelledRunToken was vacuous (it built a cancelled token it never passed anywhere) and LaunchDefaultsToTheRealClientLauncher asserted only non-null, which any delegate satisfies. Both now assert something that fails if the behaviour regresses. Adds the missing mixed-selection case, where only some tests carry a uid. Also fixes a stale test-count comment and softens an overclaim in MtpPureProject, which no test currently references. Unit tests 142/142 across net11.0 and net481; MtpUnderVstestTests 16/16 on both console axes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Consume the latest MTP client drop from testfx#10297 Picks up the two commits that landed on the testfx branch after the utf8fix1 pack: the header line buffer is now reused across lines instead of allocated per line (server mode emits a notification per test, so that was a real hot-path allocation), plus comments recording why Content-Length is intentionally not capped and why the framing tests are not cross-TFM coverage. Both changes are to TcpMessageHandler, which compiles into CrossPlatEngine, so they are verified here rather than assumed. Re-verified from a cleared NuGet package cache: - CrossPlatEngine builds clean on net462, netstandard2.0 and net8.0. - MTP unit tests 142/142 across net11.0 and net481. - MtpUnderVstestTests 16/16 on both console axes. - testfx's own ServerClient unit tests 48/48, confirming the shared transport is still good on both formatter paths. The buffer is safe to hold as instance state for the same reason the existing read offsets are: reads are single-threaded, driven by exactly one read loop. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f * Consume the published MTP client package; drop the interim local feed testfx#10085 shipped the source-only MTP server-mode client to the dnceng-public dotnet-tools feed (already configured in NuGet.config), under its final name Microsoft.Testing.Platform.ServerMode.Client.Sources. Repin CrossPlatEngine from the interim local-feed drop (Microsoft.Testing.Platform.ServerClient.Source 2.4.0-dev.utf8fix2) to the published 2.4.0-preview.26410.1 and remove the whole interim scaffolding: - eng/local-mtp-feed and its NuGet.config source + .gitignore exception. - The GetNugetSourceParameters feed-order workaround in IntegrationTestBuild, which only existed to make a local-folder source restore alongside the remote https feeds. With no local folder it reverts to the simple base. The published package compiles its own down-level nullable-annotation polyfills on net462/netstandard2.0, which collide with the identical set CrossPlatEngine already imports from CoreUtilities (CS0436). Define MTP_CLIENT_EXCLUDE_NULLABLE_ATTRIBUTES so the package defers to those; it is a no-op on net8.0 where the attributes are in-box. The C# namespace (Microsoft.Testing.Platform.ServerMode.Client) is unchanged, so the retarget glue and azat's unit tests bind to the published package with no code change. Restore resolves 2.4.0-preview.26410.1 from the real feed with no local folder; build is clean on all three TFMs. 🤖 * Enable the MTP testhost in the non-ASCII acceptance test RunMtpApplicationPreservesNonAsciiTestNames drove the MTP app with a plain InvokeVsTest, which stopped detecting the app after main merged #16337 (MTP testhost disabled by default). Align it with every other MTP-driving test by using InvokeVsTestWithMtpTestHostEnabled, so the net11.0 runner finds the testhost again. net11.0 is back to a full pass; the remaining net481 /logger:trx failures are the pre-existing environmental logger-load issue on the desktop runner, unrelated to this change. 🤖 * Reject fractional MTP line numbers Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Reject selected MTP nodes without UIDs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Azat Muzafarov <azatm@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The MTP under vstest path is still experimental, but it was enabled for every MTP-marked source.
Use a single
VSTEST_DISABLE_MTP_TESTHOSTflag.FeatureFlagdefaults it totrue, the environment value takes precedence, and setting it to0opts in. This avoids conflicting enable and disable flags.Verified the Release pack build, FeatureFlag tests on net11.0 and net481, and all 16 MTP acceptance cases.
🤖