Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion docs/RFCs/018-Artifact-Post-Processing.md
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,14 @@ The orchestrator emits one telemetry event per `dotnet test` run summarizing: co
- **`.coverage` is a binary format.** Merging across architectures (x64 host merging arm64 coverage) is not obviously safe. For binary kinds, election **constrains candidates to architecture-compatible apps** (the `arch_ok` predicate in §7.5). Whether coverage should also merge per-TFM or cross-TFM is a policy the coverage processor decides from the input metadata (`TargetFramework`, `Architecture`), which is why those fields are on `InputArtifact`.
- If no arch-compatible app can merge a binary group, that group is left un-merged (originals listed) rather than merged unsafely.

### 7.13 Merged artifacts are written to a `merged/` subdirectory

Orchestrators hand the processor an output directory, and `dotnet test` sets that to the run's `--results-directory` — the same directory that already holds the per-module reports the merge consumed. A processor that wrote its output directly there would make the merged artifact a **sibling of its own inputs**, which breaks the common convention of collecting a results directory with a non-recursive glob (Arcade's "Publish TRX Test Results" step configures exactly that, with `testResultsFiles: '*.trx'`). Such a consumer would ingest the merged report *and* the reports it summarizes, double-counting every test.

Processors therefore write into a `merged/` subdirectory of the supplied output directory (`<outputDirectory>/merged/merged-<runId>.trx`). This keeps the merged artifact out of such globs by construction rather than requiring every consumer to special-case the merged file name. Because that subdirectory has a fixed, predictable name, a processor must materialize it and reject a pre-existing symlink/junction before writing: the merge confines its writes to the report's own directory, so a link there would become the confinement base and redirect output outside the supplied directory.

This also keeps the merged report co-located with any attachment tree written beside it. `TrxReportEngine.MergeToFileAsync` derives the attachment deployment root from the output path and records it in the TRX as a **relative** `runDeploymentRoot`, so the report and its attachments must stay in the same directory for those references to resolve. Nesting the report moves both together; relocating the report afterwards (for example from CI) would not.

## 8. Implementation phasing

| Phase | Repo | Deliverable | Blocked by |
Expand Down Expand Up @@ -582,7 +590,7 @@ internal sealed class TrxArtifactPostProcessor : IArtifactPostProcessor
return null;
}

string output = Path.Combine(outputDirectory, "merged.trx");
string output = Path.Combine(outputDirectory, "merged", "merged.trx");
await TrxReportEngine.MergeAsync(
inputs.Select(i => i.Path).ToArray(),
output,
Expand Down
4 changes: 2 additions & 2 deletions global.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"tools": {
"dotnet": "11.0.100-preview.7.26359.110",
"dotnet": "11.0.100-preview.7.26376.106",
"runtimes": {
"dotnet": [
"8.0.29",
Expand All @@ -24,7 +24,7 @@
}
},
"sdk": {
"version": "11.0.100-preview.7.26359.110",
"version": "11.0.100-preview.7.26376.106",
"paths": [
".dotnet",
"$host$"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ namespace Microsoft.Testing.Extensions.TrxReport.Abstractions;

internal sealed class TrxArtifactPostProcessor : IArtifactPostProcessor
{
/// <summary>
/// Subdirectory of the orchestrator-provided output directory that receives the merged report and the
/// attachment deployment root written beside it.
/// </summary>
private const string MergedReportDirectoryName = "merged";

private static readonly string[] SupportedArtifactKinds = [TrxReportEngine.TrxArtifactKind];
private static readonly string[] SupportedExtensions = [".trx"];

Expand Down Expand Up @@ -43,7 +49,29 @@ .. inputs
];
string[] inputPaths = [.. orderedInputs.Select(input => input.Path)];
Guid runId = TrxReportEngine.CreateMergeRunId(inputPaths, [.. orderedInputs.Select(input => input.ExecutionId)]);
string outputPath = Path.Combine(outputDirectory, $"merged-{runId:N}.trx");
// Nest the merged report in its own subdirectory instead of writing it as a sibling of the inputs.
// Orchestrators point outputDirectory at the run's results directory, which already holds the
// per-module reports, and those are commonly collected with a non-recursive '*.trx' glob (Arcade's
// "Publish TRX Test Results" step configures exactly that). A sibling merged report would be picked
// up alongside its own inputs and double-count every test. Nesting keeps it out of such globs by
// construction, and because MergeToFileAsync derives the attachment deployment root from this path,
// the attachment tree follows the report into the subdirectory and the relative runDeploymentRoot
// recorded in the TRX keeps resolving.
string mergedDirectory = Path.Combine(outputDirectory, MergedReportDirectoryName);

// MergeToFileAsync confines its writes to Path.GetDirectoryName(outputPath), which is now this
// fixed, predictably-named child. A pre-existing symlink/junction at that name would therefore
// become the confinement base itself and silently redirect both the report and its attachment tree
// outside the supplied output directory (Directory.CreateDirectory succeeds on an existing link).
// Materialize the directory here and refuse to merge through a reparse point instead. Returning
// null leaves the per-module reports untouched, matching the never-fail-the-run invariant.
Directory.CreateDirectory(mergedDirectory);
if (IsReparsePoint(mergedDirectory))
{
return null;
}

string outputPath = Path.Combine(mergedDirectory, $"merged-{runId:N}.trx");
await TrxReportEngine.MergeToFileAsync(
inputPaths,
outputPath,
Expand All @@ -57,4 +85,22 @@ await TrxReportEngine.MergeToFileAsync(
ExtensionResources.TrxMergedArtifactDisplayName,
string.Format(CultureInfo.CurrentCulture, ExtensionResources.TrxMergedArtifactDescription, inputs.Count));
}

/// <summary>
/// Returns <see langword="true"/> when <paramref name="path"/> is a symlink/junction, or when its
/// attributes cannot be read. An unreadable directory is treated as unsafe because we cannot prove it
/// is not a redirect, and the merged report is optional output that must never write outside the
/// orchestrator-supplied directory.
/// </summary>
private static bool IsReparsePoint(string path)
{
try
{
return (File.GetAttributes(path) & FileAttributes.ReparsePoint) == FileAttributes.ReparsePoint;
}
catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)
{
return true;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,50 @@ public async Task ProcessAsync_WithTwoInputs_WritesUniquelyNamedMergedReport()
Assert.IsTrue(File.Exists(output.Path));
Assert.AreEqual("TestRun", XDocument.Load(output.Path).Root!.Name.LocalName);

// The merged report must be nested rather than written beside its inputs, so that the
// non-recursive '*.trx' globs used to publish a results directory cannot pick up both the
// merged report and the per-module reports it consumed (which would double-count every test).
Assert.AreEqual("merged", Path.GetFileName(Path.GetDirectoryName(output.Path)));
Assert.AreEqual(
Path.GetFullPath(directory),
Path.GetFullPath(Path.Combine(Path.GetDirectoryName(output.Path)!, "..")));
Assert.AreSequenceEqual(
new[] { "first.trx", "second.trx" },
Directory.GetFiles(directory, "*.trx").Select(Path.GetFileName).OrderBy(name => name, StringComparer.Ordinal));
}
finally
{
Directory.Delete(directory, recursive: true);
}
}

[TestMethod]
public async Task ProcessAsync_WithReorderedInputs_ProducesIdenticalOutput()
{
// RFC 018 requires processing to be deterministic and idempotent, because orchestrators may retry
// transient failures. The run id is derived from the ordered inputs, so reversing them must still
// land on the same output path with byte-identical content.
string directory = Path.Combine(Path.GetTempPath(), $"trx-post-processor-{Guid.NewGuid():N}");
Directory.CreateDirectory(directory);
try
{
string firstPath = Path.Combine(directory, "first.trx");
string secondPath = Path.Combine(directory, "second.trx");
WriteMinimalReport(firstPath, "first");
WriteMinimalReport(secondPath, "second");
TrxArtifactPostProcessor processor = new();

ProcessedArtifact? output = await processor.ProcessAsync(
[
new InputArtifact(firstPath, TrxReportEngine.TrxArtifactKind, null, null, null, "execution-1"),
new InputArtifact(secondPath, TrxReportEngine.TrxArtifactKind, null, null, null, "execution-2"),
],
directory,
CancellationToken.None);

Assert.IsNotNull(output);
byte[] firstMerge = File.ReadAllBytes(output.Path);

ProcessedArtifact? retriedOutput = await processor.ProcessAsync(
[
new InputArtifact(secondPath, TrxReportEngine.TrxArtifactKind, null, null, null, "execution-2"),
Expand All @@ -66,6 +109,7 @@ public async Task ProcessAsync_WithTwoInputs_WritesUniquelyNamedMergedReport()
CancellationToken.None);

Assert.IsNotNull(retriedOutput);
Assert.AreEqual(output.Path, retriedOutput.Path);
Assert.AreSequenceEqual(firstMerge, File.ReadAllBytes(retriedOutput.Path));
}
finally
Expand All @@ -74,6 +118,105 @@ public async Task ProcessAsync_WithTwoInputs_WritesUniquelyNamedMergedReport()
}
}

[TestMethod]
public async Task ProcessAsync_NestsAttachmentDeploymentRootBesideTheMergedReport()
{
// Regression guard for the merged report's attachments: MergeToFileAsync writes the attachment
// deployment root next to the report it produces, and the merged TRX records that root as a
// RELATIVE path. Nesting the report therefore has to carry the deployment root along with it, or
// downloaded merged reports would carry dangling attachment references.
string directory = Path.Combine(Path.GetTempPath(), $"trx-post-processor-{Guid.NewGuid():N}");
Directory.CreateDirectory(directory);
try
{
string firstPath = WriteReportWithAttachment(Path.Combine(directory, "inA"), "a.trx", "depA", "AAA");
string secondPath = WriteReportWithAttachment(Path.Combine(directory, "inB"), "b.trx", "depB", "BBB");
TrxArtifactPostProcessor processor = new();

ProcessedArtifact? output = await processor.ProcessAsync(
[
new InputArtifact(firstPath, TrxReportEngine.TrxArtifactKind, null, null, null, "execution-1"),
new InputArtifact(secondPath, TrxReportEngine.TrxArtifactKind, null, null, null, "execution-2"),
],
directory,
CancellationToken.None);

Assert.IsNotNull(output);

// Resolve the recorded deployment root exactly as a consumer of the merged TRX would: relative
// to the merged report's own directory.
string mergedDirectory = Path.GetDirectoryName(output.Path)!;
string deploymentRoot = XDocument.Load(output.Path)
.Descendants().First(e => e.Name.LocalName == "Deployment")
.Attribute("runDeploymentRoot")!.Value;
string resolvedRoot = Path.Combine(mergedDirectory, deploymentRoot);

Assert.AreEqual("merged", Path.GetFileName(mergedDirectory));
Assert.IsTrue(Directory.Exists(resolvedRoot));
Assert.AreEqual("AAA", File.ReadAllText(Path.Combine(resolvedRoot, "In", "0", "machine", "log.txt")));
Assert.AreEqual("BBB", File.ReadAllText(Path.Combine(resolvedRoot, "In", "1", "machine", "log.txt")));

// Nothing of the merged report may be left in the results directory root, where a
// non-recursive '*.trx' publish glob would pick it up alongside its own inputs.
Assert.IsEmpty(Directory.GetFiles(directory, "*.trx"));
Assert.IsFalse(Directory.Exists(Path.Combine(directory, deploymentRoot)));
}
finally
{
Directory.Delete(directory, recursive: true);
}
}

#if NETCOREAPP
[TestMethod]
public async Task ProcessAsync_WhenMergedDirectoryIsAReparsePoint_DoesNotMerge()
{
// 'merged' is a fixed, predictable child name, and the merge confines its writes to the report's
// own directory. A symlink/junction planted at that name would therefore become the confinement
// base and redirect the report plus its attachment tree outside the supplied output directory,
// so the processor must refuse rather than write through it.
string root = Path.Combine(Path.GetTempPath(), $"trx-post-processor-{Guid.NewGuid():N}");
string resultsDirectory = Path.Combine(root, "results");
string outsideDirectory = Path.Combine(root, "outside");
Directory.CreateDirectory(resultsDirectory);
Directory.CreateDirectory(outsideDirectory);
try
{
try
{
Directory.CreateSymbolicLink(Path.Combine(resultsDirectory, "merged"), outsideDirectory);
}
catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or PlatformNotSupportedException)
{
// Creating a directory symlink needs elevation or Developer Mode on Windows.
Assert.Inconclusive("Host cannot create directory symbolic links.");
return;
}

string firstPath = Path.Combine(resultsDirectory, "first.trx");
string secondPath = Path.Combine(resultsDirectory, "second.trx");
WriteMinimalReport(firstPath, "first");
WriteMinimalReport(secondPath, "second");
TrxArtifactPostProcessor processor = new();

ProcessedArtifact? output = await processor.ProcessAsync(
[
new InputArtifact(firstPath, TrxReportEngine.TrxArtifactKind, null, null, null, "execution-1"),
new InputArtifact(secondPath, TrxReportEngine.TrxArtifactKind, null, null, null, "execution-2"),
],
resultsDirectory,
CancellationToken.None);

Assert.IsNull(output);
Assert.IsEmpty(Directory.GetFileSystemEntries(outsideDirectory));
}
finally
{
Directory.Delete(root, recursive: true);
}
}
#endif

[TestMethod]
public void CreateMergeRunId_IsIndependentOfInputOrder()
{
Expand Down Expand Up @@ -109,4 +252,41 @@ private static void WriteMinimalReport(string path, string name)
new XElement(ns + "Counters", new XAttribute("total", 0)))))
.Save(path);
}

private static string WriteReportWithAttachment(string inputDirectory, string fileName, string deploymentRoot, string attachmentContent)
{
XNamespace ns = "http://microsoft.com/schemas/VisualStudio/TeamTest/2010";
Directory.CreateDirectory(inputDirectory);

// Physical attachment under "<deploymentRoot>/In/machine/log.txt", referenced by the
// machine-relative href "machine/log.txt" that the merge rewrites when it relocates the file.
string attachmentDirectory = Path.Combine(inputDirectory, deploymentRoot, "In", "machine");
Directory.CreateDirectory(attachmentDirectory);
File.WriteAllText(Path.Combine(attachmentDirectory, "log.txt"), attachmentContent);

string path = Path.Combine(inputDirectory, fileName);
new XDocument(
new XElement(
ns + "TestRun",
new XAttribute("id", Guid.NewGuid()),
new XAttribute("name", fileName),
new XElement(
ns + "TestSettings",
new XAttribute("name", "default"),
new XElement(ns + "Deployment", new XAttribute("runDeploymentRoot", deploymentRoot))),
new XElement(
ns + "ResultSummary",
new XAttribute("outcome", "Completed"),
new XElement(ns + "Counters", new XAttribute("total", 0)),
new XElement(
ns + "CollectorDataEntries",
new XElement(
ns + "Collector",
new XAttribute("collectorDisplayName", "Code Coverage"),
new XElement(
ns + "UriAttachments",
new XElement(ns + "UriAttachment", new XElement(ns + "A", new XAttribute("href", "machine/log.txt")))))))))
.Save(path);
return path;
}
}