Report MSTest [Retry] attempts to the platform instead of discarding them - #10295
Draft
Evangelink wants to merge 1 commit into
Draft
Report MSTest [Retry] attempts to the platform instead of discarding them#10295Evangelink wants to merge 1 commit into
[Retry] attempts to the platform instead of discarding them#10295Evangelink wants to merge 1 commit into
Conversation
`UnitTestRunner` kept only the last retry attempt, so an in-process `[Retry]` was invisible everywhere: the terminal showed a plain passing test and the CTRF report reported `flaky: 0` with no `retryAttempts[]`, even though `CtrfReportEngine` already implements the correct rule. Every attempt is now published as its own `TestNodeUpdateMessage` under the same test node uid, tagged with a new `RetryAttemptProperty` (`AttemptNumber` + `IsSuperseded`): - Terminal: attempts are annotated (`failed (try 1) MyTest`), the test is counted once and the superseded failures feed the `(+N retried)` summary. `TestProgressState` now orders attempts by the pair (host attempt, in-process attempt) so this composes with `--retry-failed-tests` instead of colliding with it. - `TestApplicationResult`: superseded attempts are not counted, so a retried-then-passing test no longer fails the build. - TRX / JUnit / Azure DevOps: superseded attempts are filtered out, so those reports keep exactly one row per test. - CTRF and HTML: unchanged, they already collapse/annotate per uid. - VSTest: the recorder drops superseded attempts, so Test Explorer and the VSTest TRX behave exactly as before. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa37ea7e-23a2-458b-bb8a-68ad56c832e6
Contributor
There was a problem hiding this comment.
Pull request overview
Surfaces every MSTest [Retry] attempt to Microsoft.Testing.Platform, enabling terminal retry reporting and CTRF flaky-test detection.
Changes:
- Adds
RetryAttemptPropertyand propagates retry metadata. - Reconciles attempts in terminal output and filters superseded results from selected consumers.
- Adds unit, acceptance, API, RFC, and changelog updates.
Show a summary per file
| File | Description |
|---|---|
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Services/TestApplicationResultTests.cs |
Tests exit-code filtering. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.cs |
Tests retry rendering and totals. |
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/RetryTests.cs |
Validates retry terminal output. |
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/JUnitReportTests.cs |
Verifies final-only JUnit results. |
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/CtrfReportTests.cs |
Verifies CTRF flaky reporting. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/Helpers/AcceptanceAssert.cs |
Supports retried summary assertions. |
src/TestFramework/TestFramework/InternalAPI/InternalAPI.Unshipped.txt |
Tracks new internal result properties. |
src/TestFramework/TestFramework/Attributes/TestMethod/TestResult.cs |
Stores retry attribution. |
src/TestFramework/TestFramework/Attributes/TestMethod/RetryResult.cs |
Documents attempt publication. |
src/TestFramework/TestFramework/Attributes/TestMethod/RetryBaseAttribute.cs |
Updates retry contract documentation. |
src/TestFramework/TestFramework/Attributes/TestMethod/RetryAttribute.cs |
Updates retry behavior documentation. |
src/Platform/Microsoft.Testing.Platform/Services/TestApplicationResult.cs |
Ignores superseded outcomes. |
src/Platform/Microsoft.Testing.Platform/PublicAPI/PublicAPI.Unshipped.txt |
Declares the new public property. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs |
Passes retry metadata to terminal reporting. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.cs |
Reconciles host and in-process attempts. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.cs |
Annotates and counts attempts. |
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.cs |
Documents retried totals. |
src/Platform/Microsoft.Testing.Platform/Messages/RetryAttemptProperty.cs |
Defines retry attempt metadata. |
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxDataConsumer.cs |
Filters superseded TRX results. |
src/Platform/Microsoft.Testing.Extensions.JUnitReport/JUnitReportGenerator.cs |
Filters superseded JUnit results. |
src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsTestResultsPublisher.cs |
Filters superseded published results. |
src/Platform/Microsoft.Testing.Extensions.AzureDevOpsReport/AzureDevOpsReporter.cs |
Suppresses superseded annotations. |
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.RunSingleTest.cs |
Flattens and tags retry attempts. |
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Runner.cs |
Uses final outcomes for verdicts. |
src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestNodeConverter.cs |
Adds retry properties to test nodes. |
src/Adapter/MSTest.TestAdapter/Services/TestResultRecorderExtensions.cs |
Preserves VSTest final-only behavior. |
docs/RFCs/016-JUnit-Report.md |
Documents JUnit retry handling. |
docs/Changelog.md |
Records the MSTest behavior change. |
docs/Changelog-Platform.md |
Records the platform API change. |
Review details
Comments suppressed due to low confidence (1)
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.RunSingleTest.cs:357
- A custom
RetryBaseAttributemay legally add an empty final result array. PreviouslyTryGetLast()returned that empty array, allowingSendTestResultsAsyncto callRecordEmptyResultAsync; this combination instead retains only earlier superseded results, so no final outcome is recorded andAllPassedcan return true after skipping them. Preserve the existing empty-result behavior when the final retry attempt is empty.
IReadOnlyList<TestResult[]> retryAttempts = retryResult.AllResults;
if (retryAttempts.Count == 0)
{
// A RetryBaseAttribute implementation is expected to add at least one attempt.
throw ApplicationStateGuard.Unreachable();
- Files reviewed: 29/29 changed files
- Comments generated: 7
- Review effort level: Medium
Comment on lines
+237
to
+241
| public void ReportPassingTest(string testNodeUid, string instanceId, int retryAttemptNumber) | ||
| => ReportGenericTestResult(testNodeUid, instanceId, retryAttemptNumber, static entry => entry with { Passed = entry.Passed + 1 }, static @this => @this._passedTests++); | ||
|
|
||
| public void ReportSkippedTest(string testNodeUid, string instanceId, int retryAttemptNumber) | ||
| => ReportGenericTestResult(testNodeUid, instanceId, retryAttemptNumber, static entry => entry with { Skipped = entry.Skipped + 1 }, static @this => @this._skippedTests++); |
Comment on lines
+131
to
+135
| // The orchestrator attributes retries per host instance; it does not surface a test framework's | ||
| // in-process retry attempt, so results arriving through this path are always attempt 1 of their host | ||
| // attempt. | ||
| retryAttemptNumber: 1, | ||
| isRetryAttempt: false); |
Comment on lines
+347
to
+348
| /// The final attempt stays last so callers that look at <c>results[^1]</c> (class and assembly cleanup) and | ||
| /// consumers that collapse per test node uid keep observing the test's real outcome. |
Comment on lines
+93
to
+95
| if (result.RetryAttemptNumber > 1 || result.IsSupersededRetryAttempt) | ||
| { | ||
| testNode.Properties.Add(new RetryAttemptProperty(result.RetryAttemptNumber, result.IsSupersededRetryAttempt)); |
| @@ -1,4 +1,4 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| @@ -0,0 +1,72 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
Comment on lines
+178
to
+184
| asm.ReportFailedTest(testNodeUid, instanceId, retryAttemptNumber); | ||
| break; | ||
| case TestOutcome.Passed: | ||
| asm.ReportPassingTest(testNodeUid, instanceId); | ||
| asm.ReportPassingTest(testNodeUid, instanceId, retryAttemptNumber); | ||
| break; | ||
| case TestOutcome.Skipped: | ||
| asm.ReportSkippedTest(testNodeUid, instanceId); | ||
| asm.ReportSkippedTest(testNodeUid, instanceId, retryAttemptNumber); |
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 free
to 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.
Draft implementation of #10292.
Problem
UnitTestRunner.RunSingleTestAsynckept only the last retry attempt:so an in-process
[Retry]was invisible everywhere — the terminal showed a plain passing test, and the CTRF report claimedflaky: 0with noretryAttempts[]even thoughCtrfReportEngine.CollapseAttempts/IsFlakyalready implement the correct rule.Approach
Publish every attempt as its own
TestNodeUpdateMessageunder the sameTestNode.Uid, tagged with a new platform property:IsSupersededis what lets each consumer opt in or out without guessing: consumers that want one row per test skip superseded attempts, consumers that want the history keep them all.Consumer audit
I checked every
TestNodeUpdateMessageconsumer for what happens when two completed updates share a uid:TestApplicationResult(exit code)TrxReportEngine<UnitTestResult>rows for one<TestDefinition>JUnitReportEngine[attempt N], inflated totalsAzureDevOpsTestResultsPublisher/ reporterCtrfReportEngineflakyHtmlReportEngineattemptIndex/attemptOfPerRequestServerDataConsumerServiceTotalPassedRetries/TotalFailedRetriesDotnetTestDataConsumer,GitHubActionsReporterTestProgressState)(host attempt, in-process attempt)The
(host attempt, in-process attempt)pair, compared lexicographically, is what makes this compose with the out-of-process--retry-failed-testsorchestrator rather than collide with it — one of the open questions in the issue.Result
[Retry(3)]on a test that fails once and then passes:and the CTRF report now emits
"flaky": 1,"retries": 1and a populatedretryAttempts[].Open design questions (why this is a draft)
JUnitReportGenerator.TryCapture/TrxDataConsumerif we'd rather expose the history there too.(try N)on the first attempt — currently rendered for every attempt of a retry sequence, including attempt 1. A test that passes first try is untouched.RetryAttemptProperty— made public so third-party frameworks can produce it and third-party reporters consume it. Easy to demote to internal if we'd rather not commit to the shape yet.--retry-failed-testsannotation — when both retry mechanisms are active, the per-test line shows the in-process attempt. A compositetry H.Rrendering would need a new localized resource.NotifyTestCompleted()now fires once per attempt, so the progress bar's completed count can exceed the discovered count during a retry-heavy run. Not addressed here.Validation
Microsoft.Testing.Platform.UnitTests: 1911/1911 pass (2 new)MSTestAdapter.PlatformServices.UnitTests: 1036/1036 passTestFramework.UnitTests: 1490/1490 pass-pack):RetryTests(2),CtrfReport*+JUnitReport*(6, 1 new),Trx*(6), platform*Retry*(32) — all passFixes #10292