Uh oh!
There was an error while loading. Please reload this page.
Remove duplicated #if NETCOREAPP / #else primitives in BaseSerializer - #9776
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors BaseSerializer in Microsoft.Testing.Platform’s IPC serializers to remove duplicated #if NETCOREAPP / #else primitive read/write implementations, consolidating onto a single byte[]-based path and centralizing exact-read behavior to avoid short-read corruption on non-NETCOREAPP targets.
Changes:
- Removed duplicated NETCOREAPP vs non-NETCOREAPP implementations for serializer primitives, keeping one
byte[]implementation. - Introduced a private
ReadExactlyhelper that usesStream.ReadExactlyon NETCOREAPP and a looped read on other TFMs. - Fixed historical short-read behavior by routing primitive reads through
ReadExactly.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Platform/IPC/Serializers/BaseSerializer.cs | Collapses duplicated primitive serialization logic and centralizes exact-read behavior to avoid short-read corruption. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Low
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
✅ 22/22 dimensions clean — no findings.
Notes on the review:
- Algorithmic Correctness:
ReadExactlyloop is correct — properly handlescount = 0, short reads, and EOF. - IPC Wire Compatibility: Wire format unchanged — same length-prefix encoding for strings, same byte ordering for primitives.
- Cross-TFM:
Stream.ReadExactly(byte[], int, int)correctly guarded behind#if NETCOREAPP(available .NET 7+); fallback loop uses standardStream.Read. - Performance: The removal of
stackalloc/ArrayPoolon NETCOREAPP in favor ofbyte[]is a deliberate trade-off for maintainability (noSystem.Memoryreference, shared source). For fixed-size primitives (4–8 bytes) the GC cost is negligible; for string buffers this is bounded by IPC message size. - Bug fix: Short-read correction on non-NETCOREAPP is sound —
EndOfStreamExceptionmatches the framework'sStream.ReadExactlybehavior. - Public API: No surface changes —
internal abstractclass withprotected staticmembers only.
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
🔍 Build Failure AnalysisSummary — The build fails with 6 Root cause: Missing internal API declaration after |
| Code | TFM | File:Line | Symbol |
|---|---|---|---|
RS0051 | net8.0, net9.0 | PlatformServicesConfigurationAdapter.cs:12 | PlatformServicesConfigurationAdapter (class) |
RS0051 | net8.0, net9.0 | PlatformServicesConfigurationAdapter.cs:16 | .ctor(IConfiguration!) |
RS0051 | net8.0, net9.0 | PlatformServicesConfigurationAdapter.cs:19 | this[string!].get |
Proposed fix
After rebasing on latest main, add the following 3 lines to src/Adapter/MSTest.TestAdapter/InternalAPI/InternalAPI.Unshipped.txt:
#nullable enable
+Microsoft.VisualStudio.TestTools.UnitTesting.PlatformServicesConfigurationAdapter+Microsoft.VisualStudio.TestTools.UnitTesting.PlatformServicesConfigurationAdapter.PlatformServicesConfigurationAdapter(Microsoft.Testing.Platform.Configurations.IConfiguration! configuration) -> void+Microsoft.VisualStudio.TestTools.UnitTesting.PlatformServicesConfigurationAdapter.this[string! key].get -> string?Note: This fix targets a file that doesn't exist on the PR branch yet — it was introduced on
mainafter this PR was created. Rebasing the branch onto currentmainwill pull in the InternalAPI tracking infrastructure, at which point the 3 lines above can be appended toInternalAPI.Unshipped.txt.
Build overview
| Field | Value |
|---|---|
| Result | FAILED |
| Duration | 241.1 s |
| MSBuild | 18.8.0-preview-26302-115 |
| Solution | NonWindowsTests.slnf |
| Failed project | MSTest.TestAdapter.csproj (net8.0 + net9.0) |
| Error count | 7 (6 unique RS0051 + 1 "Build failed") |
| Warnings | 0 |
All MSBuild errors (7)
| # | Code | Project | File:Line | Message (truncated) |
|---|---|---|---|---|
| 1 | RS0051 | MSTest.TestAdapter (net8.0) | PlatformServicesConfigurationAdapter.cs:12 | Symbol '...PlatformServicesConfigurationAdapter' is not part of the declared API |
| 2 | RS0051 | MSTest.TestAdapter (net8.0) | PlatformServicesConfigurationAdapter.cs:16 | Symbol '...PlatformServicesConfigurationAdapter(...)' is not part of the declared API |
| 3 | RS0051 | MSTest.TestAdapter (net8.0) | PlatformServicesConfigurationAdapter.cs:19 | Symbol '...this[string! key].get' is not part of the declared API |
| 4 | RS0051 | MSTest.TestAdapter (net9.0) | PlatformServicesConfigurationAdapter.cs:12 | Symbol '...PlatformServicesConfigurationAdapter' is not part of the declared API |
| 5 | RS0051 | MSTest.TestAdapter (net9.0) | PlatformServicesConfigurationAdapter.cs:16 | Symbol '...PlatformServicesConfigurationAdapter(...)' is not part of the declared API |
| 6 | RS0051 | MSTest.TestAdapter (net9.0) | PlatformServicesConfigurationAdapter.cs:19 | Symbol '...this[string! key].get' is not part of the declared API |
| 7 | — | Build.proj | — | Build failed. |
Why this isn't caused by this PR's changes
This PR modifies only:
src/Platform/Microsoft.Testing.Platform/IPC/Serializers/BaseSerializer.cs(refactor)test/UnitTests/Microsoft.Testing.Platform.UnitTests/IPC/BaseSerializerPartialReadTests.cs(new test)
Neither file is in the MSTest.TestAdapter project. The failure is triggered by new InternalAPI tracking infrastructure on main that this PR hasn't picked up yet.
🤖 Generated by the Build Failure Analysis workflow using (a href="(dev.azure.com/redacted) · commit 4c23558
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 224.8 AIC · ⌖ 10.5 AIC · ⊞ 7.3K · [◷]( · ◷)
…rimitives Collapse the 12 dual-implemented read/write primitives into a single byte[]-based implementation whose only conditional piece is a private ReadExactly helper, which also fixes the historical short-read bug on the non-NETCOREAPP path where a single Stream.Read could return fewer bytes than requested and silently corrupt data. Fixes#9765 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Existing serializer round-trip tests only use MemoryStream, which never returns short reads, so the new centralized ReadExactly behavior was untested. Add tests that deserialize through a one-byte-per-read stream (round-trips correctly) and a truncated stream (throws EndOfStreamException). These exercise the framework Stream.ReadExactly path on NETCOREAPP and the hand-written read-until-complete loop on net462. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…esConfigurationAdapter in InternalAPI These internal symbols landed on main (ReadFields/WriteListPayload via the DotnetTest serializer dedup, and PlatformServicesConfigurationAdapter) after InternalAPI tracking was enabled, but were never added to the declared API, so RS0051 fires across every project that compiles BaseSerializer.cs as shared source (Platform, HangDump, MSBuild, Retry, TrxReport) plus MSTest.TestAdapter. Declaring them in the corresponding InternalAPI.Unshipped files unblocks the build. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
4c23558 to
0724c73Compare🧪 Test quality grade — PR #9776
This advisory comment was generated automatically. Grades are heuristic
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Fixes#9765.
BaseSerializer.cs previously implemented every read/write primitive twice — once under
#if NETCOREAPP(Span<byte>/ArrayPool<byte>/ReadExactly) and once under#else(heapbyte[]+stream.Read). The two branches were structurally identical, so adding or fixing a primitive meant editing both.This collapses the 12 duplicated primitives into a single
byte[]-based implementation. The only remaining conditional is a small privateReadExactlyhelper:NETCOREAPPit delegates to the frameworkStream.ReadExactly.EndOfStreamExceptionon premature EOF.Why byte[] instead of Span everywhere
Microsoft.Testing.Platformintentionally does not referenceSystem.Memory(dropped in #4652), and this file is shared as source into several other projects.Span/ArrayPoolare therefore unavailable on thenetstandard2.0target, so the unified path staysbyte[]-based.Bug fix included
The old
#elsebranch used singlestream.Read(...)calls that ignored short reads, which could silently return fewer bytes than requested and corrupt data on the .NET Framework / netstandard2.0 path. Routing all reads throughReadExactlyfixes this.Tests
Added
BaseSerializerPartialReadTests, which deserialize through a stream that returns one byte perRead(forcing every length prefix / field id / string payload through theReadExactlyloop) and through a truncated stream (assertingEndOfStreamException). These run on net8.0/net9.0 (frameworkStream.ReadExactlypath) and net462 (the hand-written loop).InternalAPI baseline changes (to unblock CI — not part of the refactor)
BaseSerializer.ReadFields/WriteListPayload— declared in Platform, Extensions.HangDump, Extensions.MSBuild, Extensions.Retry, Extensions.TrxReport (each compilesBaseSerializer.csas shared source).PlatformServicesConfigurationAdapter— declared in MSTest.TestAdapter. This one is unrelated to this change and was pulled in via the merge withmain; happy to split it (and the other baseline updates) into a dedicated PR if preferred.Validation
net8.0,net9.0, andnetstandard2.0(all previously-failingRS0051errors resolved).BaseSerializer(bodies only); the API-baseline files above are updated purely to declare pre-existing-on-mainsymbols.