test(SurrealDb): bump VerifySurrealDbResource and WithDataShouldPersistStateBetweenUsages timeouts to 10 minutes - #1431
Conversation
…stStateBetweenUsages timeouts to 10 minutes These two tests still used a 5-minute CancellationTokenSource while the slower tests in the same class (VerifyWaitForOnSurrealDbBlocksDependentResources and VerifyWaitForOnSurrealDbBlocksDependentResourcesUntilCancellation) already use 10 minutes. WithDataShouldPersistStateBetweenUsages does two full app lifecycles back-to-back (start surreal -> wait healthy -> write -> stop -> start again -> wait healthy -> read), so on a cold runner the first SurrealDB image pull plus container start can consume most of the 5-minute budget. The post-merge .NET main run for CommunityToolkit#1394 hit this: cancellation fired at 5m 03s with the resource already reporting Current State: Running and Current Health: Healthy -- the healthy event arrived just as the token tripped (run 28141532703, attempt 2). Aligning these two tests with the existing 10-minute budget removes the race.
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.sh | bash -s -- 1431Or
iex "& { $(irm https://raw.githubusercontent.com/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.ps1) } 1431" |
There was a problem hiding this comment.
Pull request overview
This PR adjusts SurrealDB functional test cancellation timeouts to reduce flakiness on cold CI runners where the container image pull and startup can consume most of the prior 5-minute budget, especially for tests that perform multiple full app lifecycles.
Changes:
- Bump
VerifySurrealDbResourcetimeout from 5 minutes to 10 minutes. - Bump
WithDataShouldPersistStateBetweenUsagestimeout from 5 minutes to 10 minutes (covers two back-to-back app lifecycles).
|
I've done some investigating into this more deeply and I think that there's a mistaken assumption on the test failure here being due to the timeout and not that there's an underlying problem. Looking at sibling tests in the class that With the above mentioned test run I have expanded the amount of logging that we do in these kinds of tests, and it looks like the root cause is that it fails to create the namespace because it's unable to connect to the remote server - the websocket connection fails. If the namespace isn't created because it can't connect to the server we have a cascade of problems which results in the resource never becoming healthy and eventually the test hits the timeout and is terminated. Reviewing the tests in this test class, I actually don't believe we should have them in at all, the tests are really testing Aspire core behaviour, not really the behaviour of the integration - at least not in a way that isn't already covered by our integration test stack using the example app host. Let's take the method |
|
Closing in favour of #1441 |
Why
The post-merge
.NET mainrun for #1394 hit a SurrealDB test failure (run 28141532703, attempt 2):SurrealDbFunctionalTests.WithDataShouldPersistStateBetweenUsages(useVolume: False)cancelled at 5m 03s with the resource already reporting:i.e. the healthy event arrived right as the 5-minute CTS tripped.
Root cause
WithDataShouldPersistStateBetweenUsagesruns two full app lifecycles back to back (start surreal → wait healthy → write data → stop, then build a new app → start → wait healthy → read data). On a cold runner the first SurrealDB image pull + container start + healthy check eats most of the 5-minute budget, leaving no headroom for the second app. The other slow tests in this file (VerifyWaitForOnSurrealDbBlocksDependentResources,VerifyWaitForOnSurrealDbBlocksDependentResourcesUntilCancellation) already useFromMinutes(10)-- aligning these two with that.What changes
Two-line change:
VerifySurrealDbResourceFromMinutes(5)FromMinutes(10)WithDataShouldPersistStateBetweenUsagesFromMinutes(5)FromMinutes(10)Leaves the
[Fact(Skip = ...)]test (AddDatabaseCreatesDatabaseWithCustomScript) at 5 minutes since it doesn't run.Aaron mentioned image caching is supposed to be in place; if it later proves to be working, the test will simply finish well under the new 10-minute ceiling -- this purely raises the upper bound, no behavior change.
cc @aaronpowell