Collect all crash dumps in dump directory (#4186) by Evangelink · Pull Request #8268 · microsoft/testfx

Evangelink · 2026-05-15T17:05:34Z

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:

Convert the dump file name pattern into a wildcard search pattern by replacing every %X placeholder (%p, %e, %h, %t, …) with * (collapsing adjacent wildcards).
Enumerate every file in the dump directory matching that pattern and publish each as a FileArtifact. This collects both the testhost dump and any child-process dumps written during the same crash.
The existing *.dmp fallback (when nothing matches the pattern) is kept; the fallback path is also normalized to use CrashDumpArtifactDisplayName instead of the extension's own CrashDumpDisplayName for 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 12 CrashDumpTests (6 new placeholder cases).

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>

Copilot

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 %X placeholders with *) and publishes all matching dump files as artifacts.
Keeps the existing *.dmp fallback 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

Review Summary

#	Dimension	Verdict
1	Algorithmic Correctness	🔴 1 MAJOR
13	Test Completeness & Coverage	🟡 1 NIT

✅ 19/21 dimensions clean.

Correctness — Path.GetDirectoryName returns "" (not null) for bare filenames, causing Directory.Exists("") → false and 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>

Copilot

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 3

- 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>

…all-dumps

Copilot

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 2

…all-dumps

Evangelink · 2026-05-17T19:15:33Z

@copilot address review comments

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot · 2026-05-17T19:48:23Z

@copilot address review comments

Addressed in 4965f7b.

I updated the generated CrashDump acceptance test asset to dispose both Process.GetCurrentProcess() and the spawned child Process via using declarations, resolving the two remaining review comments about process handle disposal.

Evangelink · 2026-05-18T13:19:54Z

@copilot address review comments

…all-dumps

Copilot

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 6

Copilot · 2026-05-18T13:37:05Z

@copilot address review comments

Addressed already in 4965f7b.

The remaining review items were fixed there by disposing both Process.GetCurrentProcess() and the spawned child Process via using declarations in the generated CrashDump acceptance test asset.

Evangelink · 2026-05-18T18:32:50Z

@copilot resolve the merge conflicts in this pull request

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot · 2026-05-18T18:43:02Z

@copilot resolve the merge conflicts in this pull request

Resolved in 6c6c335.

I merged latest main into this branch and resolved the conflicts in CrashDumpProcessLifetimeHandler.cs and CrashDumpTests.cs.

- Use OS-appropriate path comparer (case-sensitive on Linux/macOS, case-insensitive on Windows) for the pre-existing dump file snapshot so freshly produced dumps that differ only by casing are not skipped. - Narrow the primary dump enumeration to files sharing the configured extension (Path.GetExtension of DumpFileNamePattern) to avoid scanning every entry of a busy results folder. - Treat %% as a literal % in BuildDumpFileNameRegexPattern so the regex does not over-match unrelated files (matches createdump tool semantics). Added unit tests covering My%%App_%p.dmp, %%%p.dmp, %p%%.dmp, plus a regex over-match guard. - In the integration test asset, prefer Environment.ProcessPath when available and use ProcessStartInfo.ArgumentList to avoid platform-specific quoting issues for paths containing spaces/special characters. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- Track 'testhost dump produced' separately from 'any dump published'. The crash banner and the 'expected dump not found' warning are scoped to the testhost dump specifically, so publishing a dump for a crashed child no longer suppresses the warning when the testhost's own dump never materialized. - Make _preExistingDumpFiles readonly and union into it on OnTestHostProcessStartedAsync (instead of reassigning), so multiple start callbacks (e.g. host restart) cannot drop already-classified pre-existing entries. - Document the known limitation that two testhosts sharing the same dump directory can publish each other's dumps. - Drop the redundant Regex.Escape('%') wrapper (% is not a regex metacharacter). - Add unit test OnTestHostProcessExitedAsync_OnlyPublishesDumpsThatAppearedDuringTheRun that pre-populates a real temp directory with stale dumps, then verifies only fresh ones are published to the message bus. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- Fix M1 regression: derive testhostDumpProduced from the regex loop using a PID-baked-in 'testhost-specific' regex, rather than relying on File.Exists with the literal '%p'-substituted path. This restores correct behavior for patterns that use other placeholders (e.g. '%e', '%h', '%t') alongside '%p'. The File.Exists check is retained as a fallback for the '%p'-only/no-placeholder case so behavior is preserved when the dump directory does not exist. - Add unit test OnTestHostProcessExitedAsync_PatternWithMultiplePlaceholders_DoesNotEmitFalseMissingDumpWarning that uses a CapturingOutputDevice to assert the 'expected dump not found' warning is NOT emitted when the testhost dump is recognized via the regex. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Round 3 review came back with no major findings. Address the three minor items: - Comment clarity: tighten the testhostDumpRegex comment to call out the degenerate '%p'-absent case where the regex collapses to dumpFileNameRegex and testhost cannot be distinguished from a child dump by name. - Comment clarity: rewrite the File.Exists(expectedDumpFile) fallback comment to accurately describe the one scenario where it materially fires (testhost dump file was snapshotted as pre-existing) instead of overstating its '%p'-only coverage. - Tests: add OnTestHostProcessExitedAsync_PatternWithRepeatedPidPlaceholder_RecognizesTesthostDump (proves string.Replace substitutes every '%p') and OnTestHostProcessExitedAsync_TesthostAndChildBothCrashWithMultiPlaceholderPattern_PublishesBothAndSuppressesWarning (the actual M1 regression vector: testhost + child dumps with a multi-placeholder pattern; both must publish and the warning must stay quiet). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 2

…all-dumps

- Narrow OnTestHostProcessStartedAsync snapshot to the configured dump extension search pattern (same as exit-time enumeration) and honor the cancellation token while iterating. - Drop the unreachable `return 0` after Environment.FailFast in the child-crash branch on TFMs where FailFast is annotated [DoesNotReturn] (net8.0+); keep it under `#if NETFRAMEWORK` so the compiler still sees a return path on .NET Framework. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 2

…ame defense - Guard Environment.ProcessPath and ProcessStartInfo.ArgumentList with #if NET6_0_OR_GREATER so the generated CrashDump asset also compiles when targeting net462 (it is part of TargetFrameworks.All on Windows). Fall back to ProcessStartInfo.Arguments with manual quoting on .NET Framework. The child-crash branch is not exercised on .NET Framework but must still compile. - Filter Directory.GetFiles results by Path.GetExtension == .dmp in CrashDump_TesthostAndChildBothCrash_CollectsAllDumps to defend against Windows' legacy 8.3 short-name wildcard matching, mirroring the same defense already present in CrashDumpProcessLifetimeHandler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 15, 2026 17:05

Copilot started reviewing on behalf of Evangelink May 15, 2026 17:11 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpProcessLifetimeHandler.cs Outdated

Evangelink commented May 15, 2026

View reviewed changes

Comment thread src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpProcessLifetimeHandler.cs

Comment thread test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CrashDumpTests.cs Outdated

Evangelink and others added 2 commits May 15, 2026 19:45

Copilot AI review requested due to automatic review settings May 16, 2026 08:48

Copilot started reviewing on behalf of Evangelink May 16, 2026 08:49 View session

Copilot AI reviewed May 16, 2026

View reviewed changes

Copilot AI and others added 2 commits May 16, 2026 15:04

Merge branch 'main' into dev/evangelink/issue-4186-crashdump-collect-…

4c202f3

…all-dumps

Copilot AI review requested due to automatic review settings May 16, 2026 14:27

Copilot started reviewing on behalf of Evangelink May 16, 2026 14:28 View session

Copilot AI reviewed May 16, 2026

View reviewed changes

Comment thread test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CrashDumpTests.cs Outdated

Comment thread test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CrashDumpTests.cs Outdated

Evangelink mentioned this pull request May 16, 2026

Fix TelemetryTests flake from parallel dotnet test races #8292

Merged

Merge branch 'main' into dev/evangelink/issue-4186-crashdump-collect-…

a613c87

…all-dumps

Copilot started work on behalf of Evangelink May 17, 2026 19:42 View session

Dispose process handles in crash dump acceptance asset

4965f7b

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 17, 2026 19:48

Evangelink review requested due to automatic review settings May 17, 2026 19:48

Copilot finished work on behalf of Evangelink May 17, 2026 19:48

Merge branch 'main' into dev/evangelink/issue-4186-crashdump-collect-…

6b09a06

…all-dumps

Copilot AI review requested due to automatic review settings May 18, 2026 13:19

Copilot AI reviewed May 18, 2026

View reviewed changes

Copilot started work on behalf of Evangelink May 18, 2026 13:32 View session

Copilot finished work on behalf of Evangelink May 18, 2026 13:37

Copilot started reviewing on behalf of Evangelink May 18, 2026 13:57 View session

Copilot started work on behalf of Evangelink May 18, 2026 18:33 View session

Merge main and resolve crash dump conflicts

6c6c335

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot finished work on behalf of Evangelink May 18, 2026 18:43

Amaury Levé and others added 4 commits May 19, 2026 09:40

Copilot AI review requested due to automatic review settings May 19, 2026 09:05

Copilot started reviewing on behalf of Evangelink May 19, 2026 09:10 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

Comment thread src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpProcessLifetimeHandler.cs

Comment thread test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CrashDumpTests.cs Outdated

Evangelink and others added 2 commits May 19, 2026 12:57

Merge branch 'main' into dev/evangelink/issue-4186-crashdump-collect-…

586fb51

…all-dumps

Copilot AI review requested due to automatic review settings May 19, 2026 12:00

Copilot AI reviewed May 19, 2026

View reviewed changes

Comment thread test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CrashDumpTests.cs

Comment thread test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CrashDumpTests.cs Outdated

Copilot started reviewing on behalf of Evangelink May 19, 2026 12:16 View session

Evangelink merged commit 2983f9a into main May 20, 2026
39 of 44 checks passed

Evangelink deleted the dev/evangelink/issue-4186-crashdump-collect-all-dumps branch May 20, 2026 10:39

Conversation

Evangelink commented May 15, 2026

Problem

Fix

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Evangelink left a comment

Choose a reason for hiding this comment

Review Summary

Key finding

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Evangelink commented May 17, 2026

Uh oh!

Copilot AI commented May 17, 2026

Uh oh!

Evangelink commented May 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented May 18, 2026

Uh oh!

Evangelink commented May 18, 2026

Uh oh!

Copilot AI commented May 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants