Retarget the MTP client onto Microsoft.Testing.Platform.ServerMode.Client.Sources - #16300
Merged
Jakub Jareš (nohwnd) merged 20 commits intoAug 14, 2026
Merged
Conversation
…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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR retargets vstest’s Microsoft.Testing.Platform (MTP) server-mode JSON-RPC client integration in CrossPlatEngine from a hand-maintained transport implementation to the new source-only Microsoft.Testing.Platform.ServerClient.Source package, aiming to prevent protocol/serialization drift from the server.
Changes:
- Added a source-only
PackageReferenceto compile the MTP server client intoMicrosoft.TestPlatform.CrossPlatEngine.dll. - Updated MTP discovery/execution proxies and node conversion to use
MtpServerClientAPIs and typedMtpTestNodeUpdateupdates. - Removed the legacy in-repo transport/protocol helpers (
MtpServerConnection,MtpJson,MtpConstants,MtpClientHelpers) and introducedMtpClientOptionsFactory.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.TestPlatform.CrossPlatEngine/Microsoft.TestPlatform.CrossPlatEngine.csproj | Adds source-only package reference for the new MTP server client. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpTestNodeConverter.cs | Converts typed MTP node updates into vstest TestCase/TestResult and reads remaining fields from the raw property bag. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpServerConnection.cs | Deletes legacy TCP/LSP-framed JSON-RPC transport implementation. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpProxyExecutionManager.cs | Switches execution path to MtpServerClient (launch/init/run/exit) and consumes typed node updates + artifacts. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpProxyDiscoveryManager.cs | Switches discovery path to MtpServerClient (launch/init/discover/exit) and consumes typed node updates. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpJson.cs | Deletes legacy Jsonite-based JSON access helpers. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpConstants.cs | Deletes legacy protocol constants (method names, keys, state values). |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpClientOptionsFactory.cs | Adds options/logger glue for launching the MTP client and mapping log levels to vstest. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpClientHelpers.cs | Deletes legacy helpers for init params, completion sentinel, and node enumeration. |
| NuGet.config | Adds an interim local package source for restoring the unshipped source-only package. |
| eng/Versions.props | Adds interim version property for Microsoft.Testing.Platform.ServerClient.Source. |
Comment on lines
7
to
12
| <clear /> | ||
| <!-- INTERIM local feed for the unshipped source-only MTP client package | ||
| (Microsoft.Testing.Platform.ServerClient.Source). This is a machine-local path and will not | ||
| resolve in CI. Remove once the package ships to a public feed via microsoft/testfx#10085. --> | ||
| <add key="local-mtp" value="Q:\q\local-mtp-feed" /> | ||
| <!--Begin: Package sources managed by Dependency Flow automation. Do not edit the sources below.--> |
Comment on lines
+25
to
+27
| <!-- Source-only MTP server-mode JSON-RPC client: compiled into this assembly (no shipped DLL, no | ||
| runtime dependency, no new public API). INTERIM: restored from a local feed until | ||
| microsoft/testfx#10085 ships Microsoft.Testing.Platform.ServerClient.Source publicly. --> |
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. 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
NuGet.config:12
- Adding a repo-local package source (
eng/local-mtp-feed) plus checking in the .nupkg makes restores depend on committed binary artifacts and can mask updates from real feeds (the same version from a remote feed would still prefer the local copy). Before this PR is mergeable, this interim feed should be removed and restore should come from an authenticated/public feed.
<clear />
<!-- INTERIM committed local feed for the unshipped source-only MTP client package
(Microsoft.Testing.Platform.ServerClient.Source). Repo-relative so it resolves on every
machine and in CI. Remove once the package ships to a public feed via microsoft/testfx#10085. -->
<add key="local-mtp" value="eng/local-mtp-feed" />
<!--Begin: Package sources managed by Dependency Flow automation. Do not edit the sources below.-->
Comment on lines
+175
to
+186
| private static bool TryGetRawInt(MtpTestNodeUpdate update, string key, out int result) | ||
| { | ||
| switch (update.Node.TryGetValue(key, out object? value) ? value : null) | ||
| { | ||
| case int i: result = i; return true; | ||
| case long l: result = unchecked((int)l); return true; | ||
| case double d: result = (int)d; return true; | ||
| case float f: result = (int)f; return true; | ||
| case decimal m: result = (int)m; return true; | ||
| default: result = 0; return false; | ||
| } | ||
| } |
…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. 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/Microsoft.TestPlatform.CrossPlatEngine/Microsoft.TestPlatform.CrossPlatEngine.csproj:28
- The comment says the source-only package adds "no new public API", but source-embedded code can introduce public types into CrossPlatEngine if the package sources declare them as public. It would be safer to phrase this as an intended property contingent on the package keeping its types internal, to avoid misleading future readers (and to align with PublicApiAnalyzers expectations).
<!-- Source-only MTP server-mode JSON-RPC client: compiled into this assembly (no shipped DLL, no
runtime dependency, no new public API). INTERIM: restored from a local feed until
microsoft/testfx#10085 ships Microsoft.Testing.Platform.ServerClient.Source publicly. -->
<PackageReference Include="Microsoft.Testing.Platform.ServerClient.Source" Version="$(MicrosoftTestingPlatformServerClientSourceVersion)" PrivateAssets="all" />
src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpTestNodeConverter.cs:183
- TryGetRawInt uses unchecked casts from long/double/float/decimal to int. If the server sends a large value (or a negative one), this can wrap and produce an invalid TestCase.LineNumber. Since this is parsing untrusted wire data, it should range-check and reject values outside 0..int.MaxValue.
case int i: result = i; return true;
case long l: result = unchecked((int)l); return true;
case double d: result = (int)d; return true;
case float f: result = (int)f; return true;
case decimal m: result = (int)m; return true;
Comment on lines
+510
to
+517
| // Guard against both: resolve relative local-folder sources to absolute paths, and emit the remote | ||
| // sources first so all local-folder sources come last. Remote sources (containing "://") keep their | ||
| // configured order and pass through unchanged. | ||
| var remote = sources.Where(s => s.Contains("://")); | ||
| var local = sources | ||
| .Where(s => !s.Contains("://")) | ||
| .Select(s => Path.IsPathRooted(s) ? s : Path.GetFullPath(Path.Combine(root, s))); | ||
| var feeds = remote.Concat(local).ToList(); |
| connection.SendNotification(MtpConstants.ExitMethod, null); | ||
| return connection.ProcessId; | ||
| CollectAttachments(runResult, attachments); | ||
| client.ExitAsync(_cancellationTokenSource.Token).GetAwaiter().GetResult(); |
| } | ||
|
|
||
| connection.SendNotification(MtpConstants.ExitMethod, null); | ||
| client.ExitAsync(_cancellationTokenSource.Token).GetAwaiter().GetResult(); |
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. 🤖
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. 🤖
… 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). 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpProxyDiscoveryManager.cs:123
- Cleanup uses the shared cancellation token. If cancellation is requested during discovery, ExitAsync can throw OperationCanceledException and skip the protocol shutdown, potentially leaving the MTP process running and leaking resources. Use a non-cancelable token for cleanup/shutdown.
client.ExitAsync(_cancellationTokenSource.Token).GetAwaiter().GetResult();
src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpProxyExecutionManager.cs:391
- ExitAsync is invoked with the shared cancellation token. If a run is canceled, this can prevent the shutdown request from being sent/awaited and may leave the MTP process alive. Use a non-cancelable token (and optionally swallow failures) for shutdown/cleanup paths.
CollectAttachments(runResult, attachments);
client.ExitAsync(_cancellationTokenSource.Token).GetAwaiter().GetResult();
return client.ProcessId;
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
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
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
…ount 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
Replaces the interim local-feed pack with a build of microsoft/testfx#10297, which stacks the Content-Length byte/char fix onto microsoft#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 microsoft#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
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
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
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
Harden the MTP client: UTF-8 framing, shutdown, uid filter, and tests
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 39 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- src/Microsoft.TestPlatform.CrossPlatEngine/Resources/Resources.Designer.cs: Generated file
Comments suppressed due to low confidence (1)
test/TestAssets/MtpPureProject/PureTestFramework.cs:20
- The summary says this asset “mirrors the MSTest asset”, but MtpMSTestProject currently has 6 test methods (it also includes RunSettingsEnvironmentVariableIsInjected). This comment is now misleading; consider rewording to avoid claiming exact parity while keeping the expected counts for this asset correct.
…ent-integration # Conflicts: # src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpServerConnection.cs # test/Microsoft.TestPlatform.Acceptance.IntegrationTests/MtpUnderVstestTests.cs
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. 🤖
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. 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- src/Microsoft.TestPlatform.CrossPlatEngine/Resources/Resources.Designer.cs: Generated file
Suppressed comments (3)
src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpTestNodeConverter.cs:220
- TryGetRawInt’s float coercion truncates any in-range float to int. For non-integer floats (or values that are only approximately integral), this can silently produce a wrong but believable line number; add an integer-ness check before casting.
case float f
// (float)int.MaxValue rounds up to 2147483648f, so comparing a float against
// int.MaxValue directly lets that value through and the cast then saturates. Widen to
// double first so the bound is exact.
when (double)f is >= int.MinValue and <= int.MaxValue:
result = (int)f;
return true;
src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpTestNodeConverter.cs:224
- TryGetRawInt truncates any in-range decimal to int. If the server ever sends a non-integer decimal, this would silently produce an incorrect line number; reject non-whole decimals instead.
case decimal m when m is >= int.MinValue and <= int.MaxValue:
result = (int)m;
return true;
src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpTestNodeConverter.cs:212
- TryGetRawInt currently accepts any in-range double and truncates it to int. If the server ever sends a non-integer (e.g., 42.5), this would silently turn into a plausible but incorrect line number; it should reject non-whole values instead.
This issue also appears in the following locations of the same file:
- line 214
- line 222
case double d when d is >= int.MinValue and <= int.MaxValue:
result = (int)d;
return true;
Jakub Jareš (nohwnd)
changed the title
Jakub Jareš (nohwnd)
marked this pull request as ready for review
August 10, 2026 19:36
Jakub Jareš (nohwnd)
enabled auto-merge (squash)
August 10, 2026 19:36
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- src/Microsoft.TestPlatform.CrossPlatEngine/Resources/Resources.Designer.cs: Generated file
Suppressed comments (2)
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/MtpUnderVstestTests.cs:171
- The method-level comment above this test still says the mixed-run TRX contains "all eight tests", but the asserted totals are now 5 passed + 2 failed + 2 skipped = 9 tests. Updating that comment will avoid confusion when diagnosing failures.
ValidateSummaryStatus(5, 2, 2);
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/MtpUnderVstestTests.cs:147
- The mixed-run summary comment still references the old MTP counts (3/1/1), but MtpMSTestProject now has 4 passed, 1 failed, 1 skipped (see updated ValidateSummaryStatus below). Please update the comment so it matches the asserted totals.
This issue also appears on line 171 of the same file.
// Classic 1/1/1 + MTP 3/1/1 aggregated into one run summary.
Azat Mukhametshin (azat-msft)
approved these changes
Aug 14, 2026
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.
What this does
vstest hand-maintained an MTP (Microsoft.Testing.Platform) server-mode JSON-RPC client under
src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/. testfx now ships that client as a source-onlypackage,
Microsoft.Testing.Platform.ServerMode.Client.Sources, built from the MTP server's own protocoland serialization source, so the wire format cannot drift from the server.
This deletes vstest's transport core and retargets the glue onto the package's
IMtpServerClient:MtpServerConnection,MtpJson,MtpConstants,MtpClientHelpers.MtpProxyDiscoveryManager,MtpProxyExecutionManager,MtpTestNodeConverter— launch viaMtpServerClient.Launch, driveInitializeAsync/DiscoverTestsAsync/RunTestsAsync/ExitAsync, readnode updates from the
TestNodesUpdatedevent with typedMtpTestNodeUpdateaccessors, and bridgeEqtTracethroughDelegateMtpClientLogger.MtpClientOptionsFactory— buildsMtpServerClientOptionsand maps server log levels.MtpServerClientFactory— a testability seam so the proxies can be unit-tested against a fakeclient.
MtpProxyManagerFactory,MtpDataCollectionForwarder(transport-agnostic).The package is a compile-time source dependency (
PrivateAssets="all"): it compiles intoMicrosoft.TestPlatform.CrossPlatEngine.dllwith no shipped DLL, no runtime dependency, and no newpublic API. On net462/netstandard2.0 the package compiles its own nullable-annotation polyfills; vstest
already imports the identical set from CoreUtilities, so
MTP_CLIENT_EXCLUDE_NULLABLE_ATTRIBUTESisdefined on CrossPlatEngine to defer to those and avoid a CS0436 clash (no-op on net8.0).
Package dependency
The package ships from microsoft/testfx#10085 and is restored from the public dnceng
dotnet-toolsfeed(already in
NuGet.config). Pinned to2.4.0-preview.26410.1viaMicrosoftTestingPlatformServerModeClientSourcesVersionineng/Versions.props. No local feed and noNuGet.configchange are needed — the earlier interimeng/local-mtp-feedis removed.Local validation
Build: CrossPlatEngine compiles clean on all three TFMs (net462, netstandard2.0, net8.0) under
TreatWarningsAsErrorsand the public-API analyzers, and adds no public API.CrossPlatEngine MTP unit tests: 71/71 on both target frameworks (net481 and net11.0) — the converter,
options factory, both proxies, and the client factory seam, exercised against a fake client.
MtpUnderVstestTestsacceptance (9 methods on each of the net481 and net11.0 console runners, 18 total):output, run-settings environment-variable injection, an out-of-proc data collector, blame, a mixed
classic+MTP run, a single-TRX run, and a non-ASCII test-name round-trip. This is the end-to-end proof
that the retarget drives the real System.Text.Json client correctly.
/logger:trxtests, which fail to load the TRX logger in my locally-built net462 runner at loggerargument-processing, before the MTP client runs. This is a pre-existing local-deployment issue, not the
retarget: a classic (non-MTP)
/logger:trxtest fails identically on this runner, the same test methodspass on the .NET runner, and the source package is compile-only so it adds no runtime assembly. It does
not reproduce on a clean checkout.
This branch reconciles parallel hardening from Azat Mukhametshin (@azat-msft) and Jakub Jareš (@nohwnd) on the same feature (the client
factory seam, the six MTP unit-test files, and the non-ASCII framing guard) and merges the latest
main,including the change that disables the MTP testhost by default. The non-ASCII acceptance test is aligned
with the other MTP tests to opt back into the MTP testhost.
Resolved questions (confirmed by the package owner against server + client source)
net462/netstandard2.0, System.Text.Json on .NET) materialize the graph the same way: every JSON object
becomes
IDictionary<string, object?>, every arrayICollection<object>, strings/bools/null as-is.Numbers are boxed per formatter, so numerics must be coerced, never hard-cast. vstest reads
standardOutput,standardError,location.file,location.line-start,traits, and thevstest.*bridge properties from the raw
Nodedictionary by string key;MtpTestNodeConvertercoerces thenumeric ones (
TryGetRawInthandles int/long/double/float/decimal).DiscoverTestsAsync/RunTestsAsynccompletes, everyTestNodesUpdatedhandler has already run, which lets vstest drop its old completion-sentinel drain.node.Uidalone when building the run filter andnever reads
DisplayName, so sending the uid as the identity is correct.MtpRunResult.Artifactsis the complete set (absolutefile path, producer uid, type, display name, description), mapped to
AttachmentSet. TheAttachmentsReceivedevent is not additive on this request/response path, so there is no doublecounting. Validated by the passing data-collector and blame tests on the net462 axis.
MtpServerClientOptions.EnvironmentVariablesis keyed ordinal. This PR keys both of vstest'sown environment-variable dictionaries the same way vstest's classic path does (case-insensitive on
Windows) so case-variant keys collapse before the values reach the ordinal package dictionary.
🤖