diff --git a/docs/RFCs/018-Artifact-Post-Processing.md b/docs/RFCs/018-Artifact-Post-Processing.md index ea000a338d..91793477db 100644 --- a/docs/RFCs/018-Artifact-Post-Processing.md +++ b/docs/RFCs/018-Artifact-Post-Processing.md @@ -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 (`/merged/merged-.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 | @@ -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, diff --git a/global.json b/global.json index 42216cd0b4..3df3aa23f8 100644 --- a/global.json +++ b/global.json @@ -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", @@ -24,7 +24,7 @@ } }, "sdk": { - "version": "11.0.100-preview.7.26359.110", + "version": "11.0.100-preview.7.26376.106", "paths": [ ".dotnet", "$host$" diff --git a/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxArtifactPostProcessor.cs b/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxArtifactPostProcessor.cs index 911d13e0d7..e936b59807 100644 --- a/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxArtifactPostProcessor.cs +++ b/src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxArtifactPostProcessor.cs @@ -8,6 +8,12 @@ namespace Microsoft.Testing.Extensions.TrxReport.Abstractions; internal sealed class TrxArtifactPostProcessor : IArtifactPostProcessor { + /// + /// Subdirectory of the orchestrator-provided output directory that receives the merged report and the + /// attachment deployment root written beside it. + /// + private const string MergedReportDirectoryName = "merged"; + private static readonly string[] SupportedArtifactKinds = [TrxReportEngine.TrxArtifactKind]; private static readonly string[] SupportedExtensions = [".trx"]; @@ -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, @@ -57,4 +85,22 @@ await TrxReportEngine.MergeToFileAsync( ExtensionResources.TrxMergedArtifactDisplayName, string.Format(CultureInfo.CurrentCulture, ExtensionResources.TrxMergedArtifactDescription, inputs.Count)); } + + /// + /// Returns when 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. + /// + 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; + } + } } diff --git a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxArtifactPostProcessorTests.cs b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxArtifactPostProcessorTests.cs index 1f5992fc7c..71ba1d06d4 100644 --- a/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxArtifactPostProcessorTests.cs +++ b/test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxArtifactPostProcessorTests.cs @@ -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"), @@ -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 @@ -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() { @@ -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 "/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; + } }