Add --crash-report option to the CrashDump extension#8191
Conversation
Co-authored-by: Evangelink <[email protected]>
There was a problem hiding this comment.
Pull request overview
Extends the CrashDump MTP extension with new --crashreport and --crashreport-only options that wire the test host runtime variables DOTNET_EnableCrashReport / DOTNET_EnableCrashReportOnly and publish the resulting *.crashreport.json as artifacts.
Changes:
- New CLI options with mutual-exclusion validation (
--crashreportrequires--crashdump;--crashreport-onlyexcludes the others). - Environment variable provider sets the new runtime variables (and keeps
DbgEnableMiniDumpfor--crashreport-only); lifetime handler emits new "dump only / report only / dump + report" messages and publishes crash report artifacts. - New resx/xlf entries, PACKAGE.md doc update, unit/acceptance/help-info test coverage for the new options.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpCommandLineOptions.cs | Adds option name constants for the two new flags. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpCommandLineProvider.cs | Registers new options and adds combined-options validation via chained ternary. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpEnvironmentVariableProvider.cs | Sets/validates DOTNET_EnableCrashReport(Only) and conditionalizes DbgEnableMiniDump. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpProcessLifetimeHandler.cs | Picks crash message variant; publishes .crashreport.json artifacts in addition to dumps. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/CrashDumpResources.resx | New resource strings for descriptions, errors, and crash messages. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/*.xlf | Auto-added state="new" entries mirroring the resx additions across all locales. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/PACKAGE.md | Documents new crash report capabilities. |
| test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CrashDumpTests.cs | Adds unit tests for valid/invalid combinations of the new flags. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CrashDumpTests.cs | Acceptance tests for dump+report, report-only, and --crashreport without --crashdump. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoAllExtensionsTests.cs | Updates expected help/info output to include the new options. |
Copilot's findings
- Files reviewed: 22/22 changed files
- Comments generated: 3
…shDump tests/code - Reorder --crashreport / --crashreport-only after --crashdump-type in the Help and Info expectations to match the platform's alphabetical ordering (CommandLineHandler.PrintOptionsAsync OrderBy(option.Name)). - Refactor CrashDump option validation into explicit if-statements for clarity. - Rename EnableMiniDumpValue -> EnabledValue (now reused for crash report vars). - Make CrashReportOnly_CustomDumpName_CreateOnlyCrashReport robust on Windows by doing an exact filename comparison instead of relying on Directory.GetFiles pattern matching, which can match 'customdumpname.dmp.crashreport.json'. Co-authored-by: Copilot <[email protected]>
Revert the validation refactor, EnabledValue rename, and the Windows-safe filename check in CrashReportOnly_CustomDumpName_CreateOnlyCrashReport. The HelpInfoAllExtensionsTests ordering fix is preserved. Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpProcessLifetimeHandler.cs:1
- Mirror of the existing dump-fallback path: if
Path.GetDirectoryName(expectedCrashReportFile)returns an empty string (when the dump file pattern is just a filename with no directory component),Directory.GetFiles("", ...)will throwArgumentException. The pre-existing dump branch has the same issue, but consider falling back to the current directory or skipping the scan when the directory is empty so the new code path doesn't introduce another instance of the same brittleness.
// Copyright (c) Microsoft Corporation. All rights reserved.
- Files reviewed: 22/22 changed files
- Comments generated: 8
- CrashDumpCommandLineProvider: replace chained ternary with explicit if-statements for clarity and easier extension. - CrashDumpEnvironmentVariableProvider: rename EnableMiniDumpValue to EnabledValue since it's now reused for the crash report environment variables (DOTNET_EnableCrashReport / DOTNET_EnableCrashReportOnly). - CrashReportOnly_CustomDumpName_CreateOnlyCrashReport: do an explicit filename comparison instead of relying on Directory.GetFiles' pattern matching, which on Windows can also match 'customdumpname.dmp.crashreport.json' for the literal pattern 'customdumpname.dmp'. Co-authored-by: Copilot <[email protected]>
|
@copilot address review comments |
Co-authored-by: Evangelink <[email protected]>
Replaces the two-flag design (--crashreport + --crashreport-only) with a single composable flag --crash-report. Behavior: - '--crashdump' alone: dump only (unchanged) - '--crash-report' alone: report only (DOTNET_EnableCrashReportOnly=1) - '--crashdump --crash-report': both (DOTNET_EnableCrashReport=1) Removes obsolete validation rules and resource strings (CrashReportRequiresCrashDumpErrorMessage, CrashReportOnlyCannotBeCombinedErrorMessage, CrashReportOnlyOptionDescription) and the corresponding XLF entries. Updates PACKAGE.md, unit tests, acceptance tests and HelpInfoAllExtensionsTests. Co-authored-by: Copilot <[email protected]>
CLI option rename and simplification (commit 64d9b6f)After discussing the CLI surface, I simplified the design from two flags (
Why:
This also lets us drop two error-message resources (and their XLF entries) and the option-mutual-exclusion test cases. All 8 review comments from the second review are addressed individually above. |
| if (crashDumpEnabled || crashReportEnabled) | ||
| { | ||
| foreach (string prefix in Prefixes) | ||
| { | ||
| environmentVariables.SetVariable(new($"{prefix}{EnableMiniDumpVariable}", EnabledValue, false, true)); | ||
| } |
| internal static class CrashDumpCommandLineOptions | ||
| { | ||
| public const string CrashDumpOptionName = "crashdump"; | ||
| public const string CrashReportOptionName = "crash-report"; |
| <data name="CrashReportArtifactDescription" xml:space="preserve"> | ||
| <value>The testhost process crash report file</value> | ||
| </data> | ||
| <data name="CrashReportArtifactDisplayName" xml:space="preserve"> | ||
| <value>Crash report file</value> | ||
| </data> | ||
| <data name="CrashReportOptionDescription" xml:space="preserve"> | ||
| <value>Generate a JSON crash report when the test process crashes. Combine with '--crashdump' to also generate a dump file. Requires .NET 7+ when used alone; .NET 6+ when combined with '--crashdump'.</value> | ||
| </data> | ||
| <data name="CrashDumpProcessCrashedDumpAndReportFileCreated" xml:space="preserve"> | ||
| <value>Test host process with PID '{0}' crashed, a dump file and crash report were generated</value> | ||
| </data> | ||
| <data name="CrashDumpProcessCrashedDumpFileCreated" xml:space="preserve"> | ||
| <value>Test host process with PID '{0}' crashed, a dump file was generated</value> | ||
| </data> | ||
| <data name="CrashDumpProcessCrashedReportFileCreated" xml:space="preserve"> | ||
| <value>Test host process with PID '{0}' crashed, a crash report was generated</value> | ||
| </data> |
- Reject --crash-report on Windows due to dotnet/runtime#80191 (runtime ignores DOTNET_EnableCrashReport/EnableCrashReportOnly on Windows). - Skip Windows-incompatible acceptance/unit tests and add Windows validation coverage. - Emit the crash banner based on what was actually produced on disk (per-artifact `generated`/`could not find` messaging), to avoid claiming success for files the runtime did not emit. - Remove redundant gate checks in CrashDump environment variable provider (IsEnabledAsync already gates the call). - Re-sort CrashDumpResources.resx alphabetically and regenerate XLF. - Update CrashReportOptionDescription and PACKAGE.md to mention the Windows limitation. - Use StringComparer.Ordinal for --help / --info option sorting in MTP so option order is deterministic across .NET FX and .NET Core (otherwise '--crash-report' vs '--crashdump' sort differently per runtime). - Add a CrashDumpProcessCrashed resource for the neutral banner used when no artifacts were produced. Co-authored-by: Copilot <[email protected]>
New Feature
What does this feature do?
The CrashDump extension can now ask the .NET runtime to generate JSON crash reports in addition to, or instead of, a dump. This makes crash triage lighter-weight in CI, especially for environments where full dump collection is expensive or impractical.
A single composable flag
--crash-reportwas chosen over the original two-flag (--crashreport+--crashreport-only) design: one flag = one artifact type, the option can be combined freely with--crashdump, and no awkward mutual-exclusion validation is needed.Why is this feature needed?
DOTNET_EnableCrashReportandDOTNET_EnableCrashReportOnlyare already available in the runtime, but the MTP CrashDump extension only exposed dump generation. Surfacing crash reports gives a cheaper diagnostic path and improves crash investigation on machines where developers cannot inspect native dumps directly.Implementation details
CLI surface
--crash-report(kebab-case, matching the broader MTP CLI convention such as--results-directory,--diagnostic-output-directory, etc.)--crashdump→ dump only--crash-report→ crash report only--crashdump --crash-report→ dump + crash report--crash-reportis rejected at command-line validation time because the .NET runtime ignoresDOTNET_EnableCrashReport/DOTNET_EnableCrashReportOnlyon Windows (see dotnet/runtime#80191). The error message points users to--crashdumpas the alternative.Runtime configuration
--crashdump --crash-report→DOTNET_DbgEnableMiniDump=1+DOTNET_EnableCrashReport=1--crash-report→DOTNET_DbgEnableMiniDump=1+DOTNET_EnableCrashReportOnly=1(createdump still needs MiniDump activation to emit the report)Artifacts and user-visible behavior
*.crashreport.jsongenerated/could not findmessaging), so it no longer claims success for an artifact the runtime did not emit.PACKAGE.mdto describe the new option and the Windows limitation.Platform fix (deterministic option ordering)
Microsoft.Testing.Platformnow sorts CLI options for--help/--infousingStringComparer.Ordinalinstead of the culture-aware default. Without this, the relative order of--crash-reportvs--crashdumpdiffered between .NET Framework (word sort ignores-) and .NET (Core) (ordinal-like ICU sort), which would make the help/info acceptance tests non-deterministic across TFMs.Coverage
--crash-reportalone,--crash-reportcombined with--crashdump, and the Windows-only validation rejection.--crashdump --crash-report(dump + report, Linux/macOS only)--crash-reportalone with default name (report only, Linux/macOS only)--crash-reportwith--crashdump-filename(report only with custom name, Linux/macOS only)--crash-reporton Windows → fails with the platform-limitation error--help/--infoexpectations for the new CLI option ordering.Example