Uh oh!
There was an error while loading. Please reload this page.
Share dotnet test wire contract (ObjectFieldIds + Constants) as source - #9218
Merged
Amaury Levé (Evangelink) merged 2 commits intoJun 17, 2026
Merged
Conversation
The 'dotnet test' named-pipe protocol's serializer-id/field-id registry (ObjectFieldIds) and wire constants (Constants) are duplicated by hand in dotnet/sdk, where ObjectFieldIds carries a "must be kept aligned with the testfx repo" warning. Any drift is a silent wire-protocol break. This makes those two zero-dependency files a single source of truth that can be compiled into another assembly via a shared .props, instead of copied: - Add DotnetTestProtocolContract.props that <Compile>-includes ObjectFieldIds and Constants. The platform itself does not import it (it globs these files); it is for external consumers (a standalone proof here, and eventually dotnet/sdk via a source NuGet). - Add a standalone consumer test project that does NOT reference the platform's protocol types: it compiles the shared source into its own assembly and pins every serializer id (incl. reserved id 4), handshake property name, execution mode, session-event type, test state and the protocol version. This proves the contract is self-contained and consumable with one source of truth. - Register the project in TestFx.slnx. Also fill gaps in the in-repo protocol contract tests (ProtocolTests): round-trip CommandLineOptionMessages/FileArtifactMessages/TestSessionEvent and add SerializerIds_AreStable pinning the full id registry. Models/serializers are intentionally not shared yet: they couple to TestMetadataProperty (Extensions.Messages) and use a different class shape than the SDK copy, so they require decoupling/unifying first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces the risk of silent dotnet test named-pipe wire-protocol drift by introducing a source-shared “contract” (IDs + constants) that can be compiled into external consumers, and by strengthening in-repo tests that pin the protocol surface.
Changes:
- Add
DotnetTestProtocolContract.propsto source-shareObjectFieldIds.cs+Constants.csas a single wire-contract source of truth for external consumers. - Add a standalone unit-test consumer project that compiles the contract source into its own assembly and pins protocol values.
- Extend existing platform IPC protocol tests with additional round-trip coverage and full serializer-id registry pinning.
Show a summary per file
| File | Description |
|---|---|
| TestFx.slnx | Registers the new standalone contract consumer unit-test project in the solution. |
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/IPC/ProtocolTests.cs | Adds round-trip serialization tests and pins serializer IDs for protocol stability. |
| test/UnitTests/Microsoft.Testing.Platform.DotnetTestProtocolContract.UnitTests/Program.cs | Test-host entrypoint for the new standalone contract consumer test project. |
| test/UnitTests/Microsoft.Testing.Platform.DotnetTestProtocolContract.UnitTests/Microsoft.Testing.Platform.DotnetTestProtocolContract.UnitTests.csproj | Defines the standalone consumer project and imports the shared contract .props. |
| test/UnitTests/Microsoft.Testing.Platform.DotnetTestProtocolContract.UnitTests/DotnetTestProtocolContractTests.cs | Pins handshake/property/state/version constants and serializer IDs compiled from the shared contract source. |
| src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/DotnetTestProtocolContract.props | New shared-source manifest that Compile-includes the protocol contract files for reuse. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 2
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
Amaury Levé (Evangelink)
left a comment
MemberAuthor
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 11 | Assertion Quality / Build Infrastructure | 🟡 1 MODERATE — missing BannedSymbols.txt |
| 15 | Code Structure | 🟡 1 MODERATE — misleading uniqueness-guarantee comment |
| 21 | Scope & PR Discipline | 🟡 1 MODERATE — SerializerIds_AreStable duplicated verbatim in both projects |
| 13 | Test Completeness | 🔵 1 NIT — TestStates_AreStable / SessionEventTypes_AreStable / ProtocolVersion_IsStable not mirrored in ProtocolTests.cs |
✅ 18/22 dimensions clean.
- Missing
BannedSymbols.txt—Microsoft.Testing.Platform.DotnetTestProtocolContract.UnitTestshas noBannedSymbols.txt/<AdditionalFiles>reference; assertion-library policy is unenforced. - Misleading comment — Both new
SerializerIds_AreStabletests (inProtocolTests.csandDotnetTestProtocolContractTests.cs) claim the[key] = valuedict initializer "additionally guarantees every id is unique", but that style does not throw on duplicate keys — collisions are only caught indirectly by downstreamAreEqualfailures. Rephrase, or switch to{ key, value }Add-style. - Duplicate pinning test —
SerializerIds_AreStableis copy-pasted into both files. When a new serializer is added the contributor must update two tests — the same alignment problem this PR is eliminating at the source-file level. Consider a cross-reference comment or a compile-time exhaustive check. - Asymmetric coverage —
TestStates_AreStable,SessionEventTypes_AreStable, andProtocolVersion_IsStablelive only in the standalone consumer project; the mainProtocolTests.cssuite doesn't pin these constants.
Design-level notes (not blocking):
- The
.propsbackslash paths (IPC\ObjectFieldIds.cs) are consistent with the existing repo convention — MSBuild normalizes them on Linux — no action needed. - The
FileArtifactMessagesSerializeDeserializemethod was flagged in the task description as havingnew MemoryStream()without assignment, but the actual PR diff confirmsvar stream = new MemoryStream();is present — no bug. Microsoft.Testing.Platform.slnf(the filtered solution) does not appear to include the new test project; worth checking whether it should be added for developers working with that filtered view.
🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review (on PR ready) workflow. · 754 AIC · ⌖ 12.9 AIC · ◷
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ts, add BannedSymbols - Reword the misleading 'dictionary initializer guarantees uniqueness' comments in both SerializerIds_AreStable tests; indexer initializers silently overwrite duplicates, so the assertions are what enforce stability. - Mirror SessionEventTypes_AreStable, TestStates_AreStable, and ProtocolVersion_IsStable into ProtocolTests.cs so the main MTP unit-test suite pins the same constants as the standalone contract project. - Add cross-reference comments linking the duplicated SerializerIds_AreStable tests. - Add BannedSymbols.txt (+ AdditionalFiles) to the contract test project to enforce MSTest assertions, matching peer projects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
commented
Jun 17, 2026
MemberAuthor
🧪 Test quality grade — PR #921813 tests graded across 2 files (1 new, 1 modified). 10 earned A and 3 earned B — a strong result. The only deductions are for bodies in the 31–34-line range on three serialization/contract-pin tests, which is a natural consequence of the number of wire-protocol values being pinned. No critical, high, or even medium anti-patterns beyond the borderline length. This is a high-quality addition.
This advisory comment was generated automatically. Grades are heuristic
|
Petr Pokorny (0101)
approved these changes
Jun 17, 2026
Petr Pokorny (0101)
left a comment
Member
There was a problem hiding this comment.
Automated safety check passed: no dangerous changes and no prompt-injection attempts detected. Approving as requested. Note: this is a quick safety sanity check, not a full code review.
Uh oh!
There was an error while loading. Please reload this page.
Amaury Levé (Evangelink)
deleted the
copilot/dotnet-test-protocol-src-package
branch
June 17, 2026 20:31
Amaury Levé (Evangelink) pushed a commit
that referenced
this pull request
Jun 19, 2026
…ling gate) Establishes the single-source-of-truth share for the terminal reporter, mirroring the DotnetTestProtocol contract (#9218): - TerminalReporterContract.props: shared-source manifest listing the reporter + rendering + state source, the small platform abstractions it needs (IConsole/IStopwatch/IColor/ System*, RoslynString/ApplicationStateGuard/StackTraceHelper/TargetFrameworkParser/ TestRunSummaryHelper) and TerminalResources.resx. Excludes the MTP-CLI-specific TerminalTestReporterCommandLineOptionsProvider and the hand-written resx accessor. - Microsoft.Testing.Platform.TerminalReporterContract.UnitTests: an independent project that does NOT reference MTP's terminal types; it compiles the shared source into its own assembly via the .props, proving the reporter is self-contained and consumable the same way dotnet/sdk's 'dotnet test' will consume it. Verified: the consumer compiles on net8.0/net9.0 and its smoke tests pass (TerminalResources resolve from the consumer's own embedded resx; shared types usable). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jun 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Establish a source-shared single source of truth for the
dotnet testnamed-pipe protocol wire contract.ObjectFieldIds.cs(serializer-id + field-id registry) andConstants.cs(handshake property names, execution modes, session-event types, test states, protocol version) are duplicated by hand indotnet/sdk— where itsObjectFieldIdsliterally carries a "must be kept aligned with the testfx repo" warning. Any drift is a silent wire-protocol break.These two files are zero-dependency (plain
internalconsts, no[Embedded], noExtensions.Messages), so they can be compiled into another assembly via a shared.propsinstead of copied.Changes
DotnetTestProtocolContract.props— shared-source manifest that<Compile>-includesObjectFieldIds.cs+Constants.cs. Microsoft.Testing.Platform does not import it (it already globs these files); it is for external consumers.Microsoft.Testing.Platform.DotnetTestProtocolContract.UnitTests— a standalone consumer that does not reference the platform's protocol types. It compiles the shared source into its own assembly and pins every serializer id (incl. reserved id4), handshake property name, execution mode, session-event type, test state and the protocol version. This is the proof that the contract is self-contained and consumable with one source of truth.TestFx.slnx.ProtocolTests.cs): round-tripCommandLineOptionMessages/FileArtifactMessages/TestSessionEventandSerializerIds_AreStablepinning the full id registry.Why only the contract (not models/serializers) — yet
While prototyping a fuller share, I verified hard couplings that block the models/serializers today:
[Embedded]transport (NamedPipe*,IRequest/IResponse/INamedPipeSerializer) is per-assembly, not runtime-unified (confirmed via anInvalidCastException) and is shared platform-wide.DiscoveredTestMessagecouples toTestMetadataProperty(Microsoft.Testing.Platform.Extensions.Messages).NamedPipeSerializer<T>) than the SDK copy (BaseSerializer+INamedPipeSerializer).So the contract is the cleanly shareable, highest-drift-risk piece. Extending the package to models/serializers is a follow-up that first needs to decouple
TestMetadataPropertyand converge the serializer shape.Verification
Standalone consumer: 6/6 tests passed, 0 warnings, 0 errors (net8.0). The consumer build compiles
Microsoft.Testing.Platformtransitively, confirming the platform is unaffected.ProtocolTestsadditions: 13/13 passing.Follow-ups (not in this PR)
.propsinto a real source NuGet (contentFiles/buildTransitive) sodotnet/sdkconsumes it.DiscoveredTestMessagefromTestMetadataPropertyand converge serializer shape, then extend the package to models + serializers.dotnet/sdkto consume the package and delete its hand-copiedObjectFieldIds.