Attempt to improve test reliability#73
Conversation
Mistake in 70306a0. Correct approach copied from "Objects.createCounter sends COUNTER_CREATE operation" test.
Wait until we're sure that GC of an entry should have occurred, regardless of clock skew. Also fix what may have been a bug in 792683d where we may not wait for as many GC cycles as we intended to (because of counting AsyncStream buffered values).
I've been observing a bunch of failures and hangs etc in these tests; see the comment and the referenced issue. In order to unblock CI whilst we prepare for a release, run the tests in serial, which seems to improve reliability at the unfortunate cost of making them a _lot_ slower to run (from ~15s on my machine up to 4 minutes).
WalkthroughIntroduces a no-buffering stream for garbage-collection completion in InternalDefaultRealtimeObjects and updates references. Test suite execution is serialized. JS integration tests switch from GC cycle-count waits to tombstone-time-based waits, refactor map creation to deterministic ordering, and use typed GC options with adjusted intervals. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as JS Integration Test
participant Obj as Realtime Objects
participant GC as GC Scheduler
participant Stream as GC Completion Stream (no-buffering)
rect rgba(230,240,255,0.5)
note over Test,Obj: Setup
Test->>Obj: Configure GarbageCollectionOptions(interval=0.5, gracePeriod=0.25)
end
rect rgba(240,255,240,0.5)
note over Test,Obj: Tombstone
Test->>Obj: Tombstone object (record tombstonedAt)
Obj->>GC: Schedule collection after gracePeriod
end
rect rgba(255,250,230,0.6)
note over GC,Stream: Collection & Event
GC-->>Obj: Collect tombstoned object
Obj-->>Stream: Emit GC completion (no buffering)
end
rect rgba(255,240,245,0.6)
note over Test: Await completion
Test->>Test: waitForTombstonedObjectsToBeCollected(tombstonedAt)
Test-->>Obj: Assert removal from pool/data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
lawrence-forooghian
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (2)
3893-3898: Typed GC options in tests improve clarity; minor stylistic nit.Using InternalDefaultRealtimeObjects.GarbageCollectionOptions(interval: 0.5, gracePeriod: 0.25) is clear. If you want to follow the project guideline “prefer implicit .init when type is inferable,” you could inline the type on the variable and use .init:
- let garbageCollectionOptions = InternalDefaultRealtimeObjects.GarbageCollectionOptions( - interval: 0.5, - gracePeriod: 0.25, - ) + let garbageCollectionOptions: InternalDefaultRealtimeObjects.GarbageCollectionOptions = .init( + interval: 0.5, + gracePeriod: 0.25 + )
3910-3920: Avoid capturing non-Sendable state in a @sendable closure by pre-capturing the stream.The closure waitForTombstonedObjectsToBeCollected is declared @sendable. Capturing internallyTypedObjects (likely non-Sendable) can trigger strict-concurrency diagnostics. Pre-capture the AsyncStream (which is Sendable when Element is Sendable) and only close over that.
Apply this diff locally:
- let internallyTypedObjects = try #require(objects as? PublicDefaultRealtimeObjects) - let waitForTombstonedObjectsToBeCollected: @Sendable (Date) async throws -> Void = { (tombstonedAt: Date) in + let internallyTypedObjects = try #require(objects as? PublicDefaultRealtimeObjects) + // Capture the stream outside the @Sendable closure to avoid capturing a non-Sendable object. + let gcEvents = internallyTypedObjects.testsOnly_proxied.testsOnly_completedGarbageCollectionEventsWithoutBuffering + let waitForTombstonedObjectsToBeCollected: @Sendable (Date) async throws -> Void = { (tombstonedAt: Date) in // Sleep until we're sure we're past tombstonedAt + gracePeriod let timeUntilGracePeriodExpires = (tombstonedAt + garbageCollectionOptions.gracePeriod).timeIntervalSince(.init()) if timeUntilGracePeriodExpires > 0 { try await Task.sleep(nanoseconds: UInt64(timeUntilGracePeriodExpires * Double(NSEC_PER_SEC))) } - // Wait for the next GC event - await internallyTypedObjects.testsOnly_proxied.testsOnly_completedGarbageCollectionEventsWithoutBuffering.first { _ in true } + // Wait for the next GC event + await gcEvents.first { _ in true } }Optionally (not required), consider Task.sleep(for:) with Duration for readability and to avoid floating-point rounding:
try await Task.sleep(for: .seconds(timeUntilGracePeriodExpires))This requires iOS 17/tvOS 17 deployment in tests (already present elsewhere in this file).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(3 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
**/*.swift: When initializer type is inferable, prefer the implicit .init(...) form over repeating the type name
When enum type is inferable, prefer the shorthand .caseName over repeating the type name
For JSONValue or WireValue, prefer literal syntax enabled by ExpressibleBy*Literal conformances where possible
For JSON strings, prefer Swift raw string literals rather than escaping double quotes
When an array literal begins with an initializer expression, place the initializer on the line after the opening bracket
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
**/Tests/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
**/Tests/**/*.swift: Use the Swift Testing framework (import Testing), not XCTest.
Do not usefatalErrorin response to a test expectation failure. Favour the usage of Swift Testing's#requiremacro.
Only add labels to test cases or suites when the label is different to the name of the suitestructor test method.
Follow the guidelines given under 'Attributing tests to a spec point' in the fileCONTRIBUTING.mdin order to tag the unit tests with the relevant specification points. Make sure to follow the exact format of the comments as described in that file. Pay particular attention to the difference between the meaning of@specand@specPartialand be sure not to write@specmultiple times for the same specification point.
Add comments that explain when some piece of test data is not important for the scenario being tested.
When you need to import the following modules in the tests, do so in the following way: Ably: useimport Ably; AblyLiveObjects: use@testable import AblyLiveObjects; AblyPlugin: useimport AblyPlugin; do not dointernal import.
When you need to pass a logger to internal components in the tests, passTestLogger().
When you need to unwrap an optional value in the tests, favour using#requireinstead ofguard let.
When creatingtestsOnly_property declarations, do not write generic comments of the form 'Test-only access to the private createOperationIsMerged property'; the meaning of these properties is already well understood.
Files:
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
🔇 Additional comments (8)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (4)
101-101: Good call: no-backlog GC completion stream using bufferingNewest(0).Using AsyncStream.makeStream(bufferingPolicy: .bufferingNewest(0)) is a precise fit for “only care about the next completion after a wait begins,” which helps avoid races with stale GC completions.
335-336: Plumbing the new continuation into performGarbageCollection is correct.Routing eventsContinuation: completedGarbageCollectionEventsWithoutBufferingContinuation ensures tests wait for a fresh GC cycle rather than receiving buffered signals.
340-346: testsOnly_completedGarbageCollectionEventsWithoutBuffering accessor matches semantics.The naming and visibility are clear for test-only usage. No issues.
355-361: Test helper now finishes the new GC stream — nice.Finishing completedGarbageCollectionEventsWithoutBufferingContinuation avoids leaked tasks in tests that await the stream.
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (4)
337-345: Serialize the suite to deflake CI (temporary) — pragmatic.Adding .serialized to the Suite is an acceptable short-term mitigation for the hangs/timeouts described in PR context (issue #72). Keep an eye on runtime impact and revert once the underlying issue is addressed.
If helpful, I can generate a small “timing snapshot” script to compare wall-clock duration before/after serialization across CI runs to quantify the impact.
2927-2944: Deterministic ordering for concurrent map creation — solid fix for flakiness.Returning (index, map) from the task group, then sorting by index, removes nondeterminism from concurrent operations while preserving concurrency benefits.
3791-3795: Tombstone-time-based waits reduce flakiness compared to “N GC cycles.”Deriving tombstonedAt and waiting for the first GC after gracePeriod has elapsed is a robust approach and matches the new no-buffering stream semantics.
Also applies to: 3867-3870
3741-3741: Context includes an async wait function — good seam for reuse.Adding waitForTombstonedObjectsToBeCollected into the test Context keeps scenarios clean and reusable. This plays well with the no-buffering GC stream.
2 participants
This attempts to address the intermittently failing and hanging CI tests by:
See commit messages for more details.
(Note that CI is failing because of the repo rename causing the docs builds to fail; I'll probably merge this one anyway because the flaky tests are quite a nuisance, making it hard to get anything else merged.)
Summary by CodeRabbit