Disambiguate TRX filenames when running multiple test modules#54555
Disambiguate TRX filenames when running multiple test modules#54555Evangelink wants to merge 5 commits into
Conversation
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>
There was a problem hiding this comment.
Pull request overview
Disambiguates the TRX file name passed to Microsoft.Testing.Platform's TRX report extension when the SDK orchestrates multiple test modules, so each module writes a distinct file instead of overwriting siblings. Substitution is performed by the SDK so the fix works against MTP versions that predate the {asm}/{tfm} placeholders.
Changes:
- New
TrxReportArgumentsRewriterrewrites--report-trx[-filename]per module (preserving directory, handling=form, sanitizing, inferring TFM fromTargetPathfor--test-modules), with timestamp injection when only--report-trxwas passed. - Plumbs an
isMultiTestModuleflag fromMicrosoftTestingPlatformTestCommandthroughTestApplicationActionQueuetoTestApplication, where the rewriter is invoked when assembling the child process arg list. - Adds
TotalTestModuleCounttoITestHandler(and bothMSBuildHandler/TestModulesFilterHandler) so the command knows whether it is running >1 module.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/dotnet/Commands/Test/MTP/TrxReportArgumentsRewriter.cs | New rewriter that produces a unique TRX filename per module. |
| src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs | Stores _isMultiTestModule and routes test arguments through the rewriter. |
| src/Cli/dotnet/Commands/Test/MTP/TestApplicationActionQueue.cs | Threads isMultiTestModule through to TestApplication construction. |
| src/Cli/dotnet/Commands/Test/MTP/MicrosoftTestingPlatformTestCommand.cs | Computes isMultiTestModule from testHandler.TotalTestModuleCount. |
| src/Cli/dotnet/Commands/Test/MTP/ITestHandler.cs | Adds TotalTestModuleCount to the handler contract. |
| src/Cli/dotnet/Commands/Test/MTP/MSBuildHandler.cs | Implements TotalTestModuleCount by summing per-group module counts. |
| src/Cli/dotnet/Commands/Test/MTP/TestModulesFilterHandler.cs | Implements TotalTestModuleCount from the resolved test-module path list. |
| test/dotnet.Tests/CommandTests/Test/TrxReportArgumentsRewriterTests.cs | New unit tests covering pass-through, rewrite, equals form, placeholder short-circuit, directory preservation, TFM inference, no-extension, and timestamp uniqueness. |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 0
Adds end-to-end coverage for the fix to microsoft/testfx#7345: * MultipleTestModulesWithExplicitTrxFilename_ProducesOneTrxPerModule - verifies that `dotnet test --report-trx --report-trx-filename results.trx` against a multi-module solution writes results_<asm>_<tfm>.trx for each module instead of having modules race to overwrite a shared file. * MultipleTestModulesWithDefaultTrxFilename_ProducesUniqueFilesWithoutOverwriteWarning - verifies that `dotnet test --report-trx` (no explicit filename) produces <asm>_<tfm>_<timestamp>.trx files and that MTP's "will be overwritten" warning is not emitted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| var testApp = new TestApplication(module, buildOptions, testOptions, output, onHelpRequested); | ||
| BuildOptions buildOptionsForModule = buildOptions with | ||
| { | ||
| TestApplicationArguments = TrxReportArgumentsRewriter.RewriteIfNeeded(buildOptions.TestApplicationArguments, module, isMultiTestModule) |
There was a problem hiding this comment.
The "test module" naming here is confusing. Looking at the name I would expect this to apply only to --test-modules, but it's not.
| private const string ReportTrxOption = "--report-trx"; | ||
| private const string ReportTrxFilenameOption = "--report-trx-filename"; | ||
| private const string TrxExtension = ".trx"; |
There was a problem hiding this comment.
I'm not convinced SDK should be concerned about any of that.
Again, I think this is purely an MTP feature, the feature was provided, and users are expected to update to latest MTP.
I wouldn't complicate the implementation in SDK and introduce additional maintenance and potential risks just because "people might be on older MTP version that doesn't support placeholders".
This even violates the initial designs around MTP that we think shipping via NuGet out of SDK release cycle helps us getting fixes to the users faster.
Move TRX argument rewriting before TestApplication creation, avoid special-casing platform placeholders, preserve malformed filename arguments, and use a higher-precision timestamp for injected TRX names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e7a605c to
d1e8212
Compare
Per review feedback on PR #54555, restrict the SDK's TRX filename handling to the minimum: only inject --report-trx-filename when running multiple test modules with --report-trx and no explicit filename. If the user supplies --report-trx-filename, the SDK passes it through untouched and leaves any overwrite warning to MTP. Removed placeholder expansion, TFM-from-path inference, and equals-form value extraction. Tests updated to match the simplified contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I'll fix by doing a change in the default name directly in MTP |
Fixes microsoft/testfx#7345.
When
dotnet testorchestrates more than one test module (via--test-modulesglob or a multi-project / multi-TFM solution), the same--report-trx-filenamevalue was forwarded verbatim to every module, so each module's TRX overwrote the previous one.Approach
The SDK now disambiguates 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 in2.3.0-preview.26274.1(microsoft/testfx#8223).Behavior
--report-trx-filename foo.trx→ rewritten tofoo_<asm>_<tfm>.trxper module. No timestamp is added here so a user who explicitly chose a stable file name keeps the same semantics they get today with MTP.--report-trxonly → SDK injects--report-trx-filename <asm>_<tfm>_<yyyy-MM-dd_HH_mm_ss>.trx. The UTC timestamp mirrors MTP's natural default (<user>_<machine>_<time>.trx) so re-runs don't trip theWarning: Trx file '...' already exists and will be overwritten.message.{asm}/{pname}/{pid}placeholder → left alone (already unique per process).{tfm}alone is not considered unique — two modules can share a TFM, which is exactly the original bug.subdir/foo.trx→subdir/foo_<asm>_<tfm>.trx).--opt valueand--opt=valueforms.--test-modules(whereTargetFrameworkis null), infers TFM by walking parent directories ofTargetPathand parsing each segment as a TFM folder name (with astarts-with "net"pre-filter so RIDs likewin-x64aren't mistaken for frameworks).Testing
TrxReportArgumentsRewriterTests(incl. theory cases → 17 executed) covering: single-module pass-through, multi-module rewrite, equals-form args,{asm}/{pname}/{pid}short-circuit,{tfm}not short-circuiting, directory preservation, TFM inference from path (including RID layouts), no-extension case, unrelated arg preservation, and the timestamp-disambiguated injected default + re-run uniqueness.TestApplicationHandlerTests,TestModulesFilterHandlerTests,TestCommandValidationTests) still pass.