-
Notifications
You must be signed in to change notification settings - Fork 782
Wait with exponential backoff for expected Executables #12647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12647Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12647" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces a hardcoded Task.Delay(2000) with a proper retry mechanism using Polly's resilience pipeline to wait for executable resources to be created. The change makes the test more robust and deterministic by actively polling for the expected condition instead of relying on a fixed delay.
Key Changes
- Replaced fixed 2-second delay with a retry-based polling mechanism using Polly
- Added
RetryTillTrueOrTimeouthelper method to encapsulate the retry logic - Updated assertion to provide better error messaging when the expected resource count is not met
| developerCertificateService); | ||
| } | ||
|
|
||
| private static bool RetryTillTrueOrTimeout(Func<bool> check, int timeoutMilliseconds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use AssertIsTrueRetryAsync from AsyncTestHelpers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I do not like it 😄 It does not allow me to set the maximum delay between attempts, it forces async behavior (which I do not need here), and it does not allow me to set the timeout, only maximum number of retries.
If it turns out that we use the new method in other DCP executor tests, I will think about moving it to some utility class.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes #12133