Add crash report support to the CrashDump extension#8191
Conversation
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
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
| => commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashReportOptionName) && | ||
| !commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashDumpOptionName) | ||
| ? ValidationResult.InvalidTask(CrashDumpResources.CrashReportRequiresCrashDumpErrorMessage) | ||
| : commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashReportOnlyOptionName) && | ||
| (commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashDumpOptionName) || | ||
| commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashReportOptionName)) | ||
| ? ValidationResult.InvalidTask(CrashDumpResources.CrashReportOnlyCannotBeCombinedErrorMessage) | ||
| : ValidationResult.ValidTask; |
There was a problem hiding this comment.
Done in e8b4818. Refactored into explicit if-statements (one per invalid combination) so future mutually-exclusive crash options are easy to add.
| if (crashReportEnabled) | ||
| { | ||
| foreach (string prefix in Prefixes) | ||
| { | ||
| environmentVariables.SetVariable(new($"{prefix}{EnableCrashReportVariable}", EnableMiniDumpValue, false, true)); | ||
| } | ||
| } | ||
|
|
||
| if (crashReportOnlyEnabled) | ||
| { | ||
| foreach (string prefix in Prefixes) | ||
| { | ||
| environmentVariables.SetVariable(new($"{prefix}{EnableCrashReportOnlyVariable}", EnableMiniDumpValue, false, true)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Done in e8b4818. Renamed EnableMiniDumpValue to the neutral EnabledValue since it's now also used for DOTNET_EnableCrashReport and DOTNET_EnableCrashReportOnly.
…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 <223556219+Copilot@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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
| => commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashReportOptionName) && | ||
| !commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashDumpOptionName) | ||
| ? ValidationResult.InvalidTask(CrashDumpResources.CrashReportRequiresCrashDumpErrorMessage) | ||
| : commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashReportOnlyOptionName) && | ||
| (commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashDumpOptionName) || | ||
| commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashReportOptionName)) | ||
| ? ValidationResult.InvalidTask(CrashDumpResources.CrashReportOnlyCannotBeCombinedErrorMessage) | ||
| : ValidationResult.ValidTask; |
There was a problem hiding this comment.
Done in 64d9b6f (and the prior d68feac). Beyond the requested refactor: the entire validation method is now a no-op because the CLI surface was simplified to a single --crash-report flag (see #8191 (comment) / the rename discussion in PR comments). The two mutually-exclusive rules and their error messages no longer exist.
| <value>[net6.0+ only] Generate a crash report file if the test process crashes. Requires '--crashdump'.</value> | ||
| </data> | ||
| <data name="CrashReportRequiresCrashDumpErrorMessage" xml:space="preserve"> | ||
| <value>You specified '--crashreport' but did not enable crash dumps, add --crashdump to the command line</value> |
There was a problem hiding this comment.
| if (crashDumpEnabled || crashReportOnlyEnabled) | ||
| { | ||
| foreach (string prefix in Prefixes) | ||
| { | ||
| environmentVariables.SetVariable(new($"{prefix}{EnableMiniDumpVariable}", EnableMiniDumpValue, false, true)); | ||
| } | ||
| } |
| { | ||
| await _outputDisplay.DisplayAsync(this, new ErrorMessageOutputDeviceData(string.Format(CultureInfo.InvariantCulture, CrashDumpResources.CannotFindExpectedCrashDumpFile, expectedDumpFile)), cancellationToken).ConfigureAwait(false); | ||
| foreach (string dumpFile in Directory.GetFiles(Path.GetDirectoryName(expectedDumpFile)!, "*.dmp")) | ||
| string expectedCrashReportFile = $"{expectedDumpFile}.crashreport.json"; |
There was a problem hiding this comment.
Done in d68feac. Extracted private const string CrashReportFileExtension = .crashreport.json; and private const string CrashReportFileSearchPattern = * + CrashReportFileExtension; so the suffix is defined in one place. The resource string is parameterized with {1} to receive the search pattern.
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task CrashReportOnly_CustomDumpName_CreateOnlyCrashReport() |
| if (_commandLineOptions.IsOptionSet(CrashDumpCommandLineOptions.CrashReportOptionName)) | ||
| { | ||
| foreach (string prefix in Prefixes) | ||
| { | ||
| if (!environmentVariables.TryGetVariable($"{prefix}{EnableCrashReportVariable}", out OwnedEnvironmentVariable? enableCrashReport) | ||
| || enableCrashReport.Value != EnableMiniDumpValue) | ||
| { | ||
| AddError(errors, $"{prefix}{EnableCrashReportVariable}", EnableMiniDumpValue, enableCrashReport?.Value); |
There was a problem hiding this comment.
Done in d68feac. Extracted a ValidateBothPrefixes(string variableName, string expectedValue) local function and replaced all three duplicated blocks with calls to it.
| [TestMethod] | ||
| public async Task CrashReportOnly_Cannot_Be_Combined_With_Other_Crash_Options() | ||
| { | ||
| var provider = new CrashDumpCommandLineProvider(); | ||
| var options = new Dictionary<string, string[]> | ||
| { | ||
| { CrashDumpCommandLineOptions.CrashDumpOptionName, [] }, |
There was a problem hiding this comment.
Done in d68feac. The CrashReportOnly_Cannot_Be_Combined_With_Other_Crash_Options test became data-driven with [DataRow(CrashDumpCommandLineOptions.CrashDumpOptionName)] and [DataRow(CrashDumpCommandLineOptions.CrashReportOptionName)] so both invalid combinations are covered. The test was subsequently removed in 64d9b6f because --crashreport-only was eliminated as part of the CLI simplification.
| if (crashReportEnabled) | ||
| { | ||
| foreach (string prefix in Prefixes) | ||
| { | ||
| environmentVariables.SetVariable(new($"{prefix}{EnableCrashReportVariable}", EnableMiniDumpValue, false, true)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Done in d68feac. Renamed EnableMiniDumpValue to the neutral EnabledValue since it's reused by all three runtime variables.
- 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 <223556219+Copilot@users.noreply.github.com>
|
@copilot address review comments |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
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 <223556219+Copilot@users.noreply.github.com>
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> |
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.
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
--crashreportto generate a crash report alongside--crashdump--crashreport-onlyto request only the JSON crash report--crashreportrequires--crashdump--crashreport-onlycannot be combined with other crash optionsRuntime configuration
DOTNET_EnableCrashReportDOTNET_EnableCrashReportOnlyDbgEnableMiniDumpenabled for--crashreport-only, since the runtime still requires createdump activation to emit the reportArtifacts and user-visible behavior
*.crashreport.jsonCoverage
--crashreportusageExample