Simplify retry extension logic to use built-in filters#7511
Open
Evangelink wants to merge 4 commits intomainfrom
Open
Simplify retry extension logic to use built-in filters#7511Evangelink wants to merge 4 commits intomainfrom
Evangelink wants to merge 4 commits intomainfrom
Conversation
Youssef1313
reviewed
Mar 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR simplifies the Retry extension by removing the custom test execution filter factory and instead relying on the platform’s built-in --filter-uid functionality when re-running failed tests.
Changes:
- Update
RetryOrchestratorto remove any existing test filter options and apply--filter-uidwith the failed test UIDs on retry. - Remove the Retry extension’s custom
ITestExecutionFilterFactoryintegration (and the defaultTestHostManagerrequirement). - Add/adjust acceptance coverage to validate retry behavior when an initial filter is present.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs | Adds new acceptance scenarios for retry behavior with pre-existing filters and updates the embedded test asset to respect UID filtering. |
| src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs | Replaces the previous in-process filter approach by rewriting command line filters on retry and introduces helper logic to remove existing options. |
| src/Platform/Microsoft.Testing.Extensions.Retry/RetryExtensions.cs | Removes registration of the custom retry execution filter factory and the dependency on the default TestHostManager. |
| src/Platform/Microsoft.Testing.Extensions.Retry/RetryExecutionFilterFactory.cs | Deletes the custom filter factory implementation (no longer needed with built-in filtering). |
src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs
Outdated
Show resolved
Hide resolved
...grationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs
Outdated
Show resolved
Hide resolved
Youssef1313
reviewed
Mar 9, 2026
Youssef1313
reviewed
Mar 9, 2026
src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs
Outdated
Show resolved
Hide resolved
...grationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/RetryFailedTestsTests.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs
Outdated
Show resolved
Hide resolved
Youssef1313
reviewed
Mar 17, 2026
| await writer.WriteAsync($"--{PlatformCommandLineProvider.FilterUidOptionKey}").ConfigureAwait(false); | ||
| foreach (string uid in lastListOfFailedId) | ||
| { | ||
| await writer.WriteAsync($" \"{uid}\"").ConfigureAwait(false); |
Member
There was a problem hiding this comment.
I think we need to verify what are the escaping rules from our rsp parsing implementation and if we need to escape some stuff if the uid contains them
Youssef1313
reviewed
Mar 17, 2026
| string resultDir = Environment.GetEnvironmentVariable("RESULTDIR")!; | ||
| bool crash = Environment.GetEnvironmentVariable("CRASH") == "1"; | ||
|
|
||
| var uidFilter = (context.Request as TestExecutionRequest)?.Filter as TestNodeUidListFilter; |
Member
There was a problem hiding this comment.
I'm wondering how did this test work before? How was it respecting the retry filter?
This was referenced Apr 1, 2026
Youssef1313
reviewed
Apr 9, 2026
| } | ||
| } | ||
|
|
||
| finalArguments.Add($"@{responseFilePath}"); |
Member
There was a problem hiding this comment.
If this path has a space, we might fail.
We need proper escaping.
Member
There was a problem hiding this comment.
Hmm, actually not, that's handled. finalArguments is the unescaped arguments which will either be handled by runtime or by PasteArguments.
It might still be worth to add a test
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.
Fixes #7500