-
Notifications
You must be signed in to change notification settings - Fork 297
[CrashDump] Support {placeholder} tokens in --crashdump-filename via ArtifactNamingHelper#8957
[CrashDump] Support {placeholder} tokens in --crashdump-filename via ArtifactNamingHelper #8957Evangelink wants to merge 2 commits into
Conversation
...ArtifactNamingHelper
Wires ArtifactNamingHelper into CrashDumpEnvironmentVariableProvider so users can use the same {pname}/{pid}/{asm}/{tfm}/{time} template tokens as HangDump and the report extensions.
Because CrashDump only hands a filename pattern to the .NET runtime's createdump BEFORE the testhost launches, the testhost PID/exe name are not yet known. We resolve {asm}/{tfm}/{time} to literal values up-front and map {pid} -> %p and {pname} -> %e so the runtime expands them at crash-write time.
Also fixes the existing crash-report lookup which previously did only a single .Replace(%p, PID): when the dump pattern contains %e (now possible via {pname}), the literal expected path would not exist on disk. Switched to regex-based enumeration that reuses the existing testhostDumpRegex (which already turns %X into .* wildcards).
The sequence file name is now derived from the resolved user pattern (with {X} substituted) rather than the raw user input, keeping the sequence file aligned with the actual dump file name.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two Copilot bot review comments on #8953: - ArtifactNamingHelper entry: linked TrxReport (other report extensions were already linked). - JUnitReport entry: corrected 'declared in buildTransitive props' -> declared in buildMultiTargeting (the build/ and buildTransitive/ props only import that file). Cross-PR reconciliation: - ArtifactNamingHelper entry: re-added CrashDump as a direct consumer and documented the {pid}->%p / {pname}->%e deferred-mapping pattern, matching #8957 which actually wires CrashDump into ArtifactNamingHelper. Avoids a conflict between #8953 and #8957 on this same line. - JUnitReport entry: kept the 'no opt-in property at the package level' note for direct package consumers, and added a paragraph documenting the MSTest.Sdk-level <EnableMicrosoftTestingExtensionsJUnitReport> opt-in introduced in #8956. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR updates the CrashDump MTP extension to support the same {placeholder} filename template syntax used by other extensions, by wiring in ArtifactNamingHelper and improving crash-report discovery when runtime placeholders (e.g. %e) are involved.
Changes:
- Added
{pname}/{pid}/{asm}/{tfm}/{time}template resolution for--crashdump-filename, mapping{pid}→%pand{pname}→%efor runtime-time expansion. - Fixed crash-report discovery to handle dump name patterns containing runtime placeholders beyond
%p. - Updated help text, resources, glossary, and added a unit test covering crash-report matching with
%e.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CrashDumpTests.cs | Adds a unit test validating crash-report publication when the dump pattern includes runtime placeholders like %e. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoAllExtensionsTests.cs | Updates expected --help / --info output for the expanded --crashdump-filename placeholder documentation. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.zh-Hant.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.zh-Hans.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.tr.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.ru.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.pt-BR.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.pl.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.ko.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.ja.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.it.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.fr.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.es.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.de.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.cs.xlf | Updates localized resource entry for the expanded crashdump filename description. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/CrashDumpResources.resx | Expands CrashDumpFileNameOptionDescription to document placeholder tokens and runtime-deferred mappings. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Microsoft.Testing.Extensions.CrashDump.csproj | Links in ArtifactNamingHelper (and its dependency TargetFrameworkParser) into the CrashDump extension project. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpProcessLifetimeHandler.cs | Updates crash-report lookup to use regex-based matching so %e / other runtime placeholders don’t break discovery. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpExtensions.cs | Passes IClock to the environment-variable provider to support {time} resolution. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpEnvironmentVariableProvider.cs | Resolves user --crashdump-filename templates via ArtifactNamingHelper and derives sequence filename from the resolved template. |
| docs/glossary.md | Expands the ArtifactNamingHelper glossary entry with consumer details and CrashDump runtime-placeholder mapping notes. |
Copilot's findings
- Files reviewed: 21/21 changed files
- Comments generated: 2
Evangelink
commented
Jun 9, 2026
🧪 Test quality grade — PR #89573 tests graded (1 new unit test, 2 modified integration tests). Grade distribution: ×ばつ2, ×ばつ1. The new unit test earns B due to its ~57-line body — unavoidable when creating real files to simulate OS-level crash-dump behavior, but worth knowing. Both modified integration tests earn A: the changes are purely expected-string updates inside well-structured, focused process-level tests.
This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Re-run with
|
@Evangelink
Evangelink
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test Completeness & Coverage — 1 ISSUE
The crash-report regex matching (primary new behavior) is well covered by the new PatternWithRuntimePlaceholders test and the existing multi-placeholder/child-crash tests. ArtifactNamingHelper itself has thorough standalone tests.
The one gap is that ResolveUserDumpFileNameTemplate is new logic that is never exercised through UpdateAsync in any test. The specific contract — {pname} → %e (not %p) and {pid} → %p (not %e) — is not pinned. A swapped-argument regression silently produces a wrong dump-file pattern whose crash-report regex would then not match the file actually written by the runtime. See the inline comment for a suggested [DataRow] test.
Generated by Expert Code Review (on open) for issue #8957 · sonnet46 13.2M
@Youssef1313
Youssef1313
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is introducing a regression.
CrashDump should never replace placeholders that are known to runtime. The placeholders should remain as-is so that they flow properly to child processes.
@Youssef1313
Youssef1313
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other placeholders that are not known by runtime might as well be confusing if the crash happens in a child process.
Evangelink
commented
Jun 10, 2026
Sorry @Youssef1313, I think I miscommunicated the intent in the PR description — let me clarify and we can decide where to go from here.
What I actually wanted from this PR: just make --crashdump-filename accept the same {placeholder} vocabulary ({asm}, {tfm}, {time}, {pname}, {pid}) that --hangdump-filename, --report-trx-filename, --report-html-filename, and --report-junit-filename already accept. The motivation is purely API consistency — a user who learned the {...} syntax from one extension shouldn''t hit a different vocabulary on another. I do not want to change default behavior (no --crashdump-filename still resolves to {testAppName}_%p_crash.dmp), and I do not want to break the runtime per-process expansion contract.
On "should never replace placeholders known to runtime": agreed, and the PR doesn''t touch %p / %e / %h / %t — ArtifactNamingHelper.ResolveTemplate only matches {...} tokens and preserves everything else verbatim, so a user pattern like Dump_%p.dmp flows to the runtime unchanged. The {pid} → %p and {pname} → %e mapping was specifically intended to preserve runtime per-child-process expansion (we substitute {pid} with the literal string %p so the runtime still fills it in per crashing process). But I take your point that this overlaps confusingly with the runtime placeholders that already exist — happy to drop {pid} / {pname} entirely and let users keep using %p / %e directly if you''d prefer.
On "other placeholders... confusing if the crash happens in a child process": also a fair concern for {asm} / {tfm} — they''d resolve to the parent (entry assembly / TFM) values regardless of which child crashes. My read is they still describe the test run identity (which is the same across the child tree) rather than the crashing process identity, so they''re informative rather than misleading, but I see the argument the other way.
Concretely, three options I see — let me know which you''d like:
- Close this PR — accept that CrashDump should not adopt the
{placeholder}vocabulary at all because of the runtime-driven, child-process-aware nature of dump generation. - Scope down to deterministic-only tokens — keep only
{asm}/{tfm}/{time}(no{pid}/{pname}since the runtime equivalents already exist). This drops the need for theCrashDumpProcessLifetimeHandlerregex change because no new%eis introduced. - Keep the current shape — full
{asm}/{tfm}/{time}/{pid}/{pname}set, with{pid}/{pname}translated to%p/%eso the runtime continues to expand them per child.
My slight preference is (2) — it gives users the consistency win without overlapping with runtime placeholders or requiring the crash-report-lookup regex change — but I''m fine with (1) if you''d rather not add the surface area at all. Will you let me know which direction to take?
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Follow-up to PR #8951 (Copilot review comments) — wires ArtifactNamingHelper into the CrashDump extension so
--crashdump-filenamesupports the same{placeholder}template syntax as HangDump and the report extensions.What changed
CrashDumpEnvironmentVariableProvider{pname}/{pid}/{asm}/{tfm}/{time}tokens in the user-supplied filename viaArtifactNamingHelper.ResolveTemplate.createdumpbefore the testhost launches, the testhost PID / executable name are not yet known. We:{asm}/{tfm}/{time}to literal values up-front, and{pid}→%pand{pname}→%eso the runtime expands them at crash-write time.{X}substituted) rather than the raw user input, keeping the sequence file aligned with the actual dump file name.CrashDumpProcessLifetimeHandler— crash-report lookup fixstring expectedCrashReportFile = $"{expectedDumpFile}{CrashReportFileExtension}"only replaces%pwith the PID. When the dump pattern contains%e(now possible via{pname}), the literal expected path would not exist on disk and the crash report would be silently dropped.testhostDumpRegex(which already turns%Xplaceholders into.*wildcards) so any.crashreport.jsonwhose prefix matches the dump pattern is correctly discovered and published.Help text / resx
CrashDumpFileNameOptionDescriptionto document all supported placeholders, including the{pname}→%e/{pid}→%pruntime-deferred mapping and that{time}is captured when the crashdump environment is configured.HelpInfoAllExtensionsTestsupdated to match.Glossary
ArtifactNamingHelperentry expanded to list direct consumers (HangDump, CrashDump) and indirect consumers (HtmlReport / JUnitReport / TrxReport viaReportFileNameHelper), plus a note about the CrashDump runtime-placeholder mapping.Tests
OnTestHostProcessExitedAsync_CrashReport_PatternWithRuntimePlaceholders_PublishesMatchedReportcovers the regex-based crash-report matching when the dump pattern contains%e.Microsoft.Testing.Extensions.UnitTestssuite passes on net8 / net9 / net462 / net472.{testAppName}_%p_crash.dmp) is unchanged when--crashdump-filenameis not supplied.