[AIT-846] ably-cocoa liveobjects UTS ground work#2209
Conversation
The unified clock abstraction work (the `mocking-for-uts` branch) adds two new boundary value protocols, `APContinuousClockInstant` and `APSchedulerHandle`, to ably-cocoa-plugin-support. Those changes have not yet been released, so this commit redirects the SPM dependency to the sibling worktree on the corresponding `mocking-for-uts` branch there so that subsequent ably-cocoa commits can adopt the new headers and still build. This commit is scaffolding. Before this branch is merged the dependency should be switched back to a versioned reference once a release of plugin-support containing the new protocols is available. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce a new private protocol, `ARTTimeProvider`, that unifies the three time-related primitives used by ably-cocoa's internal code: - `wallClockNow` (currently spelled `[NSDate date]`) - `continuousClockNow` (currently `[ARTContinuousClock now]`, internally `clock_gettime_nsec_np(CLOCK_MONOTONIC_RAW)`) - `scheduleAfter:queue:block:` (currently `artDispatchScheduled`) `ARTSystemTimeProvider` is the default implementation, backed by the real system primitives. The protocol's clock and scheduler return types are the new boundary value protocols `APContinuousClockInstant` and `APSchedulerHandle` from ably-cocoa-plugin-support, so that values originating from ably-cocoa's internal time provider can later flow across the plugin boundary unchanged. The existing concrete types, `ARTContinuousClockInstant` and `ARTScheduledBlockHandle`, gain conformance to those protocols. Conforming `ARTContinuousClockInstant` to `APContinuousClockInstant` requires `isAfter:` to take `id<APContinuousClockInstant>`, which in turn triggers Swift's automatic API translation to rename the method to `is(after:)`. Drop the previous `NS_SWIFT_NAME(isAfter(_:))` override and update the one existing Swift caller in `ContinuousClockTests` accordingly. `ARTContinuousClockInstant`'s designated initialiser, previously in a class extension in the .m file, moves to the header so that `ARTSystemTimeProvider` can construct instants. The `timeInNanosecondsSinceClockReferenceInstant` property stays in the extension since it remains an implementation detail. No call site is migrated to the new abstraction yet; the existing `ARTContinuousClock`, `[NSDate date]`, and `artDispatchScheduled` call sites continue to work unchanged. Migration happens in subsequent phases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tinuousClock Add a `timeProvider` property to `ARTTestClientOptions`, defaulting to an `ARTSystemTimeProvider` instance, so that tests can substitute a fake-time implementation that controls all clock-dependent behaviour in ably-cocoa (and, in due course, in plugins as well). `ARTRestInternal` and `ARTRealtimeInternal` now read the provider from `options.testOptions.timeProvider` in their `-init`, stash it as an ivar, and expose it via a class-extension property. This follows the same pattern they already use for `logger` and the internal dispatch queue. In `ARTRestInternal` the previous `_continuousClock` ivar is replaced by the new time provider. Its two consumers in `ARTRest.m` switch from `[self.continuousClock now]` and `[self.continuousClock addingDuration:toInstant:]` to `[self.timeProvider continuousClockNow]` and the new one-argument `-[APContinuousClockInstant addingDuration:]`. The `fallbackRetryExpiration` property's declared type changes from `ARTContinuousClockInstant *` to `id<APContinuousClockInstant>` to reflect that the value now flows through the boundary protocol. With its only consumer migrated, the `ARTContinuousClock` class is now unused and is deleted, along with its tests in `ContinuousClockTests.swift`. `ARTContinuousClockInstant` remains as the concrete implementation of `APContinuousClockInstant`; the containing file keeps the name `ARTContinuousClock.h` for now to avoid a rename in an otherwise mechanical commit. No semantic behaviour change: with the default `ARTSystemTimeProvider` installed, every migrated call site is functionally identical to its pre-commit form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace every direct `[NSDate date]` call in `ARTRealtime.m` with `[_timeProvider wallClockNow]`. The affected reads are the `_lastActivity`, `_connectionLostAt`, and suspension-mode timestamps, plus the idle-timer's elapsed-interval log line. Pure mechanical substitution. The default `ARTSystemTimeProvider` returns `[NSDate date]`, so production behaviour is unchanged; the indirection lets tests install a fake provider. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`-didRemovedMemberNoLongerPresent:` stamps the synthesised `LEAVE` message with the current wall-clock time. Replace its `[NSDate date]` call with `[_timeProvider wallClockNow]`. Add a `_timeProvider` ivar to `ARTRealtimePresenceInternal`, populated in `-init` from the channel's realtime client's options. No init signature change; the provider is read off the existing constructor arguments. Production behaviour is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`-log:withLevel:` previously stamped every `ARTLogLine` with `[NSDate date]`. Add a `timeProvider` property to `ARTLog` (declared in `ARTLog+Private.h`, defaulting to an `ARTSystemTimeProvider` instance) and read the timestamp from it instead. `ARTLog` is a user-facing class whose `init` takes no options, so the provider is not threaded automatically from `testOptions`. Internal code or tests that want log-line timestamps to follow a fake clock can install one via the new private property. Production behaviour is unchanged: the default `ARTSystemTimeProvider` returns `[NSDate date]`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rovider `-tokenRequestToDictionary:` falls back to the current wall-clock time when the supplied `ARTTokenRequest` has no timestamp. Replace its `[NSDate date]` call with `[_timeProvider wallClockNow]`. Add a `_timeProvider` ivar; the rest-having initialiser reads it from the rest client's `testOptions.timeProvider`, while the standalone initialisers fall back to an `ARTSystemTimeProvider` (those code paths do not produce token requests but still get a sensible default). Production behaviour is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`-currentDate` is the single point through which the auth flow obtains the current wall-clock time, with `_timeOffset` applied. Replace its direct `[NSDate date]` call with `[_timeProvider wallClockNow]`. The `_timeOffset` arithmetic and the `NSSystemClockDidChangeNotification` observer (which clears `_timeOffset` on system clock changes) are unchanged. Add a `_timeProvider` ivar populated from `options.testOptions` in `-init:withOptions:logger:`. Production behaviour is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`ARTRealtime` schedules two delayed operations directly via `artDispatchScheduled` from `ARTGCD.h`: - the auth-timeout work scheduled while waiting for `_authorize:` to complete; and - the idle timer that fires if no activity is seen from the realtime connection within `realtimeRequestTimeout + maxIdleInterval`. Replace both with `[_timeProvider scheduleAfter:queue:block:]`. The `_authenitcatingTimeoutWork` and `_idleTimer` ivars switch from the concrete `ARTScheduledBlockHandle *` type to the boundary protocol `id<APSchedulerHandle>`. The four `artDispatchCancel(...)` call sites that operate on these handles become plain `[handle cancel]` messages (Objective-C messaging to a nil receiver is a no-op, matching `artDispatchCancel`'s previous nil-check). `artDispatchScheduled` itself is not yet removed; it is still used by `ARTSystemTimeProvider` and by `ARTEventListener` (the latter is migrated in the next commit), at which point the function can be inlined into `ARTSystemTimeProvider` and deleted from `ARTGCD`. No semantic behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ovider `ARTEventListener.-startTimer` is the last remaining direct caller of `artDispatchScheduled` in ably-cocoa, used to implement `setTimer:onTimeout:` for one-shot subscription timeouts. Route it through `ARTTimeProvider.scheduleAfter:queue:block:` and drop `artDispatchScheduled` from `ARTGCD`. `ARTEventEmitter` now takes a time provider at construction: - The designated initialisers gain a `timeProvider:` parameter alongside the existing `queue` (and optional `userQueue`). - `ARTInternalEventEmitter` mirrors the new signatures. - `ARTPublicEventEmitter` reads the provider from `rest.timeProvider` (newly exposed on `ARTRestInternal` via `ARTRest+Private.h` to support this). - `ARTEventListener.-startTimer` schedules through `_eventHandler.timeProvider`, and `_work` is now typed as `id<APSchedulerHandle>`; the existing `artDispatchCancel(_work)` calls become plain `[_work cancel]` messages. Every existing construction site of `ARTInternalEventEmitter` is updated to pass an appropriate provider — sourced from `_rest.timeProvider`, `_realtime.rest.timeProvider`, or the local `_timeProvider` ivar that earlier phases already wired into the owning class. Two test-only construction sites in `UtilitiesTests` pass a fresh `ARTSystemTimeProvider`. With no remaining call sites in the SDK, `artDispatchScheduled` and the `artDispatchCancel` inline helper are removed from `ARTGCD.h`/`ARTGCD.m`. `ARTSystemTimeProvider.-scheduleAfter:...` inlines the previous body — a direct construction of `ARTScheduledBlockHandle` — and the `GCDTests` unit test calls that constructor directly. `art_dispatch_sync`/`art_dispatch_async`, which are unrelated to scheduling, are unchanged. No semantic behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a "Time-related operations" section to `CONTRIBUTING.md` under "Coding standards" describing the rule that all time-dependent code must go through an injected `id<ARTTimeProvider>` rather than calling clock or scheduler primitives directly, the list of what not to do (`[NSDate date]`, `clock_gettime_nsec_np`, `dispatch_after`, etc.), and the standard pattern by which a class obtains its provider. Add a corresponding doc comment in `ARTTimeProvider.h` pointing back to the contributing guide. The header is the more discoverable entry point for a contributor adding new time-related code, so it restates the prohibitions in short form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Provide `ARTPluginAPI`'s implementation of the three new `APPluginAPIProtocol` methods introduced in 29f699a in ably-cocoa-plugin-support. Each method is a thin forwarder that looks up the supplied client's `ARTTimeProvider` and delegates to its corresponding method. To support this, `ARTRealtimeInternal.timeProvider` moves from the class extension in `ARTRealtime.m` to `ARTRealtime+Private.h` so that the plugin API conformer in `ARTPluginAPI.m` can read it. The header carries a doc comment cross-referencing the plugin API methods, mirroring the convention used by `latestConnectionDetails`. With this in place, an Ably-authored plugin that goes through `APPluginAPI` for all time and scheduling operations will pick up whatever provider was installed via `options.testOptions.timeProvider` on the SDK side, including any future fake-time implementation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Harness (Test/UTS/Harness): - MockWebSocket: a real ARTWebSocketTransport over a faked ARTWebSocket, so URL/query-param building stays real; the provider drives the simulated server (respondWithSuccess/sendToClient/simulateDisconnect). - MockHTTP: fake ARTHTTPExecutor to intercept/observe requests and respond. - FakeTimeProvider: deterministic clock, opt-in via enableFakeTimers(). - UTSTestCase: installMock / makeRealtime / makeRest, AWAIT_STATE polling, CapturingLog, NoOpReachability. Tests: ConnectionRecoveryTests (RTN16g/g1, g2, f, f1, j, k) and TimeTests (RSC16, 0-4). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
CI can't resolve the temporary local-path dependency on ../ably-cocoa-plugin-support (the folder doesn't exist on the runner). Repoint to the pushed `mocking-for-uts` branch, which carries the unreleased APContinuousClockInstant / APSchedulerHandle protocols this branch adopts. Still scaffolding — switch to a versioned release before merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cocoa equivalent of ably-java's .claude/skills/uts-to-kotlin/SKILL.md (PR #1209). Invocable as `/uts-to-swift <spec-file>`, it walks the translation from a UTS pseudocode spec to a Swift test in the UTS target, mapped onto this repo's harness: installMock + makeRealtime/ makeRest, MockWebSocketProvider/MockWebSocket, MockHTTP, the ARTProtocolMessage +UTS factories, awaitConnectionState/awaitChannelState/ poll, and enableFakeTimers/advanceTime. Encodes the conventions established for this suite: the local-array capture pattern (vs. a mock property), seconds-not-wire-milliseconds for connectionStateTtl/maxIdleInterval, CapturingLog for log assertions, the test_<SPEC>_<desc> + `// UTS:` naming, RUN_DEVIATIONS env-gated skips, and the distinction between an SDK deviation (deviations.md) and a harness-driving choice (code comment). Requires that spec comments are copied verbatim and every ASSERT is either translated or annotated when it has no Swift equivalent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| @@ -0,0 +1,399 @@ | |||
| import XCTest | |||
There was a problem hiding this comment.
Is there a reason we're not using Swift Testing?
| // TODO: Unpin before release. Points at the unreleased `mocking-for-uts` branch of | ||
| // ably-cocoa-plugin-support (adds APContinuousClockInstant / APSchedulerHandle); switch back | ||
| // to a versioned reference once a release containing them is available. | ||
| .package(name: "ably-cocoa-plugin-support", url: "https://github.com/ably/ably-cocoa-plugin-support.git", .branch("mocking-for-uts")) |
There was a problem hiding this comment.
from Slack:
btw I see that your PR introduces a new version of ably-cocoa-plugin-support . i think that this is unnecessary for the first PR (that'll only be needed once we want to get this working with LiveObjects, and we don't yet know if we're going to merge the repos or not). we can just include those new types (SchedulerHandle and whatever else) inside ably-cooca for now
| let mockHTTP = MockHTTP( | ||
| onConnectionAttempt: { connection in connection.respondWithSuccess() }, | ||
| onRequest: { request in | ||
| capturedRequests.append(request) |
There was a problem hiding this comment.
I'm pretty sure that if we were compiling in the Swift 6 language mode (i.e. strict concurrency checking) then this wouldn't compile (the compiler can't reason about whether this append() is safe). It would be great if this new test suite could use the latest Swift language so that we get all these compiler checks.
There was a problem hiding this comment.
(I would have loved for our existing test suite to also use strict concurrency checking, but porting it all would be a lot of work. But I think we should be doing it for all new targets.)
Addresses review feedback (PR #2209): the local captured-array pattern (`var captured = []` appended from a mock handler) is a data race the Swift 6 compiler rejects, and the suite should use the latest language mode so such races are caught. - Package.swift: build the UTS target with `-swift-version 6`. - Import Ably / Ably.Private `@preconcurrency` in the harness, since the Objective-C SDK isn't Sendable-audited; this keeps strict checking for our own code while treating SDK interop as warnings. - Make the mocks Sendable with their existing internal synchronisation: MockWebSocketProvider / MockWebSocket / MockHTTP / PendingHTTP* / FakeTimeProvider are `@unchecked Sendable`, and their handler closures are `@Sendable`. - Add `Captured<T>`, a lock-guarded, Sendable collector for the spec's local `captured_*` arrays; tests use it instead of a mutable `var` array (which now fails to compile inside a `@Sendable` handler). The "first attempt vs later" branches switch on `Captured.count` rather than a separate mutable counter. - Update the /uts-to-swift skill and README accordingly. All 11 tests pass; the suite builds warning-clean under Swift 6. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Universal Test Suite (UTS) target (RTN16, RSC16) Harness (Test/UTS/Harness): - MockWebSocket: a real ARTWebSocketTransport over a faked ARTWebSocket, so URL/query-param building stays real; the provider drives the simulated server (respondWithSuccess/sendToClient/simulateDisconnect). - MockHTTP: fake ARTHTTPExecutor to intercept/observe requests and respond. - FakeTimeProvider: deterministic clock, opt-in via enableFakeTimers(). - UTSTestCase: installMock / makeRealtime / makeRest, AWAIT_STATE polling, CapturingLog, NoOpReachability. Tests: ConnectionRecoveryTests (RTN16g/g1, g2, f, f1, j, k) and TimeTests (RSC16, 0-4). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Add /uts-to-swift translation skill Cocoa equivalent of ably-java's .claude/skills/uts-to-kotlin/SKILL.md (PR #1209). Invocable as `/uts-to-swift <spec-file>`, it walks the translation from a UTS pseudocode spec to a Swift test in the UTS target, mapped onto this repo's harness: installMock + makeRealtime/ makeRest, MockWebSocketProvider/MockWebSocket, MockHTTP, the ARTProtocolMessage +UTS factories, awaitConnectionState/awaitChannelState/ poll, and enableFakeTimers/advanceTime. Encodes the conventions established for this suite: the local-array capture pattern (vs. a mock property), seconds-not-wire-milliseconds for connectionStateTtl/maxIdleInterval, CapturingLog for log assertions, the test_<SPEC>_<desc> + `// UTS:` naming, RUN_DEVIATIONS env-gated skips, and the distinction between an SDK deviation (deviations.md) and a harness-driving choice (code comment). Requires that spec comments are copied verbatim and every ASSERT is either translated or annotated when it has no Swift equivalent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Use Swift Testing instead of XCTest in the UTS suite Convert the UTS target from XCTest to Swift Testing (`import Testing`): - UTSTestCase becomes a plain base class (no longer XCTestCase); suites are `@Suite(.serialized) final class ...: UTSTestCase`. Swift Testing creates a fresh instance per `@Test`, so teardown moves from `tearDown()` to `deinit`. - Assertions: XCTAssert* -> #expect, XCTUnwrap -> #require, XCTFail -> Issue.record (with SourceLocation), file/line params -> #_sourceLocation. - TimeTests' async REST calls drop XCTestExpectation for `async` tests that bridge the completion handler via withCheckedContinuation. Also updates the /uts-to-swift skill, README, and deviations.md to document the Swift Testing conventions (#expect/#require, @Test/@suite, the .enabled(if:) env-gated deviation skip). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Build the UTS suite in the Swift 6 language mode Addresses review feedback (PR #2209): the local captured-array pattern (`var captured = []` appended from a mock handler) is a data race the Swift 6 compiler rejects, and the suite should use the latest language mode so such races are caught. - Package.swift: build the UTS target with `-swift-version 6`. - Import Ably / Ably.Private `@preconcurrency` in the harness, since the Objective-C SDK isn't Sendable-audited; this keeps strict checking for our own code while treating SDK interop as warnings. - Make the mocks Sendable with their existing internal synchronisation: MockWebSocketProvider / MockWebSocket / MockHTTP / PendingHTTP* / FakeTimeProvider are `@unchecked Sendable`, and their handler closures are `@Sendable`. - Add `Captured<T>`, a lock-guarded, Sendable collector for the spec's local `captured_*` arrays; tests use it instead of a mutable `var` array (which now fails to compile inside a `@Sendable` handler). The "first attempt vs later" branches switch on `Captured.count` rather than a separate mutable counter. - Update the /uts-to-swift skill and README accordingly. All 11 tests pass; the suite builds warning-clean under Swift 6. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Address Copilot review (PR #2210) - ARTContinuousClock.m: -isAfter: now guards the downcast of the id<ARTContinuousClockInstantProtocol> argument and raises a clear exception on a type mismatch, instead of force-casting and risking an invalid memory read. - CONTRIBUTING.md: the "Time-related operations" bullets referenced the old APContinuousClockInstant / APSchedulerHandle names; point them at the actual ARTContinuousClockInstantProtocol / ARTSchedulerHandle. - Package.swift: the UTS target comment said "XCTest suite"; it's a Swift Testing suite. - MockHTTP: execute(_:completion:) now fails fast (assertion + error callback) when no onRequest handler is installed, rather than leaving the request (and any awaiting test) hanging with the callback uncalled. The README layout drift Copilot also flagged was already corrected in the preceding commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Keep UTS in Swift 6 mode without @preconcurrency
Adds a Universal Test Suite (UTS) target deriving tests from the language-neutral pseudo-code specs in
ably/specificationunderuts/.What's here
UTSXCTest target (Test/UTS), separate fromAblyTests, no Nimble.Test/UTS/Harness):ARTWebSocketTransportover a fakedARTWebSocket, so URL/query-param building stays real; the provider drives the simulated server (respondWithSuccess/sendToClient/simulateDisconnect).ARTHTTPExecutorto intercept/observe requests and inject responses.enableFakeTimers(); self-containedadvanceTime.installMock/makeRealtime/makeRest,AWAIT_STATEpolling,CapturingLog,NoOpReachability.ConnectionRecoveryTests(RTN16g/g1, g2, f, f1, j, k) andTimeTests(RSC16 ×5).Test names/IDs and fake-timer usage mirror the ably-java UTS PR (ably/ably-java#1209); requests/connection attempts are captured into local arrays per the spec.
deviations.mdis currently empty — no SDK non-compliance found for these specs.Builds on
This branch also contains the
ARTTimeProvidergroundwork (injectable clock/scheduler) that the fake-timer harness relies on.Includes a temporary branch dependency on
ably-cocoa-plugin-support(Package.swift) — must be unpinned before merge.Test plan
swift test --filter UTS→ 11 tests, all passing.🤖 Generated with Claude Code