Report retried and flaky tests by name (#5362) - #10329
Conversation
Retrying was visible only as counts, and two of those counts were wrong. This makes "which tests are flaky?" answerable from the console. Model (matching the CTRF definition already implemented in CtrfReportEngine.TestCollapsing): a test is *retried* when it produced results in more than one attempt, and *flaky* when it was retried, an earlier attempt failed, and the final attempt passed. Attribution is per TestNodeUid, so a folded data-driven test counts once — the honest answer given several rows share one uid. New `--show-flaky-tests` (on/off/true/false, on by default) controls the `flaky: N` line and the `Flaky tests:` section. Retried-but-never-recovered tests are deliberately not listed: they are already reported as failures with full error output, so a second listing would only duplicate. `total: N (+M retried)` is replaced by a dedicated `retried: N test(s), M extra run(s)` line, which separates two things the old suffix conflated. Fixes along the way: - The retry orchestrator counted failed *results*, not failed *tests*. `RetryFailedTestsPipeServer.FailedUID` (a List appended once per failed result) becomes `FailedTests`, a uid-keyed dictionary. A 4-test run with one flaky and one permanently failing test reported `total: 4 (+4 retried)`; it now reports `retried: 2 test(s), 4 extra run(s)`. The same inflation affected `--retry-failed-tests-max-tests`, which compared a result count against a test threshold. - `flaky` was `firstAttemptFailed - finalFailed`, a subtraction that can never yield names. It is now the exact set difference `union(retried attempts) \ final failed set`. - No summary at all was printed when the failure-threshold policy tripped or an attempt exited unexpectedly. The summary is now printed on every path, with a distinct "retrying stopped early" verdict so the attempt count is not read as "all attempts were used". The retry named pipe now carries each failed test's display name. Both ends of that pipe are always the same build — the orchestrator relaunches its own executable — so there is no protocol-compatibility window, unlike the dotnet test pipe. Not addressed here, and worth separate changes: MSTest's `[Retry]` attribute still discards every prior attempt in `UnitTestRunner.RunSingleTest` (`RetryResult.TryGetLast()`), so in-process retries stay invisible to the terminal and to CTRF; and the orchestrator still moves only the last attempt's report files instead of merging the per-attempt ones. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf79d9ce-970f-4c01-a087-26c3c686f60f
…tempt ones A retried run printed a "Test run summary" block per attempt. Every attempt after the first re-runs only the previously-failed tests, so those blocks described a shrinking filtered subset while reading as the run's verdict: a three-test suite ended with "total: 1" directly above a retry summary saying "total: 3". A four-attempt run produced five summary blocks, four of them wrong about the run. Retry attempts (the second and later) no longer print a run summary. The first attempt executes the whole suite, so its summary is accurate and is kept — as is every run that is not retried at all. Suppression is keyed off the attempt number the orchestrator already stamps on each test host via TESTINGPLATFORM_DOTNETTEST_ATTEMPTNUMBER, so nothing new crosses the process boundary. Only the verdict and its counts are dropped. Produced artifacts, the --show-slowest-tests ranking and the error recaps still render for every attempt, because the orchestrator does not restate them. For the retry summary to stand in for what was removed it had to become a complete run summary, so it gains "succeeded" and "skipped" and orders its lines like the platform one. That exposed a counting bug: RetryDataConsumer excluded skipped tests from the total it reported, so the retry summary's "total" silently disagreed with the platform's, which includes them. Skipped tests are now counted, and kept separate from the executed count that --retry-failed-tests-max-percentage measures against. "succeeded" is derived (total - failed - skipped) rather than reported: no single attempt knows it, but total and skipped are fixed for the run because retries only ever re-run failures. TotalTestsRunRequest becomes TestRunCountsRequest and carries the passed/failed/skipped breakdown. Both ends of that pipe are always the same build, so there is no protocol-compatibility window. An earlier revision of this change suppressed the summary for every attempt including the first. SdkTests caught the blast radius: enabling the retry extension then removed "Test run summary" — and with it the assembly and TFM identity — even from runs that pass on the first try. Keeping the first attempt's summary preserves that while still dropping the misleading blocks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf79d9ce-970f-4c01-a087-26c3c686f60f
…unit Code review of the previous two commits found three defects in the retry accounting. All three came from deriving facts the orchestrator never actually observed. Recovery was inferred as "was retried and is no longer in the failed set". That also matches a test which never ran at all, so an attempt that exited for any reason other than "tests failed" — an empty --filter-uid match (exit code 8), a crash partway through, a cancellation — promoted its pending tests to both "succeeded" and "flaky". The newly added summary for the interrupted path made this reachable and visible: a red "Failed!" verdict directly above "failed: 0 / succeeded: 3" and a test listed as "failed -> passed" that had not been re-run. The same inference also mislabelled a test that came back skipped on the retry. Attempts now report which of the tests they were asked to retry actually passed, and which came back skipped, so the orchestrator unions facts rather than subtracting sets. The list is bounded by the retry set, so it stays small on a large suite. The retry set is read in OnTestSessionStartingAsync rather than InitializeAsync because the host only populates it in BeforeRunAsync, which runs after extension initialization; reading it earlier would always have observed null and silently disabled the tracking. A retried test that ends skipped also has its outcome moved from failed to skipped in the suite counts. Those were captured on the first attempt, where the test was still failing, so without this it would have been counted as having succeeded. The previous commit made the failed set uid-keyed while the totals stayed per-result, and then combined them. For a folded data-driven test — several results under one uid — a 3-row all-failing suite reported "total: 3, failed: 1, succeeded: 2" where nothing succeeded, contradicting the platform summary printed directly above it. It also silently loosened --retry-failed-tests-max-percentage, whose numerator became uid-based while its denominator stayed result-based. Every count in the summary and in the threshold policy is now per result, matching the platform block; the uid set is kept only for what is inherently per-test: the retry filter and naming the flaky tests. When the first attempt dies before its session finishes it never reports counts at all. Those zeros were printed as a real suite, yielding a total smaller than the failed count. The summary now omits the counts it does not have rather than inventing them. Also from review: added the missing InternalAPI entries for the platform project, including *REMOVED* for the three Report*Test overloads whose signature changed; added the UTF-8 BOM required by .editorconfig to the two new files; restored alphabetical ordering in TerminalResources.cs. Adds an acceptance test for the skipped-on-retry case, which none of the existing tests covered. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf79d9ce-970f-4c01-a087-26c3c686f60f
Both were introduced by the previous commit and are the same mistake in different places: trusting a number the attempt never actually reported. `finalFailedTestResults` was assigned from every attempt before the exit-code branch was evaluated. An attempt that dies before its session finishes reports no counts, so the value silently reset to zero and overwrote the last real figure. A 10-test suite with 3 failures whose retry attempt crashes therefore rendered "failed: 0 / succeeded: 10" directly under a red verdict. It is now only taken from attempts that reported, so the last known figures survive. This is the same guard the previous commit applied to the first attempt's suite totals, which was simply missing for the final one. The skipped-after-retry adjustment was both order-dependent and in the wrong unit. It tracked a set of uids, adding on a skipped result and removing on a failing one, so a folded data-driven test whose rows produced both outcomes landed in the set or not purely by message arrival order. That per-uid count was then added to a per-result skipped total, which is off by (skipped rows - 1) for such a test. Attempts now report how many of each retried test's results were skipped, and the orchestrator keeps the newest attempt's value per uid — a test is only re-run while it still has a failing result, so the latest attempt describes its final state. Order cannot matter because nothing is removed. Also from review of the user-facing output: `flaky: N` sat between `skipped` and `duration`, where it reads as a fourth outcome category alongside failed/succeeded/skipped even though flaky tests are already counted in succeeded. It now reads `flaky: N (passed after retry)`, which both places it within succeeded and explains the term at the point a reader first meets it. Adds acceptance coverage for the two paths involved, neither of which was exercised before: a retry attempt that crashes without reporting, and a retried test that comes back skipped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf79d9ce-970f-4c01-a087-26c3c686f60f
… here The round-3 review found the adjustment double-counts, and the trace holds up. It assumed a retried test contributed nothing to the first attempt's skipped count — true only when one result maps to one test node uid. A folded data-driven test shares a uid across its rows, so a uid with one failing and one skipped row is retried as a whole, its skipped row is counted again, and a two-test suite renders "total: 2, failed: 1, succeeded: 0, skipped: 2": three outcome counts summing to more than the total, contradicting the first attempt's own summary printed just above. The Math.Max(0, ...) that was meant to be defensive turned the resulting negative into a silently wrong zero. The per-uid values were also never cleared when a test was re-run, so a test that eventually passed kept contributing a stale skipped result. Making it correct needs the first attempt's per-test skipped breakdown so the adjustment can be a delta against a baseline, plus a reset whenever a test is re-run. That is a per-uid outcome protocol, which is a larger piece of work than the imprecision justifies — the same protocol issue #10292 would need anyway. So the adjustment is removed. A test whose outcome changes from failed to skipped is counted in "succeeded" rather than "skipped". Total and failed remain correct, and the block always adds up. That is a smaller and more predictable error than a count that contradicts itself, and this sub-case has now produced a defect in two consecutive review rounds, which is its own argument against carrying the machinery. The acceptance test keeps asserting the flaky behaviour, which is the part that matters and was the original bug, and now also pins the counts so the internal consistency of the block is covered. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf79d9ce-970f-4c01-a087-26c3c686f60f
There was a problem hiding this comment.
Pull request overview
Enhances out-of-process retry reporting with accurate retry/flaky accounting, named flaky tests, and consolidated summaries.
Changes:
- Adds
--show-flaky-testsand named flaky-test reporting. - Reworks retry IPC and per-UID/result accounting.
- Suppresses misleading summaries for later retry attempts and expands coverage.
Show a summary per file
| File | Description |
|---|---|
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.cs |
Tests retry summary rendering. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/SilenceDrivenHeartbeatRendererTests.cs |
Updates reporter calls. |
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/HelpInfoTests.cs |
Adds option help expectation. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryWithMaxFailedTestsTests.cs |
Verifies early-stop summary. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs |
Expands retry acceptance coverage. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoTests.cs |
Updates help/info output. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoAllExtensionsTests.cs |
Updates extension help/info. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.Initialization.cs |
Configures flaky and summary options. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.zh-Hant.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.zh-Hans.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.tr.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.ru.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.pt-BR.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.pl.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.ko.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.ja.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.it.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.fr.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.es.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.de.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.cs.xlf |
Updates localized resources. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.cs |
Tracks retried and flaky UIDs. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporterOptions.cs |
Adds reporter options. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporterCommandLineOptionsProvider.cs |
Registers flaky option. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.cs |
Passes display names to accounting. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.cs |
Renders revised summaries. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalResources.resx |
Adds terminal strings. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalResources.cs |
Adds resource accessors. |
src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt |
Tracks internal API changes. |
src/Platform/Microsoft.Testing.Platform/CommandLine/FlakyTestsReportingOptions.cs |
Resolves flaky-reporting configuration. |
src/Platform/Microsoft.Testing.Extensions.Retry/Serializers/TotalTestsRunRequest.cs |
Removes obsolete count payload. |
src/Platform/Microsoft.Testing.Extensions.Retry/Serializers/TestRunCountsRequest.cs |
Adds detailed attempt payload. |
src/Platform/Microsoft.Testing.Extensions.Retry/Serializers/FailedTestRequest.cs |
Adds failed-test display names. |
src/Platform/Microsoft.Testing.Extensions.Retry/RetryThresholdPolicy.cs |
Revises threshold accounting. |
src/Platform/Microsoft.Testing.Extensions.Retry/RetrySummaryReporter.cs |
Renders reconciled retry summaries. |
src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs |
Aggregates attempt outcomes. |
src/Platform/Microsoft.Testing.Extensions.Retry/RetryLifecycleCallbacks.cs |
Registers the new serializer. |
src/Platform/Microsoft.Testing.Extensions.Retry/RetryFailedTestsPipeServer.cs |
Receives detailed attempt data. |
src/Platform/Microsoft.Testing.Extensions.Retry/RetryDataConsumer.cs |
Collects outcomes and recoveries. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.zh-Hant.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.zh-Hans.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.tr.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.ru.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.pt-BR.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.pl.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.ko.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.ja.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.it.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.fr.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.es.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.de.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.cs.xlf |
Updates localized retry strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/Resources/ExtensionResources.resx |
Adds retry summary strings. |
src/Platform/Microsoft.Testing.Extensions.Retry/InternalAPI/InternalAPI.Unshipped.txt |
Tracks retry internal APIs. |
Review details
- Files reviewed: 54/54 changed files
- Comments generated: 5
- Review effort level: Medium
This comment has been minimized.
This comment has been minimized.
Five review comments accepted, two declined. A folded uid can produce both passing and skipped rows, and the terminal reporter's flaky predicate only required zero failures. That disagreed with the retry extension, which already treats any skipped result as non recovery. The predicate now also requires zero skipped rows, so both surfaces answer "did this recover" the same way. Retry accounting was committed when the next attempt was scheduled rather than when it produced anything, so a launch that died before reporting still claimed a retried test and an extra run. The scheduled set is now held pending and only committed once the following attempt reports counts and at least one result. StoppedEarly was set for any threshold trip or interruption, including on the final configured attempt and when zero retries were configured. In those cases every attempt was used, so the verdict now only says retrying stopped early when attempts actually remained. --retry-failed-tests-max-tests compared failed results, so a folded test with several failing rows counted as several tests against an option whose own description is a number of tests. It now compares and reports distinct failed uids, while --retry-failed-tests-max-percentage stays result-based so its numerator and denominator remain in the same unit. The two options are mutually exclusive, so each branch stays internally consistent. Flaky test display names come from user code and were rendered unescaped, letting a name containing a newline or an ESC sequence forge summary lines or steer the terminal. They now go through the same control-picture policy the terminal reporter applies. The platform's helper is private to an embedded type and cannot be shared, so the policy is reimplemented with a note explaining why. Declined the two nullable-dereference reports: ApplicationStateGuard.Ensure carries [DoesNotReturnIf(false)], so Roslyn's flow analysis already treats the following dereference as safe, the projects build clean, and the Ensure-then-dereference idiom is used throughout the repository. Changing only these two call sites would make the file inconsistent for no gain. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf79d9ce-970f-4c01-a087-26c3c686f60f
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (4)
src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs:255
- A zero-test retry still sends a counts message, so
CountsReportedis true and this assignment overwrites the previous failure count with zero. BecauseZeroTestsis a non-success exit, the resulting retry summary is red while reportingfailed: 0and deriving the previously failing tests as succeeded—the same contradiction this guard avoids for a crash. Preserve the last reported tally when a later attempt reports no results.
if (retryFailedTestsPipeServer.CountsReported)
{
finalFailedTestResults = retryFailedTestsPipeServer.FailedTestResults;
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.cs:429
- This increments only for the first result under a UID in each retry attempt. Folded data-driven tests can emit several results for that same UID, so the platform summary undercounts
extra run(s)while the retry extension counts every observed retry result (RetryOrchestrator.cs:212-223). For example, the pass-plus-skip retry covered byTerminalTestReporterTestsemits two extra results but is currently asserted as one, making the two summaries disagree.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs:137 - The comment establishes that this scenario must finish successfully, but the generic header assertion also accepts
Retry summary: Failed!. Assert the exit code and exact passed verdict so a regression in all-skipped retry handling cannot leave this test green.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporterCommandLineOptionsProvider.cs:46 - No test actually invokes
--show-flaky-tests; the existing renderer test setsTerminalTestReporterOptions.ShowFlakyTestsdirectly, while the acceptance files only snapshot help text. Add an acceptance case using--show-flaky-tests offso option parsing and the separate retry-orchestrator resolver are both verified to suppress the flaky count and named section while retaining the retried line.
- Files reviewed: 54/54 changed files
- Comments generated: 0 new
- Review effort level: Medium
The latest Copilot review suppressed four observations as low confidence. Three were real. A retry attempt whose filter matches no test exits ZeroTests but still finishes its session, so it reports a well-formed set of zeros and CountsReported is true. The failing count was therefore reset to zero while the run stayed red, deriving the still-failing tests as succeeded. This is the same contradiction the crash guard already prevented, reached by a different path: reporting counts is not the same as having run something. Both the failing count and the retry accounting now require the attempt to have observed at least one result. The platform reporter incremented its extra-run counter once per test node per attempt, on the transition to a new attempt. A folded data-driven test reports one result per row under a single uid, so every row after the first went uncounted and the platform disagreed with the retry extension, which counts results. It now counts each result belonging to an attempt after the uid's first, which is what "extra runs" means. The folded pass-plus-skip test asserted the undercount and now asserts two. The skipped-on-retry test asserted only the summary header, which also matches a failing verdict, so a regression that turned that run red would have left it green. It now asserts the exit code and the exact verdict. Also added the missing end-to-end coverage for --show-flaky-tests. The unit tests set the reporter option directly and the acceptance files only snapshot help text, so nothing exercised the flag itself or the retry orchestrator's separate resolver, which runs in a different process. The fourth observation is covered by the third and needed no separate change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf79d9ce-970f-4c01-a087-26c3c686f60f
🧪 Test quality grade — PR #10329No new or modified test methods were identified in the changed regions Re-run with
|
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs:225
- This commits every scheduled UID as retried as soon as the attempt reports any result. If several UIDs were scheduled but one no longer matches (for example, a dynamic test disappears) while another runs, the missing UID is still included in
retriedTests, even though the PR's definition requires that specific test to report a result in multiple attempts. The child protocol needs to return the observed UIDs (including skipped ones), and this loop should commit only the intersection withpendingRetriedTests.
if (attemptObservedResults)
{
// Scheduling a retry is not enough to count it: the child can die before the selected tests
// produce results. Commit the pending retry only after the following attempt reports counts.
foreach (KeyValuePair<string, string> retriedTest in pendingRetriedTests)
{
retriedTests[retriedTest.Key] = retriedTest.Value;
}
src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs:259
- When an attempt publishes one or more failed results and then crashes before
OnTestSessionFinishingAsync,CountsReportedis false butFailedTestsalready contains those streamed failures. This branch leavesfinalFailedTestResultsat zero, so the new unknown-suite path prints only the red verdict and duration, despite its own contract saying known failures are shown. Preserve a streamed failed-result count as the fallback when aggregate counts are absent.
if (attemptObservedResults)
{
finalFailedTestResults = retryFailedTestsPipeServer.FailedTestResults;
- Files reviewed: 54/54 changed files
- Comments generated: 0 new
- Review effort level: Medium
Integrates #10329 ("Report retried and flaky tests by name"), which landed in main and reworked the same terminal retry subsystem. The two changes are complementary and now compose: #10329 reports flaky tests by name for out-of-process host retries, and in-process `[Retry]` attempts feed the same reporting, so a fail-then-pass `[Retry]` test is now listed under "Flaky tests:" with its attempt count. My ad-hoc "(+N retried)" total suffix is dropped in favor of #10329's richer `flaky:` / `retried:` lines. Review findings addressed: - Superseded attempts reached five outcome-oriented consumers that were missed in the original audit, each of which treats every terminal update as a real outcome: `AbortForMaxFailedTestsExtension` (a fail-then-pass `[Retry]` could trip `--maximum-failed-tests` and force exit code 13), `RetryDataConsumer` (requested an out-of-process host relaunch for a test whose final in-process attempt passed), `MSBuildConsumer` and `GitHubActionsAnnotationReporter` (logged build errors / annotations for a test that went on to pass), and `SimplifiedConsoleOutputDeviceBase` (counted and printed superseded failures). All now filter via the shared `IsSupersededRetryAttempt()` helper. - Class and assembly cleanup results are appended after the retry sequence and kept the default attempt number 1. Reported under the same test node uid as a test that finished on attempt 2+, the terminal's attempt ordering saw an out-of-order arrival and threw `Unreachable()`. Cleanup results now inherit the attempt the test finished on. - An empty final retry attempt was misreported. `SendTestResultsAsync` treats an empty array as an execution error, but the combined array was non-empty (superseded results only), so that path was skipped, every entry was then filtered out, and `AllPassed` vacuously returned true - turning a missing result into a passing test that unblocked dependents. The empty-result contract is now preserved. - Restored the UTF-8 BOM that a scripted edit stripped from TerminalOutputDevice.DataConsumption.cs, and added it to the two new files, per `.editorconfig` (`charset = utf-8-bom`). - Declared the new `TestProgressState` overloads in InternalAPI.Unshipped.txt, matching how the existing overloads are tracked. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa37ea7e-23a2-458b-bb8a-68ad56c832e6
The reviewer's suppressed (low-confidence) comments turned out to contain three real defects and one stale doc claim. Verified each before acting. - `AzureDevOpsArtifactUploader` was a sixth outcome-oriented consumer missing the superseded-attempt guard: it set `_hasTestFailures` on any failed update, so a fail-then-pass `[Retry]` run still received the Azure DevOps test-failures build tag. Guarded like the others. - The JUnit RFC's "Retry handling" section had duplicated bullets after the earlier merge: the stale MSTest bullet still claimed only the final attempt is surfaced, directly contradicting the new bullet two lines above, and the orchestrator was documented twice. This was a regression from resolving that conflict by keeping both sides. Kept the new MSTest bullet and main's detailed orchestrator bullet, dropped the stale and redundant ones. - `GetFlakyTests` derived the attempt count from the final `(host attempt, in-process attempt)` pair via `Math.Max`, which undercounts whenever both mechanisms advance: (host 1, retry 1) -> (host 1, retry 2) -> (host 2, retry 1) is three executions but reported two. The count is now tracked per uid as transitions arrive. Covered by `GetFlakyTests_WhenBothRetryMechanismsAdvance_CountsEveryExecution`, verified by mutation (restoring `Math.Max` fails it). - The changelog still promised the `total: N (+M retried)` suffix, which #10329 replaced with dedicated `flaky:` / `retried:` lines. Reworded to describe what the code actually emits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa37ea7e-23a2-458b-bb8a-68ad56c832e6
Implements the out-of-process half of #5362 —
--retry-failed-testsnow reports which tests were retried and which were flaky, instead of only counts (two of which were wrong).The in-process
[Retry]half and per-attempt report merging are tracked separately in #10292 and #10293.Before → after
A 3-test suite where one test fails and then passes on the retry.
Before — two summaries disagreeing about the same run:
After:
A 4-attempt run went from 5 summary blocks, 4 of them wrong about the run, to 2.
What changed
Model. A test is retried when it ran in more than one attempt and that attempt reported results, and flaky when it was retried and recovered. This is the definition
Microsoft.Testing.Extensions.CtrfReportalready implements. Attribution is perTestNodeUid, so a folded data-driven test counts once — the honest answer to the caveat @nohwnd raised on the issue, documented rather than worked around.--show-flaky-tests—on/off/true/false, arity 0..1, on by default. Controls theflaky:line and theFlaky tests:section. Retried-but-never-recovered tests are deliberately not listed: they are already reported as failures with full error output.total: N (+M retried)→ a dedicatedretried:line. The old suffix conflated two different things and counted the wrong one.Per-attempt summaries suppressed for retry attempts only. Attempt 1 runs the whole suite so its numbers are true and it is kept; attempts 2+ re-run a filtered subset and would report
total: 1for a 3-test suite. Keyed off the attempt number the orchestrator already sets viaTESTINGPLATFORM_DOTNETTEST_ATTEMPTNUMBER, so nothing new crosses the process boundary. Artifacts, the--show-slowest-testsranking and the error recaps still render for every attempt.Bugs fixed along the way
(+N retried)counted executions, not teststotal: 4 (+4 retried)for 2 retried testsflakywasfirstFailed − finalFailed--retry-failed-tests-max-testscounted failed results--retry-failed-tests-max-percentagestays result-based so its numerator and denominator matchtotal:excluded skipped, unlike the platform'sReview rounds
Three self-review rounds before opening, plus one round of PR feedback. Rounds 1 and 2 each found bugs in the previous round's fixes, so later rounds deliberately targeted the newest code.
ZeroTests, or crashing partway, promoted its pending tests to both succeeded and flaky. Attempts now report explicitly what recovered. Also: per-result and per-uid counts were being mixed (which had silently loosened--retry-failed-tests-max-percentage), and a first attempt that died before reporting renderedtotalsmaller thanfailed.finalFailedTestResultswas clobbered to zero by a non-reporting attempt, renderingfailed: 0under a red verdict; and the skipped-tracking was order-dependent.[Retry]attempts are discarded before they reach the platform, so in-process retries are invisible everywhere #10292 needs), and this sub-case had produced a defect in two consecutive rounds. A test whose outcome changes from failed to skipped is counted insucceeded; total and failed stay correct and the block always adds up. Documented in code and pinned by a test.StoppedEarlygated on attempts actually remaining; max-tests threshold switched to distinct uids; display names escaped). Two declined as false positives, sinceApplicationStateGuard.Ensurecarries[DoesNotReturnIf(false)]and the compiler's nullable analysis is already satisfied.Open question for reviewers
A run that ultimately passes still shows a red
Test run summary: Failed!(attempt 1) before the greenRetry summary: Passed!. Both pre-merge reviewers flagged the two-verdict model. The clean fix is one authoritative block — child attempts printAttempt N/M, the orchestrator prints the singleTest run summary. I did not do it here because it changes shipped platform output and theAssertOutputContainsSummarycontract used across the acceptance suites, so it deserves an explicit decision rather than being folded into this PR. Happy to follow up.Validation
build.cmd -packNew coverage: flaky named and counted; retried-but-still-failing is not flaky;
--show-flaky-tests off; no retry lines when nothing was retried; run summary suppressed/kept per attempt; a retry attempt that crashes without reporting; a retried test that comes back skipped; a folded uid that passes some rows and skips others; a threshold trip with zero retries configured.Verified by direct capture that a plain run without
--retry-failed-testsis byte-identical to before.Note for CI:
_GitRepositoryRootresolves empty in my local worktree, so SourceLink cannot supplyRepositoryCommitand-packneeds it passed explicitly. This affects every packable project equally, and this branch touches no build infrastructure.