Collect all crash dumps in dump directory (#4186)#8268
Open
Evangelink wants to merge 5 commits into
Open
Conversation
When the testhost crashes alongside one or more child processes, the .NET runtime writes a separate dump for each crashing process using the configured dump file name pattern. Previously the crash-dump extension only reported the testhost's own dump (matched by PID), losing the child-process dumps. Convert the dump file name pattern to a wildcard search pattern by replacing every '%X' placeholder with '*' and enumerate every matching file in the dump directory, publishing each one as a FileArtifact. 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 CrashDump extension to collect and publish all crash dumps produced in the configured dump directory when the testhost crashes, including dumps generated by crashing child processes (fixing #4186).
Changes:
- Converts the configured dump filename pattern into a wildcard pattern (replacing
%Xplaceholders with*) and publishes all matching dump files as artifacts. - Keeps the existing
*.dmpfallback when no pattern matches, and normalizes the artifact display name used in that fallback. - Adds parameterized unit coverage for placeholder-to-wildcard conversion behavior.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CrashDumpTests.cs | Adds unit tests for converting dump name placeholders into wildcard search patterns. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpProcessLifetimeHandler.cs | Updates dump discovery to publish all dumps matching the configured pattern and adds the placeholder-to-wildcard helper. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
Evangelink
commented
May 15, 2026
Member
Author
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | 🔴 1 MAJOR |
| 13 | Test Completeness & Coverage | 🟡 1 NIT |
✅ 19/21 dimensions clean.
- Correctness —
Path.GetDirectoryNamereturns""(notnull) for bare filenames, causingDirectory.Exists("")→falseand silently skipping all file enumeration when no directory prefix is in the pattern. - Test Coverage — no test covers the no-directory-path case in
OnTestHostProcessExitedAsync.
Key finding
Path.GetDirectoryName("dump_%p.dmp") → "" on .NET Core/5+. The null-guard passes but Directory.Exists("") is false, so neither the wildcard search nor the *.dmp fallback scan executes. Fix: default to "." when the result is null-or-empty.
Generated by Expert Code Review (on open) for issue #8268 · ● 4.9M
Adds CrashDump_TesthostAndChildBothCrash_CollectsAllDumps which spawns a child process from the testhost that also crashes via FailFast, then asserts both dumps end up on disk AND are reported as out-of-process file artifacts. The artifact-output assertion is what guards the #4186 fix: prior to the fix, only the testhost's own PID-matching dump was published as an artifact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses PR review feedback:
- Switch from glob pattern (Directory.EnumerateFiles searchPattern) to regex matching so that literal glob metacharacters ('*' or '?') in user-supplied dump file names (legal on Linux/macOS) are matched literally and do not cause unrelated .dmp files to be picked up.
- Treat an empty dumpDirectory (returned by Path.GetDirectoryName for a bare filename on .NET Core/5+) as '.' (current working directory) so enumeration is not silently skipped when the configured pattern has no directory prefix.
- Replace ReplaceCrashDumpPlaceholdersWithWildcard with BuildDumpFileNameRegex / BuildDumpFileNameRegexPattern, add GetDumpDirectory helper, and update unit tests to cover literal '*'/'?' in the file name and the no-directory-component case.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Snapshot pre-existing files in the dump directory before the testhost starts and exclude them when publishing crash dumps, so stale dumps from previous runs that match the same pattern are no longer surfaced as artifacts.
- Use Path.GetFileName + ordinal-ignore-case comparison instead of EndsWith for the dotnet executable detection in the CrashDump test asset.
- Add a 60-second bounded WaitForExit for the spawned child process and kill it on timeout to keep acceptance tests reliable.
- Handle empty pattern in GetDumpDirectory so Path.GetDirectoryName('') no longer throws ArgumentException on .NET Framework.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // exercise the crashdump extension's ability to collect dumps from child processes. | ||
| if (Environment.GetEnvironmentVariable("CRASHDUMP_SPAWN_CHILD_THAT_CRASHES") == "1") | ||
| { | ||
| Process self = Process.GetCurrentProcess(); |
| ? $"exec \"{Assembly.GetEntryAssembly()!.Location}\" " | ||
| : string.Empty; | ||
|
|
||
| Process child = Process.Start(new ProcessStartInfo(path, $"{argPrefix}--child-crash") |
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 #4186.
Problem
When the testhost crashes alongside one or more child processes, the .NET runtime writes a separate dump for each crashing process using the configured dump file name pattern (e.g. MyApp_%p_crash.dmp, where %p expands to the PID). Previously the crash-dump extension only reported the testhost's own dump (matched by PID), so dumps produced by child processes that crashed at the same time were silently dropped.
Fix
In CrashDumpProcessLifetimeHandler.OnTestHostProcessExitedAsync:
%Xplaceholder (%p,%e,%h,%t, …) with*(collapsing adjacent wildcards).*.dmpfallback (when nothing matches the pattern) is kept; the fallback path is also normalized to useCrashDumpArtifactDisplayNameinstead of the extension's ownCrashDumpDisplayNamefor the artifact display name.Tests
Added a parameterized unit test in CrashDumpTests.cs (ReplaceCrashDumpPlaceholdersWithWildcard_ConvertsPlaceholdersToWildcards) that covers single placeholder, multiple/adjacent placeholders, no placeholder, and trailing
%.Validated with:
dotnet build src/Platform/Microsoft.Testing.Extensions.CrashDump/Microsoft.Testing.Extensions.CrashDump.csproj -c Debug– 0 warnings, 0 errors.Microsoft.Testing.Extensions.UnitTests(net8.0): all 90 tests pass, including 12CrashDumpTests(6 new placeholder cases).