feat: migrate from Newtonsoft.Json to System.Text.Json / Jsonite

[nohwnd]

Summary

Migrate vstest JSON serialization from Newtonsoft.Json to System.Text.Json (on .NET Core) and Jsonite (on .NET Framework/netstandard2.0). Removes Newtonsoft.Json from all production code paths.

Architecture

Platform	Serializer	Condition
.NET Core (net10.0+)	System.Text.Json	#if NETCOREAPP
.NET Framework (net462)	Jsonite (embedded BSD-2, from xoofx/jsonite)	#if !NETCOREAPP
netstandard2.0	Jsonite	#if !NETCOREAPP

Key Changes

• JsonDataSerializer split into 3 partial classes (shared / STJ / Jsonite) — zero #if blocks in main file
• 12+ custom STJ converters for vstest types (TestCase, TestResult, TestProperty, etc.)
• Full Jsonite converter suite handling V1/V2 wire formats, private setters, collection Add()
• Message class unified — removed Payload property, uses RawMessage string + deferred deserialization
• DepsJsonParser replaces DependencyContextJsonReader on .NET Framework (eliminates transitive STJ dep)
• Removed ALL Newtonsoft.Json from production code (Legacy fallback deleted)
• System.Text.Json.dll removed from TestHostNetFramework packages
• Jsonite added to THIRD-PARTY-NOTICES.txt
• All serialization tests run on both TFMs — zero #if NET compiler conditions in tests
• Jsonite control char fix — escape chars < 0x20 as \uXXXX instead of throwing (testfx#5120)
• ObjectDictionaryConverterFactory — fixes STJ deserializing JSON integers as double in IDictionary<string, object> (metrics)
• TestCaseEventArgs — changed private set → set on TestCaseId, TestCaseName, IsChildTestCase, TestElement, TestOutcome for serialization compatibility

Test Coverage

Suite	Count	Status
Serialization unit tests (per-message wire format)	1125 (375 × 3 TFMs)	✅ All pass
Performance benchmarks (including 1000-result batch)	78 (26 × 3 TFMs)	✅ All pass
Serialization edge cases (control chars, unicode, emoji)	Included in above	✅ All pass
CommunicationUtilities unit tests	1434 (3 TFMs)	✅ All pass
All 19 unit test projects	7576+	✅ All pass
CodeCoverage integration tests (Library)	24	✅ All pass
Telemetry acceptance tests	10	✅ All pass
Translation layer telemetry tests	18	✅ All pass
Acceptance tests (non-compat)	352+	✅ All pass

100% MessageType coverage — every single one of the 52 message types in MessageType class has serialization tests (V1 serialize, V7 serialize, V1 deserialize, V7 deserialize, round-trip).

Wire Format Verification

Captured diag logs from blame test runs on both main (Newtonsoft) and stj-migration branches. Parsed and compared all JSON messages from runner, testhost, and datacollector logs:

• Runner (vstest.console): All 8 message types — ✅ Identical JSON structure
• Testhost: All message types present — ✅ Same data (property ordering differs due to dictionary iteration, semantically irrelevant)
• Datacollector: 3 shared types — ✅ Identical
• Telemetry output: 24 keys, all matching — ✅ Identical (except timing values)
• Header field order: All messages start with Version,MessageType — ✅ Fast header parser compatible

Performance: STJ vs Newtonsoft (1000-result batch, V7)

Operation	STJ (net10.0)	Jsonite (net48)
Serialize 1000-result StatsChange	~11ms	~36ms
Deserialize 1000-result StatsChange	~60ms	~96ms
JSON size	911KB	911KB

Performance: STJ vs Newtonsoft (single message, 1000 iterations)

Operation	STJ	Newtonsoft baseline
Serialize TestMessage	14ms	~20ms
Serialize TestCasesFound	58ms	~65ms
Serialize ExecutionComplete	117ms	~130ms

Breaking Changes

• Message.Payload (JToken?) removed — replaced by Message.RawMessage (string?) + Message.Version
• VersionedMessage class removed — unified into Message
• Newtonsoft.Json no longer shipped in vstest packages
• TestCaseEventArgs.TestCaseId/TestCaseName/IsChildTestCase/TestElement setters changed from private/internal to public
• TestCaseEndEventArgs.TestOutcome setter changed from private to public

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

Migrates VSTest’s serialization infrastructure from Newtonsoft.Json to System.Text.Json, including protocol payload handling and test updates, while introducing golden baseline tests to preserve wire-format compatibility.

Changes:

• Replaced JToken/JObject usage with JsonElement/JsonDocument and STJ nodes across runtime + tests.
• Rewrote core serializer configuration (JsonDataSerializer) and multiple custom converters for protocol v1/v2 payload shapes.
• Updated package references and added golden JSON baseline tests to validate compatibility.

Reviewed changes

Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/TranslationLayer.UnitTests/VsTestConsoleRequestSenderTests.cs	Updates test message payloads from JToken to JsonElement via STJ serialization.
test/Microsoft.TestPlatform.TestUtilities/IntegrationTestBuild.cs	Switches checksum serialization from Newtonsoft to STJ with indented output.
test/Microsoft.TestPlatform.ObjectModel.UnitTests/Microsoft.TestPlatform.ObjectModel.UnitTests.csproj	Replaces Newtonsoft.Json package reference with System.Text.Json.
test/Microsoft.TestPlatform.ObjectModel.UnitTests/Client/DiscoveryCriteriaTests.cs	Migrates serialization/deserialization assertions to STJ.
test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/EventHandlers/TestRequestHandlerTests.cs	Updates protocol message payloads to JsonElement.
test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelRunEventsHandlerTests.cs	Updates mocked message payloads to STJ JsonElement.
test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelDiscoveryEventsHandlerTests.cs	Updates mocked message payloads to STJ JsonElement.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/TestResultSerializationTests.cs	Replaces JObject/dynamic JSON assertions with JsonDocument/JsonElement.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/TestCaseSerializationTests.cs	Replaces Newtonsoft JSON parsing/formatting with STJ equivalents.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/SerializationGoldenTests.cs	Adds golden baseline serialization tests for v1/v2 protocol formats.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs	Updates serializer tests for STJ behavior (cycle handling, payload assertions).
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventSenderTests.cs	Updates payload creation to STJ JsonElement.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventHandlerTests.cs	Updates message payloads to STJ and adjusts a complex payload construction.
test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionRequestHandlerTests.cs	Updates test messages’ payloads to STJ JsonElement.
test/Microsoft.TestPlatform.Client.UnitTests/DesignMode/DesignModeClientTests.cs	Updates design-mode protocol message payloads to STJ JsonElement.
test/Microsoft.TestPlatform.AdapterUtilities.UnitTests/Microsoft.TestPlatform.AdapterUtilities.UnitTests.csproj	Replaces Newtonsoft.Json package reference with System.Text.Json.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DotnetHostArchitectureVerifierTests.cs	Replaces runtimeconfig JSON patching with JsonNode + ToJsonString().
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DotnetArchitectureSwitchTests.Windows.cs	Replaces runtimeconfig JSON patching with JsonNode + ToJsonString().
src/package/Microsoft.TestPlatform/Microsoft.TestPlatform.csproj	Packages System.Text.Json instead of Newtonsoft.Json.
src/package/Microsoft.TestPlatform.Portable/Microsoft.TestPlatform.Portable.csproj	Packages System.Text.Json instead of Newtonsoft.Json.
src/package/Microsoft.TestPlatform.CLI/Microsoft.TestPlatform.CLI.csproj	Swaps Newtonsoft.Json packaging/copy logic to System.Text.Json.
src/Microsoft.TestPlatform.TestHostProvider/Microsoft.TestPlatform.TestHostProvider.csproj	Replaces Newtonsoft.Json dependency with System.Text.Json.
src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs	Replaces JObject/JToken parsing with JsonDocument for runtimeconfig probing paths.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestRunStatisticsConverter.cs	Rewrites to JsonConverter<ITestRunStatistics> for STJ.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestResultConverterV2.cs	Introduces STJ converter implementing v2 flat TestResult JSON shape.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestResultConverter.cs	Rewrites v1 TestResult converter from Newtonsoft to STJ.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestPropertyConverter.cs	Adds STJ converter to serialize TestProperty without delegate members.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestPlatformContractResolver1.cs	Removes Newtonsoft contract resolver (now STJ options setup lives in JsonDataSerializer).
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestObjectConverter.cs	Rewrites TestObject property-bag converter for STJ and removes experimental v7 code.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestObjectBaseConverter.cs	Adds factory + converter for TestObject-derived types that only serialize the property bag.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestExecutionContextConverter.cs	Adds STJ converter ensuring DataMember-only behavior for TestExecutionContext.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestCaseConverterV2.cs	Adds STJ converter implementing v2 flat TestCase JSON shape.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestCaseConverter.cs	Rewrites v1 TestCase converter from Newtonsoft to STJ.
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/DefaultTestPlatformContractResolver.cs	Removes Newtonsoft contract resolver (moved to STJ options in JsonDataSerializer).
src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/AttachmentConverters.cs	Adds STJ converters for AttachmentSet and UriDataAttachment ctor constraints.
src/Microsoft.TestPlatform.CommunicationUtilities/PublicAPI/PublicAPI.Shipped.txt	Updates shipped API surface for Message.Payload to JsonElement? and converter overrides.
src/Microsoft.TestPlatform.CommunicationUtilities/ObjectModel/TestRunCriteriaWithTests.cs	Replaces Newtonsoft Json attributes namespace with STJ.
src/Microsoft.TestPlatform.CommunicationUtilities/ObjectModel/TestRunCriteriaWithSources.cs	Replaces Newtonsoft Json attributes namespace with STJ.
src/Microsoft.TestPlatform.CommunicationUtilities/Microsoft.TestPlatform.CommunicationUtilities.csproj	Replaces Newtonsoft.Json dependency with System.Text.Json.
src/Microsoft.TestPlatform.CommunicationUtilities/Messages/Message.cs	Changes Message.Payload type to JsonElement? and updates ToString().
src/Microsoft.TestPlatform.CommunicationUtilities/JsonDataSerializer.cs	Rewrites serializer core to STJ with protocol-specific options and “fast path”.
eng/Versions.props	Adds SystemTextJsonVersion and updates projects to consume it.
docs/stj-migration-overview.md	Adds manager-facing overview and risks (including breaking API).
docs/stj-migration-detailed.md	Adds detailed migration plan and critical compat details.
docs/stj-migration-changelog.md	Adds file-by-file migration change log.

Comments suppressed due to low confidence (2)

test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1

• The updated comment describes the expected post-deserialization shape, but the test no longer asserts it. This reduces the test’s ability to catch regressions in cycle-handling semantics. Add assertions for the expected object graph (e.g., result.InfiniteReference presence and its InfiniteReference being null) to match the documented behavior.
  test/Microsoft.TestPlatform.ObjectModel.UnitTests/Client/DiscoveryCriteriaTests.cs:1
• This assertion is order-sensitive and relies on the serializer emitting properties in a specific order. Since JSON object order is not semantically significant, this can make the test brittle across runtime/tooling changes. Consider comparing parsed JSON (e.g., via JsonDocument) or reusing the normalization approach used in the golden tests, unless strict ordering is explicitly required for the wire protocol here.

[Copilot]

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (4)

test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/TestRunStatsChangeSerializationTests.cs:1

• These new wire-format tests are currently disabled via [Ignore], which leaves the STJ migration unvalidated for deserialization/round-trip of TestRunStatsPayload. Since this PR is explicitly about preserving protocol compatibility, it would be better to either (1) implement the required custom converter(s) (e.g., for TestRunChangedEventArgs) and enable these tests, or (2) move ignored cases into a tracked work item with an explicit TODO + target milestone so the gap can’t silently persist.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/ExecutionCompleteSerializationTests.cs:1
• Same issue as above but more critical: ExecutionComplete is described as the most complex message in the protocol, yet deserialization/round-trip tests are ignored. For a migration with wire-format guarantees, enabling these by adding a JsonConverter for TestRunCompleteEventArgs (and any other required types) should be treated as required before considering the migration complete.
  test/TranslationLayer.UnitTests/VsTestConsoleRequestSenderTests.cs:1
• Many tests now construct Message.Payload using JsonSerializer.SerializeToElement(...) with default STJ options. This bypasses the custom JsonDataSerializer options/converters (and protocol-version-specific shapes), so the tests may no longer be exercising the real wire format. Prefer constructing payload elements via the production serializer (e.g., serialize with JsonDataSerializer.Instance.Serialize(..., version) and parse/clone into a JsonElement) to keep unit tests aligned with the protocol behavior.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Microsoft.TestPlatform.CommunicationUtilities.UnitTests.csproj:1
• This introduces a hard-coded Newtonsoft.Json version while the repo already has a central $(NewtonsoftJsonVersion) property in eng/Versions.props. Using the shared property here would avoid accidental version drift between production and test comparison infrastructure.

[Copilot]

Pull request overview

Copilot reviewed 81 out of 81 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (11)

test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionRequestHandlerTests.cs:1

• AfterTestRunEnd payload is wire-formatted as a JSON boolean in the new serialization tests (e.g., AfterTestRunEndSerializationTests expects false, not "false"). Using SerializeToElement("false") produces a JSON string and can break DeserializePayload<bool> behavior or mask mismatches; switch this to JsonSerializer.SerializeToElement(false) to match the protocol payload type.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/StopTestSessionSerializationTests.cs:1
• These tests are ignored because STJ cannot populate TestSessionInfo.Id (private setter), which likely reflects a real runtime incompatibility for StopTestSession payloads. Instead of ignoring, address deserialization by adding an explicit STJ solution (e.g., a custom JsonConverter<TestSessionInfo> that reads Id and sets it via reflection, or adjusting the model to be STJ-deserializable via a [JsonConstructor]/[JsonInclude]-compatible approach). Keeping these ignored reduces confidence that stop-session flows work cross-process.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/StopTestSessionSerializationTests.cs:1
• These tests are ignored because STJ cannot populate TestSessionInfo.Id (private setter), which likely reflects a real runtime incompatibility for StopTestSession payloads. Instead of ignoring, address deserialization by adding an explicit STJ solution (e.g., a custom JsonConverter<TestSessionInfo> that reads Id and sets it via reflection, or adjusting the model to be STJ-deserializable via a [JsonConstructor]/[JsonInclude]-compatible approach). Keeping these ignored reduces confidence that stop-session flows work cross-process.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/StopTestSessionSerializationTests.cs:1
• These tests are ignored because STJ cannot populate TestSessionInfo.Id (private setter), which likely reflects a real runtime incompatibility for StopTestSession payloads. Instead of ignoring, address deserialization by adding an explicit STJ solution (e.g., a custom JsonConverter<TestSessionInfo> that reads Id and sets it via reflection, or adjusting the model to be STJ-deserializable via a [JsonConstructor]/[JsonInclude]-compatible approach). Keeping these ignored reduces confidence that stop-session flows work cross-process.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/AttachDebuggerSerializationTests.cs:1
• The ignored tests indicate STJ cannot deserialize TestProcessAttachDebuggerPayload due to constructor parameter name mismatch (pid vs ProcessID). This is typically fixable via a STJ-friendly constructor binding (e.g., renaming the parameter to match the JSON property, or annotating the constructor/parameter with STJ metadata such as [JsonConstructor] and [JsonPropertyName("ProcessID")], or providing a custom converter). Leaving this ignored risks breaking the AttachDebugger message handling at runtime.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/AttachDebuggerSerializationTests.cs:1
• The ignored tests indicate STJ cannot deserialize TestProcessAttachDebuggerPayload due to constructor parameter name mismatch (pid vs ProcessID). This is typically fixable via a STJ-friendly constructor binding (e.g., renaming the parameter to match the JSON property, or annotating the constructor/parameter with STJ metadata such as [JsonConstructor] and [JsonPropertyName("ProcessID")], or providing a custom converter). Leaving this ignored risks breaking the AttachDebugger message handling at runtime.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/AttachDebuggerSerializationTests.cs:1
• The ignored tests indicate STJ cannot deserialize TestProcessAttachDebuggerPayload due to constructor parameter name mismatch (pid vs ProcessID). This is typically fixable via a STJ-friendly constructor binding (e.g., renaming the parameter to match the JSON property, or annotating the constructor/parameter with STJ metadata such as [JsonConstructor] and [JsonPropertyName("ProcessID")], or providing a custom converter). Leaving this ignored risks breaking the AttachDebugger message handling at runtime.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventHandlerTests.cs:1
• JsonDocument.Parse(...) returns an IDisposable document, but it’s not disposed here. Wrap the parse in a using var (and then clone the root element) to avoid leaking unmanaged buffers in the test process, especially since this pattern may repeat across tests.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• The updated comment describes the expected post-deserialization object graph (InfiniteReference.InfiniteReference = null), but the test no longer asserts that behavior (it only asserts the type). Add an assertion that matches the new expected structure so the test continues to validate the cycle-handling behavior rather than only “doesn’t throw”.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• The interpolated raw string literal uses 4 $ plus {{{{version}}}}, which is hard to read and easy to get wrong when editing. Consider switching to a simpler construction (standard interpolation with escaped quotes, or a helper that builds expected JSON) to reduce brittleness and improve maintainability of these tests.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/VersionCheckSerializationTests.cs:1
• Normalizing by parsing and re-serializing can hide wire-format differences that are relevant for cross-version compatibility (notably escaping behavior like \u003C vs <, and any serializer-specific formatting quirks). If these are meant to be strict wire-format tests, consider comparing against a compact “golden” expected string directly (store the compact string instead of pretty JSON), or add targeted assertions for escaping/formatting that must match across serializers.

[Copilot]

Pull request overview

Copilot reviewed 96 out of 96 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (6)

src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestSessionInfoConverter.cs:1

• Guid.NewGuid() as a fallback when "Id" is missing/null changes semantics and can break protocol expectations (missing Id should not silently become a new unique session). Prefer preserving the default value (typically Guid.Empty) or treating missing Id as invalid/null rather than generating a new identifier.
  src/package/Microsoft.TestPlatform.CLI/Microsoft.TestPlatform.CLI.csproj:1
• The MSBuild condition compares against the literal string NetFrameworkRunnerTargetFramework instead of the property value. This will evaluate incorrectly and can cause the package references (and associated copy logic) to apply in unintended TFMs. Use $(NetFrameworkRunnerTargetFramework) in the condition, and ensure the SystemTextJson include/copy logic is guarded by the same condition to avoid unresolved $(PkgSystem_Text_Json) paths in TFMs where the package isn’t referenced.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Microsoft.TestPlatform.CommunicationUtilities.UnitTests.csproj:1
• This introduces a hard-coded Newtonsoft.Json version while the repo already centralizes versions in eng/Versions.props. To prevent version drift, reference $(NewtonsoftJsonVersion) here instead of a literal.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventHandlerTests.cs:1
• This test constructs Message.Payload via serialize → parse → clone, which is comparatively expensive and (in this test) likely unnecessary because deserialization is mocked. Prefer JsonSerializer.SerializeToElement(...) (or even a minimal JsonDocument.Parse("{}") payload) unless the handler under test actually depends on the exact serialized shape.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• The comment directly above indicates a specific expected post-deserialization shape for cyclic references under STJ, but the assertion verifying that behavior was removed. This reduces coverage for an important behavior change between Newtonsoft vs STJ (and between protocol versions). Add an assertion that validates the expected cyclical-reference outcome (e.g., InfiniteReference existence and/or its nested reference being null) so regressions are caught.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• The comment directly above indicates a specific expected post-deserialization shape for cyclic references under STJ, but the assertion verifying that behavior was removed. This reduces coverage for an important behavior change between Newtonsoft vs STJ (and between protocol versions). Add an assertion that validates the expected cyclical-reference outcome (e.g., InfiniteReference existence and/or its nested reference being null) so regressions are caught.

[Copilot]

Pull request overview

Copilot reviewed 119 out of 119 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (4)

test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1

• This test’s assertions depend on the ambient VSTEST_USE_NEWTONSOFT_JSON_SERIALIZER environment variable, which can make test results vary across machines/CI agents. To keep the unit test deterministic, set/unset the env var within the test (and restore the previous value in a try/finally) to explicitly cover both code paths.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• This test now documents a specific expected shape for the deserialized object (nested InfiniteReference becomes null), but it no longer asserts that behavior. Please add an assertion that validates the expected object graph (e.g., that result.InfiniteReference is non-null and result.InfiniteReference.InfiniteReference is null), so the behavior is actually covered.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestResultConverterV2.cs:1
• Parsing Duration/StartTime/EndTime using CultureInfo.CurrentCulture makes deserialization dependent on the executing machine’s culture, but these values are produced in a culture-invariant wire format (TimeSpan 'c' / DateTimeOffset ISO). Use CultureInfo.InvariantCulture (or RoundtripKind where applicable) to keep protocol parsing deterministic and avoid culture-specific failures.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/VersionCheckSerializationTests.cs:1
• Many of the new wire-format test classes repeat the same AssertJsonEqual/Minify helpers. Consider extracting these into a shared internal test utility (e.g., SerializationTestHelpers) to reduce duplication and keep future updates (like normalization options) consistent. This is optional but will make the suite easier to maintain.

[Copilot]

Pull request overview

Copilot reviewed 123 out of 123 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestSessionInfoConverter.cs:1

• Falling back to Guid.NewGuid() makes deserialization nondeterministic when Id is missing/null, which can cause hard-to-reproduce behavior and flaky tests. Prefer a deterministic fallback (e.g., Guid.Empty) or treat missing Id as invalid input and throw a JsonException.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• This test no longer asserts the expected post-deserialization shape of the self-referencing loop (the comment describes a specific structure, but the assertions only check the type). Add assertions that validate the intended STJ behavior (e.g., that InfiniteReference exists and that its nested reference is null), so the test continues to protect against regressions.
  test/Microsoft.TestPlatform.ObjectModel.UnitTests/Client/DiscoveryCriteriaTests.cs:1
• Comparing raw JSON strings makes this test brittle because STJ may legitimately change formatting/escaping or property ordering across runtimes/options. Prefer structural JSON comparison (parse both and compare normalized forms) or assert on the specific properties you care about.
  src/package/Microsoft.TestPlatform.CLI/Microsoft.TestPlatform.CLI.csproj:1
• The MSBuild condition compares $(TargetFramework) to the literal string NetFrameworkRunnerTargetFramework, so it will almost always evaluate to true and include the package unexpectedly. If the intent is to compare against the property value, use Condition=\"'$(TargetFramework)' != '$(NetFrameworkRunnerTargetFramework)'\" (or the correct property name used in this repo).

[Copilot]

Pull request overview

Copilot reviewed 144 out of 145 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

• src/Microsoft.TestPlatform.CommunicationUtilities/Resources/Resources.Designer.cs: Language not supported

Comments suppressed due to low confidence (7)

src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestSessionInfoConverter.cs:1

• When the JSON omits Id (or has null), the converter generates a new Guid, making deserialization non-deterministic and potentially changing semantics (e.g., a missing Id should not silently become a brand-new session identifier). Prefer preserving the previous behavior (commonly Guid.Empty) or treating missing Id as an invalid payload (throw) depending on protocol expectations.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• StringBuilder is mutated from the Process error-data callback and read from other threads without synchronization. This can lead to data races and undefined behavior. Add a lock around both appends and reads (or switch to a thread-safe buffering strategy). Also consider capping the captured output size to prevent unbounded memory growth when a child process is noisy.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• StringBuilder is mutated from the Process error-data callback and read from other threads without synchronization. This can lead to data races and undefined behavior. Add a lock around both appends and reads (or switch to a thread-safe buffering strategy). Also consider capping the captured output size to prevent unbounded memory growth when a child process is noisy.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• StringBuilder is mutated from the Process error-data callback and read from other threads without synchronization. This can lead to data races and undefined behavior. Add a lock around both appends and reads (or switch to a thread-safe buffering strategy). Also consider capping the captured output size to prevent unbounded memory growth when a child process is noisy.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• These unit test expectations depend on an external environment variable, which can make test runs non-deterministic across machines/CI jobs. Set/clear the environment variable within the test (and restore the prior value) to explicitly test both code paths, or restructure the serializer to allow injecting the mode in tests without relying on ambient process state.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Microsoft.TestPlatform.CommunicationUtilities.UnitTests.csproj:1
• This hard-codes the Newtonsoft.Json version in the test csproj, which can drift from the repo’s central version management (e.g., eng/Versions.props). Consider using $(NewtonsoftJsonVersion) (and optionally PrivateAssets="All") so updates remain centralized and consistent.
  test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/EventHandlers/TestRequestHandlerTests.cs:1
• This silently serializes a null payload whenever RawMessage is not set, which can hide mistakes in test setup and produce wire data that doesn’t match the intended message. Consider failing fast (e.g., Assert/throw) when RawMessage is unexpectedly null, or adjust the helper to require callers to provide the payload explicitly so the emitted JSON matches the constructed message semantics.

[Copilot]

Pull request overview

Copilot reviewed 145 out of 146 changed files in this pull request and generated 3 comments.

Files not reviewed (1)

• src/Microsoft.TestPlatform.CommunicationUtilities/Resources/Resources.Designer.cs: Language not supported

Comments suppressed due to low confidence (7)

src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestSessionInfoConverter.cs:1

• Generating a new Guid when Id is missing changes semantics and makes deserialization non-deterministic. Prefer preserving the default (e.g., Guid.Empty) or leaving the property unset when the wire payload has no Id.
  test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/EventHandlers/TestRequestHandlerTests.cs:1
• This fallback path silently serializes a null payload when RawMessage is missing, which can make tests send an invalid/incorrect wire message without failing. Consider failing fast (throw/assert) when RawMessage is null in this helper, or restructure tests to always send explicit raw JSON so payload correctness is preserved.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestPropertyConverter.cs:1
• Calling Type.GetType on a string coming from the wire can trigger unintended type/assembly resolution. Since this is cross-process input, consider restricting ValueType to a safe allow-list (or mapping known type names) instead of using Type.GetType directly.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestResultConverterV2.cs:1
• These values are written in an invariant ISO/constant format by the serializer, but are parsed using CurrentCulture. This can cause culture-specific parsing bugs. Prefer CultureInfo.InvariantCulture (or ParseExact where feasible) to make the wire format culture-independent.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• IProcessManager exposes ProcessId/ExitCode as get-only, but the implementation exposes public setters. To keep these values authoritative and avoid accidental mutation by consumers, consider making setters private (or at least internal) and updating them only from StartProcess/exit handlers.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• This test’s expectations vary based on an external environment variable, which makes it harder to ensure both code paths stay correct. Prefer explicitly setting/unsetting the variable within the test (and restoring it in a finally block), and assert both behaviors deterministically. Also consider using the FeatureFlag.VSTEST_USE_NEWTONSOFT_JSON_SERIALIZER constant to avoid string drift.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Microsoft.TestPlatform.CommunicationUtilities.UnitTests.csproj:1
• This hard-codes the Newtonsoft.Json version instead of using the repo property (e.g., $(NewtonsoftJsonVersion)), which can drift over time. Consider referencing the shared version property for consistency with the rest of the repo.

[Copilot]

Pull request overview

Copilot reviewed 150 out of 151 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• src/Microsoft.TestPlatform.CommunicationUtilities/Resources/Resources.Designer.cs: Language not supported

Comments suppressed due to low confidence (12)

src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1

• StringBuilder is not thread-safe. Process_ErrorDataReceived can be raised on a different thread while ErrorOutput is being read (e.g., during timeout error reporting), which can cause torn reads or exceptions. Protect _vstestConsoleError with a lock, or replace it with a thread-safe buffer (e.g., ConcurrentQueue + string.Join) and consider capping stored output to prevent unbounded memory growth.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• StringBuilder is not thread-safe. Process_ErrorDataReceived can be raised on a different thread while ErrorOutput is being read (e.g., during timeout error reporting), which can cause torn reads or exceptions. Protect _vstestConsoleError with a lock, or replace it with a thread-safe buffer (e.g., ConcurrentQueue + string.Join) and consider capping stored output to prevent unbounded memory growth.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• StringBuilder is not thread-safe. Process_ErrorDataReceived can be raised on a different thread while ErrorOutput is being read (e.g., during timeout error reporting), which can cause torn reads or exceptions. Protect _vstestConsoleError with a lock, or replace it with a thread-safe buffer (e.g., ConcurrentQueue + string.Join) and consider capping stored output to prevent unbounded memory growth.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleWrapper.cs:1
• Including full childProcessErrorOutput in the thrown exception can be extremely large and may contain sensitive data (paths, environment hints, etc.), which can bloat logs/telemetry. Consider truncating error output to a bounded size (e.g., last N KB/lines) and/or stripping ANSI control characters before embedding it in the exception message.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestSessionInfoConverter.cs:1
• Generating a new Guid when Id is missing/nullable changes semantics during deserialization (the same JSON can produce different objects across runs). Prefer a deterministic default (e.g., Guid.Empty) or treat missing Id as invalid and fail, depending on the protocol expectations.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestCaseConverter.cs:1
• TestCase.ExecutorUri can be null (the v2 converter already accounts for this via ?.OriginalString). This v1 converter will throw NullReferenceException during serialization if ExecutorUri is unset. Write a null value (or empty string if the wire format requires it) when ExecutorUri is null.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestResultConverterV2.cs:1
• These values are wire-format data and should be parsed culture-invariantly. Using CultureInfo.CurrentCulture introduces locale-dependent failures (especially for DateTimeOffset and potentially TimeSpan formats). Parse with CultureInfo.InvariantCulture and, ideally, round-trip formats consistent with how STJ writes these values.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• This test documents expected cycle-handling behavior but no longer asserts it. Add assertions that match the stated expectation (e.g., verify InfiniteReference is not null and InfiniteReference.InfiniteReference is null), so regressions in reference-handling don't silently pass.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Microsoft.TestPlatform.CommunicationUtilities.UnitTests.csproj:1
• This hard-codes Newtonsoft.Json version instead of using the repo’s version properties (e.g., $(NewtonsoftJsonVersion)). Using the central version property reduces drift when the dependency is updated.
  test/Microsoft.TestPlatform.TestUtilities/IntegrationTestBuild.cs:1
• This JSON is used as a build cache/checksum identifier. System.Text.Json does not guarantee deterministic property ordering across all dictionary implementations/TFMs, which can cause spurious checksum changes and unnecessary rebuilds. Consider normalizing output (e.g., serialize via a JsonDocument/JsonNode with sorted properties, or use a deterministic representation) before writing checksum.json.
  src/testhost.x86/app.config:1
• The System.Text.Json bindingRedirect newVersion (10.0.0.0) conflicts with the comment stating netframework uses an exact assembly version (and you keep the oldVersion range up to 10.0.0.5). If the shipped System.Text.Json assembly has version 10.0.0.5, redirecting to 10.0.0.0 can cause assembly load failures. Align the redirect target with the actual AssemblyVersion of the shipped DLL (or change what you ship) and keep System.Text.Json and System.Text.Encodings.Web consistent.
  src/testhost.x86/app.config:1
• The System.Text.Json bindingRedirect newVersion (10.0.0.0) conflicts with the comment stating netframework uses an exact assembly version (and you keep the oldVersion range up to 10.0.0.5). If the shipped System.Text.Json assembly has version 10.0.0.5, redirecting to 10.0.0.0 can cause assembly load failures. Align the redirect target with the actual AssemblyVersion of the shipped DLL (or change what you ship) and keep System.Text.Json and System.Text.Encodings.Web consistent.

[Copilot]

Pull request overview

Copilot reviewed 157 out of 158 changed files in this pull request and generated 5 comments.

Files not reviewed (1)

• src/Microsoft.TestPlatform.CommunicationUtilities/Resources/Resources.Designer.cs: Language not supported

Comments suppressed due to low confidence (9)

src/testhost.x86/app.config:1

• The comment indicates .NET Framework needs an “exact version” redirect, but the active redirect targets 10.0.0.0 while the range ends at 10.0.0.5. This mismatch can cause runtime binding failures depending on the shipped assembly version. Align the newVersion with the actual AssemblyVersion of the System.Text.Json.dll you ship for netfx, and keep the comment consistent with the chosen redirect strategy.
  src/testhost.x86/app.config:1
• The comment indicates .NET Framework needs an “exact version” redirect, but the active redirect targets 10.0.0.0 while the range ends at 10.0.0.5. This mismatch can cause runtime binding failures depending on the shipped assembly version. Align the newVersion with the actual AssemblyVersion of the System.Text.Json.dll you ship for netfx, and keep the comment consistent with the chosen redirect strategy.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• _vstestConsoleError grows unbounded for the lifetime of the process manager, which can lead to high memory usage on noisy stderr (especially for long-running sessions). Consider bounding this buffer (e.g., keep last N KB/lines) and/or truncating when composing exceptions so diagnostics remain useful without risking memory pressure.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• _vstestConsoleError grows unbounded for the lifetime of the process manager, which can lead to high memory usage on noisy stderr (especially for long-running sessions). Consider bounding this buffer (e.g., keep last N KB/lines) and/or truncating when composing exceptions so diagnostics remain useful without risking memory pressure.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• _vstestConsoleError grows unbounded for the lifetime of the process manager, which can lead to high memory usage on noisy stderr (especially for long-running sessions). Consider bounding this buffer (e.g., keep last N KB/lines) and/or truncating when composing exceptions so diagnostics remain useful without risking memory pressure.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestSessionInfoConverter.cs:1
• Generating a new GUID when Id is missing makes deserialization nondeterministic and can silently change semantics (a missing/invalid Id becomes a brand-new session identity). Prefer preserving the default (Guid.Empty) or failing fast (throw) when Id is missing/invalid, depending on expected contract, to avoid masking malformed payloads.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/ObjectConverter.cs:1
• This converter auto-coerces JSON strings into DateTimeOffset when parseable. That can break consumers that expect the original JSON type to remain a string (and also contradicts the comment about “IConvertible/primitives” — DateTimeOffset is not a primitive and may not be handled the same way downstream). Consider always returning string for JsonTokenType.String (or making DateTime parsing opt-in via options) to avoid surprising type changes in IDictionary<string, object> payloads.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• This test no longer asserts the expected post-deserialization shape of the self-referencing loop (the previous assertion was removed, and the new comment suggests a specific structure). Add a concrete assertion that validates the intended behavior (e.g., result.InfiniteReference is non-null and result.InfiniteReference.InfiniteReference is null, or whatever the correct expected structure is) so the test actually guards the cycle-handling contract.
  test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/EventHandlers/TestRequestHandlerTests.cs:1
• When RawMessage is null, this helper serializes a payload of null and ignores message.Version and any intended payload content. That can produce incorrect wire data for tests that construct Message objects without RawMessage. Consider either (1) requiring RawMessage to be set for all test messages and throwing/asserting when it’s missing, or (2) extending the helper to construct raw JSON using the intended protocol version and payload source(s).

[Copilot]

Pull request overview

Copilot reviewed 157 out of 158 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• src/Microsoft.TestPlatform.CommunicationUtilities/Resources/Resources.Designer.cs: Language not supported

Comments suppressed due to low confidence (9)

src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestSessionInfoConverter.cs:1

• Generating a new GUID when Id is absent makes deserialization nondeterministic and can silently change semantics (e.g., a missing/invalid payload becomes a new session). Prefer Guid.Empty (or leaving the default TestSessionInfo value) and consider treating missing Id as invalid input if the protocol expects it.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• Process_ErrorDataReceived can run on a different thread than callers of ErrorOutput; StringBuilder is not thread-safe. Protect _vstestConsoleError with a lock (or swap to a thread-safe buffer like ConcurrentQueue<string> and join on read) to avoid races/corruption.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• Process_ErrorDataReceived can run on a different thread than callers of ErrorOutput; StringBuilder is not thread-safe. Protect _vstestConsoleError with a lock (or swap to a thread-safe buffer like ConcurrentQueue<string> and join on read) to avoid races/corruption.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• Process_ErrorDataReceived can run on a different thread than callers of ErrorOutput; StringBuilder is not thread-safe. Protect _vstestConsoleError with a lock (or swap to a thread-safe buffer like ConcurrentQueue<string> and join on read) to avoid races/corruption.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleWrapper.cs:1
• Including full stderr (childProcessErrorOutput) in an exception can create very large exception messages and may leak sensitive data into logs/telemetry. Consider truncating to a reasonable size (e.g., last N KB/lines) and/or stripping control characters before formatting.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleWrapper.cs:1
• Including full stderr (childProcessErrorOutput) in an exception can create very large exception messages and may leak sensitive data into logs/telemetry. Consider truncating to a reasonable size (e.g., last N KB/lines) and/or stripping control characters before formatting.
  src/testhost.x86/app.config:1
• The comment states you ship the .NET Framework STJ assembly and therefore need an exact version redirect, but the active redirect uses newVersion=\"10.0.0.0\" (and the exact redirect is commented out). Please align the comment and the actual binding redirect strategy (and ensure newVersion matches the assembly version you ship).
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• This test now documents a specific expected post-deserialization shape for cyclic references, but it no longer asserts it. Please add assertions that validate the new behavior (e.g., result.InfiniteReference exists and result.InfiniteReference.InfiniteReference is null), otherwise regressions in cycle handling won’t be caught.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/VersionCheckSerializationTests.cs:1
• The new serialization test files duplicate the same AssertJsonEqual/Minify helpers (and related docstrings). To reduce maintenance cost and keep behavior consistent across message tests, consider extracting these helpers into a shared internal test utility (e.g., SerializationTestHelpers) and reusing it.

[Copilot]

Pull request overview

Copilot reviewed 157 out of 158 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• src/Microsoft.TestPlatform.CommunicationUtilities/Resources/Resources.Designer.cs: Language not supported

Comments suppressed due to low confidence (8)

src/testhost.x86/app.config:1

• The comment says the netframework build ships the .NET Framework version of System.Text.Json and needs an exact redirect, but the active redirect targets 10.0.0.0 while System.Text.Encodings.Web targets 10.0.0.5. If the shipped System.Text.Json.dll has an assembly version ending in .5, redirecting to .0 can cause binding failures at runtime. Align the System.Text.Json redirect newVersion with the assembly version you actually ship for net462/net48 (and keep this consistent with the Encodings.Web redirect).
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestSessionInfoConverter.cs:1
• Generating a new random Guid when Id is missing makes deserialization non-deterministic and can silently invent a session ID that never existed on the sender side. Prefer a deterministic default (e.g., Guid.Empty) or treat a missing Id as invalid input and fail fast, depending on protocol expectations.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestResultConverterV2.cs:1
• These values are on-the-wire protocol fields and are serialized in an invariant format (e.g., ISO 8601 / constant TimeSpan format). Parsing with CultureInfo.CurrentCulture can break on machines with non-default cultures. Use CultureInfo.InvariantCulture (or DateTimeStyles.RoundtripKind/round-trip parsing patterns) to ensure protocol correctness across locales.
  test/Microsoft.TestPlatform.ObjectModel.UnitTests/Client/DiscoveryCriteriaTests.cs:1
• This test compares raw JSON strings, which is brittle because property ordering/escaping can differ between runtimes and serializers even when the JSON is semantically equivalent. To keep this test stable, compare normalized JSON (parse both to JsonDocument and re-serialize) or compare the deserialized object graph instead of exact string equality.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• The test name/comment indicates a specific expected shape after round-trip (e.g., InfiniteReference not null and its nested reference null), but the assertions no longer validate that behavior. Add targeted assertions so this test actually guards the intended IgnoreCycles contract for both serialization and deserialization.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• Capturing all stderr into an unbounded StringBuilder can lead to high memory usage (or OOM) if the child process is noisy over long runs. Consider capping the stored error output (e.g., keep only the last N KB/lines) since it’s primarily used for diagnostics on startup/connection failures.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• Capturing all stderr into an unbounded StringBuilder can lead to high memory usage (or OOM) if the child process is noisy over long runs. Consider capping the stored error output (e.g., keep only the last N KB/lines) since it’s primarily used for diagnostics on startup/connection failures.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Microsoft.TestPlatform.CommunicationUtilities.UnitTests.csproj:1
• This introduces a hard-coded Newtonsoft.Json version in a repo that already centralizes versions via properties (e.g., $(NewtonsoftJsonVersion)). Consider switching to the shared property for consistency and easier upgrades.

[Copilot]

Pull request overview

Copilot reviewed 157 out of 158 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• src/Microsoft.TestPlatform.CommunicationUtilities/Resources/Resources.Designer.cs: Language not supported

Comments suppressed due to low confidence (9)

src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleWrapper.cs:1

• The thrown message always uses ConnectionTimeoutWithErrorMessage (“process … exited with exitCode …”), but timeouts can occur while the child process is still running (ExitCode remains -1). Consider choosing between ConnectionTimeoutErrorMessage (still running) vs ConnectionTimeoutWithErrorMessage (exited) based on actual process state/exit, and only include exit-code/error-output details when the process has exited or there is captured stderr.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• StringBuilder is not thread-safe; Process.ErrorDataReceived can be raised on background threads while other code reads ErrorOutput (which calls _vstestConsoleError.ToString()). Add synchronization (e.g., a private lock around append/read) or store stderr lines in a thread-safe collection to avoid data races and potential StringBuilder state corruption.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• StringBuilder is not thread-safe; Process.ErrorDataReceived can be raised on background threads while other code reads ErrorOutput (which calls _vstestConsoleError.ToString()). Add synchronization (e.g., a private lock around append/read) or store stderr lines in a thread-safe collection to avoid data races and potential StringBuilder state corruption.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• This test’s assertions now depend on an external environment variable, making the test outcome non-deterministic across machines/CI agents. Prefer setting/unsetting the variable inside the test (and restoring it), or better: drive the serializer selection via an injectable/configurable option in the test so both paths can be tested deterministically.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• The comment states an expected shape for cyclic references under STJ (InfiniteReference.InfiniteReference = null), but the test no longer asserts that behavior. Add an assertion that validates the expected post-deserialization object graph so regressions in reference-loop handling are caught.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• The comment states an expected shape for cyclic references under STJ (InfiniteReference.InfiniteReference = null), but the test no longer asserts that behavior. Add an assertion that validates the expected post-deserialization object graph so regressions in reference-loop handling are caught.
  test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/EventHandlers/TestRequestHandlerTests.cs:1
• Falling back to SerializePayload(message.MessageType, null) changes semantics vs the previous SerializePayload(message.MessageType, message.Payload) and can emit "Payload":null when the intended wire format is a header-only message (or a message with a real payload). Consider using a dedicated “serialize message without payload” API (if available) or ensure tests always provide RawMessage for messages that require payload, and include the correct Version in the envelope when serializing.
  test/Microsoft.TestPlatform.TestUtilities/IntegrationTestBuild.cs:1
• If cacheId is a dictionary-based object graph, STJ property ordering can differ (across runtimes/frameworks or due to dictionary enumeration), potentially causing nondeterministic checksum.json content and unnecessary rebuilds. Consider producing a canonical JSON (e.g., write properties in sorted order or use a sorted dictionary/tree) before comparing/writing the checksum.
  src/testhost.x86/app.config:1
• The comment says the testhost ships the .NET Framework STJ assembly and needs a redirect to the exact version, but the active redirect targets 10.0.0.0 while the “exact version” redirect (10.0.0.5) is commented out. Either update the comment to match the intended “downgrade to major-only version” behavior, or change the redirect to the exact assembly version you actually ship to avoid confusing future maintenance and potential load issues.

[Copilot]

Pull request overview

Copilot reviewed 156 out of 157 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• src/Microsoft.TestPlatform.CommunicationUtilities/Resources/Resources.Designer.cs: Language not supported

Comments suppressed due to low confidence (8)

src/testhost.x86/app.config:1

• The comment says .NET Framework needs an exact-version redirect, but the active bindingRedirect redirects to 10.0.0.0 while the commented line suggests 10.0.0.5. If the shipped System.Text.Json.dll has assembly version 10.0.0.5, redirecting to 10.0.0.0 can cause load failures. Align newVersion with the actual shipped assembly version (and keep the comment consistent with the chosen approach).
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• Process.ErrorDataReceived callbacks can arrive on background threads; appending to a shared StringBuilder without synchronization is not thread-safe. Protect _vstestConsoleError with a lock (or switch to a thread-safe buffer such as ConcurrentQueue<string> and join on read) to avoid memory corruption/races.
  src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/VsTestConsoleProcessManager.cs:1
• Process.ErrorDataReceived callbacks can arrive on background threads; appending to a shared StringBuilder without synchronization is not thread-safe. Protect _vstestConsoleError with a lock (or switch to a thread-safe buffer such as ConcurrentQueue<string> and join on read) to avoid memory corruption/races.
  src/Microsoft.TestPlatform.CommunicationUtilities/Serialization/TestResultConverterV2.cs:1
• Parsing wire-format Duration/StartTime/EndTime using CultureInfo.CurrentCulture makes deserialization culture-dependent and can break on machines with non-invariant cultures. Use CultureInfo.InvariantCulture (and for DateTimeOffset, prefer round-trip parsing such as DateTimeStyles.RoundtripKind) to reliably parse protocol JSON.
  src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs:1
• x.GetString()! will throw or produce an ArgumentNullException in Path.Combine if an element is null/non-string in additionalProbingPaths. Prefer reading the string once into a local, and continue when it’s null/empty, to avoid crashing when the runtimeconfig contains unexpected values.
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/JsonDataSerializerTests.cs:1
• This test documents a new expected behavior (cycle becomes null), but it no longer asserts it. Add an assertion that verifies the expected structure (e.g., result.InfiniteReference is not null and result.InfiniteReference.InfiniteReference is null), so the behavior change is actually guarded by the test.
  src/Microsoft.TestPlatform.CommunicationUtilities/Resources/xlf/Resources.zh-Hans.xlf:1
• The new ConnectionTimeoutWithErrorMessage resource is added with <target state=\"new\"> containing the English source text. If this ships, localized builds will show English for this string. Either provide translations (or mark it as untranslated intentionally and ensure that’s acceptable for the release).
  test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/Serialization/VersionCheckSerializationTests.cs:1
• AssertJsonEqual/Minify helpers are duplicated across many of the new wire-format test files in this PR. Consider centralizing them in a shared test helper (e.g., a SerializationTestUtils) to reduce duplication and ensure consistent normalization behavior across message tests.

[nohwnd]

Wire Format Verification: main (Newtonsoft) vs STJ branch

Ran blame tests with /Diag on both main (Newtonsoft.Json) and stj-migration (System.Text.Json / Jsonite) branches. Captured all JSON messages from runner, testhost, and datacollector diag logs, then compared them.

Runner (vstest.console) — 8 message types

Message Type	Result
ProtocolVersion (×2)	✅ Identical
TestExecution.Initialize	✅ Identical
TestExecution.StartWithSources	✅ Identical
TestExecution.Completed	✅ Identical
TestSession.Message	✅ Identical
DataCollection.BeforeTestRunStartResult	✅ Identical
DataCollection.AfterTestRunEndResult	✅ Identical
DataCollection.SendMessage	✅ Identical

Testhost — 4 message types

Message Type	Result
ProtocolVersion	✅ Identical
TestExecution.Initialize	✅ Identical
TestExecution.StartWithSources	✅ Identical
TestExecution.Completed	✅ Same data — TestCase Properties array has same set of property IDs but in different iteration order (dictionary iteration order, semantically irrelevant — deserialization looks up by Key.Id not position)
TestSession.Message	✅ Identical
TestSession.Terminate	✅ Identical

Datacollector — 3 shared + 7 new

Message Type	Result
DataCollection.BeforeTestRunStart	✅ Identical
DataCollection.TestHostLaunched	✅ Identical
DataCollection.AfterTestRunEnd	✅ Identical
DataCollection.TestStart (×3)	STJ-only (new logging)
DataCollection.TestEnd (×3)	STJ-only (new logging)
TestSession.Terminate	STJ-only (new logging)

Property ordering detail

TestExecution.Completed → LastRunTests.NewTestResults[*].TestCase.Properties — the Properties array contains the same property IDs and values on both branches, just in different order. Example for PassingTest:

Main: Hierarchy, ManagedType, ..., ParameterTypes, Traits, ManagedMethod
STJ:  ManagedType, ManagedMethod, ..., Traits, Hierarchy, ParameterTypes

Same set, different dictionary iteration order. This is expected and does not affect deserialization since properties are identified by Key.Id.

Conclusion

Zero structural or semantic wire format differences. The STJ migration produces identical JSON to Newtonsoft on the wire.

[nohwnd]

Already built on main, we changed base.