Split Json.cs into partial-class files#9040
Conversation
Refactors the 789-line ServerMode/JsonRpc/Json/Json.cs into four partial files, each well under the 350-line acceptance criterion from #9017: - Json.cs (210 lines): core engine (constructor, SerializeAsync, Deserialize, Bind/TryBind/TryArrayBind, ValidateJsonRpcHeader) - Json.Serializers.cs (173 lines): RegisterDefaultSerializers - Json.Deserializers.cs (247 lines): RegisterDefaultDeserializers - Json.TestNodeSerializer.cs (198 lines): BuildTestNodeProperties helper (split out the large TestNode property-building lambda) The class is now declared internal sealed partial class Json. The new files are also added to the netstandard2.0 <Compile Remove> exclusion list in Microsoft.Testing.Platform.csproj. No behavioral changes. All 40 JsonTests/JsoniteTests/FormatterUtilities tests pass. Build is clean across net8.0, net9.0, and netstandard2.0. Fixes #9017 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the MTP server-mode JSON-RPC Json formatter (Microsoft.Testing.Platform.ServerMode.Json) by splitting the former large Json.cs implementation into multiple focused partial class files to improve navigation and reduce merge-conflict surface, aligning with issue #9017’s “file diet” goal.
Changes:
- Converted
Jsontointernal sealed partial class Jsonand moved default serializer/deserializer registrations intoRegisterDefaultSerializers/RegisterDefaultDeserializershelpers. - Extracted the large
TestNodeserialization lambda into a dedicated helper (BuildTestNodeProperties) in its own partial file. - Updated
Microsoft.Testing.Platform.csprojto exclude the new JSON files from non-.NETCoreAppTFMs (matching existing netstandard exclusion behavior).
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.cs | Becomes the core partial with constructor delegating to registration helpers and keeping the main serialize/deserialize engine. |
| src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Serializers.cs | Adds RegisterDefaultSerializers partial containing the default type→serializer mappings. |
| src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.Deserializers.cs | Adds RegisterDefaultDeserializers partial containing the default type→deserializer mappings. |
| src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/Json.TestNodeSerializer.cs | Adds BuildTestNodeProperties helper extracted from the previous inline TestNode serializer lambda. |
| src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj | Excludes the newly added partial files for non-.NETCoreApp TFMs alongside existing JSON-related removals. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 2
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review: Split Json.cs into partial-class files
Verdict: clean refactor — no blocking findings.
I fetched the PR branch and compared the new partial-class files against the base commit (4516f02) line-by-line.
Dimensions checked
| Dimension | Result |
|---|---|
| Serializer completeness (all 35 types) | ✅ 1:1 match with original |
| Deserializer completeness (all 20 types) | ✅ 1:1 match with original |
BuildTestNodeProperties extraction |
✅ Byte-identical to the original TestNode lambda |
| IPC wire compatibility | ✅ No serialized output changes |
private static method signatures |
✅ Correct — lambdas receive json as a parameter, no unexpected this capture |
ValidateJsonRpcHeader accessibility |
✅ Defined as private static in Json.cs; valid in partial-class lambdas in sibling files |
.csproj Compile Remove entries |
✅ All three new files correctly excluded from netstandard2.0, in alphabetical order |
using directive redistribution |
✅ Microsoft.Testing.Platform.Extensions.Messages correctly removed from Json.cs and added only to the files that reference it |
with(capacity: 16) syntax |
✅ Valid C# 13 preview collection-expression capacity hint (LangVersion=preview in Directory.Build.props); already present in the original |
StandardOutput/StandardError fall-through |
✅ Both are sealed non-TestNodeStateProperty types; the missing continue is harmless and intentional — matches original |
| Threading / concurrency | ✅ No new shared state; static helper methods are inherently safe |
| MSBuild authoring | ✅ Single ItemGroup condition; no duplicate entries |
One informational observation (pre-existing, out of scope)
The ErrorMessage serializer emits "code" both at the top level of the JSON-RPC envelope and inside the "error" sub-object. JSON-RPC 2.0 §5.1 places code only inside error. The round-trip Deserializer reads only from the nested object, so no client-visible regression exists today — but a future client that strictly validates the envelope could reject the message. This was present in the original Json.cs and is faithfully preserved; worth a follow-up issue to clean up.
The spirit and letter of the refactor are sound. All logic is preserved, the file-count goal is met, and the partial-class structure is idiomatic.
🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review (on PR ready) workflow. · 1K AIC · ⌖ 12.7 AIC · ◷
| return | ||
| [ | ||
| (JsonRpcStrings.JsonRpc, "2.0"), | ||
| (JsonRpcStrings.Code, error.ErrorCode), |
There was a problem hiding this comment.
Pre-existing observation (not introduced by this PR, informational only)
(JsonRpcStrings.Code, error.ErrorCode) appears at the top level of the response envelope (here) and inside the nested errorMsg / "error" sub-object (line 41). Per JSON-RPC 2.0 §5.1, code belongs only inside the error object. The stray top-level "code" is therefore non-conforming.
This was copied faithfully from the original Json.cs and has always been there. The RegisterDefaultDeserializers side only reads error.code from within the nested object, so round-trip behaviour is unaffected in practice. Raising here only so it can be tracked; a follow-up issue to clean it up would keep spec compliance explicit.
- Json.Deserializers.cs: bind ResponseMessage.Result to
IDictionary<string, object?> to match the registered deserializer.
The previous IDictionary<string, object> binding would throw
InvalidOperationException("Cannot find deserializer for ...") for
any non-null result payload because Deserialize<T> performs an exact
typeof(T) lookup. Pre-existing bug carried over from the unsplit
Json.cs but fixed here as part of the review.
- Json.Serializers.cs: refresh the commented-out TimeSpan serializer
line to reference the 'serializers' parameter instead of the old
'_serializers' field.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Windows Debug job failed with an access violation (0xC0000005) inside Mono.Cecil.Pdb.ISymUnmanagedWriter2.Close() while ILLink wrote the trim publish output for MSTestTrimAnalysisTest_net10.0. The crash is a known flake in the bundled microsoft.net.illink.tasks 10.0.7 native PDB writer on Windows and is unrelated to this PR's Json.cs split refactor. No code change; pushing an empty commit to retrigger the failed job. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #9017.
Validation: Json.cs is not a Jsonite copy
Confirmed the target file is a custom MTP
System.Text.Json-based serializer (usesSystem.Text.Json, namespaceMicrosoft.Testing.Platform.ServerMode.Json, no Alexandre Mutel copyright header). The actual Jsonite vendored copy lives in a separateServerMode/JsonRpc/Json/Jsonite/subfolder, is credited inTHIRD-PARTY-NOTICES.TXT, and is not touched by this refactor.(Side note: the Jsonite folder is missing from
eng/vendored-files.json. Not in scope for this PR, but worth a follow-up.)Refactor
Splits the 789-line
Json.csinto four partial-class files, all under the 350-line acceptance criterion:Json.csSerializeAsync,Deserialize,Bind/TryBind/TryArrayBind,ValidateJsonRpcHeaderJson.Serializers.csRegisterDefaultSerializersJson.Deserializers.csRegisterDefaultDeserializersJson.TestNodeSerializer.csBuildTestNodePropertieshelper (the previously inline ~188-lineTestNodelambda)Notes:
internal sealed partial class Json.staticand take the target dictionary as a parameter — the original lambdas only captured thejsonargument the infrastructure passes them, notthis.Json.TestNodeSerializer.csexists because theTestNodelambda alone was ~188 lines; keeping it inline would have pushedJson.Serializers.csover 350 lines. The issue proposed 3 files; this delivers 4. The spirit of the request — small, focused, navigable files — is preserved.netstandard2.0<Compile Remove>exclusion list inMicrosoft.Testing.Platform.csproj, matching the existing convention for these JSON files.Validation
net8.0,net9.0,netstandard2.0(0 warnings, 0 errors)JsonTests/JsoniteTests/FormatterUtilitiestests passNo behavioral changes — pure file split with one helper extraction.