Allow paths in --report-trx-filename#8406
Merged
Merged
Conversation
… and refined guidance - Add repository layout overview (MTP, TestFramework, Adapter, Analyzers, SDK, tests, eng) - Add build/test command table and how to run a single test via --filter-uid / --treenode-filter - Document UpdateXlf MSBuild target and PlatformResources.cs IS_MTP_UNIT_TESTS caveat - Correct testing guidance: assertion style is per-project (BannedSymbols.txt) - Note TestContainer convention and [DoNotParallelize] requirement for shared-asset acceptance tests - Pull TODO comment policy from CONTRIBUTING.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Today the --report-trx-filename option only accepts a bare file name; any path component fails validation with 'file name argument must not contain path'. The original restriction predated placeholder support and was meant to avoid stomping on result files when multiple test projects targeted the same TestResults folder. Placeholder support (#5364) now lets users disambiguate per-project/tfm, so it is safe to lift the restriction. This change: - Removes the path rejection in TrxReportGeneratorCommandLine. The .trx extension check is now applied to the leaf file name so paths like 'subdir/report.trx' still validate. A new error message rejects inputs whose leaf file name is empty (e.g. 'sub/'). - Updates TrxReportEngine to resolve placeholders on the full input and then sanitize only the leaf file name with the existing invalid-char scrubber, so path separators in the directory portion are preserved. - Combines the sanitized name with the test results directory using Path.Combine, which short-circuits to the user's path when absolute and nests under the test results directory when relative. - Calls IFileSystem.CreateDirectory on the parent directory before opening the stream so missing intermediate directories are created automatically. - Updates the option description, refreshes the help/info acceptance test expectations, and adds unit and acceptance coverage for relative, nested and absolute paths plus the new empty-file-name validation. Fixes #5634. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the TRX report filename option (--report-trx-filename) to accept paths (relative or absolute), aligning with placeholder support and enabling users to direct TRX output into nested or external directories.
Changes:
- Updated TRX filename validation to allow relative/absolute paths while enforcing non-empty leaf file name and
.trxextension. - Updated TRX report generation to preserve directory components, sanitize only the leaf filename, and create intermediate directories.
- Updated unit + acceptance tests, help/info expectations, and localized resources for the new behavior/wording.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxTests.cs | Adds unit coverage for relative/absolute paths and leaf-only sanitization behavior. |
| test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxReportGeneratorCommandLineTests.cs | Updates validation tests to cover relative/absolute paths and empty-leaf rejection. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TrxTests.cs | Converts prior “path should fail” acceptance cases into success cases for relative/absolute paths. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoAllExtensionsTests.cs | Updates help/info expected text for --report-trx-filename description. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs | Preserves directory portion, sanitizes only leaf filename, combines with results dir, and ensures directories exist. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxCommandLine.cs | Relaxes validation to allow paths; enforces non-empty leaf + .trx extension. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/ExtensionResources.resx | Updates option description and adds new “leaf must not be empty” message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.cs.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.de.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.es.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.fr.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.it.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.ja.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.ko.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.pl.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.pt-BR.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.ru.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.tr.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.zh-Hans.xlf | Regenerated localization entries for updated description + new validation message. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.zh-Hant.xlf | Regenerated localization entries for updated description + new validation message. |
| .github/copilot-instructions.md | Expands contributor guidance (repo layout, build/test commands, localization + testing notes, TODO policy). |
Copilot's findings
- Files reviewed: 21/21 changed files
- Comments generated: 3
Evangelink
commented
May 20, 2026
Member
Author
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary — PR #8406 "Allow paths in --report-trx-filename"
Overall the PR is well-structured: the validation change is clean, the engine correctly splits directory + leaf before sanitising, CreateDirectory is called upfront, and the xlf files were regenerated rather than hand-edited. Tests cover the new happy and error paths.
Dimension results
| # | Dimension | Result |
|---|---|---|
| 1 | Algorithmic Correctness | |
| 2 | Threading & Concurrency | ✅ LGTM |
| 3 | Security & IPC Contract Safety | ../foo.trx now silently escapes the test-results directory — intentional but undocumented |
| 4 | Public API & Binary Compatibility | ✅ LGTM — no public surface changed |
| 5 | Performance & Allocations | ✅ LGTM |
| 6 | Cross-TFM Compatibility | ✅ LGTM — only Path.* and string.* APIs used |
| 7 | Resource & IDisposable Management | ✅ LGTM |
| 8 | Defensive Coding at Boundaries | ✅ LGTM |
| 9 | Localization & Resources | ✅ LGTM — new resource added to .resx, xlf regenerated, old string retained |
| 10 | Test Isolation | ✅ LGTM — acceptance tests use Guid.NewGuid() paths and clean up in finally |
| 11 | Assertion Quality | ✅ LGTM — MSTest Assert.* used in the unit-test project (correct for this project) |
| 12 | Flakiness Patterns | ✅ LGTM |
| 13 | Test Completeness & Coverage | ✅ LGTM — relative, absolute, empty-leaf, and extension cases covered |
| 14 | Data-Driven Test Coverage | "sub\\foo.trx" DataRow has platform-specific behaviour |
| 15 | Code Structure & Simplification | ✅ LGTM — final TrxCommandLine.cs is clean (no dead code) |
| 16 | Naming & Conventions | ✅ LGTM |
| 17 | Documentation Accuracy | ✅ LGTM — TrxReportFileNameOptionDescription updated accurately |
| 18 | Analyzer Quality | N/A |
| 19 | IPC Wire Compatibility | N/A |
| 20 | Build Infrastructure & Dependencies | ✅ LGTM |
| 21 | Scope & PR Discipline | ✅ LGTM — single concern, references issue #5634 |
Notable findings (non-blocking)
[DataRow("sub\\foo.trx")]— on Linux this is treated as a bare filename with a literal\character, not a sub-directory path. Either remove or annotate as Windows-only.[DataRow("../foo.trx")]— is valid and will silently write the TRX file outside the test results directory (one level up). This is intentional per the PR but deserves a comment in the code.RetryWhenIOExceptionAsync+ explicit path — a pre-existing issue made more reachable: retries on a fixed explicit path spin for up to 5 s before re-throwing anIOException. Not a blocker.
Generated by Expert Code Review (on open) for issue #8406 · ● 11.7M
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Addressed the three actionable Copilot review comments in 7f7f3af and pushed the update. |
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Contributor
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| @@ -132,8 +144,20 @@ public TrxReportEngine( | |||
| } | |||
|
|
|||
| string outputDirectory = _configuration.GetTestResultDirectory(); // add var for this | |||
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Reject Windows drive-relative paths (e.g. `C:foo.trx`) that are rooted but not fully qualified and would silently escape the test results directory. - Simplify `EscapesResultsDirectory` to use `Any` and a single shared separator array, replacing the foreach loop and removing a tautological comparison flagged by github-code-quality. - Drop the stray `// add var for this` leftover comment in `TrxReportEngine`. - Add a Windows-only unit test for drive-relative inputs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+69
to
+71
| <target state="new">The name of the generated TRX report. May include a relative or absolute path; relative paths are resolved against the test results directory and missing directories are created. | ||
| Supports the following placeholders: {pname} (test application name), {pid} (process ID), {asm} (entry assembly name), {tfm} (target framework moniker), {time} (timestamp). | ||
| Example: MyReport_{tfm}.trx</target> |
Member
Author
There was a problem hiding this comment.
Restored in dc49329. The previous translated value is back in place and marked as state="needs-review-translation" so the localization team can re-review the updated source text without regressing the existing translation.
Comment on lines
+69
to
+71
| <target state="new">The name of the generated TRX report. May include a relative or absolute path; relative paths are resolved against the test results directory and missing directories are created. | ||
| Supports the following placeholders: {pname} (test application name), {pid} (process ID), {asm} (entry assembly name), {tfm} (target framework moniker), {time} (timestamp). | ||
| Example: MyReport_{tfm}.trx</target> |
Member
Author
There was a problem hiding this comment.
Restored in dc49329. The previous translated value is back in place and marked as state="needs-review-translation" so the localization team can re-review the updated source text without regressing the existing translation.
Address PR feedback: tr/ru/pt-BR/pl/it xlf files had previous translated <target> values overwritten with the new English source and state=new. Restore the prior localized strings and mark them as needs-review-translation so the source-text update can be re-reviewed by the localization team without regressing the existing translations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5634.
Today the
--report-trx-filenameoption only accepts a bare file name; any path component fails validation withfile name argument must not contain path. The original restriction predated placeholder support (#5364) and was meant to avoid stomping on result files when multiple test projects targeted the sameTestResultsfolder. Now that placeholders can disambiguate per project / TFM, it is safe to lift the restriction.Behavior
--report-trx-filenamenow accepts a bare file name, a relative path, or an absolute path.Path.Combineshort-circuits on rooted second arg).IFileSystem.CreateDirectory..trxextension check now targets the leaf file name. A new validation rejects inputs whose leaf is empty (e.g.sub/).Out of scope
Tests
TrxReportGeneratorCommandLineTestsfor bare/relative/absolute paths and empty-leaf rejection.TrxTestsfor relative subdir, absolute path, and per-leaf sanitization (verifiesCreateDirectoryis invoked).TrxTestsacceptance tests: replaced the two*_TrxReportShouldFailcases with success counterparts (relative subdir + absolute path).HelpInfoAllExtensionsTestsupdated to match new option description.Localization
TrxReportFileNameOptionDescriptionand addedTrxReportFileNameMustNotBeEmpty. Regenerated all 13 xlf files viadotnet msbuild .../Microsoft.Testing.Extensions.TrxReport.csproj /t:UpdateXlf. LegacyTrxReportFileNameShouldNotContainPathkept in place to minimize localization churn.Verification
build.cmd -c Release -packsucceeds.Microsoft.Testing.Extensions.UnitTests: 297/297 pass on net9.0 and net462.TrxTests, 30HelpInfotests, 6MSTest TrxReporttests pass.