Disable resilience handler in test builders - #1504
Draft
afscrome wants to merge 5 commits into
Draft
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://github.com/ghraw/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.sh | bash -s -- 1504Or
iex "& { $(irm https://github.com/ghraw/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.ps1) } 1504" |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Set ASPIRE_TESTING_DISABLE_HTTP_CLIENT via IConfiguration on the test builder rather than as a process-wide environment variable. This is cleaner and avoids potential cross-test pollution from static state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DbGate 6.1.4 has no /health route. In Docker mode it serves static files at /, so that is the correct readiness endpoint. Using /health returned 404, causing WaitForResourceHealthyAsync to time out. This fixes the transitive timeout in all tests that depend on dbgate: MongoDB.Extensions, PostgreSQL.Extensions, Redis.Extensions, SqlServer.Extensions and MySql.Extensions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.
Theory
The
AddStandardResilienceHandler()calls inTestDistributedApplicationBuilderandAspireIntegrationTestFixtureadd automatic HTTP retry logic to all test HTTP clients. The hypothesis is that this masks genuine race conditions in integration tests: when a resource isn't truly ready to serve requests, the resilience pipeline silently retries until it succeeds — turning what should be a guaranteed, reproducible failure into an intermittent, flaky test.Why this is particularly dangerous
The specific race being hidden is between:
This race is almost never visible locally because:
In CI, however, the agent has to pull the image from scratch on every run. This means the resource startup takes significantly longer, and the resilience handler's retry budget can be exhausted before the HTTP server is ready — which is exactly what we see in the failures below. Without retries, the test fails fast and visibly every time; with retries, it passes most of the time but occasionally flakes when the machine is slow or under load.
The correct fix is not retries — it's a proper health check so that
WaitForResourceHealthyAsync(...)means "the HTTP server is actually ready", not just "the container process started".This PR removes the resilience handler from both test helpers and sets
ASPIRE_TESTING_DISABLE_HTTP_CLIENT=trueto prevent Aspire from adding its own HTTP client reliability defaults during tests.Evidence
After making that removal, CI immediately surfaced 6 real failures across Ubuntu runners:
Hosting.Azure.Extensions.Tests—AppHostTests.ResourceStartsAndRespondsOkblobs-explorer(Azure Storage Explorer)HttpRequestException→HttpIOException: response ended prematurelyHosting.DbGate.Tests—AppHostTests.ResourceStartsAndRespondsOkdbgateHosting.MongoDB.Extensions.Tests—AppHostTests.ResourceStartsAndRespondsOkdbgate(via.WithDbGate())Hosting.Redis.Extensions.Tests—AppHostTests.ResourceStartsAndRespondsOkdbgate(via.WithDbGate())Hosting.PostgreSQL.Extensions.Tests—AppHostTests.ResourceStartsAndRespondsOkdbgate(via.WithDbGate())Hosting.Java.Tests—JavaHostingComponentTests.AppResourceWillRespondWithOk("containerapp")All 6 tests call
WaitForResourceHealthyAsync(...)before issuingGET /, but none of the affected resources had an explicit HTTP health check registered — so "healthy" state was being reached before the HTTP server was actually ready to accept connections.Changes
Remove resilience handlers from test helpers
tests/CommunityToolkit.Aspire.Testing/TestDistributedApplicationBuilder.cs— removed.AddStandardResilienceHandler()and setASPIRE_TESTING_DISABLE_HTTP_CLIENT=trueto ensure aspire doesn't add the same resilience back.tests/CommunityToolkit.Aspire.Testing/AspireIntegrationTestFixture.cs— sameAdd explicit HTTP health checks
src/CommunityToolkit.Aspire.Hosting.DbGate/DbGateBuilderExtensions.cs) — added.WithHttpHealthCheck("/health", ...). DbGate's upstream API exposes a dedicated/healthendpoint, making this the correct readiness signal.src/CommunityToolkit.Aspire.Hosting.Azure.Extensions/AzureStorageExplorerBuilderExtensions.cs) — added.WithHttpHealthCheck("/", ...). Upstream Helm/k8s manifests use/for readiness/liveness probes; no dedicated health endpoint exists.Out of scope
The Java
containerappfailure is handled by #1500 and is intentionally not fixed here.