Retarget the MTP client onto Microsoft.Testing.Platform.ServerClient.Source - #16300
Draft
nohwnd wants to merge 15 commits into
Draft
Retarget the MTP client onto Microsoft.Testing.Platform.ServerClient.Source#16300nohwnd wants to merge 15 commits into
nohwnd wants to merge 15 commits into
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.
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.ServerClient.Source, built from the MTP server's own protocol andserialization 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.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.
Do not merge — this is a draft
The package is not on any public or dnceng feed yet. It is introduced by microsoft/testfx#10085, which
is an unmerged draft. This PR references an interim local feed committed under
eng/local-mtp-feed(
NuGet.config,eng/Versions.props, and thePackageReferenceeach carry an INTERIM comment). CIcannot restore the package from a public feed, so the build cannot go green here. Merge is blocked on
testfx publishing the package. Remove
eng/local-mtp-feedand revert theNuGet.configsource once itships.
Build status against the current package drop
The current drop (
2.4.0-dev.numberfix, uniquely named to avoid NuGet same-version cachecollisions while it is served from the committed local feed) builds CrossPlatEngine clean on all three
TFMs (net462, netstandard2.0, net8.0) with zero warnings under
TreatWarningsAsErrorsand the public-APIanalyzers, and adds no public API. Earlier package build blockers (public types leaking as public API,
missing per-file usings, missing down-level polyfills) are resolved in this drop.
Local validation
MtpUnderVstestTestsruns on both console axes (7 methods each), 12/14 pass:current package drop, every test on this axis passes — discovery, execution, per-test standard output,
run-settings environment-variable injection, an out-of-proc data collector, blame, a mixed classic+MTP
run, and a single-TRX run. This axis was 0/7 before the fix (see "Resolved" below), so it is the
end-to-end proof that the retarget drives the real System.Text.Json client correctly.
except 2
/logger:trxtests. Those 2 fail to load the TRX logger in my locally-built net462 runner,at logger argument-processing, before the MTP client runs — a pre-existing local-deployment issue, not
the retarget. Proof: the same two test methods pass on the .NET runner, and a classic (non-MTP) trx test
fails identically on this runner. The source package is compile-only and adds no runtime assembly, so it
cannot be the cause.
A test-infrastructure fix is included: mixing the interim local-folder feed with the remote https feeds
in the acceptance-test
dotnet restoremade NuGet mis-root the relative local path and mis-normalize thehttps URLs (NU1301).
GetNugetSourceParametersnow resolves local sources to absolute paths and ordersremote feeds first. This is only needed while the package lives on the interim local feed.
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 (int/long/double), so numerics must be coerced, never hard-cast. vstest
reads
standardOutput,standardError,location.file,location.line-start,traits, and thevstest.*bridge properties from the rawNodedictionary by string key;MtpTestNodeConvertercoercesthe numeric ones (
TryGetRawInthandles int/long/double/float/decimal), so it is robust to theper-formatter boxing.
IMtpServerClient. OnceDiscoverTestsAsync/RunTestsAsynccompletes, everyTestNodesUpdatedhandler has already run. This iswhat lets vstest drop its old completion-sentinel drain.
node.Uidalone when building the runfilter and never reads
DisplayName, so sending the uid as the identity is correct.MtpRunResult.Artifactsis the complete, authoritativeset (absolute file path, producer uid, type, display name, description), mapped to
AttachmentSet. TheAttachmentsReceivedevent is not additive on this request/response path (it fires only on the separatepush-only protocol), so there is no double counting. Validated by the passing data-collector and blame
tests on the net462 axis.
MtpServerClientOptions.EnvironmentVariablesis keyed ordinal. vstest's classic path foldsenvironment-variable keys case-insensitively on Windows. This PR keys both of vstest's own
environment-variable collection dictionaries the same way (commit
5ceda8598) so case-variant keyscollapse before the values reach the ordinal package dictionary, preserving the classic Windows behavior.
Resolved: the package's STJ number decode
The .NET axis was previously red because the package's compiled-in System.Text.Json deserializer called
JsonElement.GetInt32()on every JSON number, which threwFormatExceptionon the non-Int32 numbers(durations, timestamps) in real test-node notifications. The Jsonite path (net462) decodes numbers
generically, which is why only the .NET axis was affected. Original runtime stack:
The package owner fixed it upstream (testfx#10085): the two untyped number sinks now decode generically
(
ReadNumber:TryGetInt32→TryGetInt64→TryGetUInt64→double), a pure superset — anything thatfits Int32 still boxes as
int, so nothing that worked before regresses. This PR consumes that fixeddrop, and the .NET axis went from 0/7 to 7/7 with no vstest-side change. It stays draft only until the
package reaches a restorable feed.
🤖