[CrashDump] Support {placeholder} tokens in --crashdump-filename via ArtifactNamingHelper#8957
[CrashDump] Support {placeholder} tokens in --crashdump-filename via ArtifactNamingHelper#8957Evangelink wants to merge 1 commit 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.
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
| ### ArtifactNamingHelper | ||
|
|
||
| A shared static helper compiled into MTP extensions via file linking (no NuGet service registration or InternalsVisibleTo required) that provides template-based naming for test artifact files (dump files, report files, etc.). Templates are strings containing `{placeholder}` tokens (case-sensitive, lowercase): `{pname}` (process name), `{pid}` (process ID), `{asm}` (entry-assembly name), `{tfm}` (target framework moniker, best-effort runtime detection), and `{time}` (high-precision UTC timestamp). Legacy `%p` patterns from earlier hang-dump versions continue to work. Custom per-call overrides can replace default placeholder values via a `Dictionary<string, string>`. Used by the [HangDump](#hangdump) and [CrashDump](#crashdump) extensions. | ||
| A shared static helper compiled into MTP extensions via file linking (no NuGet service registration or InternalsVisibleTo required) that provides template-based naming for test artifact files (dump files, report files, etc.). Templates are strings containing `{placeholder}` tokens (case-sensitive, lowercase): `{pname}` (process name), `{pid}` (process ID), `{asm}` (entry-assembly name), `{tfm}` (target framework moniker, best-effort runtime detection), and `{time}` (high-precision UTC timestamp). Legacy `%p` patterns from earlier hang-dump versions continue to work. Custom per-call overrides can replace default placeholder values via a `Dictionary<string, string>`. Used directly by the [HangDump](#hangdump) and [CrashDump](#crashdump) extensions, and indirectly by the report extensions ([HtmlReport](#htmlreport), [JUnitReport](#junitreport), and TrxReport) via the shared `ReportFileNameHelper`. The CrashDump consumer maps `{pid}` to the .NET runtime's `%p` and `{pname}` to `%e` so they expand at crash-write time (the testhost PID is not known when the environment variables are configured). |
| if (matchedCrashReportFile is not null) | ||
| { | ||
| await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(expectedCrashReportFile), CrashDumpResources.CrashReportArtifactDisplayName, CrashDumpResources.CrashReportArtifactDescription)).ConfigureAwait(false); | ||
| await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(matchedCrashReportFile), CrashDumpResources.CrashReportArtifactDisplayName, CrashDumpResources.CrashReportArtifactDescription)).ConfigureAwait(false); | ||
| } |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| catch | ||
| { | ||
| // Best effort cleanup. | ||
| } |
🧪 Test quality grade — PR #89573 tests graded (1 new unit test, 2 modified integration tests). Grade distribution: A×2, B×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
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
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
| // {pname}/{pid} entries are the runtime placeholders themselves. This keeps the resolver | ||
| // generic and avoids a second post-processing pass. | ||
| Dictionary<string, string> replacements = ArtifactNamingHelper.GetStandardReplacements( | ||
| processName: "%e", |
There was a problem hiding this comment.
Test Completeness & Coverage — ISSUE
ResolveUserDumpFileNameTemplate is new logic with no end-to-end test.
Concrete failing scenario: if %e and %p are accidentally swapped in the call to GetStandardReplacements (or if a future refactor changes the argument order), {pname} → %p and {pid} → %e. The dump would then be named e.g. 123_testhost.dmp instead of testhost_123.dmp. The crash-report regex (built from the wrong pattern) would not match the actual file on disk → no crash report published, silently.
No existing test would catch this regression because:
- The new test for crash-report matching hardcodes
DumpFileNamePattern = "Dump_%e_%p.dmp"directly — it bypassesUpdateAsyncentirely. ArtifactNamingHelperTestscovers the{pname}→value and{pid}→value mechanics, but not the specific contract that in a crash-dump context{pname}must resolve to%eand{pid}to%p.
Suggested addition (in CrashDumpTests.cs, using the existing TestCommandLineOptions / FakeConfiguration helpers already present in the file):
[TestMethod]
[DataRow("{pname}_{pid}_crash.dmp", "%e_%p_crash.dmp")]
[DataRow("{asm}_{pid}.dmp", null)] // {asm} is resolved to the real asm name — just verify it doesn't contain a literal placeholder
[DataRow("literal.dmp", "literal.dmp")]
public async Task UpdateAsync_UserFilenameTemplate_SetsCorrectDumpFileNamePattern(
string userTemplate, string? expectedSuffix)
{
var commandLineOptions = new TestCommandLineOptions(new Dictionary<string, string[]>
{
{ CrashDumpCommandLineOptions.CrashDumpOptionName, [] },
{ CrashDumpCommandLineOptions.CrashDumpFileNameOptionName, [userTemplate] },
});
var configuration = new FakeConfiguration(); // or whatever stub is used in the file
var crashConfig = new CrashDumpConfiguration();
var provider = new CrashDumpEnvironmentVariableProvider(
configuration, commandLineOptions, crashConfig,
NullLoggerFactory.Instance, new FakeClock());
await provider.UpdateAsync(new FakeEnvironmentVariables());
if (expectedSuffix is not null)
Assert.IsTrue(crashConfig.DumpFileNamePattern?.EndsWith(expectedSuffix, StringComparison.Ordinal),
$"Expected pattern ending '{expectedSuffix}', got '{crashConfig.DumpFileNamePattern}'");
else
Assert.IsFalse(crashConfig.DumpFileNamePattern?.Contains('{'),
"Unresolved placeholder left in pattern");
}The {pname} → %e / {pid} → %p rows are the most important; they pin the semantic contract that makes the crash-report matching test meaningful.
Youssef1313
left a comment
Youssef1313
left a comment
There was a problem hiding this comment.
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.
microsoft-github-policy-service
Bot
added
the
needs/author-feedback
Youssef1313
left a comment
Youssef1313
left a comment
There was a problem hiding this comment.
Other placeholders that are not known by runtime might as well be confusing if the crash happens in a child process.
|
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 On "should never replace placeholders known to runtime": agreed, and the PR doesn''t touch On "other placeholders... confusing if the crash happens in a child process": also a fair concern for Concretely, three options I see — let me know which you''d like:
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? |
microsoft-github-policy-service
Bot
added
needs/attention
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.