Copy per-test result files to results directory in TRX reports#7449
Copy per-test result files to results directory in TRX reports#7449Evangelink wants to merge 1 commit intomainfrom
Conversation
When using MTP, files added via TestContext.AddResultFile were not copied to the results directory. The TRX file referenced them with absolute paths, making the results directory non-self-contained and causing warnings in Azure DevOps builds.
Changes:
- Copy per-test FileArtifactProperty files into the results directory under {runDeploymentRoot}/In/{executionId}/{MachineName}/
- Use relative paths in TRX <ResultFile> elements instead of absolute paths
- Replace unbounded while(true) loops with bounded for loops (max 10 attempts) when resolving duplicate file names, in both the new and existing artifact copy methods
Fixes #6782
| await AddArtifactsToCollectionAsync(_artifactsByExtension, collectorDataEntries, runDeploymentRoot).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private async Task<string> CopyTestResultFileAndReturnRelativePathAsync(FileInfo artifact, string runDeploymentRoot, string executionId) |
There was a problem hiding this comment.
We could also consider the copy function to be part of the core platform (or vstest bridge) so that the feature is also available outside of trx being enabled
There was a problem hiding this comment.
Pull request overview
Updates TRX generation in Microsoft.Testing.Extensions.TrxReport so per-test result files (from TestContext.AddResultFile / FileArtifactProperty) are copied into the test results directory and referenced via relative paths, making results self-contained.
Changes:
- Copy per-test
FileArtifactPropertyfiles into{resultsDir}/{runDeploymentRoot}/In/{executionId}/{MachineName}/and emit relative<ResultFile path=...>values. - Refactor result generation to an async tuple-returning
AddResultsAsync(...)to support awaiting file-copy operations. - Replace unbounded filename-collision loops with bounded retry loops (max 10) when choosing destination filenames.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxTests.cs | Updates TRX assertion to expect relative ResultFile paths (and includes TRX text on failure). |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs | Implements per-test result file copying + relative paths; refactors result-building to async; bounds duplicate-name retry loops. |
Comments suppressed due to low confidence (1)
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs:383
- Same issue as above: with the 10-try loop,
destinationmay still exist andCopyFileAsyncwill overwrite it (FileMode.Create). Ensure you either keep searching for a free name or fail deterministically once the retry budget is exhausted to prevent data loss.
// If the file already exists, append a number to the end of the file name
for (int nameCounter = 1; File.Exists(destination) && nameCounter <= 10; nameCounter++)
{
destination = Path.Combine(artifactDirectory, $"{Path.GetFileNameWithoutExtension(fileName)}_{nameCounter}{Path.GetExtension(fileName)}");
}
| string testResultDirectory = Path.Combine(_configuration.GetTestResultDirectory(), runDeploymentRoot, "In", executionId, _environment.MachineName); | ||
| if (!Directory.Exists(testResultDirectory)) | ||
| { | ||
| Directory.CreateDirectory(testResultDirectory); | ||
| } |
There was a problem hiding this comment.
CopyTestResultFileAndReturnRelativePathAsync creates directories and probes the real filesystem even when _isCopyingFileAllowed is false. This makes “copy disabled” mode still have side effects (and can break unit tests / restricted environments). Consider short-circuiting early when copying is disabled (return the relative path without touching disk).
| string relativePath = await CopyTestResultFileAndReturnRelativePathAsync( | ||
| testFileArtifact.FileInfo, runDeploymentRoot, executionId).ConfigureAwait(false); | ||
| resultFiles.Add(new XElement( | ||
| "ResultFile", | ||
| new XAttribute("path", testFileArtifact.FileInfo.FullName))); | ||
| new XAttribute("path", relativePath))); |
There was a problem hiding this comment.
New behavior copies per-test result files into the results directory and changes <ResultFile path=...> to be relative. There’s no unit test that exercises the real copy path (all tests construct TrxReportEngine with isCopyingFileAllowed: false), so regressions in directory layout / naming / copy failures won’t be caught. Add a test that enables copying and asserts the file is created under the expected {runDeploymentRoot}/In/{executionId}/{MachineName}/ layout and that the TRX references it correctly.
| // If the file already exists, append a number to the end of the file name | ||
| for (int nameCounter = 1; File.Exists(destination) && nameCounter <= 10; nameCounter++) | ||
| { | ||
| destination = Path.Combine(testResultDirectory, $"{Path.GetFileNameWithoutExtension(fileName)}_{nameCounter}{Path.GetExtension(fileName)}"); | ||
| } |
There was a problem hiding this comment.
The bounded name-collision loop can still leave destination pointing to an existing file after 10 attempts. Because CopyFileAsync uses FileMode.Create, this will overwrite an existing artifact and silently lose data. After the loop, handle the “still exists” case explicitly (e.g., generate a GUID-based name or throw) and/or use an atomic CreateNew strategy to avoid overwrites.
This issue also appears on line 379 of the same file.
When using MTP, files added via TestContext.AddResultFile were not copied to the results directory. The TRX file referenced them with absolute paths, making the results directory non-self-contained and causing warnings in Azure DevOps builds.
Changes:
Fixes #6782