feat: new async vstest client with STJ serialization#15573
feat: new async vstest client with STJ serialization#15573nohwnd wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a new Microsoft.TestPlatform.Client.Async library implementing an async, multi-session vstest.console client using System.Text.Json, plus an accompanying unit test project.
Changes:
- Added a new async client API surface (
IAsyncVsTestClient,IAsyncTestSession) with discovery and execution operations. - Implemented socket protocol plumbing (handshake, message framing, DTOs) and process lifecycle management.
- Added unit tests for basic DTO/POCO behavior and STJ-based (de)serialization.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.TestPlatform.Client.Async/AsyncTestSession.cs | Adds session object to own socket + process lifecycle and expose session state. |
| src/Microsoft.TestPlatform.Client.Async/AsyncVsTestClient.cs | Implements async client operations: start session, discover tests, run tests, parse results. |
| src/Microsoft.TestPlatform.Client.Async/DiscoveryResult.cs | Adds discovery result model returned by the async client. |
| src/Microsoft.TestPlatform.Client.Async/IAsyncTestSession.cs | Defines the async session contract (PID, connection status, end session). |
| src/Microsoft.TestPlatform.Client.Async/IAsyncVsTestClient.cs | Defines the async client contract (start session, discover, run). |
| src/Microsoft.TestPlatform.Client.Async/Internal/AsyncRequestSender.cs | Implements length-prefixed TCP messaging + protocol handshake. |
| src/Microsoft.TestPlatform.Client.Async/Internal/ProcessManager.cs | Launches/monitors vstest.console process and captures stderr for failure diagnostics. |
| src/Microsoft.TestPlatform.Client.Async/Internal/ProtocolConstants.cs | Defines protocol message type constants and protocol version. |
| src/Microsoft.TestPlatform.Client.Async/Internal/ProtocolMessage.cs | Adds protocol message DTOs and STJ-based property-bag (de)serialization helpers. |
| src/Microsoft.TestPlatform.Client.Async/Microsoft.TestPlatform.Client.Async.csproj | Adds new library project targeting netstandard2.0/net8.0 with STJ dependency. |
| src/Microsoft.TestPlatform.Client.Async/TestRunResult.cs | Adds test run result model returned by the async client. |
| test/Microsoft.TestPlatform.Client.Async.UnitTests/AsyncVsTestClientTests.cs | Adds initial unit tests for constructor/dispose and basic argument validation. |
| test/Microsoft.TestPlatform.Client.Async.UnitTests/DiscoveryResultTests.cs | Adds unit tests for DiscoveryResult constructor/property behavior. |
| test/Microsoft.TestPlatform.Client.Async.UnitTests/Microsoft.TestPlatform.Client.Async.UnitTests.csproj | Adds new unit test project referencing the new async client project. |
| test/Microsoft.TestPlatform.Client.Async.UnitTests/TestObjectDeserializerTests.cs | Adds unit tests for property-bag deserialization and DTO round-tripping via STJ. |
| test/Microsoft.TestPlatform.Client.Async.UnitTests/TestRunResultTests.cs | Adds unit tests for TestRunResult constructor/property behavior. |
| TestPlatform.sln | Wires the new projects into the solution and adds x64/x86 solution configs. |
| public Task<TestRunResult> RunTestsAsync( | ||
| IAsyncTestSession session, | ||
| IEnumerable<TestCase> testCases, | ||
| string? runSettings = null, | ||
| IProgress<TestRunChangedEventArgs>? progress = null, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| var payload = new TestRunRequestPayloadDto | ||
| { | ||
| TestCases = testCases.Select(TestCaseDto.FromTestCase).ToList(), | ||
| RunSettings = runSettings, | ||
| KeepAlive = false, | ||
| }; | ||
|
|
||
| return RunTestsCoreAsync(session, payload, progress, cancellationToken); | ||
| } |
There was a problem hiding this comment.
The RunTestsAsync(... IEnumerable<TestCase> ...) overload builds a payload with TestCases, but RunTestsCoreAsync always sends ProtocolConstants.TestRunAllSourcesWithDefaultHost. That message type is intended for running all tests from sources and may ignore the TestCases payload on the server side. Fix by using the protocol message type intended for running a selected set of test cases (and choose the message type based on whether payload.TestCases vs payload.Sources is populated).
| await s.Sender.SendMessageAsync( | ||
| ProtocolConstants.TestRunAllSourcesWithDefaultHost, |
There was a problem hiding this comment.
The RunTestsAsync(... IEnumerable<TestCase> ...) overload builds a payload with TestCases, but RunTestsCoreAsync always sends ProtocolConstants.TestRunAllSourcesWithDefaultHost. That message type is intended for running all tests from sources and may ignore the TestCases payload on the server side. Fix by using the protocol message type intended for running a selected set of test cases (and choose the message type based on whether payload.TestCases vs payload.Sources is populated).
| await s.Sender.SendMessageAsync( | |
| ProtocolConstants.TestRunAllSourcesWithDefaultHost, | |
| var messageType = payload.TestCases != null && payload.TestCases.Any() | |
| ? ProtocolConstants.TestRunSelectedTestsDefaultHost | |
| : ProtocolConstants.TestRunAllSourcesWithDefaultHost; | |
| await s.Sender.SendMessageAsync( | |
| messageType, |
| foreach (var (key, value) in customProperties) | ||
| { | ||
| var testProperty = TestProperty.Register( | ||
| key.Id, | ||
| key.Label, | ||
| key.Category, | ||
| key.Description, | ||
| GetTypeFromName(key.ValueType), | ||
| null, | ||
| (TestPropertyAttributes)key.Attributes, | ||
| typeof(TestObject)); | ||
| testCase.SetPropertyValue(testProperty, GetStringValue(value)); | ||
| } |
There was a problem hiding this comment.
Custom property values are always written via GetStringValue(value) regardless of the declared key.ValueType. This can produce incorrect property values (e.g., setting an int, bool, Guid, etc. property to a string) and can break downstream consumers expecting the correct runtime type. Deserialize the JsonElement into the declared type (or a supported subset) and pass the correctly-typed object to SetPropertyValue.
| var testProperty = TestProperty.Register( | ||
| key.Id, | ||
| key.Label, | ||
| key.Category, | ||
| key.Description, | ||
| GetTypeFromName(key.ValueType), | ||
| null, | ||
| (TestPropertyAttributes)key.Attributes, | ||
| typeof(TestObject)); |
There was a problem hiding this comment.
TestProperty.Register(...) mutates global process-wide state and is executed for every unknown property encountered. This can (a) violate the PR's stated goal of 'no shared static state' at the library level, (b) create memory pressure over time, and (c) risk registration conflicts if the same ID is encountered with a different type/metadata. Prefer caching by property ID (and only registering once), or avoid registering unknown properties unless strictly needed.
| #pragma warning disable CA2016 | ||
| var acceptTask = _listener.AcceptTcpClientAsync(); | ||
| #pragma warning restore CA2016 | ||
| var exitTask = processManager.ExitedTask; | ||
| var cancelTcs = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
|
||
| using var registration = cancellationToken.Register(() => cancelTcs.TrySetResult(true)); | ||
|
|
||
| var completed = await Task.WhenAny(acceptTask, exitTask, cancelTcs.Task).ConfigureAwait(false); | ||
|
|
||
| if (completed == cancelTcs.Task) | ||
| { | ||
| _listener.Stop(); | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| } | ||
|
|
||
| if (completed == exitTask) | ||
| { | ||
| _listener.Stop(); | ||
| throw processManager.CreateExitException("connection"); | ||
| } |
There was a problem hiding this comment.
When cancellation or process-exit wins, _listener.Stop() can cause acceptTask to fault (e.g., ObjectDisposedException/SocketException). Since acceptTask is not awaited/observed on those paths, this can surface as an unobserved task exception. Consider explicitly observing acceptTask after stopping the listener (e.g., by awaiting it in a try/catch, or attaching a continuation to swallow expected disposal exceptions).
| public async Task<ProtocolMessage?> ReceiveMessageAsync(ProcessManager processManager, CancellationToken cancellationToken) | ||
| { | ||
| // BinaryReader.ReadString() is synchronous. Run it on a thread pool thread | ||
| // so we can race it against process exit and cancellation. | ||
| var readTask = Task.Run(() => | ||
| { | ||
| try | ||
| { | ||
| string json = ReadString(); | ||
| return json; | ||
| } | ||
| catch (EndOfStreamException) | ||
| { | ||
| return null; | ||
| } | ||
| catch (IOException) | ||
| { | ||
| return null; | ||
| } | ||
| }, CancellationToken.None); |
There was a problem hiding this comment.
AsyncRequestSender advertises fully async I/O, but sending is synchronous (BinaryWriter.Write + Flush) and receiving uses Task.Run around a blocking BinaryReader.ReadString(). This can block threads (send side) and create one thread-pool work item per receive (read side), which can become a bottleneck under load or with many concurrent sessions. Consider implementing true async framing on the underlying stream (ReadAsync/WriteAsync) and avoiding Task.Run for I/O.
| public void Dispose() | ||
| { | ||
| try | ||
| { | ||
| if (!_process.HasExited) | ||
| { | ||
| _process.Kill(); | ||
| } | ||
| } | ||
| catch (InvalidOperationException) | ||
| { | ||
| // Process already exited. | ||
| } | ||
|
|
||
| _process.Dispose(); | ||
| } |
There was a problem hiding this comment.
Dispose() should be resilient and generally not throw. Process.Kill() can throw exceptions other than InvalidOperationException (e.g., Win32Exception / NotSupportedException). Consider expanding the handled exceptions (or using a broader catch with a narrow comment) to prevent disposal from crashing callers during cleanup.
| if (completeArgs.TryGetProperty("ElapsedTimeInRunningTests", out var elapsedElem)) | ||
| { | ||
| string? elapsedStr = elapsedElem.GetString(); | ||
| if (elapsedStr != null && TimeSpan.TryParse(elapsedStr, out var parsed)) | ||
| { | ||
| elapsedTime = parsed; | ||
| } | ||
| } |
There was a problem hiding this comment.
TimeSpan.TryParse(elapsedStr, out ...) uses the current culture by default. Since these values are coming from a wire protocol, parsing should be culture-invariant (similar to how duration parsing is already done with CultureInfo.InvariantCulture in TestObjectDeserializer). Use an invariant parse overload (or TryParseExact if the format is stable) to avoid culture-dependent failures.
TestPlatform.sln
Outdated
| Debug|Any CPU = Debug|Any CPU | ||
| Debug|x64 = Debug|x64 | ||
| Debug|x86 = Debug|x86 | ||
| Release|Any CPU = Release|Any CPU | ||
| Release|x64 = Release|x64 | ||
| Release|x86 = Release|x86 |
There was a problem hiding this comment.
This PR introduces new solution configurations (x64/x86) in addition to adding the new async client projects. The PR description doesn't mention changing solution configurations; if this is intentional, it would help to document why these configs are needed. If it's incidental (e.g., generated by tooling), consider reverting to minimize unrelated churn.
| public Task<TestRunResult> RunTestsAsync( | ||
| IAsyncTestSession session, | ||
| IEnumerable<TestCase> testCases, | ||
| string? runSettings = null, | ||
| IProgress<TestRunChangedEventArgs>? progress = null, | ||
| CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
The RunTestsAsync(... IEnumerable<TestCase> ...) path introduces distinct behavior (serializing selected tests and using a different wire protocol path than source-based runs), but the added unit tests only cover constructor/dispose and argument validation. Add tests that validate the request message type/payload for selected test execution (e.g., via a fake sender or by factoring RunTestsCoreAsync to be testable) to prevent regressions.
Initial implementation of a new async vstest client library for microsoft#15570. Key features: - Fully async APIs (Task-based, no blocking) - Reports errors from vstest.console process exits immediately - Non-blocking async I/O for child process output - Supports multiple concurrent sessions (no shared static state) - Uses System.Text.Json for serialization (no Newtonsoft.Json) - Minimal dependencies (ObjectModel + System.Text.Json) - Core operations: StartSession, EndSession, DiscoverTests, RunTests Architecture: - AsyncVsTestClient: main client orchestrating process + socket - AsyncTestSession: represents a live vstest.console connection - AsyncRequestSender: length-prefixed TCP protocol communication - ProcessManager: async process lifecycle management - TestObjectDeserializer: STJ-based property-bag deserialization Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a995998 to
5354be1
Compare
1b7830a to
47a169d
Compare
Initial implementation for #15570
Summary
New async vstest client library (\Microsoft.TestPlatform.Client.Async) designed from scratch with modern patterns.
Key features:
Architecture:
New files:
\
src/Microsoft.TestPlatform.Client.Async/
├── Microsoft.TestPlatform.Client.Async.csproj
├── IAsyncVsTestClient.cs / IAsyncTestSession.cs
├── AsyncVsTestClient.cs / AsyncTestSession.cs
├── DiscoveryResult.cs / TestRunResult.cs
└── Internal/
├── AsyncRequestSender.cs
├── ProcessManager.cs
├── ProtocolConstants.cs
└── ProtocolMessage.cs (includes TestObjectDeserializer)
test/Microsoft.TestPlatform.Client.Async.UnitTests/
├── AsyncVsTestClientTests.cs
├── DiscoveryResultTests.cs
├── TestRunResultTests.cs
└── TestObjectDeserializerTests.cs
\\
All 15 unit tests pass.