-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix SuppliesCancellationTokenThatSignalsWhenRevalidationLoopIsBeingDiscarded flake #63522
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
Fix SuppliesCancellationTokenThatSignalsWhenRevalidationLoopIsBeingDiscarded flake #63522
Conversation
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 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.Collection
toAssert.All
to 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 aspnetcore/src/Components/Server/src/Circuits/RevalidatingServerAuthenticationStateProvider.cs Line 73 in 5afdc06
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): aspnetcore/src/Components/Server/test/Circuits/RevalidatingServerAuthenticationStateProviderTest.cs Line 248 in 5afdc06
To avoid confusion, it is correct that the revalidation loop (possibly) runs multiple iterations of |
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.
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.RevalidationLoop
can do multiple iterations between the relevant lines of the test. This means thatValidateAuthenticationStateAsync
is (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
TestRevalidatingServerAuthenticationStateProvider
class could be rewritten to allow stricter dependable assertions. However, this solution seems cost-efficient.Fixes #60472