Repository navigation
Fix races in test helpers that made tests flaky - #1379
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 31 minutes. View limit details
 |
| Layer / File(s) | Summary |
|---|---|
Connection and task completion waiters src/Ably.PubSub.Tests.Shared/Infrastructure/ConnectionAwaiter.cs, src/Ably.PubSub.Tests.Shared/Infrastructure/TaskCompletionAwaiter.cs, src/Ably.PubSub.Tests.Shared/Infrastructure/ConnectionAwaiterTests.cs, src/Ably.PubSub.Tests.Shared/Ably.PubSub.Tests.Shared.projitems |
ConnectionAwaiter subscribes before checking the current state, removes its listener after waiting, and handles completion near timeout. TaskCompletionAwaiter uses atomic task-count updates. New tests cover completion, timeout, listener cleanup, and a state-change race. |
Push Test Synchronization
| Layer / File(s) | Summary |
|---|---|
Push collection and LocalDevice event test src/Ably.PubSub.Tests.Shared/Push/PushTestCollection.cs, src/Ably.PubSub.Tests.Shared/Push/ActivationStateMachineTests.cs, src/Ably.PubSub.Tests.Shared/Push/LocalDeviceTests.cs, src/Ably.PubSub.Tests.Shared/Push/PushChannelTests.cs, src/Ably.PubSub.Tests.Shared/Push/PushRealtimeTests.cs, src/Ably.PubSub.Tests.Shared/Ably.PubSub.Tests.Shared.projitems |
Push test classes are assigned to a collection with parallelization disabled. The LocalDeviceTests callback is registered before the first event and signals its waiter on the second event. |
Presence Sandbox Test Waiters
| Layer / File(s) | Summary |
|---|---|
Match and await presence events src/Ably.PubSub.Tests.Shared/Realtime/PresenceSandboxSpecs.cs |
The test waits for presence events matching the expected action and client ID. The helper reports pending expectations after a timeout and disposes its waiters. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Other
Possibly related PRs
- ably/ably-pubsub-dotnet#1364: Both changes address missed connection-state transitions, though this pull request updates the test-only
ConnectionAwaiter. - ably/ably-pubsub-dotnet#1371: Both changes address flaky
Push.LocalDeviceTestsbehavior through event registration and test serialization.
Suggested reviewers: ttypic
Merge Risk: 
4f770
-
This test-only change fixes several flaky-test races. One remaining gap is that PushRealtimeTests leaves a shared LocalDevice instance behind, which can make a later push test fail depending on execution order. Resetting the instance in that class's teardown closes it, and no production behavior is affected.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main changes: fixing race conditions in test helpers and tests that caused flaky test behavior. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 9 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit waits for states to turn,
While task counts tick and listeners learn.
Push tests queue in one calm place,
Presence events match action and face.
Then every waiter hops away.
|
@coderabbitai review |
Action performed
|
There was a problem hiding this comment.
🟡 Changes recommended
Approval is blocked by an unresolved compile error in PushTestCollection and a resource-leaking test that leaves 200 realtime clients and workflows undisposed.
2 open issues
What changed in this PR
This test-only pull request hardens asynchronous test helpers and push/presence tests against event-ordering races, missed callbacks, leaked listeners, and shared-state interference. No SDK production code is changed.
Changes:
- Makes
ConnectionAwaitersubscribe safely, clean up listeners, handle timeout races, and adds focused tests. - Serializes tests sharing
LocalDevice.Instanceand fixes callback ordering inLocalDeviceTests. - Makes task counting atomic and replaces broad presence-event waits with targeted event matching.
- Addresses the following review findings before approval:
- Use a valid collection-name reference in
PushTestCollection.csso the project compiles. - Dispose the realtime clients created by the concurrency test.
- Reset
LocalDevice.InstanceduringPushRealtimeTeststeardown.
- Use a valid collection-name reference in
| File | Summary |
|---|---|
src/Ably.PubSub.Tests.Shared/Realtime/PresenceSandboxSpecs.cs |
Adds targeted presence-event waiters and improved timeout diagnostics. |
src/Ably.PubSub.Tests.Shared/Push/PushTestCollection.cs |
Defines non-parallel execution for tests sharing LocalDevice.Instance; the collection-name reference currently requires correction for compilation. |
src/Ably.PubSub.Tests.Shared/Push/PushRealtimeTests.cs |
Assigns realtime push tests to the shared collection; teardown should reset the process-wide LocalDevice.Instance. |
src/Ably.PubSub.Tests.Shared/Push/PushChannelTests.cs |
Assigns push channel tests to the shared push collection. |
src/Ably.PubSub.Tests.Shared/Push/LocalDeviceTests.cs |
Registers the activation callback before triggering the first event. |
src/Ably.PubSub.Tests.Shared/Push/ActivationStateMachineTests.cs |
Assigns activation state-machine tests to the shared push collection. |
src/Ably.PubSub.Tests.Shared/Infrastructure/TaskCompletionAwaiter.cs |
Uses an atomic decrement for concurrent ticks. |
src/Ably.PubSub.Tests.Shared/Infrastructure/ConnectionAwaiterTests.cs |
Adds coverage for connection-wait scenarios; the concurrency test should dispose each created realtime client. |
src/Ably.PubSub.Tests.Shared/Infrastructure/ConnectionAwaiter.cs |
Fixes state-wait subscription ordering and cleanup behavior. |
src/Ably.PubSub.Tests.Shared/Ably.PubSub.Tests.Shared.projitems |
Includes the new awaiter tests and push collection definition. |
Review effort: Lite
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
482de5d to
0feb7bd
Compare
Test-only changes; no product code. - ConnectionAwaiter (behind WaitForState) checked the connection state before subscribing, so a change landing between the two was missed and the wait timed out. It now subscribes first, then checks, removes its listener on every exit path, uses TrySetResult, runs continuations asynchronously and cancels its timeout delay. New ConnectionAwaiterTests cover the already-there, reached, timeout and listener-removal cases; the concurrency test disposes the client each of its 200 iterations creates. - LocalDeviceTests registered ProcessingEventCallback after the call that raises the second GotPushDeviceDetails on a background task, so it could miss it. The callback is now registered first. - Test classes that read or reset the process-wide LocalDevice.Instance (LocalDeviceTests, the ActivationStateMachineTests classes, PushChannelTests and PushRealtimeTests) now share one non-parallel xUnit collection, and PushRealtimeTests clears LocalDevice.Instance after its base teardown, as the others do. - TaskCompletionAwaiter.Tick decrements atomically, so two concurrent ticks can't lose one. - PresenceSandboxSpecs.Presence_ShouldHaveInternalMapForCurrentConnectionId waits on each channel for the specific expected presence event instead of the first event of any kind, and names the missing one on timeout. An assertion that could never fail is removed.
UtsRecord.ConnectionStates and ConnectionStateChanges only see changes made after they subscribe. ConnectionStaysFailedWhenTheServerClosesAfterAFatalError created an AutoConnect client and subscribed afterwards, and its mock answers the first attempt with a fatal ERROR at once. On a slow runner the client could reach FAILED, and stay there, before the recorder subscribed, so the poll for FAILED timed out after 10 seconds. The failure reproduced locally under CPU load. Create the client with AutoConnect off, subscribe, then connect. The test still checks the same race across 20 clients. Two smoke tests in RealtimeUnitSmokeTests, and MockWebSocketTests' event-order test (which subscribed after triggering a disconnect whose retry is refused at once), had the same pattern and could lose the DISCONNECTED or first CONNECTING change they read, so they now subscribe first too.
54047a8 to
ac7a319
Compare
PR 1379: follow-up comment on the review-reply SHAs (draft, not posted)
|
Summary
Intermittent CI failures came from races in tests and test helpers, not from the SDK. Test-only changes; no product code and no CHANGELOG entry.
Fix
ConnectionAwaiter(used byWaitForState, about 300 call sites): it checked the connection state before subscribing, so a state change landing between the two was missed and the wait timed out with "Expected 'Connected' but current state was 'Connected'" (seen inDeltaSpecs.WhenMessageReceived_WithDeltaError_ShouldNotPassMessageToChannelSubscriber). It now subscribes first, then checks, removes its listener on every exit path, usesTrySetResult, runs continuations asynchronously and cancels its timeout delay. NewConnectionAwaiterTestscover the already-there, reached, timeout and listener-removal cases.LocalDeviceTests.WhenClientIdChangesAfterRegisteringDevice_...: the secondGotPushDeviceDetailsis raised on a background task as soon as the firstHandleEventreleases its lock, and the test registeredProcessingEventCallbackonly after that call, so it could miss it (CI: "Expected (taskAwaiter) to be true, but found False"). The callback is now registered first and signals on the second event.LocalDevice.Instance: the process-wide static was reset and read by parallel test classes.LocalDeviceTests, theActivationStateMachineTests,PushChannelTestsandPushRealtimeTestsclasses now share one non-parallel xUnit collection.TaskCompletionAwaiter.Tick: the decrement is now atomic (Interlocked.Decrement), so two handlers ticking at once can't lose a tick.PresenceSandboxSpecs.Presence_ShouldHaveInternalMapForCurrentConnectionId: each step now waits on each channel for the specific expected event (action and clientId) instead of taking the first presence event of any kind, and a timeout names the channel and event that is missing. An extra valid event, such as aPRESENTfrom a sync (seen once in CI), is now ignored and only listed in the timeout message. One assertion that could never fail (msgB.Should().BeNull()with no handler subscribed) was removed; all others are kept.StateWaitTests.ConnectionStaysFailedWhenTheServerClosesAfterAFatalError:UtsRecord.ConnectionStatesonly records changes made after it subscribes. The test created an AutoConnect client and then subscribed, and the mock answers with a fatal ERROR at once, so on a slow runner the client could reach FAILED (and stay there) first, and the poll for FAILED timed out (CI: "StateWaitTests.cs:109: poll_until timed out after 10000 ms"; reproduced locally under CPU load). The client is now created withAutoConnect = false, the recorder attached, thenConnect()called; the test still checks the same race across 20 clients. TwoRealtimeUnitSmokeTestsandMockWebSocketTests.EveryMockEventTypeIsEmittedInOrderhad the same subscribe-after-connect pattern and get the same change.The UTS
RTL6c2_QueuedWhenDisconnectedrace is not in this PR; it belongs with the UTS tests in #1362.The branch is based on the current
integration/v2and has two commits: the test-helper race fixes (including the review fixes), then the UTSStateWaitTestsfix.Tests
Race evidence, using throwaway harnesses that are not committed:
ConnectionAwaitercheck-then-subscribeLocalDeviceTestscallback registrationTask.Delay(300)after the firstHandleEventWithout an injected delay the windows are too narrow to hit on demand; the new
ConnectionAwaiterTestsconcurrency test passes but would not reliably catch a regression on its own.Repeated runs (net6.0):
integration/v2LocalDeviceTests×20LocalDeviceTests×20 under CPU stressConnectionAwaiterTests×10Validation
On
4f770114(the pushed head):./build.sh --target=Build.NetStandard: build succeeded, 0 warnings, 0 errors. The .NET Framework test project also builds (0/0)../build.sh --target=Test.NetStandard.Unit.WithRetry --framework=net6.0: 1511 total, 1492 passed, 19 skipped, 0 failed.On
54047a83(the same content before the branch was rebuilt on the currentintegration/v2):./build.sh --target=Build.NetStandard: 0 warnings, 0 errors../build.sh --target=Test.NetStandard.Uts --framework=net6.0 --uts-tier=unit: 1094 total, 856 passed, 238 skipped, 0 failed.StateWaitTests: before the fix, 1 of 30 runs failed under CPU load with the CI error; after it, 30 of 30 passed under the same load, and the three changed infra test classes passed 30 of 30 runs../build.sh --target=Test.NetStandard.Unit.WithRetry --framework=net6.0: 1556 total, 0 failed after retry (one timing-basedConnectionFailureSpecsbackoff test failed once and passed on retry; not touched here).On
ac7a319f(the two commits onintegration/v20feb7bd2): the content is identical to54047a83.git diffagainstintegration/v2is byte-identical to the earlier one, and the trees differ only in the nuspec<authors>lines thatintegration/v2now carries. An independent review confirmed the equivalence, the two-commit history and the commit messages: pass. CI runs the tests on this head.Summary by CodeRabbit