[AIT-846] Time mocking (ARTTimeProvider abstraction)#2210
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces ChangesTime provider abstraction and SDK migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
maratal
force-pushed
the
AIT-846-mock-for-time
branch
from
04deb80 to
0713756
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
maratal
force-pushed
the
AIT-846-mock-for-time
branch
from
0713756 to
1d0e282
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
There was a problem hiding this comment.
Pull request overview
This PR lays the groundwork for running Ably’s Universal Test Suite (UTS) against ably-cocoa by introducing an injectable ARTTimeProvider abstraction (covering wall clock, continuous clock, and scheduling) and by adding a new standalone UTS Swift Testing target with a mock transport/HTTP/time harness and initial derived tests.
Changes:
- Introduce
ARTTimeProvider+ defaultARTSystemTimeProvider, and route internal timestamps/timers/scheduling through it. - Add a new
UTSSwift Testing target (Test/UTS) with a deterministic fake clock, mocked WebSocket/HTTP, and initial RTN16/RSC16 derived tests. - Update internal/private headers, module maps, and a few existing tests to align with the new time/scheduler APIs.
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Test/UTS/Tests/TimeTests.swift | Adds RSC16 time() derived tests using mocked HTTP. |
| Test/UTS/Tests/ConnectionRecoveryTests.swift | Adds RTN16 recovery derived tests using mocked WebSocket and fake timers where needed. |
| Test/UTS/README.md | Documents UTS target layout, harness primitives, and running instructions. |
| Test/UTS/Harness/UTSTestCase.swift | Provides shared harness utilities: installMock/makeRealtime/makeRest, polling, fake-timer control, cleanup. |
| Test/UTS/Harness/NoOpReachability.swift | Disables OS reachability monitoring during unit tests. |
| Test/UTS/Harness/MockWebSocket.swift | Implements a fake ARTWebSocket + provider/factory to exercise real transport URL/query construction. |
| Test/UTS/Harness/MockHTTP.swift | Implements a fake ARTHTTPExecutor to intercept requests and inject responses without network I/O. |
| Test/UTS/Harness/FakeTimeProvider.swift | Implements deterministic ARTTimeProvider for controllable clocks and scheduling, plus timer leak safety net. |
| Test/UTS/Harness/CapturingLog.swift | Adds an ARTLog subclass to capture log output for assertions. |
| Test/UTS/Harness/ARTProtocolMessage+UTS.swift | Adds protocol-message factory helpers for UTS tests. |
| Test/UTS/deviations.md | Adds a template for documenting spec deviations and env-gated deviation tests. |
| Test/AblyTests/Tests/UtilitiesTests.swift | Updates ARTInternalEventEmitter init sites to pass a time provider. |
| Test/AblyTests/Tests/GCDTests.swift | Updates scheduled-block handle construction to the new ARTScheduledBlockHandle API. |
| Test/AblyTests/Tests/ContinuousClockTests.swift | Removes continuous clock tests that depended on the removed ARTContinuousClock class API. |
| Test/Ably.xctestplan | Adds the UTS target to the Xcode test plan. |
| Source/PrivateHeaders/Ably/ARTTimeProvider.h | Introduces the unified time abstraction protocol used internally. |
| Source/PrivateHeaders/Ably/ARTTestClientOptions.h | Adds timeProvider to test options for injection. |
| Source/PrivateHeaders/Ably/ARTSystemTimeProvider.h | Declares the default system-backed time provider. |
| Source/PrivateHeaders/Ably/ARTSchedulerHandle.h | Introduces a cancellable scheduler handle protocol. |
| Source/PrivateHeaders/Ably/ARTRest+Private.h | Exposes timeProvider and updates fallback expiration to protocol-based continuous instants. |
| Source/PrivateHeaders/Ably/ARTRealtime+Private.h | Exposes timeProvider from realtime internals for plugins and internal use. |
| Source/PrivateHeaders/Ably/ARTLog+Private.h | Adds injectable time provider for log timestamping. |
| Source/PrivateHeaders/Ably/ARTGCD.h | Updates scheduled-block handle to conform to ARTSchedulerHandle; removes old helper functions. |
| Source/PrivateHeaders/Ably/ARTEventEmitter+Private.h | Threads timeProvider through event emitter internals and updates initializers accordingly. |
| Source/PrivateHeaders/Ably/ARTContinuousClockInstantProtocol.h | Introduces protocol for continuous-clock instants (type-erased across time providers). |
| Source/PrivateHeaders/Ably/ARTContinuousClock.h | Refactors to expose only the ARTContinuousClockInstant concrete instant type conforming to the protocol. |
| Source/include/module.modulemap | Exposes new private headers via the module map. |
| Source/ARTWebSocketTransport.m | Injects timeProvider into the transport state emitter. |
| Source/ARTTestClientOptions.m | Initializes and copies the default ARTSystemTimeProvider in test options. |
| Source/ARTSystemTimeProvider.m | Implements the system-backed wall clock, continuous clock, and scheduler. |
| Source/ARTRest.m | Replaces direct continuous-clock usage with injected timeProvider. |
| Source/ARTRealtimePresence.m | Routes presence timestamps and event emitters through injected timeProvider. |
| Source/ARTRealtimeChannel.m | Passes timeProvider into internal channel event emitters. |
| Source/ARTRealtimeAnnotations.m | Passes timeProvider into annotations event emitter. |
| Source/ARTRealtime.m | Replaces [NSDate date]/dispatch scheduling with timeProvider wall clock + scheduler handles. |
| Source/ARTPluginAPI.m | Exposes time primitives/scheduling to plugins through the plugin API. |
| Source/ARTLog.m | Uses injected time provider for log line timestamps (defaulting to system provider). |
| Source/ARTJsonLikeEncoder.m | Uses injected wall clock for token-request timestamps instead of [NSDate date]. |
| Source/ARTGCD.m | Removes the old artDispatchScheduled helper implementation. |
| Source/ARTEventEmitter.m | Replaces dispatch scheduling with timeProvider scheduling; threads provider through emitter initializers. |
| Source/ARTContinuousClock.m | Refactors continuous instant comparison/addition to protocol-based API. |
| Source/ARTAuth.m | Uses injected wall clock for auth’s notion of current time and for cancellation event emitter scheduling. |
| Source/Ably.modulemap | Exposes new private headers via the framework module map. |
| Package.swift | Adds the UTS SwiftPM test target. |
| CONTRIBUTING.md | Documents the new “Time-related operations” convention using ARTTimeProvider. |
| Ably.xcodeproj/project.pbxproj | Adds new headers/sources to Xcode project and header build phases. |
| .swiftpm/xcode/xcshareddata/xcschemes/ably-cocoa.xcscheme | Adds UTS to the SwiftPM-generated Xcode scheme test action. |
| .claude/skills/uts-to-swift/SKILL.md | Adds a Claude Code skill spec for translating UTS pseudocode specs into Swift tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
maratal
added a commit
that referenced
this pull request
Jun 4, 2026
- 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>
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
maratal
added a commit
that referenced
this pull request
Jun 18, 2026
PendingHTTPConnection derived its host from the request URL with a `?? ""` fallback, silently fabricating a hostless connection. A request without a URL host means the test set-up (or the SDK) is broken, so fail fast — matching how PendingHTTPRequest already handles a missing URL. Addresses #2210 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maratal
added a commit
that referenced
this pull request
Jun 18, 2026
MockHTTPClient tracked a connection phase across requests: it fired onConnectionAttempt once before the first request and remembered the outcome (connectionResolved / connectionError, guarded by an NSLock) to apply to every later request. The cocoa HTTP seam is request-level (executeRequest:completion:), so each execute() is really standalone — model it that way: consult onConnectionAttempt per request (it returns the connection error, or nil for success), fail the request if it rejects, otherwise deliver to onRequest. This removes the lock, the withLock helper, and all shared mutable state, so each request is independent. PendingHTTPConnection and PendingHTTPRequest are immutable structs, and MockHTTPClient now earns a checked Sendable conformance (no @unchecked) — the compiler verifies its thread-safety rather than us asserting it. Addresses #2210 (comment) Addresses #2210 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maratal
added a commit
that referenced
this pull request
Jun 18, 2026
The other UTS test doubles are MockWebSocket / MockHTTPClient, so name the time provider MockTimeProvider too for consistency. The spec-derived feature name enableFakeTimers() (UTS enable_fake_timers()) is left unchanged. Addresses #2210 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maratal
added a commit
that referenced
this pull request
Jun 18, 2026
Place each translated test under Test/UTS/Tests/ following the spec's own rest/realtime + unit/integration folder layout, so specs that share a file name in different parts of the tree don't collide (e.g. uts/realtime/integration/auth.md vs uts/rest/integration/auth.md). Uses concrete example paths rather than placeholder vocabulary. Addresses #2210 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maratal
added a commit
that referenced
this pull request
Jun 18, 2026
Precede each Step 4 code example with the spec pseudocode it's a translation of (WebSocket setup + branching, HTTP setup, capturing attempts, inspecting frames, timer control), lifted faithfully from the actual spec files (connection_recovery_test.md, time.md, mock_websocket.md, mock_http.md) so the constructs match: two-call respond_with_success() + send_to_client(), events.find(CONNECTION_SUCCESS).connection, events.filter(ws_frame, client_to_server), LOOP up to N times / ADVANCE_TIME, captured arrays. Log capture is left as prose (the spec says the mechanism is implementation-specific). Addresses #2210 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maratal
added a commit
that referenced
this pull request
Jun 18, 2026
Mirror the spec's markdown headings (Setup / Test Steps / Assertions) as section comments in each test, and align the inline comments with the spec's wording, so each test reads against uts/rest/unit/time.md at a glance. Addresses #2210 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maratal
added a commit
that referenced
this pull request
Jun 18, 2026
Note on awaitTime / awaitTimeError the UTS pseudocode they're a translation of (`result = AWAIT client.time()` / `AWAIT client.time() FAILS WITH error`). Addresses #2210 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maratal
added a commit
that referenced
this pull request
Jun 18, 2026
awaitTime / awaitTimeError #require the date / error internally and return a non-optional Date / ARTErrorInfo (async throws), so callers no longer unwrap an optional at each call site. Addresses #2210 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maratal
added a commit
that referenced
this pull request
Jun 18, 2026
Same treatment as the TimeTests change (b81f97b): add `// Setup` / `// Test Steps` / `// Assertions` section comments to each realtime test, mirroring the spec's markdown headings so each test reads against uts/realtime/unit/connection/connection_recovery_test.md at a glance. RTN16g2's `// Assertions` block holds the FAILED/SUSPENDED sub-tests, matching the spec. Follows up #2210 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
lawrence-forooghian
force-pushed
the
AIT-846-mock-for-time
branch
from
253e362 to
209f1e9
Compare
lawrence-forooghian
approved these changes
Jun 18, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/PrivateHeaders/Ably/ARTTimeProvider.h`:
- Line 10: The ARTTimeProvider contract comment at line 10 currently states that
fake-time controls behavior across "ably-cocoa and any plugins", but the PR's
scope defers plugin exposure. Update the comment to accurately reflect the
current behavior by removing the reference to "any plugins" and clarifying that
the fake-time implementation currently controls clock-dependent behavior only
within ably-cocoa itself, avoiding misleading future integrations about plugin
support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0da1dd83-73c1-48bc-8854-016ecf13f31a
📒 Files selected for processing (32)
Ably.xcodeproj/project.pbxprojCONTRIBUTING.mdSource/ARTAuth.mSource/ARTContinuousClock.mSource/ARTEventEmitter.mSource/ARTGCD.mSource/ARTJsonLikeEncoder.mSource/ARTLog.mSource/ARTRealtime.mSource/ARTRealtimeAnnotations.mSource/ARTRealtimeChannel.mSource/ARTRealtimePresence.mSource/ARTRest.mSource/ARTSystemTimeProvider.mSource/ARTTestClientOptions.mSource/ARTWebSocketTransport.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTContinuousClock.hSource/PrivateHeaders/Ably/ARTContinuousClockInstant.hSource/PrivateHeaders/Ably/ARTEventEmitter+Private.hSource/PrivateHeaders/Ably/ARTGCD.hSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTRest+Private.hSource/PrivateHeaders/Ably/ARTSchedulerHandle.hSource/PrivateHeaders/Ably/ARTSystemTimeProvider.hSource/PrivateHeaders/Ably/ARTTestClientOptions.hSource/PrivateHeaders/Ably/ARTTimeProvider.hSource/include/module.modulemapTest/AblyTests/Tests/ContinuousClockTests.swiftTest/AblyTests/Tests/GCDTests.swiftTest/AblyTests/Tests/SystemTimeProviderTests.swiftTest/AblyTests/Tests/UtilitiesTests.swift
💤 Files with no reviewable changes (7)
- Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h
- Source/PrivateHeaders/Ably/ARTGCD.h
- Source/ARTContinuousClock.m
- Source/PrivateHeaders/Ably/ARTContinuousClock.h
- Test/AblyTests/Tests/ContinuousClockTests.swift
- Test/AblyTests/Tests/GCDTests.swift
- Source/ARTGCD.m
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (18)
- Source/ARTRealtimeAnnotations.m
- Source/Ably.modulemap
- Source/ARTRealtimeChannel.m
- Source/PrivateHeaders/Ably/ARTEventEmitter+Private.h
- Source/ARTTestClientOptions.m
- Source/ARTLog.m
- Source/PrivateHeaders/Ably/ARTRest+Private.h
- Source/PrivateHeaders/Ably/ARTSystemTimeProvider.h
- Test/AblyTests/Tests/UtilitiesTests.swift
- Source/ARTJsonLikeEncoder.m
- Source/ARTRest.m
- Source/ARTSystemTimeProvider.m
- Test/AblyTests/Tests/SystemTimeProviderTests.swift
- Source/ARTRealtime.m
- Source/ARTEventEmitter.m
- Source/ARTRealtimePresence.m
- Source/ARTAuth.m
- Ably.xcodeproj/project.pbxproj
github-actions
Bot
temporarily deployed
to
staging/pull/2210/markdown-api-reference
lawrence-forooghian
force-pushed
the
AIT-846-mock-for-time
branch
from
209f1e9 to
d43144e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Source/PrivateHeaders/Ably/ARTTimeProvider.h (1)
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign plugin-scope wording with current support boundary.
The contract comment currently overstates scope by saying fake-time applies to plugins; this PR scope keeps plugin exposure deferred/gated, so the wording should reflect ably-cocoa internals only.
As per coding guidelines, “Plugin support is gated behind `#ifdef ABLY_SUPPORTS_PLUGINS` and is enabled only in SPM builds.”Proposed wording update
- All time-dependent code in ably-cocoa MUST go through an injected `ARTTimeProvider` rather than calling clock or scheduler primitives directly. This indirection allows the Universal Test Suite to install a fake-time implementation that controls all clock-dependent behaviour across both ably-cocoa and any plugins. + All time-dependent code in ably-cocoa MUST go through an injected `ARTTimeProvider` rather than calling clock or scheduler primitives directly. This indirection allows the Universal Test Suite to install a fake-time implementation that controls all clock-dependent behaviour across ably-cocoa internals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/PrivateHeaders/Ably/ARTTimeProvider.h` at line 10, The contract comment for ARTTimeProvider overstates the scope by claiming the fake-time implementation controls clock-dependent behaviour in plugins, but plugin support is actually gated behind `#ifdef` ABLY_SUPPORTS_PLUGINS and not generally exposed. Update the comment to remove the reference to plugins and clarify that the fake-time implementation applies only to ably-cocoa internals, not to plugin code, to accurately reflect the current support boundary.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Source/PrivateHeaders/Ably/ARTTimeProvider.h`:
- Line 10: The contract comment for ARTTimeProvider overstates the scope by
claiming the fake-time implementation controls clock-dependent behaviour in
plugins, but plugin support is actually gated behind `#ifdef`
ABLY_SUPPORTS_PLUGINS and not generally exposed. Update the comment to remove
the reference to plugins and clarify that the fake-time implementation applies
only to ably-cocoa internals, not to plugin code, to accurately reflect the
current support boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a82080c2-7b0e-43c4-a319-1ea36aa2f520
📒 Files selected for processing (32)
Ably.xcodeproj/project.pbxprojCONTRIBUTING.mdSource/ARTAuth.mSource/ARTContinuousClock.mSource/ARTEventEmitter.mSource/ARTGCD.mSource/ARTJsonLikeEncoder.mSource/ARTLog.mSource/ARTRealtime.mSource/ARTRealtimeAnnotations.mSource/ARTRealtimeChannel.mSource/ARTRealtimePresence.mSource/ARTRest.mSource/ARTSystemTimeProvider.mSource/ARTTestClientOptions.mSource/ARTWebSocketTransport.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTContinuousClock.hSource/PrivateHeaders/Ably/ARTContinuousClockInstant.hSource/PrivateHeaders/Ably/ARTEventEmitter+Private.hSource/PrivateHeaders/Ably/ARTGCD.hSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTRest+Private.hSource/PrivateHeaders/Ably/ARTSchedulerHandle.hSource/PrivateHeaders/Ably/ARTSystemTimeProvider.hSource/PrivateHeaders/Ably/ARTTestClientOptions.hSource/PrivateHeaders/Ably/ARTTimeProvider.hSource/include/module.modulemapTest/AblyTests/Tests/ContinuousClockTests.swiftTest/AblyTests/Tests/GCDTests.swiftTest/AblyTests/Tests/SystemTimeProviderTests.swiftTest/AblyTests/Tests/UtilitiesTests.swift
💤 Files with no reviewable changes (7)
- Test/AblyTests/Tests/ContinuousClockTests.swift
- Test/AblyTests/Tests/GCDTests.swift
- Source/PrivateHeaders/Ably/ARTGCD.h
- Source/ARTGCD.m
- Source/PrivateHeaders/Ably/ARTContinuousClock.h
- Source/ARTContinuousClock.m
- Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (18)
- Source/ARTWebSocketTransport.m
- Source/PrivateHeaders/Ably/ARTSystemTimeProvider.h
- Source/ARTRealtimeAnnotations.m
- Source/ARTLog.m
- Source/ARTRealtimeChannel.m
- Source/include/module.modulemap
- Source/Ably.modulemap
- Source/ARTJsonLikeEncoder.m
- Source/ARTRealtimePresence.m
- Source/ARTRest.m
- Test/AblyTests/Tests/UtilitiesTests.swift
- Source/ARTTestClientOptions.m
- Source/PrivateHeaders/Ably/ARTRest+Private.h
- Test/AblyTests/Tests/SystemTimeProviderTests.swift
- Source/ARTAuth.m
- Source/PrivateHeaders/Ably/ARTEventEmitter+Private.h
- Source/ARTEventEmitter.m
- Source/ARTSystemTimeProvider.m
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Test/AblyTests/Tests/AuthTests.swift (1)
56-57: ⚡ Quick winRemove the redundant delegate reassignment.
The delegate is already provided in the initializer, so setting it again immediately adds noise and creates an unnecessary second instance.
♻️ Proposed cleanup
private func createJsonEncoder() -> ARTJsonLikeEncoder { let encoder = ARTJsonLikeEncoder(delegate: ARTJsonEncoder(), timeProvider: SystemTimeProvider()) - encoder.delegate = ARTJsonEncoder() return encoder }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Test/AblyTests/Tests/AuthTests.swift` around lines 56 - 57, The ARTJsonLikeEncoder initialization in the test setup is redundantly reassigning the delegate immediately after initialization. The delegate is already provided as a parameter to the ARTJsonLikeEncoder initializer constructor, so the subsequent line that reassigns encoder.delegate to a new ARTJsonEncoder() instance is unnecessary and creates duplicate instances. Remove the second delegate assignment statement to clean up the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Test/AblyTests/Tests/AuthTests.swift`:
- Around line 56-57: The ARTJsonLikeEncoder initialization in the test setup is
redundantly reassigning the delegate immediately after initialization. The
delegate is already provided as a parameter to the ARTJsonLikeEncoder
initializer constructor, so the subsequent line that reassigns encoder.delegate
to a new ARTJsonEncoder() instance is unnecessary and creates duplicate
instances. Remove the second delegate assignment statement to clean up the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c91e67ae-333b-4ea5-b08d-1642a2da11f7
📒 Files selected for processing (35)
Ably.xcodeproj/project.pbxprojCONTRIBUTING.mdSource/ARTAuth.mSource/ARTContinuousClock.mSource/ARTEventEmitter.mSource/ARTGCD.mSource/ARTJsonLikeEncoder.mSource/ARTLog.mSource/ARTMessage.mSource/ARTRealtime.mSource/ARTRealtimeAnnotations.mSource/ARTRealtimeChannel.mSource/ARTRealtimePresence.mSource/ARTRest.mSource/ARTSystemTimeProvider.mSource/ARTTestClientOptions.mSource/ARTWebSocketTransport.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTContinuousClock.hSource/PrivateHeaders/Ably/ARTContinuousClockInstant.hSource/PrivateHeaders/Ably/ARTEventEmitter+Private.hSource/PrivateHeaders/Ably/ARTGCD.hSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTRest+Private.hSource/PrivateHeaders/Ably/ARTSchedulerHandle.hSource/PrivateHeaders/Ably/ARTSystemTimeProvider.hSource/PrivateHeaders/Ably/ARTTestClientOptions.hSource/PrivateHeaders/Ably/ARTTimeProvider.hSource/include/module.modulemapTest/AblyTests/Tests/AuthTests.swiftTest/AblyTests/Tests/ContinuousClockTests.swiftTest/AblyTests/Tests/GCDTests.swiftTest/AblyTests/Tests/StatsTests.swiftTest/AblyTests/Tests/SystemTimeProviderTests.swiftTest/AblyTests/Tests/UtilitiesTests.swift
💤 Files with no reviewable changes (6)
- Source/PrivateHeaders/Ably/ARTGCD.h
- Test/AblyTests/Tests/ContinuousClockTests.swift
- Test/AblyTests/Tests/GCDTests.swift
- Source/PrivateHeaders/Ably/ARTContinuousClock.h
- Source/ARTContinuousClock.m
- Source/ARTGCD.m
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (20)
- Source/ARTWebSocketTransport.m
- Source/Ably.modulemap
- Source/ARTLog.m
- Source/ARTTestClientOptions.m
- Source/ARTRealtimeAnnotations.m
- Test/AblyTests/Tests/UtilitiesTests.swift
- Source/PrivateHeaders/Ably/ARTEventEmitter+Private.h
- Source/include/module.modulemap
- Source/ARTRealtimePresence.m
- Source/ARTRealtimeChannel.m
- Source/PrivateHeaders/Ably/ARTSystemTimeProvider.h
- Source/ARTAuth.m
- Source/PrivateHeaders/Ably/ARTRest+Private.h
- Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h
- Source/ARTSystemTimeProvider.m
- Source/ARTRest.m
- Source/ARTEventEmitter.m
- Test/AblyTests/Tests/SystemTimeProviderTests.swift
- Source/ARTRealtime.m
- Ably.xcodeproj/project.pbxproj
Introduce ARTTimeProvider: a single injectable abstraction over the wall clock (-wallClockNow), a continuous clock that keeps advancing while the system is asleep (-continuousClockNow), and delayed execution (-scheduleAfter:queue:block:). ARTSystemTimeProvider is the default; an alternative can be injected via ARTClientOptions.testOptions to drive a deterministic clock in tests. All internal time-dependent code is routed through it instead of calling [NSDate date], clock_gettime_nsec_np, or dispatch_after directly. Motivation here is AIT-846 (the upcoming UTS will require the ability to mock time). Note that we have not introduced the ARTTimeProvider abstraction into the SocketRocket code since we're operating under the assumption that UTS tests that mock time will also mock the WebSocket layer; if this turns out to be incorrect then we may need to revisit. These abstractions are not yet exposed to plugins; that will be addressed as part of AIT-781. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Lawrence Forooghian <lawrence@forooghian.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First of two PRs split out from the original combined PR. This one is the SDK-side time abstraction only: a unified
ARTTimeProvider(wall clock, continuous clock, scheduler) that all time-dependent internal code routes through, so tests can install a deterministic clock viaClientOptions.testOptions.timeProvider. Default behaviour is unchanged (ARTSystemTimeProvider).These abstractions aren't yet exposed to plugins — that's handled later in AIT-781.
Stacked PR
The Universal Test Suite (UTS) groundwork that builds on this abstraction is in #2213 (based on this branch). Review/merge this PR first.
Review
Addresses the comments from review 4483891555; per-comment commit links are posted as replies below.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Tests
Documentation