Uh oh!
There was an error while loading. Please reload this page.
Update generic-crash banner to flag missing diagnostic artifacts (#8191 follow-up) - #8328
Merged
Amaury Levé (Evangelink) merged 2 commits intoMay 18, 2026
Merged
Conversation
… follow-up) When --crashdump or --crash-report is requested but the expected artifact does not appear on disk, OnTestHostProcessExitedAsync falls through to the (false, false) branch and prints CrashDumpProcessCrashed alongside the per-artifact CannotFindExpected* message. The banner itself was generic (Test host process with PID 'X' crashed), which understated the failure: a quick skim of the output gave no indication that an expected diagnostic artifact was missing. IsEnabledAsync guarantees at least one of --crashdump / --crash-report is set whenever the handler runs, so reaching the (false, false) branch always means an expected artifact was not produced. Update the banner text to reflect that explicitly so the per-artifact diagnostic isn't the only signal. Addresses #8191 (comment). XLF files regenerated via /t:UpdateXlf. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up to #8191. When --crashdump or --crash-report is requested but no diagnostic artifact lands on disk, the crash banner falls through to the generic CrashDumpProcessCrashed message. This PR updates that resource string to explicitly call out that the expected artifact was not produced, giving users a banner-level signal that something is wrong with diagnostic capture.
Changes:
- Update
CrashDumpProcessCrashedvalue inCrashDumpResources.resxto mention missing diagnostic artifacts. - Regenerate all 13 localized
*.xlffiles with the new<source>and<target state="new">(target left in English for translation).
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/CrashDumpResources.resx | Updated English value for CrashDumpProcessCrashed. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.cs.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.de.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.es.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.fr.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.it.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.ja.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.ko.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.pl.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.pt-BR.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.ru.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.tr.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.zh-Hans.xlf | Regenerated source + new target placeholder. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.zh-Hant.xlf | Regenerated source + new target placeholder. |
Copilot's findings
- Files reviewed: 14/14 changed files
- Comments generated: 0
Amaury Levé (Evangelink)
left a comment
MemberAuthor
There was a problem hiding this comment.
Review: PR #8328 — Update generic-crash banner to flag missing diagnostic artifacts
| # | Dimension | Status | Notes |
|---|---|---|---|
| 1 | Algorithmic Correctness | ✅ | (false, false) branch is the only caller of CrashDumpProcessCrashed; IsEnabledAsync guarantees at least one flag is set, so that branch is correctly the "artifact(s) not produced" case. |
| 2 | Threading & Concurrency | ✅ N/A | No code changes. |
| 3 | Security & IPC Contract Safety | ✅ N/A | No code changes; message text only. |
| 4 | Public API & Binary Compatibility | ✅ | Resource strings are not public API surface; no PublicAPI.Unshipped.txt entry needed. |
| 5 | Performance & Allocations | ✅ N/A | No code changes. |
| 6 | Cross-TFM Compatibility | ✅ N/A | No code changes. |
| 7 | Resource & IDisposable Management | ✅ N/A | No code changes. |
| 8 | Defensive Coding at Boundaries | ✅ N/A | No code changes. |
| 9 | Localization & Resources | Two findings — see inline comments: (a) missing comma in English source string; (b) verify XLF files were auto-generated by the build rather than hand-edited. | |
| 10 | Test Isolation | ✅ N/A | No test changes. |
| 11 | Assertion Quality | ✅ | No existing integration tests assert on the old CrashDumpProcessCrashed message text directly; no test regression risk found. |
| 12 | Flakiness Patterns | ✅ N/A | No test changes. |
| 13 | Test Completeness | ✅ | Message-only change; integration test coverage for the (false, false) path (missing artifact) would be a separate concern pre-existing this PR. |
| 14 | Data-Driven Test Coverage | ✅ N/A | No test changes. |
| 15 | Code Structure | ✅ N/A | No code changes. |
| 16 | Naming & Conventions | ✅ | Resource key CrashDumpProcessCrashed is retained; naming is consistent with sibling keys. |
| 17 | Documentation Accuracy | ✅ | PR description accurately describes the change and the reasoning. |
| 18 | Analyzer Quality | ✅ N/A | No analyzer changes. |
| 19 | IPC Wire Compatibility | ✅ N/A | No serialized types changed. |
| 20 | Build Infrastructure & Dependencies | ✅ | Only .resx and .xlf files changed; no build infrastructure impact. |
| 21 | Scope & PR Discipline | ✅ | Single focused concern; references the parent PR #8191. |
✅ 19/21 dimensions clean.
Summary of findings
Grammar — missing comma (NIT) · CrashDumpResources.resx line 151
The new string "crashed but the expected..." should be "crashed, but the expected..." to match the comma-before-conjunction style used in every other message in this file ("crashed, a dump file and crash report were generated", etc.).
XLF auto-generation policy (MODERATE) · all 13 .xlf files
Per .github/copilot-instructions.md, XLF files must never be manually edited — they must be generated by the build. The content looks correct (state="new", English text as target), but please confirm these were produced by running the build and not hand-authored. If hand-authored, revert and let the build regenerate them.
Generated by Expert Code Review (on open) for issue #8328 · ● 5.4M
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Amaury Levé (Evangelink)
commented
May 18, 2026
MemberAuthor
Copilot address review comments |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Contributor
Done in |
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 freeto 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.
Follow-up to #8191 addressing the one remaining review item that genuinely needed a code change.
Background
Discussion r3258181945 pointed out that when
--crashdumpor--crash-reportis requested but the expected artifact does not appear on disk,OnTestHostProcessExitedAsyncfalls through to the(false, false)branch of the new banner switch and emits the genericCrashDumpProcessCrashedmessage (Test host process with PID 'X' crashed). The per-artifactCannotFindExpected*messages are still printed below, but a quick skim of the output gave no banner-level signal that an expected diagnostic artifact was missing.Change
IsEnabledAsyncguarantees at least one of--crashdump/--crash-reportis set whenever the handler runs, so reaching the(false, false)branch always means an expected artifact was not produced. UpdateCrashDumpProcessCrashedto reflect that explicitly:No code-side changes — only the resource value and the regenerated XLF files. The
(false, false)branch already usesCrashDumpProcessCrashed, so the new wording flows through automatically.Other PR review items
All other open review comments on #8191 (10 of them) were already addressed in the merge — replies sent on each thread acknowledging the existing fix with file:line citations. The two remaining "discussion-only" items (option naming and
--help/--inforelease-notes mention) were replied to in-thread explaining the rationale.Validation
.\build.cmd -c Release— 0 warnings, 0 errors.