RFC 018: Artifact post-processing for dotnet test (MTP)#9187
Conversation
Two-phase artifact post-processing to consolidate per-module artifacts (TRX, coverage, custom) at the end of a dotnet test run via a typed IArtifactPostProcessor extension contract, a reserved host mode, handshake-advertised capabilities, and SDK-side election/orchestration. Opened as an RFC for discussion. Notably raises two unresolved orchestration alternatives: using the existing ITool route as the invocation primitive, and a specialized post-processing host with a live IPC channel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
⚠️ Not ready to approve
The RFC includes a few documentation inaccuracies/ambiguities (repo path attribution and an internally inconsistent election tie-break snippet) that could mislead implementers.
Pull request overview
Adds a new design RFC (017) to the docs/RFCs set, describing a two-phase artifact post-processing/merging mechanism for Microsoft.Testing.Platform (MTP) runs orchestrated by dotnet test, including a proposed IArtifactPostProcessor contract, host invocation modes, and SDK election/orchestration.
Changes:
- Introduces RFC 017 documenting artifact consolidation/post-processing for multi-module
dotnet testruns under MTP. - Specifies the proposed extension contract (
IArtifactPostProcessor), manifest/result schemas, handshake capability advertisement, and election algorithm. - Documents alternative orchestration primitives (
--toolvs reserved switch vs specialized host) and open questions.
File summaries
| File | Description |
|---|---|
| docs/RFCs/017-Artifact-Post-Processing.md | New RFC describing the proposed artifact post-processing contract and SDK/host orchestration flow for consolidating artifacts after multi-module dotnet test runs. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 3
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
- Note that src/Cli/dotnet/Commands/Test/MTP/ lives in the dotnet/sdk repo. - Reword the informal 'already-ish present' Warning comment into a concrete statement. - Fix the A.2 election tie-break: compare ProducingTestModule (a file name) against Path.GetFileName(ModulePath) instead of the full path, so it actually matches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
017 is already taken on main by 017-TestHost-Launcher.md (#9349), so rename docs/RFCs/017-Artifact-Post-Processing.md -> 018-Artifact-Post-Processing.md. Rework the design into a layered, decisive recommendation: - Phase 1 (ship first): typed, invocation-agnostic IArtifactPostProcessor engine contract (experimental) + a user-facing ITool per kind (merge-trx) sharing one engine. Delivers user value with no SDK/protocol change (one platform-API change: promote ITool to public). - Phase 2: in-memory election (from data the SDK already has via handshake + live FileArtifactMessages), a platform-owned internal dispatcher ITool, and merged artifacts returned over the existing dotnet-test pipe as FileArtifactMessage. - Election is minimal set-cover (fewest relaunches), arch-constrained for binary coverage; live-channel alternative reframed to lead with reuse of the existing pipe; experimental gating; cross-arch notes. Also fixes markdownlint issues and corrects codebase-accuracy points surfaced in review (ITool is internal today; TrxReportEngine has no file-merge path yet; no --list-tools switch; public records must avoid synthesized init accessors). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Evangelink
changed the title
… path wins) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
RFC 018 Review — Artifact Post-Processing
This is an excellent, thorough RFC. The layered phasing (ship the engine + user tool first, SDK orchestration second) is a strong strategy that delivers value early and de-risks the harder integration work. The design correctly leverages MTP's existing pipe infrastructure instead of reinventing VSTest's disk-tagging plumbing.
Verdict Table
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ Clean — election algorithm is well-specified with clear edge cases |
| 2 | Public API | init accessor issue — code samples use positional records; the note acknowledges it but the samples should show the compliant form (see inline) |
| 3 | IPC Contract Stability | |
| 4 | Backward Compatibility | ✅ Clean — graceful degradation matrix in §10 is comprehensive |
| 5 | Security | |
| 6 | Error Handling | ✅ Clean — §7.9 is thorough; never-fail-the-run invariant is well-maintained |
| 7 | Performance | ✅ Clean — minimal set-cover minimizes relaunches; pipe reuse avoids extra I/O |
| 8 | Cross-TFM | ✅ Clean — §7.12 addresses arch constraints for binary kinds |
| 9 | Testing Strategy | ✅ Clean — §9 covers unit, integration, and acceptance layers |
| 10 | Localization | N/A (no user-facing strings in this RFC) |
| 11–22 | Remaining dimensions | N/A (documentation-only PR, no code changes) |
Summary
3 actionable items (all inline), none blocking. The RFC is well-structured, internally consistent, and addresses its stated goals. The main items to tighten before implementation begins:
- Show the
init-free DTO form in the code samples (not just the footnote). - Specify concrete
bytekey values for the new handshake properties. - Harden the temp-directory security story for Linux.
No blocking issues found. Good to discuss and iterate.
| public sealed record InputArtifact( | ||
| string Path, | ||
| string? Kind, // null when the producer didn't declare one | ||
| string? ProducingTestModule, | ||
| string? TargetFramework, | ||
| string? Architecture, | ||
| string? ExecutionId); | ||
|
|
||
| public sealed record ProcessedArtifact( | ||
| string Path, | ||
| string Kind, // processor MUST tag its output | ||
| string DisplayName, | ||
| string? Description); | ||
| ``` | ||
|
|
||
| > **Public-API note (no `init`).** testfx bans `init` accessors on *new* public API. C# positional records synthesize `{ get; init; }`, so if `InputArtifact` / `ProcessedArtifact` are public they must instead be declared with get-only auto-properties and an explicit constructor (the positional form above is shorthand for readability only). Alternatively keep the DTOs `internal` and expose only the interface. This must be settled at API-review time, not discovered there. |
There was a problem hiding this comment.
Public API / no-init (Principle 2). The RFC correctly flags the init accessor issue in the note below, but the code sample itself still uses positional record syntax which synthesizes { get; init; }. Since this is the most-copied snippet in the RFC, consider showing the actual intended form (explicit constructor + get-only properties) inline so implementers don't cargo-cult the shorthand:
public sealed class InputArtifact
{
public InputArtifact(string path, string? kind, string? producingTestModule,
string? targetFramework, string? architecture, string? executionId)
{
Path = path;
Kind = kind;
// ...
}
public string Path { get; }
public string? Kind { get; }
// ...
}This avoids the "settled at API-review time, not discovered there" scenario the note warns about by making the RFC itself the reference.
| - **Group has only 1 input** -> not planned (the `>= 2` filter). Original listed as today; no relaunch. | ||
| - **Two processors in the same app advertise the same Kind** -> first-wins inside the dispatcher with a warning surfaced to the SDK. (Packaging bug for the user to fix.) | ||
| - **Election succeeds but the relaunch fails** -> warning; originals listed; run exit code unchanged. | ||
| - **Same file matches both a Kind and a fallback extension** -> Kind wins; an artifact is never double-routed. |
There was a problem hiding this comment.
Edge case gap. What happens when two different extensions in two different apps each advertise the same Kind with incompatible merge logic? The "two processors in the same app" case is covered (first-wins + warning), but the cross-app case is only addressed implicitly by election (one app wins). Worth stating explicitly that the losing app's processor is never invoked, so incompatibility is sidestepped by election rather than detected.
| Each test app advertises the kinds (and legacy file extensions) it can post-process so the orchestrator can elect. `HandshakeMessage` is already a `Dictionary<byte, string>` whose values are frequently semicolon-joined lists (e.g. `SupportedProtocolVersions` = `"1.0.0;1.1.0;1.2.0;1.3.0"`), so this is additive and cheap: | ||
|
|
||
| ```text | ||
| SupportedPostProcessorKinds: "microsoft.testing.trx;microsoft.codecoverage" | ||
| SupportedPostProcessorExtensionsLegacy: ".trx;.coverage" |
There was a problem hiding this comment.
IPC backward compatibility. The handshake property keys are shown as strings here but HandshakeMessage uses byte keys (Dictionary<byte, string>). The RFC should specify the actual byte values (or at least note they need allocation from the HandshakeMessagePropertyNames enum) to avoid collisions with existing or concurrently-planned handshake properties.
| - [ ] Implementation | ||
| - [ ] Shipped | ||
|
|
||
| > **Renumber note.** This design was originally drafted as "RFC 017" in [microsoft/testfx#9187](https://github.com/microsoft/testfx/pull/9187). `017` has since been taken by `017-TestHost-Launcher.md` (merged in [#9349](https://github.com/microsoft/testfx/pull/9349)), so this document is renumbered to **018**. |
There was a problem hiding this comment.
|
|
||
| ### 7.10 Security considerations | ||
|
|
||
| - The manifest is created by the orchestrator in a process-private temp directory (`Path.GetTempPath()/dotnet-test-postproc-<guid>/manifest.json`), cleaned up after use. |
There was a problem hiding this comment.
Security (Dimension 17). The manifest is written to Path.GetTempPath() which is world-readable on some Linux configurations. A local attacker could race (TOCTOU) to replace the manifest between write and read, injecting arbitrary file paths into the merge inputs. Since the invariant "a local attacker who can tamper with the manifest can equally tamper with the test binaries" holds on Windows (user-private %TEMP%) but is weaker on Linux (/tmp shared), consider:
- Explicitly stating the temp directory should be created with
0700permissions (or usingDirectory.CreateTempSubdirectoryon .NET 7+ which does this). - Alternatively, writing the manifest under the
--results-directory/TestResultsfolder which is already user-owned.
| **Transitional result path: result JSON.** If the pipe-composition task in §7.6 is deferred, the dispatcher writes a result JSON (path supplied in the manifest) and the SDK performs the swap. This keeps the same typed contract and the same manifest; only the *return channel* changes. Keeping the manifest/result schema `schemaVersion`-versioned means the return channel can switch from files to the pipe without touching `IArtifactPostProcessor`. | ||
|
|
||
| Dispatcher exit codes (final values to be finalized so they don't overlap existing MTP `ExitCode` values — see §12 Q11): |
There was a problem hiding this comment.
IPC contract stability (Principle 6). The exit codes are (tbd) — please add a note that these must be allocated from a range that doesn't conflict with existing ExitCodes in Microsoft.Testing.Platform.Helpers.ExitCodes (currently 0–10). Open question 11 mentions this, but it would be good to have the constraint documented here where the codes are listed, so an implementer doesn't accidentally pick overlapping values.
🔴 Build Failure AnalysisOverviewThe build failed with 4 CA1416 errors in Root Cause
The errors appear twice (2 TFMs), totaling 4 errors. Suggested Fix (on
|
Summary
Opens RFC 018 — Artifact post-processing for
dotnet test(MTP) for discussion (docs/RFCs/018-Artifact-Post-Processing.md).The RFC proposes a mechanism so that, after a multi-module
dotnet testrun, artifacts of the same kind (TRX,.coverage, Cobertura, custom) are consolidated into single files instead of listed one-per-module — the MTP analogue of VSTest's collect/post-process flow.Addresses dotnet/sdk#47613; related to #7345, #7471, #6586.
Status: Under discussion
The design is now a decisive, layered recommendation rather than an open menu of orchestration routes:
IArtifactPostProcessorengine contract (experimental-gated) plus a user-facingIToolper kind (e.g.merge-trx), both delegating to one shared merge engine. This ships user-visible value (Using TrxReport with --test-modules should include test assembly name and target framework in trx file name #7345) with no SDK or protocol change (one platform-API change: promoteIToolto public), is user-runnable from a shell, and is discoverable via--info.dotnet-testpipe — then a platform-owned internal dispatcherIToolrelaunched once per elected app, with merged artifacts flowing back over the existing pipe asFileArtifactMessage(re-entering the normal reporter path).Key design refinements vs. the original draft:
--internal-post-process-artifactsswitch:--toolis the platform's existing non-test execution path (precedent:ms-trxcompare), so the manual and automated routes share one mechanism. The reserved switch is kept as the documented runner-up (§11.1).DotnetTestConnectionalready streams artifacts live, so the SDK already has the full attributed input list; the manifest is transitional and largely redundant (§7.7, §11.2).Feedback wanted
ITool(recommended) vs. reserved switch vs. specialized live-channel host (§11.1/§11.2, Open question Initial commit! 🎉 #1).Kindasstringvs. a typed wrapper, and Kind-versioning policy (Open questions Porting BugFix#260653: DataRowTests not getting appended by arguments #9/Packaging mstest for .net core #10).cc @Evangelink