Skip to content

Conversation

@karolz-ms
Copy link
Member

Fixes #12133

@karolz-ms karolz-ms requested review from danegsta and radical November 4, 2025 01:08
@karolz-ms karolz-ms requested a review from mitchdenny as a code owner November 4, 2025 01:08
Copilot AI review requested due to automatic review settings November 4, 2025 01:08
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2025

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12647

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12647"

Copy link
Contributor

Copilot AI left a 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 RetryTillTrueOrTimeout helper 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)
Copy link
Member

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?

Copy link
Member Author

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>
@karolz-ms karolz-ms enabled auto-merge (squash) November 4, 2025 02:33
@karolz-ms karolz-ms merged commit 84c30cf into main Nov 4, 2025
582 of 585 checks passed
@karolz-ms karolz-ms deleted the dev/karolz/issue12133 branch November 4, 2025 03:10
@dotnet-policy-service dotnet-policy-service bot added this to the 13.1 milestone Nov 4, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failing test: Aspire.Hosting.Tests.Dcp.DcpExecutorTests.PlainExecutable_ExtensionMode_SupportedDebugMode_RunsInIde

3 participants