Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a flaky test by addressing timing issues in the revalidation loop. The test SuppliesCancellationTokenThatSignalsWhenRevalidationLoopIsBeingDiscarded was intermittently failing because the revalidation loop could execute multiple iterations between test assertions, causing more log entries than expected.
- Removes the quarantine attribute from the flaky test
- Changes assertion from
Assert.CollectiontoAssert.Allto handle variable number of log entries - Adds explanatory comments about the timing behavior
src/Components/Server/test/Circuits/RevalidatingServerAuthenticationStateProviderTest.cs
Show resolved
Hide resolved
|
See the relevant And the test implementation of the abstract method called per loop iteration (until the revalidation is cancelled, or until it fails and causes a logout): To avoid confusion, it is correct that the revalidation loop (possibly) runs multiple iterations of |
ilonatommy
left a comment
There was a problem hiding this comment.
Looking at the RevalidationLoop logic, the reasoning looks valid.
|
/backport to release/10.0 |
|
Started backporting to release/10.0: https://github.com/dotnet/aspnetcore/actions/runs/17460064736 |
Fixes and unquarantines
Microsoft.AspNetCore.Components.RevalidatingServerAuthenticationStateProviderTest.SuppliesCancellationTokenThatSignalsWhenRevalidationLoopIsBeingDiscarded.The test was flaky because the loop in
RevalidatingServerAuthenticationStateProvider.RevalidationLoopcan do multiple iterations between the relevant lines of the test. This means thatValidateAuthenticationStateAsyncis (successfully) fired multiple times and logs more than one (identical) logs. If I understand the context correctly, this is a legitimate outcome and the assertion should not check for strict log collection equivalnce, only that the remaining logs in the collection are as expected.When runnning the test locally with the revalidation interval set to 3ms instead of 50ms, I got ~1/400 failed runs. With the change, no runs failed after 3000+ executions.
Alternatively, the
TestRevalidatingServerAuthenticationStateProviderclass could be rewritten to allow stricter dependable assertions. However, this solution seems cost-efficient.Fixes #60472