[ECO-5465] Port remaining high-value JS integration tests #57
Conversation
WalkthroughAdds plugin-backed storage for GarbageCollectionOptions, wires GC options into plugin initialization, emits GC completion events via AsyncStream, and introduces test-only publish override hooks. Multiple test helpers and JS-integration test utilities migrated to WireValue and async flows. Updates ably-cocoa submodule pointer. Changes
Sequence Diagram(s)sequenceDiagram
participant Tests
participant Public as PublicDefaultRealtimeObjects
participant Core as DefaultCoreSDK
participant Plugin as DefaultInternalPlugin
Tests->>Public: testsOnly_overridePublish(closure)
Public->>Core: testsOnly_overridePublish(closure)
Tests->>Core: publish(objectMessages)
alt override present
Core->>Core: call overriddenPublishImplementation
Core-->>Tests: return
else no override
Core->>Plugin: DefaultInternalPlugin.sendObject(...)
Plugin-->>Core: result
Core-->>Tests: result
end
sequenceDiagram
participant Plugin as DefaultInternalPlugin
participant Realtime as InternalDefaultRealtimeObjects
participant Pool as ObjectsPool
participant Tests
Plugin->>Realtime: init(..., garbageCollectionOptions)
Realtime->>Pool: performGarbageCollection(gracePeriod, clock, logger, eventsContinuation)
Pool->>Realtime: eventsContinuation.yield()
Realtime-->>Tests: testsOnly_completedGarbageCollectionEvents (AsyncStream event)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
ecc3ce6 to
e621e30
Compare
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
e621e30 to
cd5cbdc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
cd5cbdc to
d9f9144
Compare
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
0f30a70 to
97fbd07
Compare
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
97fbd07 to
7359763
Compare
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
lawrence-forooghian
changed the title
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
7359763 to
849d5dc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
Mistake in fa255c1.
So that we can inject binary data in upcoming tests.
Will be needed by some upcoming integration tests that I'm porting from JS. There might be a better way to do this (injecting a special CoreSDK and thus not having to add this hook to the RealtimeObjects itself) but it'll do.
This continues the porting of the JS integration tests that was started in fa255c1, and continues the same approach and attitude taken there. It is based on the same version of ably-js as in that commit. I've created #60 for evaluating these ported tests for correctness and consistency, and for bringing them in line with the latest ably-js.
As in 8be3aed. Cursor struggled a bit when porting the JS tests, not considering some Swift concurrency subtleties (the fact that `async let` or `Task` tasks do not have any "synchronous part" thus needing to be careful about not missing events, same as in c75fa2a, and the lack of any ordering guarantees when triggering `Task`s), so I introduced the Subscription.addListener and MainActorStorage types to perform more work synchronously to be in line with the JS tests.
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
849d5dc to
35b109a
Compare
lawrence-forooghian
changed the base branch from
2025-08-13-fix-ci
to
fix-gc-sleep-duration
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
We'll use this when porting the garbage collection JS integration tests.
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
35b109a to
c39d55f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
github-actions
bot
temporarily deployed
to
staging/pull/57/AblyLiveObjects
|
@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: 4
🔭 Outside diff range comments (3)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (2)
48-64: serialized(…) currently doesn’t return a value in any switch case (won’t compile).Each case evaluates an expression but doesn’t return it. Make each branch return.
func serialized(serializeExtraValue: (Extra) -> Any) -> Any { - switch self { - case let .object(underlying): - underlying.mapValues { $0.serialized(serializeExtraValue: serializeExtraValue) } - case let .array(underlying): - underlying.map { $0.serialized(serializeExtraValue: serializeExtraValue) } - case let .string(underlying): - underlying - case let .number(underlying): - underlying - case let .bool(underlying): - underlying - case .null: - NSNull() - case let .extra(extra): - serializeExtraValue(extra) - } + switch self { + case let .object(underlying): + return underlying.mapValues { $0.serialized(serializeExtraValue: serializeExtraValue) } + case let .array(underlying): + return underlying.map { $0.serialized(serializeExtraValue: serializeExtraValue) } + case let .string(underlying): + return underlying + case let .number(underlying): + return underlying + case let .bool(underlying): + return underlying + case .null: + return NSNull() + case let .extra(extra): + return serializeExtraValue(extra) + } }
68-71: Closure for Extra == Never returns Void, but required to return Any (type mismatch).The closure
{ _ in }has type (Never) -> Void and won’t type-check against (Never) -> Any. Use the standard “unreachable” pattern.var serialized: Any { - // swiftlint:disable:next trailing_closure - serialized(serializeExtraValue: { _ in }) + // swiftlint:disable:next trailing_closure + serialized(serializeExtraValue: { extra in + // No values of type `Never` exist; this is unreachable. + switch extra {} + }) }Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift (1)
448-454: Type inconsistency between REST and wire operations.The REST operations use
Doublefor numeric values (lines 448, 466) while the wire operations useInt(lines 181, 199). This inconsistency could lead to precision loss or unexpected behavior when values flow between REST and wire protocols.Consider using consistent numeric types:
-func counterCreateRestOp(objectId: String? = nil, nonce: String? = nil, number: Double? = nil) -> [String: JSONValue] { +func counterCreateRestOp(objectId: String? = nil, nonce: String? = nil, number: Int? = nil) -> [String: JSONValue] { var opBody: [String: JSONValue] = [ "operation": .string(Actions.counterCreate.stringValue), ] if let number { opBody["data"] = .object(["number": .number(NSNumber(value: number))]) }-func counterIncRestOp(objectId: String, number: Double) -> [String: JSONValue] { +func counterIncRestOp(objectId: String, number: Int) -> [String: JSONValue] { [ "operation": .string(Actions.counterInc.stringValue), "objectId": .string(objectId), "data": .object(["number": .number(NSNumber(value: number))]), ] }Also applies to: 466-471
🧹 Nitpick comments (6)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
46-46: Minor grammar tweak in doc comment ("are" → "is").Subject is singular ("The contract... is"). Improves readability.
- /// The contract for what this will return are the same as those of `JSONValue.toJSONSerializationInputElement`, with one addition: any values in the input of case `.extra` will be passed to the `serializeExtraValue` function, and the result of this function call will be inserted into the output object. + /// The contract for what this will return is the same as that of `JSONValue.toJSONSerializationInputElement`, with one addition: any values in the input of case `.extra` will be passed to the `serializeExtraValue` function, and the result of this function call will be inserted into the output object.ably-cocoa (1)
1-1: Prefer tying the submodule bump to a tagged ably-cocoa release for v0 readiness (if available)If a tag exists that contains these changes, consider bumping to the tag (still stored as a SHA) and referencing the tag in release notes. This simplifies provenance and rollback.
Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift (1)
43-56: Consider potential race condition in listener snapshot.The current implementation captures a snapshot of listeners at line 47, but new listeners added via
addListenerbetween lines 44-56 might miss this invocation. While this might be intentional design for predictable test behavior, it's worth documenting this behavior explicitly.Consider adding a comment to clarify the snapshot behavior:
let callListeners = mutex.withLock { let invocation = (repeat each arg) invocations.append(invocation) + // Capture a snapshot of current listeners to avoid race conditions + // Note: Listeners added after this point won't receive this invocation return { [listeners] in for listener in listeners { listener.callAsFunction(repeat each invocation) } } }Sources/AblyLiveObjects/Internal/ARTClientOptions+Objects.swift (1)
4-10: Nit: doc typo; tighten Box semantics; clarify precondition messageMinor readability and diagnostics improvements.
Apply this diff:
- private class Box<T> { + private final class Box<T> { internal let boxed: T @@ - /// Can be overriden for testing purposes. + /// Can be overridden for testing purposes. @@ - guard let box = optionsValue as? Box<InternalDefaultRealtimeObjects.GarbageCollectionOptions> else { - preconditionFailure("Expected GarbageCollectionOptionsBox, got \(optionsValue)") - } + guard let box = optionsValue as? Box<InternalDefaultRealtimeObjects.GarbageCollectionOptions> else { + preconditionFailure("Expected Box<GarbageCollectionOptions>, got \(type(of: optionsValue))") + }Also applies to: 14-15, 26-28
Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
29-35: Fix misleading doc comment for overriddenPublishImplementationThis isn’t a boolean; it’s an optional override closure used by tests.
Apply this diff:
- /// If set to true, ``publish(objectMessages:)`` will behave like a no-op. - /// - /// This enables the `testsOnly_overridePublish(with:)` test hook. - /// - /// - Note: This should be `throws(InternalError)` but that causes a compilation error of "Runtime support for typed throws function types is only available in macOS 15.0.0 or newer". + /// Optional override for ``publish(objectMessages:)`` used by tests. + /// + /// When non-nil, `publish(objectMessages:)` will invoke this instead of the normal send path. + /// + /// - Note: This should be `throws(InternalError)` but that causes a compilation error: "Runtime support for typed throws function types is only available in macOS 15.0.0 or newer". private nonisolated(unsafe) var overriddenPublishImplementation: (([OutboundObjectMessage]) async throws -> Void)?Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
101-101: Name consistency and test helper completeness for GC events
- Rename completedGarbageCollectionsEventsContinuation to completedGarbageCollectionEventsContinuation for consistency.
- Finish the GC events stream in testsOnly_finishAllTestHelperStreams so tests can deterministically await all emissions.
Apply this diff:
- (completedGarbageCollectionEvents, completedGarbageCollectionsEventsContinuation) = AsyncStream.makeStream() + (completedGarbageCollectionEvents, completedGarbageCollectionEventsContinuation) = AsyncStream.makeStream() @@ - eventsContinuation: completedGarbageCollectionsEventsContinuation, + eventsContinuation: completedGarbageCollectionEventsContinuation, @@ - private let completedGarbageCollectionsEventsContinuation: AsyncStream<Void>.Continuation + private let completedGarbageCollectionEventsContinuation: AsyncStream<Void>.Continuation @@ internal func testsOnly_finishAllTestHelperStreams() { receivedObjectProtocolMessagesContinuation.finish() receivedObjectSyncProtocolMessagesContinuation.finish() waitingForSyncEventsContinuation.finish() + completedGarbageCollectionEventsContinuation.finish() }Also applies to: 335-336, 340-346, 356-359
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Sources/AblyLiveObjects/Internal/ARTClientOptions+Objects.swift(1 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(3 hunks)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift(1 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(3 hunks)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(2 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift(1 hunks)Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swift(3 hunks)Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift(2 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift(15 hunks)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift(1 hunks)ably-cocoa(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-29T08:08:00.588Z
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:08:00.588Z
Learning: Applies to **/Tests/**/*.swift : When you need to import the following modules in the tests, do so in the following way: Ably: use `import Ably`; AblyLiveObjects: use `testable import AblyLiveObjects`; AblyPlugin: use `import AblyPlugin`; do not do `internal import`.
Applied to files:
Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swiftSources/AblyLiveObjects/Internal/CoreSDK.swift
📚 Learning: 2025-07-29T08:07:47.875Z
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to AblyLiveObjects/**/!(*Test|*Tests).swift : When importing the Ably module inside the AblyLiveObjects library code (non-test code), use `import Ably`.
Applied to files:
Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swiftSources/AblyLiveObjects/Internal/CoreSDK.swift
📚 Learning: 2025-07-29T08:07:47.875Z
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to AblyLiveObjects/**/!(*Test|*Tests).swift : When importing the AblyPlugin module inside the AblyLiveObjects library code (non-test code), use `internal import AblyPlugin`.
Applied to files:
Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swiftSources/AblyLiveObjects/Internal/ARTClientOptions+Objects.swiftSources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/CoreSDK.swift
📚 Learning: 2025-07-29T08:08:00.588Z
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:08:00.588Z
Learning: Applies to **/Tests/**/*.swift : When creating `testsOnly_` 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.
Applied to files:
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift
📚 Learning: 2025-07-29T08:07:47.875Z
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to **/*.swift : When writing JSONValue or WireValue types, favour using the literal syntax enabled by their conformance to the `ExpressibleBy*Literal` protocols where possible.
Applied to files:
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift
🧬 Code Graph Analysis (5)
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (2)
Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
testsOnly_overridePublish(78-82)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
testsOnly_overridePublish(23-25)
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (1)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
performGarbageCollection(329-338)
Sources/AblyLiveObjects/Internal/CoreSDK.swift (2)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
testsOnly_overridePublish(23-25)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (1)
testsOnly_overridePublish(126-128)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (3)
Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
testsOnly_overridePublish(78-82)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (1)
testsOnly_overridePublish(126-128)Tests/AblyLiveObjectsTests/Helpers/Assertions.swift (1)
protocolRequirementNotImplemented(2-7)
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift (2)
Sources/AblyLiveObjects/Internal/InternalLiveObject.swift (1)
tombstone(17-35)Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
map(78-99)
🔇 Additional comments (15)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
29-36: Boolean vs NSNumber distinction looks good.Using identity checks against kCFBooleanTrue/False to disambiguate Bool from NSNumber(0/1) is correct here.
ably-cocoa (2)
1-1: LGTM: Submodule pointer updated as intendedThis repo only updates the ably-cocoa submodule pointer. No other code changes in this repo. Change aligns with the PR’s objective to enable/port JS integration tests relying on new hooks in ably-cocoa.
1-1: Internal accessibility already prevents shipping in productionThe
testsOnly_overridePublishAPI is declared asinternalon aninternalclass (PublicDefaultRealtimeObjects) and theCoreSDKprotocol is alsointernal. Tests access it via@testable import AblyLiveObjects, so it won’t be exposed in release builds. No gating changes are required.Sources/AblyLiveObjects/Internal/ObjectsPool.swift (1)
411-416: LGTM! Clean implementation of GC completion signaling.The addition of the
eventsContinuationparameter and the subsequent yield at line 442 properly implements the notification mechanism for garbage collection completion. The multi-line formatting improves readability with the increased parameter count.Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift (3)
12-12: LGTM! Proper storage for variadic callbacks.The addition of the
listenersarray withCallbackWrapperelements is a clean solution to store variadic callbacks.
60-69: LGTM! Elegant solution for variadic function storage.The
CallbackWrapperstruct effectively works around Swift's limitation on storing variadic function types in collections. The implementation is clean and the documentation clearly explains its purpose.
74-88: Well-designed late-joining listener support.The
addListenermethod correctly handles both registration and replay of buffered invocations. The async dispatch ensures callbacks are executed on the expected queue, maintaining thread safety.Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift (2)
133-151: LGTM! Consistent migration to WireValue.The migration from JSONValue to WireValue in
mapCreateOpis well-implemented, maintaining the same structure while using the new type system.
318-341: Encoder delegate lifetime correctly managedI searched the entire codebase for other uses of
ARTJsonLikeEncoderand this is the only occurrence. ThewithExtendedLifetime(jsonLikeEncoderDelegate)call properly ensures the delegate remains valid for the duration ofprotocolMessage(from:). No further changes are required.Tests/AblyLiveObjectsTests/Helpers/ClientHelper.swift (2)
2-2: LGTM! Correct import annotation.The
@testableimport is appropriate for test helper code that needs access to internal types.
29-31: LGTM! Clean propagation of GC options.The implementation correctly propagates
garbageCollectionOptionsfrom the test configuration to the client options, enabling test control over garbage collection behavior.Also applies to: 75-75
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (1)
121-128: LGTM! Well-documented test hook for publish override.The test-only API is properly documented and cleanly delegates to the CoreSDK's test override mechanism. The documentation clearly explains its purpose for integration testing scenarios.
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (1)
39-47: GC options are correctly threaded into InternalDefaultRealtimeObjectsFetching client options via pluginAPI and passing through garbageCollectionOptions with a sensible default is correct and keeps the initializer single-sourced for GC behavior.
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (2)
29-39: Encodable/Hashable on GarbageCollectionOptions looks goodThis enables storage/serialization and comparison in tests and plugin options.
328-336: GC completion events are correctly exposed and yieldedPiping the continuation into ObjectsPool.performGarbageCollection is a clean way to let tests await completion.
As in 8be3aed. Only noteworthy thing here is my increasing of the overridden GC interval compared to its value in the JS tests (see code comment).
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
c39d55f to
792683d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (3)
29-39: Consider adopting Codable + Sendable for GarbageCollectionOptionsIf these options are persisted and later read back from plugin options, Decodable will be required (Codable is a common convenience). Sendable clarifies safe concurrency semantics for this value type.
Would any consumer decode GarbageCollectionOptions (e.g., when reading from plugin options)? If yes, consider this change:
-internal struct GarbageCollectionOptions: Encodable, Hashable { +internal struct GarbageCollectionOptions: Codable, Hashable, Sendable {
101-101: Fix typo: completedGarbageCollectionsEventsContinuation → completedGarbageCollectionEventsContinuationThe continuation name is pluralized inconsistently compared to the stream. Renaming improves clarity and consistency.
@@ - (completedGarbageCollectionEvents, completedGarbageCollectionsEventsContinuation) = AsyncStream.makeStream() + (completedGarbageCollectionEvents, completedGarbageCollectionEventsContinuation) = AsyncStream.makeStream() @@ - eventsContinuation: completedGarbageCollectionsEventsContinuation, + eventsContinuation: completedGarbageCollectionEventsContinuation, @@ - private let completedGarbageCollectionsEventsContinuation: AsyncStream<Void>.Continuation + private let completedGarbageCollectionEventsContinuation: AsyncStream<Void>.Continuation @@ - completedGarbageCollectionsEventsContinuation.finish() + completedGarbageCollectionEventsContinuation.finish()Also applies to: 335-336, 341-346, 360-360
335-336: Optional: avoid yielding under lockPassing the continuation into performGarbageCollection means the yield may occur while the mutex is held, increasing lock-hold time if subscribers do anything non-trivial. Consider returning a flag (e.g., didCollect: Bool) from performGarbageCollection and yielding on the continuation after the mutex scope exits.
Can ObjectsPool.performGarbageCollection be updated to return whether any GC work was done, so we can yield outside the lock?
📜 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)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/AblyLiveObjects/Internal/ObjectsPool.swift
🧰 Additional context used
📓 Path-based instructions (2)
**/!(*Test|*Tests).swift
📄 CodeRabbit Inference Engine (.cursor/rules/swift.mdc)
**/!(*Test|*Tests).swift: Satisfy SwiftLint'sexplicit_aclrule: all declarations should specify Access Control Level keywords explicitly.
When writing anextensionof a type, favour placing the access level on the declaration of the extension rather than each of its individual members.
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
**/*.swift
📄 CodeRabbit Inference Engine (.cursor/rules/swift.mdc)
**/*.swift: When writing initializer expressions, when the type that is being initialized can be inferred, favour using the implicit.init(…)form instead of explicitly writing the type name.
When writing enum value expressions, when the type that is being initialized can be inferred, favour using the implicit.caseNameform instead of explicitly writing the type name.
When writing JSONValue or WireValue types, favour using the literal syntax enabled by their conformance to theExpressibleBy*Literalprotocols where possible.
When writing an array literal that starts with an initializer expression, start the initializer expression on the line after the opening square bracket of the array literal.
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: check-documentation
- GitHub Check: Generate code coverage
🔇 Additional comments (2)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (2)
340-347: LGTM: clear, test-friendly GC completion stream exposureThe test-only API is minimal and explicit, and ACLs are correct. This should make GC-related tests deterministic.
355-355: Docs: nice addition to enumerated test helper streamsHelps test authors discover the new GC completion stream quickly.
|
There seems to be a hanging test; I need to look into it |
In the interests of time and since there are bunch of PRs stacked on top of this, I think I'll merge and then investigate |
Note: This PR is based on top of #63; please review that one first.
This ports the remaining high-value JS integration tests, continuing the work started in fa255c1. I've created #60 for mopping up the remaining work, but I think that these tests should give us enough confidence for our v0 release. See commit messages for more details.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores