Uh oh!
There was an error while loading. Please reload this page.
Add client-declared IsStateful capability to MTP - #9789
Conversation
Expose a client-declared statefulness capability so test frameworks can
distinguish a stateful client (persists an addressable set of test nodes
and their last-known state, e.g. an IDE test explorer) from a stateless
client (streams updates, e.g. dotnet test).
- Add experimental IClientCapabilities { IsStateful } surfaced via
IClientInfo.Capabilities, mirroring the wire protocol's
clientInfo/capabilities split.
- Wire isStateful through the server-mode initialize handshake under
capabilities.testing, backward-compatible (absent => stateless).
- Default stateless in console host; build from client capabilities in
server host.
- Document the isStateful client capability in the protocol intro.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>There was a problem hiding this comment.
Pull request overview
This PR introduces a client-declared statefulness capability to Microsoft.Testing.Platform (MTP), relating to issue #6494. It lets a client declare (rather than the framework infer) whether it retains test-node state for the whole session (stateful, e.g. an IDE Test Explorer) or consumes updates as a stream (stateless, e.g. dotnet test). The flag travels through the server-mode initialize handshake under capabilities.testing.isStateful alongside the existing debuggerProvider, is exposed via a new experimental IClientCapabilities surfaced from IClientInfo.Capabilities, and is fully backward compatible (absent/false ⇒ stateless).
Changes:
- New experimental
IClientCapabilities { bool IsStateful }([TPEXP]) and a newIClientInfo.Capabilitiesgetter, wired throughClientInfoService/ClientCapabilitiesService. - Both server-mode deserializer paths (STJ and Jsonite) read the optional
isStatefulflag, defaulting tofalse; console host is stateless, server host reads the client's declared value. - Protocol docs and acceptance/unit test call sites updated for the new field.
Show a summary per file
| File | Description |
|---|---|
src/Platform/Microsoft.Testing.Platform/Services/IClientCapabilities.cs | New experimental interface exposing IsStateful (missing UTF-8 BOM). |
src/Platform/Microsoft.Testing.Platform/Services/ClientCapabilitiesService.cs | New internal record implementing the interface (missing UTF-8 BOM). |
src/Platform/Microsoft.Testing.Platform/Services/IClientInfo.cs | Adds Capabilities getter to the client-info contract. |
src/Platform/Microsoft.Testing.Platform/Services/ClientInfoService.cs | Threads IClientCapabilities through the record. |
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/RpcMessages.cs | Adds IsStateful to the ClientCapabilities wire record. |
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/JsonRpcMethods.cs | Adds the "isStateful" JSON key constant. |
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/SerializerUtilities.Deserializers.cs | Jsonite path reads optional isStateful. |
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Deserializers.cs | STJ path reads optional isStateful; lacks a unit test. |
src/Platform/Microsoft.Testing.Platform/Hosts/ServerTestHost.RequestExecution.cs | Builds capabilities from the client's declared value. |
src/Platform/Microsoft.Testing.Platform/Hosts/ConsoleTestHost.cs | Console host declares stateless. |
src/Platform/Microsoft.Testing.Platform/PublicAPI/PublicAPI.Unshipped.txt | Declares the three new [TPEXP] API members. |
docs/mstest-runner-protocol/001-protocol-intro.md | Documents isStateful under capabilities.testing. |
test/.../ObjectModel/RunSettingsPatcherTests.cs | Updates constructor calls (BOM stripped). |
test/.../ObjectModel/ObjectModelConvertersTests.cs | Updates constructor calls (BOM stripped). |
test/.../ServerMode/v1.0.0/ClientCapabilities.cs | Adds IsStateful (defaulted) to the test wire record. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 5
- Review effort level: Medium
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary
Clean, well-scoped PR that adds the plumbing for a client-declared isStateful capability through the MTP initialize handshake. The design mirrors the existing debuggerProvider pattern and is properly experimental.
Verdict Table
| # | Dimension | Verdict | Notes |
|---|---|---|---|
| 1 | Algorithmic Correctness | ✅ LGTM | Both deserializers correctly handle the optional field with safe defaults |
| 2 | Threading & Concurrency | ✅ N/A | No shared mutable state introduced; ConsoleTestHost field is static readonly, ServerTestHost field is set during single-threaded init |
| 3 | Public API & Binary Compat | Adding Capabilities to shipped IClientInfo is source-breaking for external implementors — mitigated by [Experimental("TPEXP")] (see inline) | |
| 4 | IPC Wire Compat | ✅ LGTM | Optional field, absent → false, backward-compatible with older clients |
| 5 | Security & IPC Safety | ✅ N/A | No new trust boundary or input validation concerns |
| 6 | Resource & Lifetime | ✅ N/A | No disposable or long-lived resources introduced |
| 7 | Performance | ✅ LGTM | Negligible overhead — one extra boolean in deserialization and one extra record field |
| 8 | Error Handling | ✅ LGTM | Both deserializer paths handle missing/malformed isStateful gracefully |
| 9 | Cross-TFM | ✅ LGTM | No TFM-conditional code; System.Text.Json path and Jsonite path both updated |
| 10 | Localization | ✅ N/A | No user-facing strings |
| 11 | Test Completeness | No test exercises isStateful: true end-to-end (see inline); acceptable since no behavioral branching exists yet | |
| 12 | Naming & Conventions | ✅ LGTM | IClientCapabilities, IsStateful, ClientCapabilitiesService follow repo conventions |
| 13 | Documentation | ✅ LGTM | Protocol doc and XML doc comments are accurate and consistent |
| 14 | Code Structure | ✅ LGTM | Minimal surface area; follows interface → internal record pattern |
| 15 | PublicAPI.Unshipped.txt | ✅ LGTM | All 3 new API entries correctly declared with [TPEXP] prefix |
| 16 | init accessor ban | ✅ LGTM | No init accessors |
| 17 | Scope Discipline | ✅ LGTM | Single concern; follow-ups (consumer usage, client-side sending) tracked in #6494 |
| 18–22 | Remaining dimensions | ✅ N/A | Not applicable to this change |
Key Observations
Wire compatibility ✅ — Both the STJ (
Json.Deserializers.cs) and Jsonite (SerializerUtilities.Deserializers.cs) paths treatisStatefulas optional, defaulting tofalse. Older clients that don't send the field will continue to work unchanged.API evolution note — The new
Capabilitiesproperty onIClientInfois technically a source break for external implementors, butIClientInfois[Experimental("TPEXP")]so this is expected. IfIClientInfois ever graduated, a default interface method or separate interface will be needed (see inline comment).Pure plumbing — No code currently reads
IsStatefulto change behavior. The value flows from the wire → internalClientCapabilitiesrecord →ClientCapabilitiesService→IClientInfo.Capabilitiesand stops there. This is intentional per the PR description; actual behavioral branching comes in a follow-up once clients start sending the flag.
Overall: well-designed, minimal, and backward-compatible. No blocking issues found.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
🔍 Build Failure AnalysisSummary — The build fails with 21 Root cause: Missing internal API declarations in |
| Property | Value |
|---|---|
| Result | ❌ FAILED |
| Duration | 208.8s |
| MSBuild | 18.8.0-preview-26302-115+f7b4c5716 |
| Projects | 49 |
| Errors | 22 (21 RS0051 + 1 meta Build failed) |
| Warnings | 0 |
| Failed projects | Microsoft.Testing.Platform.csproj → Build.proj → NonWindowsTests.slnf (cascade) |
All MSBuild errors (21)
| # | Code | File | Symbol |
|---|---|---|---|
| 1 | RS0051 | JsonRpcMethods.cs:43 | const JsonRpcStrings.IsStateful |
| 2 | RS0051 | RpcMessages.cs:93 | ClientCapabilities.ClientCapabilities(bool, bool) |
| 3 | RS0051 | RpcMessages.cs:93 | ClientCapabilities.Deconstruct(out bool, out bool) |
| 4 | RS0051 | RpcMessages.cs:93 | ClientCapabilities.IsStateful.get |
| 5 | RS0051 | RpcMessages.cs:93 | ClientCapabilities.IsStateful.init |
| 6 | RS0051 | ClientCapabilitiesService.cs:6 | ClientCapabilitiesService (type + 11 auto-generated members) |
| 7–16 | RS0051 | ClientCapabilitiesService.cs:6 | .ctor, .IsStateful, .Equals, .GetHashCode, .ToString, ==, !=, .<Clone>$, .Deconstruct |
| 17 | RS0051 | ClientInfoService.cs:6 | ClientInfoService.Capabilities.get |
| 18 | RS0051 | ClientInfoService.cs:6 | ClientInfoService.Capabilities.init |
| 19 | RS0051 | ClientInfoService.cs:6 | ClientInfoService.ClientInfoService(string!, string!, IClientCapabilities!) |
| 20 | RS0051 | ClientInfoService.cs:6 | ClientInfoService.Deconstruct(out string!, out string!, out IClientCapabilities!) |
| 21 | RS0051 | ClientInfoService.cs:6 | ClientInfoService(string!, string!, IClientCapabilities!) |
🤖 Generated by the Build Failure Analysis workflow using (a href="(dev.azure.com/redacted) · commit f66b623
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 141.2 AIC · ⌖ 9.08 AIC · ⊞ 7.3K · [◷]( · ◷)
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 141.2 AIC · ⌖ 9.08 AIC · ⊞ 7.3K · ◷
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ful tests - Declare new internal symbols (JsonRpcStrings.IsStateful, ClientCapabilities/ ClientCapabilitiesService/ClientInfoService members) in InternalAPI.Unshipped.txt to satisfy the newly-added InternalAPI tracking analyzer (RS0051) that broke the Linux CI build after merging main. - Restore the required UTF-8 BOM (charset=utf-8-bom) on the new and edited .cs files. - Add unit tests covering both deserializer paths (System.Text.Json and Jsonite) for isStateful: true and the absent (stateless) default. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
The ClientCapabilities and ClientInfoService record constructors (and their Deconstruct methods) changed signature, removing the old ones that are declared in InternalAPI.Shipped.txt. Declare them as *REMOVED* in InternalAPI.Unshipped.txt so the Public/Internal API analyzer (RS0017) does not fail the Arcade -warnaserror build. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🧪 Test quality grade — PR #97899 test methods graded across 2 files (4 new, 5 modified). All tests pass the basics; the four new deserialization tests each make a single targeted boolean assertion — clean and focused, but could be strengthened by setting
This advisory comment was generated automatically. Grades are heuristic
|
Summary
Relates to #6494.
Introduces a client-declared statefulness capability so a test framework can distinguish:
dotnet test).This is the "stateless vs stateful" axis discussed in #6494: rather than having a framework infer the environment (parsing
--server, sniffing client name/DesignMode, etc.), the client declares how it will consume results, and the framework branches on that. It cleanly answers scenarios like "is pre-enumerating theory rows worth it?" without coupling to transport or environment.Design
IClientCapabilities { bool IsStateful }surfaced via a newIClientInfo.Capabilitiesgetter, so the C# shape mirrors the wire protocol'sclientInfo/capabilitiessplit 1:1.initializehandshake undercapabilities.testing.isStateful, right next to the existingdebuggerProvider.false⇒ stateless (current behavior). Console host defaults to stateless; server host builds the value from the client's declared capabilities.serviceProvider.GetClientInfo().Capabilities.IsStateful.Notes / follow-ups
dotnet test) start sending it — a coordination point with the VS/SDK teams, exactly likedebuggerProvider.[TPEXP](experimental), so no shipped-API break.ExplicitTestNodeStateProperty#2538) is intentionally out of scope.Testing
Microsoft.Testing.Platformbuilds clean across all TFMs (net8.0/net9.0/netstandard2.0, i.e. both the STJ and Jsonite serializer paths).Microsoft.Testing.Extensions.VSTestBridge.UnitTestsandMicrosoft.Testing.Platform.Acceptance.IntegrationTestsbuild clean.