Support placeholders in --report-trx-filename#8223
Conversation
Resolves #5364. The --report-trx-filename argument now supports placeholders provided by the artifact naming service: {pname}, {pid}, {asm}, {tfm}, {time}. Example: --report-trx-filename MyReport_{tfm}_{pid}.trx This fixes the long-standing issue where running dotnet test <name>.sln -- --report-trx --report-trx-filename test_results.trx produced a single TRX overwritten by every project, since each test project run produced a file with the same name. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds placeholder support to the --report-trx-filename option (e.g., {pname}, {pid}, {asm}, {tfm}, {time}), reusing the existing ArtifactNamingHelper (already used by HangDump). This resolves the issue where running the same --report-trx-filename across multiple test projects in a solution overwrote a single TRX file.
Changes:
- In
TrxReportEngine, resolve placeholders beforeReplaceInvalidFileNameCharsvia a newResolveTrxFileNamePlaceholdershelper. - Link
ArtifactNamingHelper.csandTargetFrameworkParser.csinto the TrxReport project. - Update the resource description and corresponding XLF files (marked
needs-review-translation); add 5 unit tests covering placeholder resolution, unknown placeholders, plain filenames,{time}, and sanitization of invalid characters from resolved values.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs | Adds ResolveTrxFileNamePlaceholders and invokes it before invalid-char replacement. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Microsoft.Testing.Extensions.TrxReport.csproj | Links shared ArtifactNamingHelper.cs and TargetFrameworkParser.cs (mirrors HangDump). |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/ExtensionResources.resx | Updates option description to document supported placeholders. |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Resources/xlf/ExtensionResources.*.xlf (14 locales) | Regenerated source text; targets marked needs-review-translation. |
| test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxTests.cs | Adds 5 unit tests covering placeholder resolution scenarios. |
Copilot's findings
- Files reviewed: 17/17 changed files
- Comments generated: 0
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary — PR #8223: Support placeholders in --report-trx-filename
The implementation is clean and well-structured. The placeholder resolution correctly reuses ArtifactNamingHelper, the #if NET guard for GeneratedRegex is in place, ReplaceInvalidFileNameChars is applied after resolution, and backward compatibility is preserved. The five new unit tests are good. Two findings below.
| Dimension | Verdict | Severity |
|---|---|---|
Documentation Accuracy ({pname} label) |
ISSUE | NIT |
Test Completeness ({asm} / {tfm} coverage) |
ISSUE | MODERATE |
| All other dimensions | LGTM | — |
Finding 1 — Documentation: {pname} labelled "process name"
The resx description says {pname} (process name) but the code returns Path.GetFileNameWithoutExtension(GetCurrentTestApplicationFullPath()), which is the test app's module filename — not Process.ProcessName. Under dotnet test those differ (the latter is "dotnet"). The PR description itself calls this out as an intentional choice. The help text should say "test application name" instead.
Finding 2 — Test Completeness: no tests for {asm} and {tfm}
{asm} is resolved via Assembly.GetEntryAssembly()?.GetName().Name (with an "unknown" fallback) and {tfm} tries TargetFrameworkAttribute then RuntimeInformation.FrameworkDescription. Both code paths go untested. Consider adding tests — even simple ones that assert the resolved value is non-empty — so any regression in the reflection-based resolution is caught.
Generated by Expert Code Review (on open) for issue #8223 · ● 11.9M
Two findings from the expert reviewer on PR #8223: 1. NIT (Documentation): The resx description labels {pname} as 'process name', but the implementation uses Path.GetFileNameWithoutExtension(GetCurrentTestApplicationFullPath()), which is the test app module name (not Process.ProcessName, which would be 'dotnet' under 'dotnet test'). Relabel as 'test application name' to match actual behaviour. 2. MODERATE (Test Completeness): The five existing tests cover {pname}, {pid}, {time}, unknown placeholders, and invalid-char sanitisation, but {asm} (resolved via Assembly.GetEntryAssembly) and {tfm} (resolved via TargetFrameworkAttribute / RuntimeInformation.FrameworkDescription) had no coverage. Add one test per placeholder asserting the placeholder is replaced and the resolved value is non-empty (we can't assert the exact value because it varies across the test runner's TFMs). XLF files regenerated via 'dotnet msbuild ... /t:UpdateXlf'. Build.cmd succeeds with 0 warnings, 0 errors. All 31 TrxTests pass on net9.0, net8.0, and net462. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Release
Review feedback:
- Re-add `{asm}` to the user-facing `--report-trx-filename` description and
to the help/info acceptance test expectations. The implementation already
supported `{asm}` and there is a unit test for it; restoring it to the
documented set keeps everything consistent. Addresses the two new comments
on PR #8223 from copilot-pull-request-reviewer.
Flaky test fix:
- TimingInfo.ToString() now formats DateTimeOffset values with
CultureInfo.InvariantCulture explicitly, instead of relying on the
ambient CurrentCulture. With method-level parallel execution and other
tests temporarily mutating CurrentCulture (e.g.
PlatformCommandLineProviderTests' culture-parsing tests),
TimingProperty_ToStringIsCorrect and
TimingProperty_WithStepTimings_ToStringIsCorrect were intermittently
failing on net462 Release with double-offset output
("+00:00 +00:00"). A diagnostic ToString() should be culture-invariant
by design; this both fixes the flake and improves the API.
XLF files regenerated via 'dotnet msbuild /t:UpdateXlf'.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Resolved. I merged Addressed in commit |
|
Awesome, thanks! When will this change be GA? |
|
I don't get have an exact date, there is quite some work I'd like to queue and I want to give some time for dogfooding. |
When `dotnet test` orchestrates more than one test module (via --test-modules
glob or a multi-project/multi-TFM solution), the same --report-trx-filename was
forwarded verbatim to each module, so every module's TRX overwrote the others.
The SDK now substitutes per-module assembly name and target framework into the
file name itself when more than one module is being run, leaving the
single-module behavior untouched. The substitution is done by the SDK rather
than by passing MTP's placeholder tokens, so the fix works against older
versions of Microsoft.Testing.Platform that don't yet support the
{asm}/{tfm} placeholders introduced in 2.3.0-preview.26274.1
(microsoft/testfx#8223).
When only `--report-trx` is passed (no filename), the SDK also includes a
UTC timestamp in the injected file name so re-running `dotnet test` doesn't
trip MTP's ""Trx file '...' already exists and will be overwritten."" warning
on every module. This mirrors MTP's natural default
`<user>_<machine>_<time>.trx`, which never collides across runs.
Fixes microsoft/testfx#7345
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolves #5364.
The
--report-trx-filenameargument now supports placeholders provided by the existing artifact naming service (ArtifactNamingHelper):{pname}{pid}{asm}{tfm}net9.0){time}yyyy-MM-dd_HH-mm-ss.fffffff)Example:
This fixes the long-standing issue where running the same
--report-trx-filenamevalue for every project in a solution produced a single TRX overwritten by every test project (the last one wins).Implementation notes
ArtifactNamingHelperandTargetFrameworkParserare linked into the TrxReport project (mirroring the HangDump extension).{pname}usesPath.GetFileNameWithoutExtension(_testApplicationModuleInfo.GetCurrentTestApplicationFullPath())instead ofProcess.ProcessNameso users get the test app name rather thandotnet.{pid}usesIEnvironment.ProcessIdto avoid both theProcess.GetCurrentProcess()banned-API rule and the CA1416 browser-platform warning.ReplaceInvalidFileNameCharsso any unsafe characters in resolved values (e.g. parentheses, spaces) are still sanitized.{...}placeholders are passed through unchanged..trxsuffix, no path separators) is unchanged — placeholders never contain/or\, and the user still provides the literal.trxsuffix.--report-trx-filenamedescription inExtensionResources.resxdocuments the supported placeholders. XLF files were regenerated viamsbuild /t:UpdateXlf.Tests
5 new unit tests added in
TrxTests.cscovering:{pname}and{pid}resolution{time}resolved from the injectedIClockAll 144 Trx-related unit tests pass on net462/net472/net8.0/net9.0.