Uh oh!
There was an error while loading. Please reload this page.
Add source NuGet package for the dotnet test wire contract (follow-up to #9218) - #9231
Conversation
Follow-up #1 to PR #9218: package the shared dotnet test named-pipe wire contract (ObjectFieldIds.cs + Constants.cs) as a source-only NuGet so the dotnet/sdk 'dotnet test' implementation can compile them from a single source of truth instead of hand-copying. - New Microsoft.Testing.Platform.DotnetTestProtocol package ships the two files as contentFiles (BuildAction=Compile); IncludeBuildOutput=false so it is a pure source package. IsShipping=false (transport/non-published) by default. - Refactor DotnetTestProtocolContract.props to declare the canonical file list once as @(DotnetTestProtocolContractSource); both the source-share <Compile> and the package's contentFiles derive from it so they cannot drift. - Register the project in TestFx.slnx. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Youssef Fahmy (Youssef1313)
left a comment
There was a problem hiding this comment.
I would prefer to ensure that all types that ship in a source-only package are marked with [Embedded]
There was a problem hiding this comment.
Pull request overview
Adds a new source-only NuGet package to share the dotnet test named-pipe wire contract (ids/constants) as compiled source, reducing the risk of cross-repo protocol drift between microsoft/testfx and dotnet/sdk.
Changes:
- Registered the new
Microsoft.Testing.Platform.DotnetTestProtocolproject inTestFx.slnx. - Refactored
DotnetTestProtocolContract.propsto centralize the contract file list in@(DotnetTestProtocolContractSource)and derive consumers from it. - Added the new source-only package project that packs the contract sources as
contentFileswithBuildAction=Compile, plusPACKAGE.mddocumentation.
Show a summary per file
| File | Description |
|---|---|
| TestFx.slnx | Registers the new package project in the solution. |
| src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/DotnetTestProtocolContract.props | Centralizes the canonical contract file list and conditionally emits <Compile> items from it. |
| src/Platform/Microsoft.Testing.Platform.DotnetTestProtocol/PACKAGE.md | Documents intent/scope of the wire-contract source package. |
| src/Platform/Microsoft.Testing.Platform.DotnetTestProtocol/Microsoft.Testing.Platform.DotnetTestProtocol.csproj | Implements the source-only NuGet packing of the contract sources as contentFiles. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 1
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 16 | Naming & Conventions | 🟢 1 NIT |
| 17 | Documentation Accuracy | 🟢 1 NIT |
✅ 20/22 dimensions clean (Threading, Security, Public API, Performance, Cross-TFM, Resource Mgmt, Defensive Coding, Localization, Test Isolation, Assertion Quality, Flakiness, Test Completeness, Data-Driven Coverage, Code Structure, Analyzer Quality, IPC Wire Compatibility, Build Infrastructure, Scope Discipline, PowerShell Hygiene not applicable or clean).
MSBuild Authoring (supplemental): Same indentation issue as dimension 16 above — no additional findings.
- Naming & Conventions / MSBuild Authoring — Items in both new
<ItemGroup>blocks inDotnetTestProtocolContract.propsuse 6-space indentation;.editorconfigrequiresindent_size = 2for.propsfiles, so level-2 items should be 4 spaces. - Documentation Accuracy — Comment at line 29–31 of
DotnetTestProtocolContract.propsclaims the package project "setsDotnetTestProtocolContractCompile=false", but the.csprojnever sets that property. The actual behavior is:<Compile>items are active (intentional compilation guard) and<None Pack="true">items pack the same files ascontentFiles. The comment should describe the hybrid mechanism or be corrected to match reality.
Notes (not blocking):
- The
PACKAGE.mdREADME is automatically wired up byDirectory.Build.targets(which setsPackageReadmeFilewhen the file exists) — no explicit<PackageReadmeFile>property is needed in the.csproj. ✅ - The new project is registered in
TestFx.slnxbut not inMicrosoft.Testing.Platform.slnf. If platform developers regularly work on or rebuild this package, consider adding it to the filtered solution; if it's a transport-only artifact consumed exclusively by the SDK flow, omitting it is fine. - The
<IsPackable>true</IsPackable>in the.csprojis redundant (already set bysrc/Platform/Directory.Build.props) but harmless.
🤖 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. · 895.5 AIC · ⌖ 12.5 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.
- Set DotnetTestProtocolContractCompile=false so the package only PACKs the contract files (no <Compile>); the standalone-compile guarantee is provided by the dedicated consumer test project, not the package build. This also makes the props USAGE comment accurate (it already described =false). - Fix props indentation (6 -> 4 spaces) per .editorconfig indent_size=2. - Correct the DevelopmentDependency/IsShipping wording: DevelopmentDependency marks a build-only/non-transitive dependency (not publication); IsShipping controls the feed (internal NonShipping vs nuget.org). Note the internal-feed mechanism still needs Arcade confirmation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
commented
Jun 18, 2026
Good point — but I'd like to align on the approach before doing it, because these two files aren't only shipped as source here:
So this is feasible but cross-cutting. I see two options:
Which would you prefer? I lean toward (2) for this contract-only PR and tackling |
Amaury Levé (Evangelink)
commented
Jun 18, 2026
Follow-up on Why: several extensions compile So today the design is inconsistent: the serializer is source-shared + Proposal: keep the contract types plain |
…f IVT Per review feedback (Youssef): all types shipped in the source-only package are now [Embedded] so each consuming assembly gets a private, non-exported copy. Because the field-ids/constants were previously handed to extensions and unit tests via InternalsVisibleTo (while the serializers were already source-shared + [Embedded]), embedding them broke those consumers (CS0103). Fix: source-link ObjectFieldIds.cs (and Constants.cs where needed) into every assembly that uses them, so they no longer rely on cross-assembly visibility: - Extensions HangDump, Retry, TrxReport, Microsoft.Testing.Extensions.MSBuild (they source-share VoidResponseSerializer and need VoidResponseFieldsId). - Microsoft.Testing.Platform.UnitTests (ProtocolTests references the ids/consts). - The standalone contract test project now also provides the EmbeddedAttribute polyfill, mirroring how an external consumer supplies it. Full repo build is green (0 warnings, 0 errors). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
commented
Jun 18, 2026
Update: implemented What I did:
Full repo build is green (0 warnings, 0 errors). Note: this source-links the whole dotnet-test |
Amaury Levé (Evangelink)
commented
Jun 18, 2026
🧪 Test quality grade — PR #9231No new or modified test methods were identified in the changed regions Re-run with
|
Petr Pokorny (0101)
left a comment
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.
What
Follow-up #1 to #9218 (which established the shared
dotnet testwire-contract source viaDotnetTestProtocolContract.props).This wires that source-share into a real source-only NuGet package,
Microsoft.Testing.Platform.DotnetTestProtocol, sodotnet/sdk'sdotnet testimplementation can compileObjectFieldIds.cs+Constants.csfrom a single source of truth instead of hand-copying them (where any drift is a silent wire-protocol break).Changes
src/Platform/Microsoft.Testing.Platform.DotnetTestProtocol/:contentFiles/cs/any/...withBuildAction=Compile, so the consumer compiles them into its own assembly (theinternaltypes are then visible with noInternalsVisibleTo).IncludeBuildOutput=false⇒ pure source package, nolib/.PACKAGE.mddocuments scope and that it is adotnet testimplementation detail, not for direct end-user use.DotnetTestProtocolContract.propsto declare the canonical file list once as@(DotnetTestProtocolContractSource). Both the source-share<Compile>(used by the standalone contract test project) and the package'scontentFilesderive from that item, so the NuGet and the in-repo source can never drift.TestFx.slnx.Verification
build.cmd -packproducesMicrosoft.Testing.Platform.DotnetTestProtocol.2.3.0-dev.nupkg(0 warnings, 0 errors). The.nuspeccarries both files under<contentFiles ... buildAction="Compile" />.Microsoft.Testing.Platform) restores the produced package and compiles, accessingMicrosoft.Testing.Platform.IPC.CommandLineOptionMessagesFieldsIdandTestStatesfrom the shared source — 0 warnings, 0 errors.Microsoft.Testing.Platform.DotnetTestProtocolContract.UnitTests) still builds acrossnet8.0/net9.0/net462after the.propsrefactor.Open question for reviewers
The package is currently
IsShipping=false— i.e. a transport/source package flowed cross-repo via dependency flow, not published to nuget.org. This seemed the idiomatic choice for an internal-only source contract consumed solely bydotnet/sdk. If you'd rather publish it publicly, it's a one-line flip (IsShipping=true+ dropDevelopmentDependency). Happy to change.Follow-ups (still open from #9218)
DiscoveredTestMessagefromTestMetadataPropertyand converge the serializer shape, then extend this package to the message models + serializers.dotnet/sdkto consume this package and delete its hand-copiedObjectFieldIds.