Deduplicate DotnetTest IPC serializer 3-phase boilerplate

[Amaury Levé (Evangelink)]

Summary

Deduplicates the 3-phase read/write wire-protocol boilerplate that was copy-pasted across the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/.

Two helpers are added to the shared BaseSerializer and every serializer is refactored to use them:

• ReadFields(Stream, Func<ushort, int, bool>) — encapsulates the field-count loop that reads each [id][size][payload] triple and skips unrecognized fields (the forward-compatibility fallback), replacing the identical loop in every DeserializeCore and per-list-item reader.
• WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) — encapsulates the deferred-size-backfill list protocol (reserve size slot, write count, write items, patch size), replacing the copy-pasted before/WriteAtPosition sequence.

Refactored serializers: AzureDevOpsLogMessage, TestSessionEvent, DisplayMessage, CommandLineOptionMessages, TestInProgressMessages, FileArtifactMessages, DiscoveredTestMessages, TestResultMessages.

Net change: +237 / −465 lines.

Wire compatibility

No wire-format change. Per-field write order and the skip-unknown/backfill semantics are preserved verbatim (including DisplayMessage's Level size-honoring read and DiscoveredTestMessages' required trait key/value validation).

Validation

• Microsoft.Testing.Platform + the 4 shared-source extensions (HangDump, Retry, TrxReport, MSBuild) build with 0 warnings / 0 errors.
• DotnetTestProtocolContract.UnitTests round-trip tests: 63/63 pass (net462/net8.0/net9.0).
• IPC Protocol tests in Microsoft.Testing.Platform.UnitTests: 33/33 pass.

Fixes#9764

[Copilot]

Pull request overview

This PR is a pure maintainability refactor that removes the copy‑pasted 3‑phase wire‑protocol boilerplate shared by the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/. Two protected static helpers are added to the shared BaseSerializer and every serializer is rewritten to call them, cutting ~228 net lines while keeping the on‑the‑wire format byte‑for‑byte identical.

Changes:

• Adds ReadFields(Stream, Func<ushort, int, bool>) to encapsulate the field-count read loop plus the skip‑unknown forward‑compatibility fallback.
• Adds WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) to encapsulate the deferred‑size‑backfill list protocol.
• Refactors all 8 serializers to use the helpers, preserving special cases (DisplayMessage.Level size‑honoring read, DiscoveredTestMessages trait key/value validation, field write order).

Show a summary per file

File	Description
IPC/Serializers/BaseSerializer.cs	Adds the ReadFields and WriteListPayload<T> helper methods with XML docs.
.../Serializers/AzureDevOpsLogMessageSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/TestSessionEventSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/DisplayMessageSerializer.cs	Deserialize loop replaced with ReadFields; Level size‑honoring read preserved.
.../Serializers/CommandLineOptionMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/TestInProgressMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/FileArtifactMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/DiscoveredTestMessagesSerializer.cs	Deserialize loops + 3 list writes replaced; trait validation preserved.
.../Serializers/TestResultMessagesSerializer.cs	Deserialize loops + 3 list writes replaced with helpers.

Notes: The refactor is behavior‑preserving — field‑id constants are all ushort (matching the new lambda parameter), the inheritance chain (NamedPipeSerializer<T> : BaseSerializer) exposes the helpers, and null/empty list and skip‑unknown semantics are unchanged. The single observation is a minor, optional performance tradeoff: the per‑element read loops now allocate a capturing delegate/closure per element. This is a serialization/wire‑protocol refactor (the field‑id file is explicitly kept aligned with the dotnet SDK), so it warrants human confirmation despite the passing round‑trip tests.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

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 — LGTM ✅

This is a clean, mechanical refactoring that successfully deduplicates the 3-phase read/write boilerplate across 8 DotnetTest IPC serializers. All dimensions reviewed came back clean.

Dimensions Evaluated

Dimension	Verdict	Notes
IPC Wire Compatibility	✅ LGTM	WriteListPayload produces the identical [fieldId][size][count][items] layout. ReadFields skip logic (stream.Position + fieldSize) is wire-compatible with the backfilled size (stream.Position - before). Verified read/write symmetry.
Algorithmic Correctness	✅ LGTM	Each serializer's field dispatch is mechanically preserved within the lambda. Nested ReadFields/WriteListPayload calls (e.g., traits inside discovered tests) compose correctly — outer backfill captures full nested content.
Threading & Concurrency	✅ LGTM	All methods are protected static, operate on a Stream parameter, and lambdas capture only method-local variables. No shared mutable state.
Security & IPC Contract Safety	✅ LGTM	No new deserialization paths. The skip-unknown-field behavior (forward compatibility) is preserved verbatim.
Public API & Binary Compatibility	✅ LGTM	BaseSerializer is internal abstract with [Embedded]. New protected static helpers are not public API — no PublicAPI.Unshipped.txt update needed.
Performance & Allocations	✅ LGTM	Read-side lambdas capture locals (unavoidable for writing to multiple out variables), allocating one closure + delegate per ReadFields call. This is negligible for per-message IPC parsing. Write-side callbacks are all static lambdas — zero allocation.
Cross-TFM Compatibility	✅ LGTM	Uses Func<ushort, int, bool> and Action<Stream, T> available on all target TFMs (net462+).
Code Structure	✅ LGTM	Clean deduplication. Good XML doc comments on both helpers.
Scope & PR Discipline	✅ LGTM	Pure refactoring, no behavioral changes mixed in.
Test Completeness	✅ LGTM	Existing round-trip tests (63/63 + 33/33) exercise all callers. Adequate for a behavior-preserving refactoring.

Skipped (not applicable)

• Analyzer Quality (no src/Analyzers/ changes)
• Test Isolation / Assertion Quality / Flakiness / Data-Driven (no test files changed)
• Build Infrastructure (no .props/.targets changes)
• PowerShell Scripting (no .ps1 changes)

Design Note (informational, non-blocking)

The ReadFields helper's Func<ushort, int, bool> signature elegantly communicates intent: return true = "I consumed this field", false = "skip it for me". The two-parameter design (fieldId, fieldSize) is forward-thinking — callers that use ReadStringValue(stream, fieldSize) benefit from having the size available without re-reading it, while fixed-size field readers (like ReadByte) simply ignore it.

Well-structured refactoring. 👍

[Amaury Levé (Evangelink)]

Summary

Deduplicates the 3-phase read/write wire-protocol boilerplate that was copy-pasted across the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/.

Two helpers are added to the shared BaseSerializer and every serializer is refactored to use them:

• ReadFields(Stream, Func<ushort, int, bool>) — encapsulates the field-count loop that reads each [id][size][payload] triple and skips unrecognized fields (the forward-compatibility fallback), replacing the identical loop in every DeserializeCore and per-list-item reader.
• WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) — encapsulates the deferred-size-backfill list protocol (reserve size slot, write count, write items, patch size), replacing the copy-pasted before/WriteAtPosition sequence.

Refactored serializers: AzureDevOpsLogMessage, TestSessionEvent, DisplayMessage, CommandLineOptionMessages, TestInProgressMessages, FileArtifactMessages, DiscoveredTestMessages, TestResultMessages.

Net change: +237 / −465 lines.

Wire compatibility

No wire-format change. Per-field write order and the skip-unknown/backfill semantics are preserved verbatim (including DisplayMessage's Level size-honoring read and DiscoveredTestMessages' required trait key/value validation).

Validation

• Microsoft.Testing.Platform + the 4 shared-source extensions (HangDump, Retry, TrxReport, MSBuild) build with 0 warnings / 0 errors.
• DotnetTestProtocolContract.UnitTests round-trip tests: 63/63 pass (net462/net8.0/net9.0).
• IPC Protocol tests in Microsoft.Testing.Platform.UnitTests: 33/33 pass.

Fixes#9764

[Copilot]

Pull request overview

This PR is a pure maintainability refactor that removes the copy‑pasted 3‑phase wire‑protocol boilerplate shared by the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/. Two protected static helpers are added to the shared BaseSerializer and every serializer is rewritten to call them, cutting ~228 net lines while keeping the on‑the‑wire format byte‑for‑byte identical.

Changes:

• Adds ReadFields(Stream, Func<ushort, int, bool>) to encapsulate the field-count read loop plus the skip‑unknown forward‑compatibility fallback.
• Adds WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) to encapsulate the deferred‑size‑backfill list protocol.
• Refactors all 8 serializers to use the helpers, preserving special cases (DisplayMessage.Level size‑honoring read, DiscoveredTestMessages trait key/value validation, field write order).

Show a summary per file

File	Description
IPC/Serializers/BaseSerializer.cs	Adds the ReadFields and WriteListPayload<T> helper methods with XML docs.
.../Serializers/AzureDevOpsLogMessageSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/TestSessionEventSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/DisplayMessageSerializer.cs	Deserialize loop replaced with ReadFields; Level size‑honoring read preserved.
.../Serializers/CommandLineOptionMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/TestInProgressMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/FileArtifactMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/DiscoveredTestMessagesSerializer.cs	Deserialize loops + 3 list writes replaced; trait validation preserved.
.../Serializers/TestResultMessagesSerializer.cs	Deserialize loops + 3 list writes replaced with helpers.

Notes: The refactor is behavior‑preserving — field‑id constants are all ushort (matching the new lambda parameter), the inheritance chain (NamedPipeSerializer<T> : BaseSerializer) exposes the helpers, and null/empty list and skip‑unknown semantics are unchanged. The single observation is a minor, optional performance tradeoff: the per‑element read loops now allocate a capturing delegate/closure per element. This is a serialization/wire‑protocol refactor (the field‑id file is explicitly kept aligned with the dotnet SDK), so it warrants human confirmation despite the passing round‑trip tests.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

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 — LGTM ✅

This is a clean, mechanical refactoring that successfully deduplicates the 3-phase read/write boilerplate across 8 DotnetTest IPC serializers. All dimensions reviewed came back clean.

Dimensions Evaluated

Dimension	Verdict	Notes
IPC Wire Compatibility	✅ LGTM	WriteListPayload produces the identical [fieldId][size][count][items] layout. ReadFields skip logic (stream.Position + fieldSize) is wire-compatible with the backfilled size (stream.Position - before). Verified read/write symmetry.
Algorithmic Correctness	✅ LGTM	Each serializer's field dispatch is mechanically preserved within the lambda. Nested ReadFields/WriteListPayload calls (e.g., traits inside discovered tests) compose correctly — outer backfill captures full nested content.
Threading & Concurrency	✅ LGTM	All methods are protected static, operate on a Stream parameter, and lambdas capture only method-local variables. No shared mutable state.
Security & IPC Contract Safety	✅ LGTM	No new deserialization paths. The skip-unknown-field behavior (forward compatibility) is preserved verbatim.
Public API & Binary Compatibility	✅ LGTM	BaseSerializer is internal abstract with [Embedded]. New protected static helpers are not public API — no PublicAPI.Unshipped.txt update needed.
Performance & Allocations	✅ LGTM	Read-side lambdas capture locals (unavoidable for writing to multiple out variables), allocating one closure + delegate per ReadFields call. This is negligible for per-message IPC parsing. Write-side callbacks are all static lambdas — zero allocation.
Cross-TFM Compatibility	✅ LGTM	Uses Func<ushort, int, bool> and Action<Stream, T> available on all target TFMs (net462+).
Code Structure	✅ LGTM	Clean deduplication. Good XML doc comments on both helpers.
Scope & PR Discipline	✅ LGTM	Pure refactoring, no behavioral changes mixed in.
Test Completeness	✅ LGTM	Existing round-trip tests (63/63 + 33/33) exercise all callers. Adequate for a behavior-preserving refactoring.

Skipped (not applicable)

• Analyzer Quality (no src/Analyzers/ changes)
• Test Isolation / Assertion Quality / Flakiness / Data-Driven (no test files changed)
• Build Infrastructure (no .props/.targets changes)
• PowerShell Scripting (no .ps1 changes)

Design Note (informational, non-blocking)

The ReadFields helper's Func<ushort, int, bool> signature elegantly communicates intent: return true = "I consumed this field", false = "skip it for me". The two-parameter design (fieldId, fieldSize) is forward-thinking — callers that use ReadStringValue(stream, fieldSize) benefit from having the size available without re-reading it, while fixed-size field readers (like ReadByte) simply ignore it.

Well-structured refactoring. 👍

[Amaury Levé (Evangelink)]

Summary

Deduplicates the 3-phase read/write wire-protocol boilerplate that was copy-pasted across the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/.

Two helpers are added to the shared BaseSerializer and every serializer is refactored to use them:

• ReadFields(Stream, Func<ushort, int, bool>) — encapsulates the field-count loop that reads each [id][size][payload] triple and skips unrecognized fields (the forward-compatibility fallback), replacing the identical loop in every DeserializeCore and per-list-item reader.
• WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) — encapsulates the deferred-size-backfill list protocol (reserve size slot, write count, write items, patch size), replacing the copy-pasted before/WriteAtPosition sequence.

Refactored serializers: AzureDevOpsLogMessage, TestSessionEvent, DisplayMessage, CommandLineOptionMessages, TestInProgressMessages, FileArtifactMessages, DiscoveredTestMessages, TestResultMessages.

Net change: +237 / −465 lines.

Wire compatibility

No wire-format change. Per-field write order and the skip-unknown/backfill semantics are preserved verbatim (including DisplayMessage's Level size-honoring read and DiscoveredTestMessages' required trait key/value validation).

Validation

• Microsoft.Testing.Platform + the 4 shared-source extensions (HangDump, Retry, TrxReport, MSBuild) build with 0 warnings / 0 errors.
• DotnetTestProtocolContract.UnitTests round-trip tests: 63/63 pass (net462/net8.0/net9.0).
• IPC Protocol tests in Microsoft.Testing.Platform.UnitTests: 33/33 pass.

Fixes#9764

[Copilot]

Pull request overview

This PR is a pure maintainability refactor that removes the copy‑pasted 3‑phase wire‑protocol boilerplate shared by the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/. Two protected static helpers are added to the shared BaseSerializer and every serializer is rewritten to call them, cutting ~228 net lines while keeping the on‑the‑wire format byte‑for‑byte identical.

Changes:

• Adds ReadFields(Stream, Func<ushort, int, bool>) to encapsulate the field-count read loop plus the skip‑unknown forward‑compatibility fallback.
• Adds WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) to encapsulate the deferred‑size‑backfill list protocol.
• Refactors all 8 serializers to use the helpers, preserving special cases (DisplayMessage.Level size‑honoring read, DiscoveredTestMessages trait key/value validation, field write order).

Show a summary per file

File	Description
IPC/Serializers/BaseSerializer.cs	Adds the ReadFields and WriteListPayload<T> helper methods with XML docs.
.../Serializers/AzureDevOpsLogMessageSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/TestSessionEventSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/DisplayMessageSerializer.cs	Deserialize loop replaced with ReadFields; Level size‑honoring read preserved.
.../Serializers/CommandLineOptionMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/TestInProgressMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/FileArtifactMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/DiscoveredTestMessagesSerializer.cs	Deserialize loops + 3 list writes replaced; trait validation preserved.
.../Serializers/TestResultMessagesSerializer.cs	Deserialize loops + 3 list writes replaced with helpers.

Notes: The refactor is behavior‑preserving — field‑id constants are all ushort (matching the new lambda parameter), the inheritance chain (NamedPipeSerializer<T> : BaseSerializer) exposes the helpers, and null/empty list and skip‑unknown semantics are unchanged. The single observation is a minor, optional performance tradeoff: the per‑element read loops now allocate a capturing delegate/closure per element. This is a serialization/wire‑protocol refactor (the field‑id file is explicitly kept aligned with the dotnet SDK), so it warrants human confirmation despite the passing round‑trip tests.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

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 — LGTM ✅

This is a clean, mechanical refactoring that successfully deduplicates the 3-phase read/write boilerplate across 8 DotnetTest IPC serializers. All dimensions reviewed came back clean.

Dimensions Evaluated

Dimension	Verdict	Notes
IPC Wire Compatibility	✅ LGTM	WriteListPayload produces the identical [fieldId][size][count][items] layout. ReadFields skip logic (stream.Position + fieldSize) is wire-compatible with the backfilled size (stream.Position - before). Verified read/write symmetry.
Algorithmic Correctness	✅ LGTM	Each serializer's field dispatch is mechanically preserved within the lambda. Nested ReadFields/WriteListPayload calls (e.g., traits inside discovered tests) compose correctly — outer backfill captures full nested content.
Threading & Concurrency	✅ LGTM	All methods are protected static, operate on a Stream parameter, and lambdas capture only method-local variables. No shared mutable state.
Security & IPC Contract Safety	✅ LGTM	No new deserialization paths. The skip-unknown-field behavior (forward compatibility) is preserved verbatim.
Public API & Binary Compatibility	✅ LGTM	BaseSerializer is internal abstract with [Embedded]. New protected static helpers are not public API — no PublicAPI.Unshipped.txt update needed.
Performance & Allocations	✅ LGTM	Read-side lambdas capture locals (unavoidable for writing to multiple out variables), allocating one closure + delegate per ReadFields call. This is negligible for per-message IPC parsing. Write-side callbacks are all static lambdas — zero allocation.
Cross-TFM Compatibility	✅ LGTM	Uses Func<ushort, int, bool> and Action<Stream, T> available on all target TFMs (net462+).
Code Structure	✅ LGTM	Clean deduplication. Good XML doc comments on both helpers.
Scope & PR Discipline	✅ LGTM	Pure refactoring, no behavioral changes mixed in.
Test Completeness	✅ LGTM	Existing round-trip tests (63/63 + 33/33) exercise all callers. Adequate for a behavior-preserving refactoring.

Skipped (not applicable)

• Analyzer Quality (no src/Analyzers/ changes)
• Test Isolation / Assertion Quality / Flakiness / Data-Driven (no test files changed)
• Build Infrastructure (no .props/.targets changes)
• PowerShell Scripting (no .ps1 changes)

Design Note (informational, non-blocking)

The ReadFields helper's Func<ushort, int, bool> signature elegantly communicates intent: return true = "I consumed this field", false = "skip it for me". The two-parameter design (fieldId, fieldSize) is forward-thinking — callers that use ReadStringValue(stream, fieldSize) benefit from having the size available without re-reading it, while fixed-size field readers (like ReadByte) simply ignore it.

Well-structured refactoring. 👍

[Amaury Levé (Evangelink)]

Summary

Deduplicates the 3-phase read/write wire-protocol boilerplate that was copy-pasted across the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/.

Two helpers are added to the shared BaseSerializer and every serializer is refactored to use them:

• ReadFields(Stream, Func<ushort, int, bool>) — encapsulates the field-count loop that reads each [id][size][payload] triple and skips unrecognized fields (the forward-compatibility fallback), replacing the identical loop in every DeserializeCore and per-list-item reader.
• WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) — encapsulates the deferred-size-backfill list protocol (reserve size slot, write count, write items, patch size), replacing the copy-pasted before/WriteAtPosition sequence.

Refactored serializers: AzureDevOpsLogMessage, TestSessionEvent, DisplayMessage, CommandLineOptionMessages, TestInProgressMessages, FileArtifactMessages, DiscoveredTestMessages, TestResultMessages.

Net change: +237 / −465 lines.

Wire compatibility

No wire-format change. Per-field write order and the skip-unknown/backfill semantics are preserved verbatim (including DisplayMessage's Level size-honoring read and DiscoveredTestMessages' required trait key/value validation).

Validation

• Microsoft.Testing.Platform + the 4 shared-source extensions (HangDump, Retry, TrxReport, MSBuild) build with 0 warnings / 0 errors.
• DotnetTestProtocolContract.UnitTests round-trip tests: 63/63 pass (net462/net8.0/net9.0).
• IPC Protocol tests in Microsoft.Testing.Platform.UnitTests: 33/33 pass.

Fixes#9764

[Copilot]

Pull request overview

This PR is a pure maintainability refactor that removes the copy‑pasted 3‑phase wire‑protocol boilerplate shared by the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/. Two protected static helpers are added to the shared BaseSerializer and every serializer is rewritten to call them, cutting ~228 net lines while keeping the on‑the‑wire format byte‑for‑byte identical.

Changes:

• Adds ReadFields(Stream, Func<ushort, int, bool>) to encapsulate the field-count read loop plus the skip‑unknown forward‑compatibility fallback.
• Adds WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) to encapsulate the deferred‑size‑backfill list protocol.
• Refactors all 8 serializers to use the helpers, preserving special cases (DisplayMessage.Level size‑honoring read, DiscoveredTestMessages trait key/value validation, field write order).

Show a summary per file

File	Description
IPC/Serializers/BaseSerializer.cs	Adds the ReadFields and WriteListPayload<T> helper methods with XML docs.
.../Serializers/AzureDevOpsLogMessageSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/TestSessionEventSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/DisplayMessageSerializer.cs	Deserialize loop replaced with ReadFields; Level size‑honoring read preserved.
.../Serializers/CommandLineOptionMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/TestInProgressMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/FileArtifactMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/DiscoveredTestMessagesSerializer.cs	Deserialize loops + 3 list writes replaced; trait validation preserved.
.../Serializers/TestResultMessagesSerializer.cs	Deserialize loops + 3 list writes replaced with helpers.

Notes: The refactor is behavior‑preserving — field‑id constants are all ushort (matching the new lambda parameter), the inheritance chain (NamedPipeSerializer<T> : BaseSerializer) exposes the helpers, and null/empty list and skip‑unknown semantics are unchanged. The single observation is a minor, optional performance tradeoff: the per‑element read loops now allocate a capturing delegate/closure per element. This is a serialization/wire‑protocol refactor (the field‑id file is explicitly kept aligned with the dotnet SDK), so it warrants human confirmation despite the passing round‑trip tests.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

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 — LGTM ✅

This is a clean, mechanical refactoring that successfully deduplicates the 3-phase read/write boilerplate across 8 DotnetTest IPC serializers. All dimensions reviewed came back clean.

Dimensions Evaluated

Dimension	Verdict	Notes
IPC Wire Compatibility	✅ LGTM	WriteListPayload produces the identical [fieldId][size][count][items] layout. ReadFields skip logic (stream.Position + fieldSize) is wire-compatible with the backfilled size (stream.Position - before). Verified read/write symmetry.
Algorithmic Correctness	✅ LGTM	Each serializer's field dispatch is mechanically preserved within the lambda. Nested ReadFields/WriteListPayload calls (e.g., traits inside discovered tests) compose correctly — outer backfill captures full nested content.
Threading & Concurrency	✅ LGTM	All methods are protected static, operate on a Stream parameter, and lambdas capture only method-local variables. No shared mutable state.
Security & IPC Contract Safety	✅ LGTM	No new deserialization paths. The skip-unknown-field behavior (forward compatibility) is preserved verbatim.
Public API & Binary Compatibility	✅ LGTM	BaseSerializer is internal abstract with [Embedded]. New protected static helpers are not public API — no PublicAPI.Unshipped.txt update needed.
Performance & Allocations	✅ LGTM	Read-side lambdas capture locals (unavoidable for writing to multiple out variables), allocating one closure + delegate per ReadFields call. This is negligible for per-message IPC parsing. Write-side callbacks are all static lambdas — zero allocation.
Cross-TFM Compatibility	✅ LGTM	Uses Func<ushort, int, bool> and Action<Stream, T> available on all target TFMs (net462+).
Code Structure	✅ LGTM	Clean deduplication. Good XML doc comments on both helpers.
Scope & PR Discipline	✅ LGTM	Pure refactoring, no behavioral changes mixed in.
Test Completeness	✅ LGTM	Existing round-trip tests (63/63 + 33/33) exercise all callers. Adequate for a behavior-preserving refactoring.

Skipped (not applicable)

• Analyzer Quality (no src/Analyzers/ changes)
• Test Isolation / Assertion Quality / Flakiness / Data-Driven (no test files changed)
• Build Infrastructure (no .props/.targets changes)
• PowerShell Scripting (no .ps1 changes)

Design Note (informational, non-blocking)

The ReadFields helper's Func<ushort, int, bool> signature elegantly communicates intent: return true = "I consumed this field", false = "skip it for me". The two-parameter design (fieldId, fieldSize) is forward-thinking — callers that use ReadStringValue(stream, fieldSize) benefit from having the size available without re-reading it, while fixed-size field readers (like ReadByte) simply ignore it.

Well-structured refactoring. 👍

[Amaury Levé (Evangelink)]

Summary

Deduplicates the 3-phase read/write wire-protocol boilerplate that was copy-pasted across the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/.

Two helpers are added to the shared BaseSerializer and every serializer is refactored to use them:

• ReadFields(Stream, Func<ushort, int, bool>) — encapsulates the field-count loop that reads each [id][size][payload] triple and skips unrecognized fields (the forward-compatibility fallback), replacing the identical loop in every DeserializeCore and per-list-item reader.
• WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) — encapsulates the deferred-size-backfill list protocol (reserve size slot, write count, write items, patch size), replacing the copy-pasted before/WriteAtPosition sequence.

Refactored serializers: AzureDevOpsLogMessage, TestSessionEvent, DisplayMessage, CommandLineOptionMessages, TestInProgressMessages, FileArtifactMessages, DiscoveredTestMessages, TestResultMessages.

Net change: +237 / −465 lines.

Wire compatibility

No wire-format change. Per-field write order and the skip-unknown/backfill semantics are preserved verbatim (including DisplayMessage's Level size-honoring read and DiscoveredTestMessages' required trait key/value validation).

Validation

• Microsoft.Testing.Platform + the 4 shared-source extensions (HangDump, Retry, TrxReport, MSBuild) build with 0 warnings / 0 errors.
• DotnetTestProtocolContract.UnitTests round-trip tests: 63/63 pass (net462/net8.0/net9.0).
• IPC Protocol tests in Microsoft.Testing.Platform.UnitTests: 33/33 pass.

Fixes#9764

[Copilot]

Pull request overview

This PR is a pure maintainability refactor that removes the copy‑pasted 3‑phase wire‑protocol boilerplate shared by the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/. Two protected static helpers are added to the shared BaseSerializer and every serializer is rewritten to call them, cutting ~228 net lines while keeping the on‑the‑wire format byte‑for‑byte identical.

Changes:

• Adds ReadFields(Stream, Func<ushort, int, bool>) to encapsulate the field-count read loop plus the skip‑unknown forward‑compatibility fallback.
• Adds WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) to encapsulate the deferred‑size‑backfill list protocol.
• Refactors all 8 serializers to use the helpers, preserving special cases (DisplayMessage.Level size‑honoring read, DiscoveredTestMessages trait key/value validation, field write order).

Show a summary per file

File	Description
IPC/Serializers/BaseSerializer.cs	Adds the ReadFields and WriteListPayload<T> helper methods with XML docs.
.../Serializers/AzureDevOpsLogMessageSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/TestSessionEventSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/DisplayMessageSerializer.cs	Deserialize loop replaced with ReadFields; Level size‑honoring read preserved.
.../Serializers/CommandLineOptionMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/TestInProgressMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/FileArtifactMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/DiscoveredTestMessagesSerializer.cs	Deserialize loops + 3 list writes replaced; trait validation preserved.
.../Serializers/TestResultMessagesSerializer.cs	Deserialize loops + 3 list writes replaced with helpers.

Notes: The refactor is behavior‑preserving — field‑id constants are all ushort (matching the new lambda parameter), the inheritance chain (NamedPipeSerializer<T> : BaseSerializer) exposes the helpers, and null/empty list and skip‑unknown semantics are unchanged. The single observation is a minor, optional performance tradeoff: the per‑element read loops now allocate a capturing delegate/closure per element. This is a serialization/wire‑protocol refactor (the field‑id file is explicitly kept aligned with the dotnet SDK), so it warrants human confirmation despite the passing round‑trip tests.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

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 — LGTM ✅

This is a clean, mechanical refactoring that successfully deduplicates the 3-phase read/write boilerplate across 8 DotnetTest IPC serializers. All dimensions reviewed came back clean.

Dimensions Evaluated

Dimension	Verdict	Notes
IPC Wire Compatibility	✅ LGTM	WriteListPayload produces the identical [fieldId][size][count][items] layout. ReadFields skip logic (stream.Position + fieldSize) is wire-compatible with the backfilled size (stream.Position - before). Verified read/write symmetry.
Algorithmic Correctness	✅ LGTM	Each serializer's field dispatch is mechanically preserved within the lambda. Nested ReadFields/WriteListPayload calls (e.g., traits inside discovered tests) compose correctly — outer backfill captures full nested content.
Threading & Concurrency	✅ LGTM	All methods are protected static, operate on a Stream parameter, and lambdas capture only method-local variables. No shared mutable state.
Security & IPC Contract Safety	✅ LGTM	No new deserialization paths. The skip-unknown-field behavior (forward compatibility) is preserved verbatim.
Public API & Binary Compatibility	✅ LGTM	BaseSerializer is internal abstract with [Embedded]. New protected static helpers are not public API — no PublicAPI.Unshipped.txt update needed.
Performance & Allocations	✅ LGTM	Read-side lambdas capture locals (unavoidable for writing to multiple out variables), allocating one closure + delegate per ReadFields call. This is negligible for per-message IPC parsing. Write-side callbacks are all static lambdas — zero allocation.
Cross-TFM Compatibility	✅ LGTM	Uses Func<ushort, int, bool> and Action<Stream, T> available on all target TFMs (net462+).
Code Structure	✅ LGTM	Clean deduplication. Good XML doc comments on both helpers.
Scope & PR Discipline	✅ LGTM	Pure refactoring, no behavioral changes mixed in.
Test Completeness	✅ LGTM	Existing round-trip tests (63/63 + 33/33) exercise all callers. Adequate for a behavior-preserving refactoring.

Skipped (not applicable)

• Analyzer Quality (no src/Analyzers/ changes)
• Test Isolation / Assertion Quality / Flakiness / Data-Driven (no test files changed)
• Build Infrastructure (no .props/.targets changes)
• PowerShell Scripting (no .ps1 changes)

Design Note (informational, non-blocking)

The ReadFields helper's Func<ushort, int, bool> signature elegantly communicates intent: return true = "I consumed this field", false = "skip it for me". The two-parameter design (fieldId, fieldSize) is forward-thinking — callers that use ReadStringValue(stream, fieldSize) benefit from having the size available without re-reading it, while fixed-size field readers (like ReadByte) simply ignore it.

Well-structured refactoring. 👍

[Amaury Levé (Evangelink)]

Summary

Deduplicates the 3-phase read/write wire-protocol boilerplate that was copy-pasted across the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/.

Two helpers are added to the shared BaseSerializer and every serializer is refactored to use them:

• ReadFields(Stream, Func<ushort, int, bool>) — encapsulates the field-count loop that reads each [id][size][payload] triple and skips unrecognized fields (the forward-compatibility fallback), replacing the identical loop in every DeserializeCore and per-list-item reader.
• WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) — encapsulates the deferred-size-backfill list protocol (reserve size slot, write count, write items, patch size), replacing the copy-pasted before/WriteAtPosition sequence.

Refactored serializers: AzureDevOpsLogMessage, TestSessionEvent, DisplayMessage, CommandLineOptionMessages, TestInProgressMessages, FileArtifactMessages, DiscoveredTestMessages, TestResultMessages.

Net change: +237 / −465 lines.

Wire compatibility

No wire-format change. Per-field write order and the skip-unknown/backfill semantics are preserved verbatim (including DisplayMessage's Level size-honoring read and DiscoveredTestMessages' required trait key/value validation).

Validation

• Microsoft.Testing.Platform + the 4 shared-source extensions (HangDump, Retry, TrxReport, MSBuild) build with 0 warnings / 0 errors.
• DotnetTestProtocolContract.UnitTests round-trip tests: 63/63 pass (net462/net8.0/net9.0).
• IPC Protocol tests in Microsoft.Testing.Platform.UnitTests: 33/33 pass.

Fixes#9764

[Copilot]

Pull request overview

This PR is a pure maintainability refactor that removes the copy‑pasted 3‑phase wire‑protocol boilerplate shared by the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/. Two protected static helpers are added to the shared BaseSerializer and every serializer is rewritten to call them, cutting ~228 net lines while keeping the on‑the‑wire format byte‑for‑byte identical.

Changes:

• Adds ReadFields(Stream, Func<ushort, int, bool>) to encapsulate the field-count read loop plus the skip‑unknown forward‑compatibility fallback.
• Adds WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) to encapsulate the deferred‑size‑backfill list protocol.
• Refactors all 8 serializers to use the helpers, preserving special cases (DisplayMessage.Level size‑honoring read, DiscoveredTestMessages trait key/value validation, field write order).

Show a summary per file

File	Description
IPC/Serializers/BaseSerializer.cs	Adds the ReadFields and WriteListPayload<T> helper methods with XML docs.
.../Serializers/AzureDevOpsLogMessageSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/TestSessionEventSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/DisplayMessageSerializer.cs	Deserialize loop replaced with ReadFields; Level size‑honoring read preserved.
.../Serializers/CommandLineOptionMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/TestInProgressMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/FileArtifactMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/DiscoveredTestMessagesSerializer.cs	Deserialize loops + 3 list writes replaced; trait validation preserved.
.../Serializers/TestResultMessagesSerializer.cs	Deserialize loops + 3 list writes replaced with helpers.

Notes: The refactor is behavior‑preserving — field‑id constants are all ushort (matching the new lambda parameter), the inheritance chain (NamedPipeSerializer<T> : BaseSerializer) exposes the helpers, and null/empty list and skip‑unknown semantics are unchanged. The single observation is a minor, optional performance tradeoff: the per‑element read loops now allocate a capturing delegate/closure per element. This is a serialization/wire‑protocol refactor (the field‑id file is explicitly kept aligned with the dotnet SDK), so it warrants human confirmation despite the passing round‑trip tests.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

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 — LGTM ✅

This is a clean, mechanical refactoring that successfully deduplicates the 3-phase read/write boilerplate across 8 DotnetTest IPC serializers. All dimensions reviewed came back clean.

Dimensions Evaluated

Dimension	Verdict	Notes
IPC Wire Compatibility	✅ LGTM	WriteListPayload produces the identical [fieldId][size][count][items] layout. ReadFields skip logic (stream.Position + fieldSize) is wire-compatible with the backfilled size (stream.Position - before). Verified read/write symmetry.
Algorithmic Correctness	✅ LGTM	Each serializer's field dispatch is mechanically preserved within the lambda. Nested ReadFields/WriteListPayload calls (e.g., traits inside discovered tests) compose correctly — outer backfill captures full nested content.
Threading & Concurrency	✅ LGTM	All methods are protected static, operate on a Stream parameter, and lambdas capture only method-local variables. No shared mutable state.
Security & IPC Contract Safety	✅ LGTM	No new deserialization paths. The skip-unknown-field behavior (forward compatibility) is preserved verbatim.
Public API & Binary Compatibility	✅ LGTM	BaseSerializer is internal abstract with [Embedded]. New protected static helpers are not public API — no PublicAPI.Unshipped.txt update needed.
Performance & Allocations	✅ LGTM	Read-side lambdas capture locals (unavoidable for writing to multiple out variables), allocating one closure + delegate per ReadFields call. This is negligible for per-message IPC parsing. Write-side callbacks are all static lambdas — zero allocation.
Cross-TFM Compatibility	✅ LGTM	Uses Func<ushort, int, bool> and Action<Stream, T> available on all target TFMs (net462+).
Code Structure	✅ LGTM	Clean deduplication. Good XML doc comments on both helpers.
Scope & PR Discipline	✅ LGTM	Pure refactoring, no behavioral changes mixed in.
Test Completeness	✅ LGTM	Existing round-trip tests (63/63 + 33/33) exercise all callers. Adequate for a behavior-preserving refactoring.

Skipped (not applicable)

• Analyzer Quality (no src/Analyzers/ changes)
• Test Isolation / Assertion Quality / Flakiness / Data-Driven (no test files changed)
• Build Infrastructure (no .props/.targets changes)
• PowerShell Scripting (no .ps1 changes)

Design Note (informational, non-blocking)

The ReadFields helper's Func<ushort, int, bool> signature elegantly communicates intent: return true = "I consumed this field", false = "skip it for me". The two-parameter design (fieldId, fieldSize) is forward-thinking — callers that use ReadStringValue(stream, fieldSize) benefit from having the size available without re-reading it, while fixed-size field readers (like ReadByte) simply ignore it.

Well-structured refactoring. 👍

[Amaury Levé (Evangelink)]

Summary

Deduplicates the 3-phase read/write wire-protocol boilerplate that was copy-pasted across the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/.

Two helpers are added to the shared BaseSerializer and every serializer is refactored to use them:

• ReadFields(Stream, Func<ushort, int, bool>) — encapsulates the field-count loop that reads each [id][size][payload] triple and skips unrecognized fields (the forward-compatibility fallback), replacing the identical loop in every DeserializeCore and per-list-item reader.
• WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) — encapsulates the deferred-size-backfill list protocol (reserve size slot, write count, write items, patch size), replacing the copy-pasted before/WriteAtPosition sequence.

Refactored serializers: AzureDevOpsLogMessage, TestSessionEvent, DisplayMessage, CommandLineOptionMessages, TestInProgressMessages, FileArtifactMessages, DiscoveredTestMessages, TestResultMessages.

Net change: +237 / −465 lines.

Wire compatibility

No wire-format change. Per-field write order and the skip-unknown/backfill semantics are preserved verbatim (including DisplayMessage's Level size-honoring read and DiscoveredTestMessages' required trait key/value validation).

Validation

• Microsoft.Testing.Platform + the 4 shared-source extensions (HangDump, Retry, TrxReport, MSBuild) build with 0 warnings / 0 errors.
• DotnetTestProtocolContract.UnitTests round-trip tests: 63/63 pass (net462/net8.0/net9.0).
• IPC Protocol tests in Microsoft.Testing.Platform.UnitTests: 33/33 pass.

Fixes#9764

[Copilot]

Pull request overview

This PR is a pure maintainability refactor that removes the copy‑pasted 3‑phase wire‑protocol boilerplate shared by the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/. Two protected static helpers are added to the shared BaseSerializer and every serializer is rewritten to call them, cutting ~228 net lines while keeping the on‑the‑wire format byte‑for‑byte identical.

Changes:

• Adds ReadFields(Stream, Func<ushort, int, bool>) to encapsulate the field-count read loop plus the skip‑unknown forward‑compatibility fallback.
• Adds WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) to encapsulate the deferred‑size‑backfill list protocol.
• Refactors all 8 serializers to use the helpers, preserving special cases (DisplayMessage.Level size‑honoring read, DiscoveredTestMessages trait key/value validation, field write order).

Show a summary per file

File	Description
IPC/Serializers/BaseSerializer.cs	Adds the ReadFields and WriteListPayload<T> helper methods with XML docs.
.../Serializers/AzureDevOpsLogMessageSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/TestSessionEventSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/DisplayMessageSerializer.cs	Deserialize loop replaced with ReadFields; Level size‑honoring read preserved.
.../Serializers/CommandLineOptionMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/TestInProgressMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/FileArtifactMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/DiscoveredTestMessagesSerializer.cs	Deserialize loops + 3 list writes replaced; trait validation preserved.
.../Serializers/TestResultMessagesSerializer.cs	Deserialize loops + 3 list writes replaced with helpers.

Notes: The refactor is behavior‑preserving — field‑id constants are all ushort (matching the new lambda parameter), the inheritance chain (NamedPipeSerializer<T> : BaseSerializer) exposes the helpers, and null/empty list and skip‑unknown semantics are unchanged. The single observation is a minor, optional performance tradeoff: the per‑element read loops now allocate a capturing delegate/closure per element. This is a serialization/wire‑protocol refactor (the field‑id file is explicitly kept aligned with the dotnet SDK), so it warrants human confirmation despite the passing round‑trip tests.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

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 — LGTM ✅

This is a clean, mechanical refactoring that successfully deduplicates the 3-phase read/write boilerplate across 8 DotnetTest IPC serializers. All dimensions reviewed came back clean.

Dimensions Evaluated

Dimension	Verdict	Notes
IPC Wire Compatibility	✅ LGTM	WriteListPayload produces the identical [fieldId][size][count][items] layout. ReadFields skip logic (stream.Position + fieldSize) is wire-compatible with the backfilled size (stream.Position - before). Verified read/write symmetry.
Algorithmic Correctness	✅ LGTM	Each serializer's field dispatch is mechanically preserved within the lambda. Nested ReadFields/WriteListPayload calls (e.g., traits inside discovered tests) compose correctly — outer backfill captures full nested content.
Threading & Concurrency	✅ LGTM	All methods are protected static, operate on a Stream parameter, and lambdas capture only method-local variables. No shared mutable state.
Security & IPC Contract Safety	✅ LGTM	No new deserialization paths. The skip-unknown-field behavior (forward compatibility) is preserved verbatim.
Public API & Binary Compatibility	✅ LGTM	BaseSerializer is internal abstract with [Embedded]. New protected static helpers are not public API — no PublicAPI.Unshipped.txt update needed.
Performance & Allocations	✅ LGTM	Read-side lambdas capture locals (unavoidable for writing to multiple out variables), allocating one closure + delegate per ReadFields call. This is negligible for per-message IPC parsing. Write-side callbacks are all static lambdas — zero allocation.
Cross-TFM Compatibility	✅ LGTM	Uses Func<ushort, int, bool> and Action<Stream, T> available on all target TFMs (net462+).
Code Structure	✅ LGTM	Clean deduplication. Good XML doc comments on both helpers.
Scope & PR Discipline	✅ LGTM	Pure refactoring, no behavioral changes mixed in.
Test Completeness	✅ LGTM	Existing round-trip tests (63/63 + 33/33) exercise all callers. Adequate for a behavior-preserving refactoring.

Skipped (not applicable)

• Analyzer Quality (no src/Analyzers/ changes)
• Test Isolation / Assertion Quality / Flakiness / Data-Driven (no test files changed)
• Build Infrastructure (no .props/.targets changes)
• PowerShell Scripting (no .ps1 changes)

Design Note (informational, non-blocking)

The ReadFields helper's Func<ushort, int, bool> signature elegantly communicates intent: return true = "I consumed this field", false = "skip it for me". The two-parameter design (fieldId, fieldSize) is forward-thinking — callers that use ReadStringValue(stream, fieldSize) benefit from having the size available without re-reading it, while fixed-size field readers (like ReadByte) simply ignore it.

Well-structured refactoring. 👍

[Amaury Levé (Evangelink)]

Summary

Deduplicates the 3-phase read/write wire-protocol boilerplate that was copy-pasted across the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/.

Two helpers are added to the shared BaseSerializer and every serializer is refactored to use them:

• ReadFields(Stream, Func<ushort, int, bool>) — encapsulates the field-count loop that reads each [id][size][payload] triple and skips unrecognized fields (the forward-compatibility fallback), replacing the identical loop in every DeserializeCore and per-list-item reader.
• WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) — encapsulates the deferred-size-backfill list protocol (reserve size slot, write count, write items, patch size), replacing the copy-pasted before/WriteAtPosition sequence.

Refactored serializers: AzureDevOpsLogMessage, TestSessionEvent, DisplayMessage, CommandLineOptionMessages, TestInProgressMessages, FileArtifactMessages, DiscoveredTestMessages, TestResultMessages.

Net change: +237 / −465 lines.

Wire compatibility

No wire-format change. Per-field write order and the skip-unknown/backfill semantics are preserved verbatim (including DisplayMessage's Level size-honoring read and DiscoveredTestMessages' required trait key/value validation).

Validation

• Microsoft.Testing.Platform + the 4 shared-source extensions (HangDump, Retry, TrxReport, MSBuild) build with 0 warnings / 0 errors.
• DotnetTestProtocolContract.UnitTests round-trip tests: 63/63 pass (net462/net8.0/net9.0).
• IPC Protocol tests in Microsoft.Testing.Platform.UnitTests: 33/33 pass.

Fixes#9764

[Copilot]

Pull request overview

This PR is a pure maintainability refactor that removes the copy‑pasted 3‑phase wire‑protocol boilerplate shared by the 8 DotnetTest IPC serializers under src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/IPC/Serializers/. Two protected static helpers are added to the shared BaseSerializer and every serializer is rewritten to call them, cutting ~228 net lines while keeping the on‑the‑wire format byte‑for‑byte identical.

Changes:

• Adds ReadFields(Stream, Func<ushort, int, bool>) to encapsulate the field-count read loop plus the skip‑unknown forward‑compatibility fallback.
• Adds WriteListPayload<T>(Stream, ushort, T[]?, Action<Stream, T>) to encapsulate the deferred‑size‑backfill list protocol.
• Refactors all 8 serializers to use the helpers, preserving special cases (DisplayMessage.Level size‑honoring read, DiscoveredTestMessages trait key/value validation, field write order).

Show a summary per file

File	Description
IPC/Serializers/BaseSerializer.cs	Adds the ReadFields and WriteListPayload<T> helper methods with XML docs.
.../Serializers/AzureDevOpsLogMessageSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/TestSessionEventSerializer.cs	Deserialize loop replaced with ReadFields.
.../Serializers/DisplayMessageSerializer.cs	Deserialize loop replaced with ReadFields; Level size‑honoring read preserved.
.../Serializers/CommandLineOptionMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/TestInProgressMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/FileArtifactMessagesSerializer.cs	Deserialize loops + list write replaced with helpers.
.../Serializers/DiscoveredTestMessagesSerializer.cs	Deserialize loops + 3 list writes replaced; trait validation preserved.
.../Serializers/TestResultMessagesSerializer.cs	Deserialize loops + 3 list writes replaced with helpers.

Notes: The refactor is behavior‑preserving — field‑id constants are all ushort (matching the new lambda parameter), the inheritance chain (NamedPipeSerializer<T> : BaseSerializer) exposes the helpers, and null/empty list and skip‑unknown semantics are unchanged. The single observation is a minor, optional performance tradeoff: the per‑element read loops now allocate a capturing delegate/closure per element. This is a serialization/wire‑protocol refactor (the field‑id file is explicitly kept aligned with the dotnet SDK), so it warrants human confirmation despite the passing round‑trip tests.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

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 — LGTM ✅

This is a clean, mechanical refactoring that successfully deduplicates the 3-phase read/write boilerplate across 8 DotnetTest IPC serializers. All dimensions reviewed came back clean.

Dimensions Evaluated

Dimension	Verdict	Notes
IPC Wire Compatibility	✅ LGTM	WriteListPayload produces the identical [fieldId][size][count][items] layout. ReadFields skip logic (stream.Position + fieldSize) is wire-compatible with the backfilled size (stream.Position - before). Verified read/write symmetry.
Algorithmic Correctness	✅ LGTM	Each serializer's field dispatch is mechanically preserved within the lambda. Nested ReadFields/WriteListPayload calls (e.g., traits inside discovered tests) compose correctly — outer backfill captures full nested content.
Threading & Concurrency	✅ LGTM	All methods are protected static, operate on a Stream parameter, and lambdas capture only method-local variables. No shared mutable state.
Security & IPC Contract Safety	✅ LGTM	No new deserialization paths. The skip-unknown-field behavior (forward compatibility) is preserved verbatim.
Public API & Binary Compatibility	✅ LGTM	BaseSerializer is internal abstract with [Embedded]. New protected static helpers are not public API — no PublicAPI.Unshipped.txt update needed.
Performance & Allocations	✅ LGTM	Read-side lambdas capture locals (unavoidable for writing to multiple out variables), allocating one closure + delegate per ReadFields call. This is negligible for per-message IPC parsing. Write-side callbacks are all static lambdas — zero allocation.
Cross-TFM Compatibility	✅ LGTM	Uses Func<ushort, int, bool> and Action<Stream, T> available on all target TFMs (net462+).
Code Structure	✅ LGTM	Clean deduplication. Good XML doc comments on both helpers.
Scope & PR Discipline	✅ LGTM	Pure refactoring, no behavioral changes mixed in.
Test Completeness	✅ LGTM	Existing round-trip tests (63/63 + 33/33) exercise all callers. Adequate for a behavior-preserving refactoring.

Skipped (not applicable)

• Analyzer Quality (no src/Analyzers/ changes)
• Test Isolation / Assertion Quality / Flakiness / Data-Driven (no test files changed)
• Build Infrastructure (no .props/.targets changes)
• PowerShell Scripting (no .ps1 changes)

Design Note (informational, non-blocking)

The ReadFields helper's Func<ushort, int, bool> signature elegantly communicates intent: return true = "I consumed this field", false = "skip it for me". The two-parameter design (fieldId, fieldSize) is forward-thinking — callers that use ReadStringValue(stream, fieldSize) benefit from having the size available without re-reading it, while fixed-size field readers (like ReadByte) simply ignore it.

Well-structured refactoring. 👍