Add a source-only MTP server-mode client package - #10085
Draft
nohwnd wants to merge 26 commits into
Draft
Conversation
MTP ships only the server side of its server-mode JSON-RPC protocol today, so every consumer that drives an MTP app has to write its own client. There are three of them: vstest's minimal Jsonite one, VSUnitTesting's mature StreamJsonRpc one, and C# Dev Kit's copy of that. The plan is to own a single client here in testfx and ship it as a source-only package so all three consume the same code. This is the first step - the client and its tests, building and green in-repo. Source-only contentFiles packaging comes later. The client reuses the server's own serialization instead of taking a dependency, so the wire format cannot drift: Jsonite on net462/netstandard, in-box System.Text.Json on .NET. Both are dependency-free and AOT-safe. The net8 leg needed two fixes in the shared STJ decoder, because the server only ever decoded client-to-server requests and never exercised the receive path a client needs: - Register an object[] deserializer. The IDictionary deserializer already binds object[] for array values, but nothing registered it, so any server-to-client message carrying an array (attachments, node changes) killed the read loop. - Keep raw params as an IDictionary for methods the server does not know. The RpcMessage params switch only knew the five server request methods, so client-received notifications dropped their params. Both are behavior-preserving for the server - its serialization tests stay 56/56. Tests run on both formatter paths, net8 (STJ) and net462 (Jsonite), 21/21 each. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Drives a real generated MTP app through the source-only client's MtpServerClient.Launch: initialize, discover, then run in two separate launches, asserting the single action node comes back as discovered and then passed. Runs the net462/net8.0/net10.0 child assets from the net11 host, so the net462 (Jsonite) server talking to the net8 (System.Text.Json) client exercises both formatter paths over the real transport. Also makes the client process launch cross-platform (apphost resolution on Windows/Linux/macOS) and exposes the internals to the acceptance project via an aliased project reference.
Convert Microsoft.Testing.Platform.ServerClient into the source-only package Microsoft.Testing.Platform.ServerClient.Source. It ships the client plus the linked server protocol and serialization source as contentFiles/cs/<tfm>/** (BuildAction=Compile), so consumers compile it as internal types into their own assembly with no shipped DLL and no runtime dependency. The pack target projects the final @(Compile) set into contentFiles, so packed == compiled by construction, and the per-TFM System.Text.Json removal keeps netstandard2.0 Jsonite-only (net462 / netstandard consumers never see the STJ path). Add MtpServerClientSourcePackageTests, the anti-drift contract test: it inspects the produced nupkg and asserts no compiled output, packed == compiled both ways, netstandard2.0 Jsonite-only with net as a superset, the client API present in every target framework, and no polyfill or generated-source leak. Name the readme PACKAGE.md so the shared Directory.Build.targets picks it up. 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a source-only MTP server-mode client package that reuses the platform’s protocol and serialization code.
Changes:
- Adds client transport, process-launching, API, and packaging infrastructure.
- Extends shared JSON-RPC deserialization for client notifications.
- Adds unit, package-contract, and end-to-end acceptance tests.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
TestFx.slnx |
Registers the new projects. |
test/UnitTests/.../TestSetup.cs |
Registers client serializers for tests. |
test/UnitTests/.../Program.cs |
Configures the test executable. |
test/UnitTests/.../MtpServerClientTests.cs |
Tests client protocol behavior. |
test/UnitTests/.../Microsoft.Testing.Platform.ServerClient.UnitTests.csproj |
Configures multi-TFM unit tests. |
test/UnitTests/.../FakeMtpServer.cs |
Implements the loopback fake server. |
test/UnitTests/.../BannedSymbols.txt |
Enforces MSTest assertions. |
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientAcceptanceTests.cs |
Exercises real MTP applications. |
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MSTest.Acceptance.IntegrationTests.csproj |
References the client project. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MtpServerClientSourcePackageTests.cs |
Validates package contents. |
src/Platform/Microsoft.Testing.Platform/.../Json.Deserializers.cs |
Adds generic arrays and notification parameters. |
src/Platform/Microsoft.Testing.Platform/.../FormatterUtilities.cs |
Selects Jsonite outside .NETCoreApp. |
src/Platform/Microsoft.Testing.Platform.ServerClient/PlatformResourcesShim.cs |
Supplies minimal resource strings. |
src/Platform/Microsoft.Testing.Platform.ServerClient/PACKAGE.md |
Documents package usage. |
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj |
Defines linked sources and source-only packing. |
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/SerializerUtilities.ClientSerializers.cs |
Adds client serialization directions. |
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerProcess.cs |
Launches and manages MTP processes. |
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClientOptions.cs |
Defines client configuration. |
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClientExceptions.cs |
Defines client exceptions. |
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClient.cs |
Implements the high-level client. |
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpJsonRpcConnection.cs |
Implements JSON-RPC correlation and dispatch. |
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/IMtpServerClient.cs |
Defines the client API and models. |
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/IMtpClientLogger.cs |
Defines client diagnostics abstractions. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
main added an ILogger (defaulting to NopLogger) to TcpMessageHandler for low-noise transport diagnostics. The source client links that file, so a clean build now needs ILogger, NopLogger, and the LoggingExtensions that define LogDebugAsync. A stale obj hid this locally; the clean CI build failed with CS0246. Link the three logging files. Client unit tests stay green on net8 (STJ) 21/21 and net462 (Jsonite) 21/21, and the source-package contract test passes 5/5. 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 21 comments.
Comments suppressed due to low confidence (7)
TestFx.slnx:61
- The new platform project and its unit-test project are missing from both
Microsoft.Testing.Platform.slnfandNonWindowsTests.slnf. Those filters explicitly enumerate the other MTP projects/tests, so product-scoped and non-Windows builds will not compile or test this package. Add both entries to both filters.
<Project Path="src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj" />
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:182
- Excluding generated global usings makes the packed sources depend on undocumented consumer imports. For example,
MtpServerProcess.csusesProcess,StringBuilder, andRuntimeInformationwithout imports because this repo supplies them fromDirectory.Build.props:143,147,149; SDK implicit usings do not include all of these. An external consumer will fail to compile the content files unless it happens to define the same globals. Ship a package-owned imports source or add explicit imports, and validate the actual nupkg in a consumer with implicit usings disabled.
- Anything generated into the intermediate obj dir (GlobalUsings.g.cs, *.AssemblyInfo.cs,
*.AssemblyAttributes.cs, …): each consumer generates its own.
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:126
- Compiling these linked files as source does not make their declarations internal. This glob ships many public platform types (
TestNodeatMessages/TestNode.cs:9, state properties atTestNodeStateProperties.cs:9,56, and others) into every consumer assembly, contradicting the package contract and potentially triggering API-baseline failures or type-conflict warnings in consumers that reference MTP. Use an internalized client model/conditional accessibility rather than packing the public server model verbatim.
<!-- Data model (TestNode + property model). Glob Messages\ then remove the server-only message bus. -->
<ItemGroup>
<Compile Include="$(MTPDir)Messages\*.cs" Link="Linked\Messages\%(Filename)%(Extension)" />
src/Platform/Microsoft.Testing.Platform.ServerClient/PACKAGE.md:28
- The shipped source requires newer syntax than C# 9: it uses file-scoped namespaces (C# 10), primary constructors such as
PendingRequest(string method), and collection expressions such as?? [](C# 12). Either rewrite the package sources to the promised language level or state the actual C# 12 requirement.
- C# language version 9 or later.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/SerializerUtilities.ClientSerializers.cs:35
- This idempotence check is not thread-safe, and the flag is set before the dictionaries are fully populated. Two concurrent
Launchcalls can let one thread observetrueand create a System.Text.Json formatter from a partially registered serializer set; the dictionaries are also being read while mutated. Serialize the whole registration operation with a lock/one-time initialization and publish completion only after all entries are installed.
if (s_clientSerializersRegistered)
{
return;
}
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Deserializers.cs:130
- Preserving notification params routes test-node payloads through the raw
IDictionarydecoder, whose number branch usesGetInt32(). The server serializestime.duration-msas adouble(Json.TestNodeSerializer.cs:170), so a normal fractional duration throws while decoding and fails the client's read loop. Decode generic JSON numbers as int/long/double (matching Jsonite) and add a fractional-duration notification test.
_ => value.ValueKind == JsonValueKind.Object
? json.Bind<IDictionary<string, object?>>(value)
: null,
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MSTest.Acceptance.IntegrationTests.csproj:51
- This acceptance test references the validation assembly, not
Microsoft.Testing.Platform.ServerClient.Source, so it never exercises NuGet contentFiles selection or compilation into a consumer. The package-inspection test only checks zip structure; neither test would catch missing consumer imports or source-level type conflicts. Consume the packed package from a generated test project and run that output end to end.
<!-- The MTP server-mode client under test (P4). Referenced through the `serverclient` extern alias so
its internal TestNode/TestNodeUpdateMessage (same namespaces as the platform's public types) stay
out of the global namespace and never clash in the non-aliased files of this assembly. -->
<ProjectReference Include="$(RepoRoot)src\Platform\Microsoft.Testing.Platform.ServerClient\Microsoft.Testing.Platform.ServerClient.csproj" Aliases="serverclient" />
This comment has been minimized.
This comment has been minimized.
The ServerClient unit test app only registered AddMSTest, so it did not know the --crashdump / --hangdump / --report-trx / --report-ctrf / --report-junit / --report-azdo / --coverage options that test/Directory.Build.targets appends when CI runs every unit test module through 'dotnet test --test-modules'. The module rejected the unknown --hangdump option and exited 5, which the orchestrator reports as 'zero tests ran' and fails the whole leg. Direct console runs never passed --hangdump, so it only reproduced in the full CI run. Register the same provider set every other testfx unit test app registers (CrashDump, HangDump, Trx, JUnit, AzureDevOps, Ctrf, CodeCoverage, OpenTelemetry) so the module accepts those options and runs its 21 tests. Verified by running the built exe directly with the CI options on net8.0 and net462: both exit 0.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (8)
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:33
- This constant is only applied while this project builds; a
contentFilespackage does not propagateDefineConstantsto consumers. The packedObjectPool.cstherefore takes its#elsenamespace (Analyzer.Utilities.PooledObjects), while the packed .NET JSON engine referencesMicrosoft.Testing.Platform.Helpers.ObjectPool, so a net8 consumer cannot compile the package. Propagate the constant through packaged build assets or remove the conditional dependency, and validate by compiling a package consumer.
<DefineConstants>$(DefineConstants);IS_CORE_MTP</DefineConstants>
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:182
- The packed sources rely on testfx's generated global usings, but those are deliberately omitted. For example,
MtpJsonRpcConnection.csusesConcurrentDictionarywithout importingSystem.Collections.Concurrent, andMtpServerProcess.csrelies onProcess,StringBuilder, and runtime interop imports. Consumer-generated implicit usings do not include all of these, so otherwise valid consumers fail to compile. Add explicit/package-owned usings and compile an actual project from the nupkg.
- Anything generated into the intermediate obj dir (GlobalUsings.g.cs, *.AssemblyInfo.cs,
*.AssemblyAttributes.cs, …): each consumer generates its own.
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:126
- This glob ships the platform model with its original public accessibility: for example,
Messages/TestNode.cs:9declarespublic class TestNode, and the linked logging files expose publicILogger/LogLevel. That contradicts the PR/package contract that injected types are internal and can leak duplicate MTP public APIs (and conflict warnings) into consumer assemblies. Internalize/curate the linked contract or explicitly revise the package design and documentation.
<!-- Data model (TestNode + property model). Glob Messages\ then remove the server-only message bus. -->
<ItemGroup>
<Compile Include="$(MTPDir)Messages\*.cs" Link="Linked\Messages\%(Filename)%(Extension)" />
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Deserializers.cs:130
- The new raw-property-bag path cannot decode all valid server numbers: the generic dictionary/array deserializers call
JsonElement.GetInt32(), but real test nodes serializeTimingProperty.GlobalTiming.Duration.TotalMillisecondsas adouble. A fractional duration throws while decodingtesting/testUpdates/tests, causing the client read loop and pending run to fail. Preserveint/long/doublevalues as appropriate and cover a non-integral duration.
_ => value.ValueKind == JsonValueKind.Object
? json.Bind<IDictionary<string, object?>>(value)
: null,
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/SerializerUtilities.ClientSerializers.cs:37
- Registration is not thread-safe, and the flag is published before the serializer dictionaries are populated. Two concurrent first calls (for example parallel
Launchcalls in a consumer) can either mutateDictionaryconcurrently or let one formatter snapshot a partially registered set. Serialize the entire registration and set the completed flag only after all entries are installed.
if (s_clientSerializersRegistered)
{
return;
}
s_clientSerializersRegistered = true;
TestFx.slnx:61
- The new platform product and unit-test projects are only added to
TestFx.slnx; both are absent fromMicrosoft.Testing.Platform.slnfandNonWindowsTests.slnf. Product-scoped and non-Windows builds will therefore skip building/packing the client and running its tests. Add both project paths to both filters, following the existing platform project convention.
<Project Path="src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj" />
src/Platform/Microsoft.Testing.Platform.ServerClient/PACKAGE.md:31
- The shipped sources require C# 12 (they use primary constructors and collection expressions), not C# 9. The linked
requiredmembers also needRequiredMemberAttributeandCompilerFeatureRequiredAttributepolyfills on older targets. Update the consumer requirements so following this documentation produces a compilable project.
- C# language version 9 or later.
- On `net462` / `netstandard2.0`: the usual polyfills (nullable attributes, `IsExternalInit`,
index/range, `System.HashCode`, `ValueTask`) and framework references (`System.Memory`,
`System.Threading.Tasks.Extensions`). This package intentionally does **not** ship polyfills, to
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MSTest.Acceptance.IntegrationTests.csproj:51
- This acceptance path consumes the validation DLL via
ProjectReference, not the source-only nupkg, so it inherits testfx's constants/global usings and never verifies thatcontentFilescompile in a consumer. The archive-inspection tests cannot catch consumer compilation failures. Generate a small client asset with aPackageReferenceto the packed Shipping package and drive the server through that compiled asset.
<!-- The MTP server-mode client under test (P4). Referenced through the `serverclient` extern alias so
its internal TestNode/TestNodeUpdateMessage (same namespaces as the platform's public types) stay
out of the global namespace and never clash in the non-aliased files of this assembly. -->
<ProjectReference Include="$(RepoRoot)src\Platform\Microsoft.Testing.Platform.ServerClient\Microsoft.Testing.Platform.ServerClient.csproj" Aliases="serverclient" />
On non-Windows, eng/Build.props builds and packs NonWindowsTests.slnf, not the full TestFx.slnx. The source-only package project was missing from that filter, so on Linux/macOS it only built transitively (as a dependency of the acceptance tests) and never packed. The acceptance tests then failed with 'Could not find Microsoft.Testing.Platform.ServerClient.Source.*.nupkg'. Add the package project and its unit tests to the filter. The unit tests already restrict net462 to Windows, so on non-Windows they build and run the net8.0 (System.Text.Json) path only. 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (22)
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:193
- The packed source is not self-contained. Files such as
MtpJsonRpcConnection.csandMtpServerProcess.csuseConcurrentDictionary,Process,StringBuilder,RuntimeInformation, and other types without file-level imports; they compile here only becauseDirectory.Build.propsgenerates repository-wide global usings. This target deliberately excludes generated sources, so a normal external consumer will receive none of those imports and fail compilation. Please add explicit/shipped imports and validate the nupkg in a clean consumer project.
<_MtpClientPackSource Include="@(Compile)"
Condition="'%(Compile.MtpClientDoNotPack)' != 'true' and
!$([System.String]::new('%(Compile.FullPath)').StartsWith('$(_MtpClientIntermediateFullPath)', System.StringComparison.OrdinalIgnoreCase))" />
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:126
- This glob ships the platform message declarations with their original accessibility. For example,
Messages/TestNode.cs:9andTestNodeUpdateMessage.cs:14arepublic, so NuGet does not compile the injected source “as internal”; it adds duplicate public MTP types to every consumer and can shadow types from Microsoft.Testing.Platform. Please make the source-package copies internal (or avoid shipping duplicate model declarations) before publishing.
<!-- Data model (TestNode + property model). Glob Messages\ then remove the server-only message bus. -->
<ItemGroup>
<Compile Include="$(MTPDir)Messages\*.cs" Link="Linked\Messages\%(Filename)%(Extension)" />
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/SerializerUtilities.ClientSerializers.cs:37
- Registration is not thread-safe, and the flag is published before the dictionaries are fully populated. Two concurrent
Launchcalls can let one thread create a formatter from a partial serializer snapshot while the other mutates the sharedDictionaryinstances. Serialize initialization under a lock and set the completed flag only after every registration has finished.
if (s_clientSerializersRegistered)
{
return;
}
s_clientSerializersRegistered = true;
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Deserializers.cs:130
- Preserving unknown notification params now routes telemetry and test-node property bags through the generic decoder, but that decoder uses
GetInt32()for every JSON number (including the new array path). The server serializer explicitly emitslong,float,double, anddecimal; a duration or non-integral telemetry metric therefore throws and terminates the client's read loop. Decode the supported numeric shapes without narrowing, and cover a double/long notification.
_ => value.ValueKind == JsonValueKind.Object
? json.Bind<IDictionary<string, object?>>(value)
: null,
src/Platform/Microsoft.Testing.Platform.ServerClient/PACKAGE.md:28
- The shipped client already uses C# 12 syntax, including primary constructors (
DelegateMtpClientLoggerandPendingRequest) and collection expressions. A consumer compiling with C# 9 cannot parse the package sources, so this requirement is incorrect.
- C# language version 9 or later.
TestFx.slnx:61
- The new platform product and its unit tests are added to the full and non-Windows solutions, but both are absent from
Microsoft.Testing.Platform.slnf(currently lines 8-35). Product-scoped platform builds therefore skip this package and its tests. Add both project paths to that filter as well.
<Project Path="src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj" />
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MSTest.Acceptance.IntegrationTests.csproj:51
- This
ProjectReferencemakes the end-to-end test run against the built DLL under testfx's global usings, polyfills, andIS_CORE_MTP; it never restores or compilesMicrosoft.Testing.Platform.ServerClient.Source. Consequently the test namedViaSourcePackageClientcannot catch source-package consumer failures. Build a clean generated asset with aPackageReferenceto the packed nupkg and drive that client instead.
<!-- The MTP server-mode client under test (P4). Referenced through the `serverclient` extern alias so
its internal TestNode/TestNodeUpdateMessage (same namespaces as the platform's public types) stay
out of the global namespace and never clash in the non-aliased files of this assembly. -->
<ProjectReference Include="$(RepoRoot)src\Platform\Microsoft.Testing.Platform.ServerClient\Microsoft.Testing.Platform.ServerClient.csproj" Aliases="serverclient" />
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/TestSetup.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/Program.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/MtpServerClientTests.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/FakeMtpServer.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientAcceptanceTests.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MtpServerClientSourcePackageTests.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/PlatformResourcesShim.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/SerializerUtilities.ClientSerializers.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerProcess.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClientOptions.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClientExceptions.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClient.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpJsonRpcConnection.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/IMtpServerClient.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/IMtpClientLogger.cs:1
- This new C# file is saved without the required UTF-8 BOM.
.editorconfig:65-68setscharset = utf-8-bomfor all C# files; please resave it with the configured encoding.
// Copyright (c) Microsoft Corporation. All rights reserved.
Evangelink
reviewed
Jul 21, 2026
| compiles from ..\Microsoft.Testing.Platform\, guaranteeing wire compatibility by construction | ||
| (single source of truth). On top of that it adds the client-only transport + API. | ||
|
|
||
| PACKAGING: this ships as a source-only NuGet package (Microsoft.Testing.Platform.ServerClient.Source). |
Member
There was a problem hiding this comment.
Could we name this Microsoft.Testing.Platform.ServerMode.Client.Sources and align the project/assembly name as well? ServerClient is ambiguous, while the namespace and documented capability already call this the server-mode client. I would not use the shorter Microsoft.Testing.Platform.Client.Sources yet: with multiple MTP protocols, that name implies a protocol-agnostic client and would make a future second client awkward. ServerMode identifies the supported protocol without unnecessarily making JSON-RPC or TCP part of the package contract. The plural .Sources also follows the more common Microsoft source-package convention.
Evangelink
reviewed
Jul 21, 2026
|
|
||
| _connection.NotificationReceived += OnNotificationReceived; | ||
| _connection.ServerRequestHandler = OnServerRequestAsync; | ||
| _connection.Start(); |
Member
There was a problem hiding this comment.
Starting the read loop in the constructor creates a subscription gap: Launch returns only after this line, so callers cannot attach TestNodesUpdated, LogReceived, TelemetryReceived, or AttachmentsReceived before messages may arrive. Request-driven test updates are normally safe because callers subscribe before calling discover/run, but unsolicited startup log or telemetry notifications can be silently lost. Could construction and startup be separated (or notifications buffered until initialization/subscription) so consumers can wire handlers before reading begins?
Member
Author
There was a problem hiding this comment.
Correct observation, and it is scoped to unsolicited notifications. The constructor wires the client's own handlers before _connection.Start(), so the client never misses a connection notification — the window is only between the client raising its public event and the consumer subscribing to it.
For the request-driven paths (discover / run and their node-update / log / attachment notifications) that is safe: the consumer subscribes to the events, then issues the request that causes the server to emit them, so they cannot arrive before the subscription. This is the ordering guarantee documented on IMtpServerClient.
The only genuinely racy case is a notification the server emits unsolicited right after connect, before any request (startup telemetry, for example). MTP does not stream that ahead of initialize/discover/run today, so it is not a v1 blocker for the core scope. Deferring _connection.Start() out of the constructor into an explicit start step the consumer calls after wiring events is a reasonable follow-up for the opt-in telemetry/debugger scenarios; leaving this open as a design decision.
🤖
The source-only ServerClient package embeds the server's Jsonite under a top-level `namespace Jsonite`. vstest already has its own internal top-level `namespace Jsonite`, so on net462/netstandard2.0 both copies compile into CrossPlatEngine and collide (CS0436), failing vstest's warnings-as-errors build. Move it under `Microsoft.Testing.Platform.ServerMode.JsonRpc.Json.Jsonite` (matches the folder). Pure namespace move, no wire-format or behavior change: the formatter Id stays "Jsonite" and the JSON output is identical. Server and client compile from the same files, so the rename is unconditional. Validated: platform + client unit tests (net462 Jsonite + net8 STJ 21/21 each, platform 1371/1393), the packed==compiled contract test (5/5), and the real-app acceptance test (3/3) all green. 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (20)
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:33
DefineConstantsonly affects this validation project; it is not propagated withcontentFiles. A package consumer therefore compilesObjectPool.cswithoutIS_CORE_MTP, placingObjectPool<T>inAnalyzer.Utilities.PooledObjects(Helpers/ObjectPool.cs:21-25), while the packedJson/Json.csimportsMicrosoft.Testing.Platform.Helpersand instantiates that type. The net8 source package will not compile. Propagate the symbol through package build assets or remove the conditional namespace dependency from the shipped source.
<DefineConstants>$(DefineConstants);IS_CORE_MTP</DefineConstants>
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:182
- Skipping generated global usings makes the packed source depend on testfx's
Directory.Build.props, which consumers do not receive. For example,MtpJsonRpcConnection.csusesConcurrentDictionarywithout importingSystem.Collections.Concurrent,MtpServerProcess.csusesProcess/StringBuilderwithout their namespaces, and the non-.NET path relies on the project-onlyPolyfillsusing. The nupkg therefore fails to compile in a normal consumer. Add explicit imports to shipped files (or a compatible packaged imports mechanism).
Skipped:
- Polyfills (MtpClientDoNotPack=true): consumers already provide their own.
- Anything generated into the intermediate obj dir (GlobalUsings.g.cs, *.AssemblyInfo.cs,
*.AssemblyAttributes.cs, …): each consumer generates its own.
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Deserializers.cs:74
- This generic decoder rejects valid server numbers that are not
Int32. In particular, test-node serialization emitsTimingProperty.GlobalTiming.Duration.TotalMillisecondsas adouble(Json.TestNodeSerializer.cs:168-170), so an ordinary timed test update makesGetInt32()throw and terminates the client read loop. The dictionary-number branch above has the same limitation. Decodeint,long, and floating-point JSON numbers in both branches.
case JsonValueKind.Number:
items.Add(element.GetInt32());
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/SerializerUtilities.ClientSerializers.cs:37
- The idempotence guard is not thread-safe. If two clients launch concurrently, one thread can observe
truewhile the first is still mutating the shared serializer dictionaries, then snapshot an incomplete set inCreateFormatter; requests later fail due to missing serializers. Synchronize the entire registration and publish the completed state only after all entries are installed.
if (s_clientSerializersRegistered)
{
return;
}
s_clientSerializersRegistered = true;
src/Platform/Microsoft.Testing.Platform.ServerClient/PACKAGE.md:28
- The shipped source uses C# 12 features, including collection expressions (
[]) and primary constructors, so it cannot compile with the documented C# 9 minimum. Either rewrite the injected source to C# 9 syntax or state the actual minimum.
- C# language version 9 or later.
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/TestSetup.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/Program.cs:1 - This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/MtpServerClientTests.cs:1 - This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/FakeMtpServer.cs:1 - This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/MtpServerClientAcceptanceTests.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MtpServerClientSourcePackageTests.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/PlatformResourcesShim.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/SerializerUtilities.ClientSerializers.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerProcess.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClientOptions.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClientExceptions.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClient.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpJsonRpcConnection.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/IMtpServerClient.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/IMtpClientLogger.cs:1
- This new C# file is UTF-8 without a BOM, but
.editorconfig:66-67requiresutf-8-bomfor all C# files. Resave it with a BOM.
// Copyright (c) Microsoft Corporation. All rights reserved.
…e MTP client MtpTestNodeUpdate now decodes standardOutput, standardError, and the location.file/line-start/line-end wire keys into StandardOutput, StandardError, FilePath, LineStart, and LineEnd, so consumers stop reaching into the raw Node bag for the common fields. Line numbers arrive as JSON numbers, so a small coercion handles whichever numeric type each formatter boxes them as. Also documents the discover/run ordering guarantee: once the returned task completes every TestNodesUpdated handler has already run, so consumers do not need a settle delay or completion sentinel. This replaces the old fixed wait the vstest client used. Tested on both formatter paths (net8 System.Text.Json, net462 Jsonite): unit 22/22 each, contract 5/5, acceptance 3/3. 🤖
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0614f13d-ea43-40b4-b541-9058fdfd87e1
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (6)
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/TcpMessageHandler.cs:187
- Header termination currently depends on having already parsed Content-Length. If a peer sends a blank line before Content-Length (missing header), the loop will continue and start consuming body bytes as header lines, potentially desynchronizing the stream. Consider breaking on an empty line unconditionally, and after the loop returning -1 (graceful disconnect) when Content-Length was never parsed.
if (line is null || (line.Length == 0 && contentSize != -1))
{
break;
}
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/TestSetup.cs:1
- MSTest discovery/execution does not reliably treat a static class as a valid
[TestClass]container for[AssemblyInitialize]. If this initializer is skipped, client tests can become order-dependent (formatters created before registration). MakeTestSetupa non-static class (e.g.,public sealed class TestSetup) while keeping thepublic staticassembly-initialize method.
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/TestSetup.cs:1 - MSTest discovery/execution does not reliably treat a static class as a valid
[TestClass]container for[AssemblyInitialize]. If this initializer is skipped, client tests can become order-dependent (formatters created before registration). MakeTestSetupa non-static class (e.g.,public sealed class TestSetup) while keeping thepublic staticassembly-initialize method.
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpJsonRpcConnection.cs:204 - The comment says the connection ALWAYS answers server-initiated requests, but if
WriteMessageAsyncfails (e.g., un-serializableresultruntime type), the catch only logs and the server remains unanswered. Consider adding a fallback attempt inside the catch to send aResponseMessage(request.Id, null)(or an error response if supported) so the 'never left waiting' guarantee holds even on serialization failures.
// Always answer so the server is never left waiting.
try
{
await WriteMessageAsync(new ResponseMessage(request.Id, result), cancellationToken).ConfigureAwait(false);
}
catch (Exception ex)
{
_logger.SafeLog(MtpClientLogLevel.Warning, $"Failed to respond to server request '{request.Method}': {ex}");
}
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/SerializerUtilities.ClientSerializers.cs:92
- This comment states handlers MUST return
Dictionary<string, object?>, butIMtpServerClient.ServerRequestHandleraccepts anyIDictionary<string, object?>andMtpServerClientnormalizes non-Dictionaryimplementations before responding. Update the comment to reflect the actual supported contract (anyIDictionary<,>is accepted but ultimately serialized asDictionary<,>; only non-dictionary result types are unsupported).
// A server-initiated request (e.g. client/attachDebugger) may be answered with a NON-null result.
// ResponseMessage serialization resolves the serializer by the result's runtime type, so register a
// pass-through for the dictionary shape a handler returns; without it a non-null result would throw
// KeyNotFoundException and hang the response write. Handlers that answer a server request MUST return
// a Dictionary<string, object?> (or null); any other runtime type has no registered serializer.
Serializers[typeof(Dictionary<string, object?>)] = new ObjectSerializer<Dictionary<string, object?>>(result => result);
test/UnitTests/Microsoft.Testing.Platform.ServerClient.UnitTests/FakeMtpServer.cs:256
- This uses
SemaphoreSlim.Wait()(blocking) while the same semaphore is also awaited viaWaitAsyncelsewhere (WriteAsync). Mixing blocking waits with async code can cause thread-pool starvation and can deadlock under certain synchronization contexts. Consider makingSendRawFrameasync and usingawait _writeLock.WaitAsync(), or switching to a dedicated synchronous lock for the raw-frame path.
_writeLock.Wait();
try
{
stream.Write(header, 0, header.Length);
stream.Write(body, 0, body.Length);
stream.Flush();
}
finally
{
_writeLock.Release();
}
- Files reviewed: 34/34 changed files
- Comments generated: 0 new
- Review effort level: Low
MtpServerClientPackagedConsumerRunTests packs the source package, generates a consumer that references it through PackageReference (the way vstest consumes it), builds that consumer so the injected client source compiles, then runs it against a real MTP app and asserts it discovers and executes the expected node. This is the durable in-repo proof that a downstream repo can consume the packed package and actually run it, not just compile it. The two multibyte round-trip unit tests assert that a UID, a display name, and a multi-KB standard output built from multibyte and surrogate-pair characters survive the transport byte-exact, guarding the Content-Length byte/char boundary from the client side. 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 35 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:138
- Removing the STJ files from the
netstandard2.0content group breaks compatible .NET consumers below net8. NuGet selects this group for anet6.0/net7.0project, but those projects defineNETCOREAPP, soFormatterUtilitiescompiles its STJ branch while the referencedJson.*sources are absent. Either ship guarded STJ sources in the fallback group or provide compatible content groups for every supported .NET TFM (and test one such consumer).
<ItemGroup Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) != '.NETCoreApp'">
<Compile Remove="$(MTPDir)ServerMode\JsonRpc\Json\Json.cs" />
<Compile Remove="$(MTPDir)ServerMode\JsonRpc\Json\Json.Deserializers.cs" />
<Compile Remove="$(MTPDir)ServerMode\JsonRpc\Json\Json.Serializers.cs" />
<Compile Remove="$(MTPDir)ServerMode\JsonRpc\Json\Json.TestNodeSerializer.cs" />
src/Platform/Microsoft.Testing.Platform.ServerClient/build/Microsoft.Testing.Platform.ServerClient.Source.targets:37
- Appending CS0436 to
NoWarnsuppresses that compiler diagnostic for the entire consuming project, including unrelated source/reference conflicts in adopter code. Scope the suppression to the injected files instead (for example, prepend#pragma warning disable CS0436in the pack-time transform) so consuming projects retain their diagnostics.
<NoWarn>$(NoWarn);CS0436</NoWarn>
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerProcess.cs:128
- The source package is intended to compile directly in
net462, butProcessStartInfo.Environmentis unavailable on .NET Framework (onlyEnvironmentVariablesexists), so the advertised net462 consumer fails with CS1061 at this line. UseEnvironmentVariableshere (and remove the key for a null value) so the same source compiles across both formatter legs.
foreach (KeyValuePair<string, string?> variable in options.EnvironmentVariables)
{
startInfo.Environment[variable.Key] = variable.Value;
}
The ServerClient source package globs the platform's Messages\ folder and removes the server-only message-bus files by name. ShutdownTimeouts.cs was added to that folder on main (message-bus shutdown timeout config that references IEnvironment), so the glob swept it into the client package and it failed to compile with CS0246 on the CI merge-with-main build. Add it to the deny-list next to the other message-bus files, and broaden the comment so future server-only Messages\ files are excluded the same way. 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 35 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/Platform/Microsoft.Testing.Platform.ServerClient/build/Microsoft.Testing.Platform.ServerClient.Source.targets:37
- Appending
CS0436toNoWarnsuppresses this warning for the consumer's entire project, not just for the injected polyfills. That hides unrelated source/import conflicts and also masks collisions with the duplicated MTP model types that this package compiles. Avoid globally disabling the diagnostic; eliminate or isolate the colliding source types, or provide targeted opt-outs for the specific polyfills.
<NoWarn>$(NoWarn);CS0436</NoWarn>
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:279
- The transform only rewrites column-0 declarations where
publicand the type keyword are on the same line. The linked Jsonite sources instead use block namespaces and split declarations such as#if JSONITE_PUBLIC/public/#else/internal(Json/Jsonite/JsonReader.cs:47-52, with the same pattern in several Jsonite files). If a consumer already definesJSONITE_PUBLIC, the packed package emits public Jsonite types, violating the source package's internal-only API guarantee. Force that branch internal or isolate the symbol during packing.
Regex usingRx = new Regex(@"^\s*using\s+(?:static\s+)?([^;=]+);", RegexOptions.Multiline);
Regex typeRx = new Regex(@"^public(\s+(?:(?:sealed|abstract|static|partial|unsafe|readonly|ref|file|new)\s+)*(?:class|struct|interface|enum|record|delegate)\b)", RegexOptions.Multiline);
src/Platform/Microsoft.Testing.Platform.ServerClient/PACKAGE.md:29
- The stated C# 12 requirement is insufficient for down-level consumers: the package includes
src/Polyfills/OperatingSystem.cs, whose active net462/netstandard2.0 path uses the C# 14 extension-block syntaxextension(OperatingSystem). The consumer compile tests useLangVersion=preview, so they do not catch this mismatch. Either rewrite that shim to C# 12-compatible syntax or document and test the actual C# 14 requirement.
- C# language version 12 or later (the shipped source uses collection expressions and other C# 12
features).
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpJsonRpcConnection.cs:217
- Cancellation can race with response dispatch: the response may already have completed this TCS while the request is still present in
_pendingRequests. In that caseTrySetCanceledreturns false, but the code still sends$/cancelRequestfor an already completed request. Only notify the server when this call actually wins completion.
pending.Completion.TrySetCanceled(cancellationToken);
// Best-effort notify the server to stop the in-flight work.
_ = SendCancelNotificationAsync(id);
Contained fixes on the new client source files (no reconciliation impact, they don't exist on main) plus two minimal edits to the shared deserializer: - MtpJsonRpcConnection: add a closed-reason latch so a send after Close fails fast with the original fault, await the read loop in Dispose with a bounded timeout, document NotificationReceived ordering. - MtpServerClient: fix the AsInt relational pattern, route Initialize/Run results through AsResultDictionary which throws on a non-dictionary result instead of silently returning empty. - MtpServerProcess: restructure Start so everything that can throw runs inside the try with catch teardown, bound SafeKill's WaitForExit. - csproj: drop AllowUnsafeBlocks (no unsafe code). - Source.targets: default LangVersion to latest only when the consumer left it unset or pinned to 7.3, so the injected modern-C# source compiles in a bare consumer without overriding an explicit newer pin. - Json.Deserializers (shared): use indexer assignment (last key wins) for the untyped dictionary so a duplicate key in untrusted input can't crash the read loop, and add ArgumentException to the params catch filter so a nested typed binder failure becomes a coded InvalidParams error. Behavior-preserving for all unique-key input; full platform suite stays green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 519bdc9e-ace7-46fa-aa7e-d07eeef955f6
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
src/Platform/Microsoft.Testing.Platform.ServerClient/build/Microsoft.Testing.Platform.ServerClient.Source.targets:49
- This package-wide
NoWarnchanges diagnostics for all consumer source, not just the injected polyfills, so unrelated CS0436 conflicts in an adopter are silently hidden. Scope the suppression to the generated package files (for example by adding a CS0436 pragma in the pack-time source preamble) instead of mutating the consumer project's global warning policy.
<NoWarn>$(NoWarn);CS0436</NoWarn>
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:137
- The
netstandard2.0content group is NuGet-compatible with .NET 6/7 consumers, but these removals make that selected group Jsonite-only. When injected into such a consumer,NETCOREAPPis defined, soFormatterUtilitiesselects the System.Text.Json implementation whose source files are absent, causing compilation errors. Add compatible .NET content groups for every supported pre-net8 TFM, or explicitly prevent/document those consumers rather than publishing a group NuGet treats as compatible.
<ItemGroup Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) != '.NETCoreApp'">
<Compile Remove="$(MTPDir)ServerMode\JsonRpc\Json\Json.cs" />
<Compile Remove="$(MTPDir)ServerMode\JsonRpc\Json\Json.Deserializers.cs" />
<Compile Remove="$(MTPDir)ServerMode\JsonRpc\Json\Json.Serializers.cs" />
<Compile Remove="$(MTPDir)ServerMode\JsonRpc\Json\Json.TestNodeSerializer.cs" />
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Deserializers.cs:121
- The deserialization filter still omits
FormatException, whichJsonElement.GetInt32()throws when a known request parameter has a numeric value outsideInt32(for example an oversizedinitialize.processId). That malformed client payload therefore still escapes this recovery path and faults the server read loop instead of becomingInvalidRequestParamsArgs. IncludeFormatExceptionin the filter.
catch (Exception ex) when (ex is MessageFormatException or InvalidOperationException or JsonException or ArgumentException)
…LangVersion oracle
The hostile-consumer oracle set <LangVersion>preview</LangVersion> on the generated
consumer, which masked whether the source package is self-sufficient on language
version. Dropping that crutch exposed a real defect: Json.TestNodeSerializer.cs used
the C# preview 'collection expression arguments' feature (with(capacity: 16)), so the
packed source could not compile under the 'latest' that build/*.targets supplies -
it required preview on every SDK.
Rewrite the seed list to a behavior-identical new(capacity: 16) { ... } initializer
(same capacity hint, same elements, same order; C# 9+), wrapped in a scoped
IDE0028 pragma so the analyzer does not rewrite it back to the preview form. The
oracle now compiles the injected source relying solely on the targets-supplied
'latest', proving the package needs no preview language features.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 519bdc9e-ace7-46fa-aa7e-d07eeef955f6
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Platform/Microsoft.Testing.Platform.ServerClient/PACKAGE.md:29
- This understates the compiler requirement. The package ships
Polyfills/OperatingSystem.cs, whose active net462/netstandard2.0 branch uses a C# 14 extension block (extension(OperatingSystem)at line 15). With a C# 12 or 13 compiler, the packaged target setsLangVersion=latestbut the injected source still fails to parse. Either avoid that C# 14 syntax in shipped source or document C# 14 as the minimum.
- C# language version 12 or later (the shipped source uses collection expressions and other C# 12
features).
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpJsonRpcConnection.cs:323
- The self-wait guard is unreliable for this async loop.
Task.Run(Func<Task>)stores an unwrapped proxy task, whileTask.CurrentIdinside an async continuation is not guaranteed to equal that proxy's ID (and is commonly null). If an event or server-request handler callsDispose, this can therefore wait five seconds on the read loop that is currently executing the handler. Track an explicit read-loop/dispatch context or avoid synchronously waiting when disposal originates from a callback.
Task? readLoop = _readLoop;
if (readLoop is not null && Task.CurrentId != readLoop.Id)
{
try
{
readLoop.Wait(ReadLoopShutdownTimeout);
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Deserializers.cs:326
- This fallback does not actually match Jsonite for all valid JSON integers. After
ulong, Jsonite triesdecimal(Jsonite/JsonReader.cs:519-523), whereas this path converts directly todouble; an integer such asdecimal.MaxValueis therefore preserved on the Jsonite TFM but rounded on the System.Text.Json TFM. Untyped telemetry/property-bag values can consequently differ or lose precision. Preserve decimal for integer-form tokens beyondulong, while retainingdoublefor fractional/exponent tokens.
if (element.TryGetUInt64(out ulong ulongValue))
{
return ulongValue;
}
return element.GetDouble();
- AsInt: test double integrality with the constant pattern d % 1d is 0d instead of d == Math.Floor(d), so the code-scanning float-equality rule does not fire (behaviorally identical). - MtpJsonRpcConnection.Dispose: guard the read-loop self-wait with an AsyncLocal<bool> flow marker instead of Task.CurrentId. ReadLoopAsync is async, so after its first await Task.CurrentId no longer matches the loop's task id and a handler-triggered Dispose would self-wait for the full 5s shutdown timeout. Adds a regression test. - MtpServerProcess: cap the retained standard-error buffer at 64 KB with a front-trim so a chatty/long-lived server cannot grow it without bound; the tail (most relevant near a crash) is kept. - PACKAGE.md: correct the C# language-version note (build targets default LangVersion=latest; a pinned version needs C# 14 on net462/netstandard2.0). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 519bdc9e-ace7-46fa-aa7e-d07eeef955f6
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClientOptions.cs:36
- The summary says
falsemakes the client perform one operation and then exit, but the implementation only sends this value during initialization; it never auto-exits after discover/run. The remarks below describe the actual behavior, so the summary should not promise lifecycle behavior the option does not implement.
/// Gets or sets a value indicating whether the client keeps the connection alive for multiple requests
/// (<c>capabilities.testing.isStateful</c> / <c>experimental_multiRequestSupport</c>). When
/// <see langword="false"/> the client performs a single discover or run and then exits. Defaults to
/// <see langword="false"/>.
src/Platform/Microsoft.Testing.Platform.ServerClient/build/Microsoft.Testing.Platform.ServerClient.Source.targets:26
- This condition also matches a consumer that explicitly pins C# 7.3, so the package silently overrides that explicit choice despite the comment saying explicit choices are never overridden. That can change compilation semantics for the consumer's own source. Only supply
latestwhenLangVersionis unset; an explicitly incompatible version should remain intact and fail with a clear compatibility diagnostic.
<LangVersion Condition="'$(LangVersion)' == '' or '$(LangVersion)' == '7.3'">latest</LangVersion>
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/TcpMessageHandler.cs:34
- The PR description states that only
FormatterUtilities.csandJson.Deserializers.cschange on the shared server side, but this hunk rewrites the server transport framing, and the diff also changesIMessageFormatter,Json.cs,Json.TestNodeSerializer.cs, and a shared polyfill. Please update the description and server-side test summary so reviewers and release notes reflect the actual compatibility surface being changed.
// The read side deliberately does NOT use a StreamReader. Content-Length is declared in UTF-8 *bytes*
// (see WriteRequestAsync), so the body must be consumed as bytes and decoded afterwards. A StreamReader
// hands out decoded characters, which for multi-byte UTF-8 content are fewer units than the declared
// length: the reader under-reads the frame, leaves its tail in the stream, and the framing permanently
// desynchronizes from the next frame onwards. Reading the headers through a StreamReader and the body
// from BaseStream would be worse still, because the reader's internal buffer would have already
// swallowed part of the body. Headers and body are therefore both read through this one byte-level
// buffer, so nothing can be buffered on the other side of the boundary.
src/Platform/Microsoft.Testing.Platform.ServerClient/Microsoft.Testing.Platform.ServerClient.csproj:355
- The transform writes these generated files under
objbut never records them in@(FileWrites), so MSBuild'sCleantarget does not know to remove them. Register the transformed outputs after the task, as other generated targets in this repository do (for exampleMicrosoft.Testing.Platform.MSBuild.targets:56).
<!-- Write the transformed copies to obj. -->
<_MtpClientTransformSource Files="@(_MtpClientTransformed)" />
The server-mode IMessageFormatter/MessageFormatter/Json.Deserialize<T> overloads changed from ReadOnlyMemory<char> to ReadOnlyMemory<byte> (the byte/char framing fix). Record that in net/InternalAPI.Unshipped.txt so PublicApiAnalyzers stops reporting the removed char overloads (RS0017) and the new byte overloads (RS0016): *REMOVED* the three char signatures that net/InternalAPI.Shipped.txt still lists, and declare the three byte ones. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 519bdc9e-ace7-46fa-aa7e-d07eeef955f6
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Deserializers.cs:326
ReadNumberdoes not fully mirror Jsonite as documented: Jsonite falls back todecimalfor integral values outsideulongbut withindecimal(JsonReader.cs:519-523), while this fallback converts them todoubleand loses precision. Preserve that integer case before usingGetDouble().
return element.GetDouble();
src/Platform/Microsoft.Testing.Platform.ServerClient/build/Microsoft.Testing.Platform.ServerClient.Source.targets:49
- Appending
CS0436to the consumer project's globalNoWarnsuppresses every source-vs-imported-type conflict in adopter code, not only collisions from this package's polyfills. Scope the suppression to the transformed package source (for example, via a generated#pragma) or exclude only the colliding polyfills so unrelated conflicts remain visible.
<NoWarn>$(NoWarn);CS0436</NoWarn>
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpServerClientOptions.cs:36
- This describes behavior the client does not implement: with the default
false, discover/run return without sendingexit, and callers/tests explicitly callExitAsync. State that this value is only advertised during initialization and that request sequencing and shutdown remain the caller's responsibility.
/// <summary>
/// Gets or sets a value indicating whether the client keeps the connection alive for multiple requests
/// (<c>capabilities.testing.isStateful</c> / <c>experimental_multiRequestSupport</c>). When
/// <see langword="false"/> the client performs a single discover or run and then exits. Defaults to
/// <see langword="false"/>.
src/Platform/Microsoft.Testing.Platform.ServerClient/build/Microsoft.Testing.Platform.ServerClient.Source.targets:26
- This condition cannot distinguish the framework's 7.3 default from a consumer that explicitly pinned C# 7.3, so the package silently overrides an explicit project choice despite the comment and package documentation. Provide the conditional default from a packaged
.propsfile ('$(LangVersion)' == '') so the consumer project can override it, and keep late composition logic in.targets.
<LangVersion Condition="'$(LangVersion)' == '' or '$(LangVersion)' == '7.3'">latest</LangVersion>
src/Platform/Microsoft.Testing.Platform.ServerClient/Client/MtpJsonRpcConnection.cs:263
- Only send
$/cancelRequestwhen cancellation actually wins the completion race. Currently, if the response completes and the token fires before the pending entry is removed,TrySetCanceledfails but a stale cancel notification is still sent for an already-completed request.
pending.Completion.TrySetCanceled(cancellationToken);
// Best-effort notify the server to stop the in-flight work.
_ = SendCancelNotificationAsync(id);
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.
MTP ships only the server side of its server-mode JSON-RPC protocol today, so every consumer that drives an MTP test app writes its own client. There are already three: the minimal Jsonite-based one in vstest, and a richer StreamJsonRpc-based one that lives (duplicated) in VSUnitTesting and in the C# Dev Kit (vs-green). This adds one canonical client, owned here in testfx next to the protocol it talks to, and ships it as a source-only package so the three consumers can drop their own copies.
What's here (the testfx leg)
src/Platform/Microsoft.Testing.Platform.ServerClient, that links the server's own protocol and serialization source (RpcMessages,JsonRpcMethods,SerializerUtilities, the Jsonite parser, and the .NET-only System.Text.Json engine) and adds the client-only transport and API on top (IMtpServerClient,MtpServerClient, the JSON-RPC connection, and the process launcher). Because it reuses the server's encoder, wire compatibility holds by construction.Microsoft.Testing.Platform.ServerClient.Source: the linked and client source ships ascontentFiles/cs/<tfm>/**withBuildAction=Compile, no DLL and no runtime dependency, compiled into each consumer as internal types. The pack target projects the final compiled set intocontentFiles, so packed == compiled by construction, and the per-TFM System.Text.Json removal keeps netstandard2.0 Jsonite-only (net462 / netstandard consumers never see the STJ path).Two shared server files change:
FormatterUtilities.csandJson.Deserializers.cs(anobject[]deserializer plus a notification-params raw-dict fallback the net8 STJ path needed). Both are behavior-preserving for the server; server serialization tests stay green (56/56).Tests
Streamfake server and exercise initialize / discover / run / run-with-filter, notifications, cancellation, malformed frames, and disconnect on both formatter paths. Green on net8 (STJ) 21/21 and net462 (Jsonite) 21/21.Scope
This is the testfx leg only. Adopting the package in vstest, VSUnitTesting, and vs-green (deleting their bespoke clients and re-homing their glue on the package API) are separate follow-up PRs in those repos. Kept as a draft while those are lined up and the package version and changelog are finalized.
🤖