[AIT-767] uts: live objects ground work#1209
Conversation
- Added `MockHttpClient`, `MockHttpEngine`, and `MockWebSocketEngineFactory` to simulate network interactions. - Extended `DebugOptions` for customizable engine injection. - Updated `HttpCore` and `WebSocketTransport` to support mock engines in debug mode.
WalkthroughThe PR adds a Clock/timer abstraction and SystemClock, replaces direct system-time and java.util.Timer usage across runtime components (HTTP, transport, realtime, hosts, auth, connection), adds test-time FakeClock and comprehensive HTTP/WebSocket mocks plus test client factories and await helpers, and supplies UTS→Kotlin guidance and example recovery tests with Gradle/CI wiring for the new uts module. ChangesClock abstraction and time-source injection throughout SDK and tests
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 docstrings
🧪 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 |
…ge handling - Added `MockWebSocket` and `MockEvent` to capture WebSocket connection attempts, messages, and error scenarios. - Added connection lifecycle events (`ConnectionEstablished`, `ConnectionRefused`, etc.) for enhanced testing. - Updated `MockWebSocketEngineFactory` and related components to support event tracking and simulation.
- Added `Clock` interface and concrete implementations (`SystemClock` and `FakeClock`) for unified time management. - Refactored classes (`Auth`, `Presence`, `Hosts`, `WebSocketTransport`, etc.) to use `Clock` instead of direct system calls. - Enabled mockable time-based operations for improved testability. - Updated `DebugOptions` to support custom clocks in debug mode.
…in tests - Introduced a new skill for converting UTS pseudocode specs into runnable Kotlin tests. - Included detailed translation rules for pseudocode to Kotlin, mock setup, and assertions. - Added file templates and steps for compilation, testing, and handling deviations. - Enhanced developer workflow for UTS test authoring.
…ssage decoding - Added support for inspecting outgoing WebSocket frames (`MessageFromClient` events) in `MockWebSocket`. - Enhanced `ClientOptionsBuilder` to set `useBinaryProtocol = false` by default for JSON text frame testing. - Updated `ConnectionRecoveryTest` to assert on outgoing ATTACH messages.
…onnections - Added `queryParams` field and `parseQueryString` utility for `PendingConnection` implementations. - Updated `MockWebSocketEngineFactory` and `MockHttpEngine` to parse and pass query parameters. - Enhanced connection recovery tests with assertions on query parameters.
ttypic
force-pushed
the
AIT-767/live-object-ground-work
branch
from
ec7e6c4 to
c8f95df
Compare
- Added KDoc comments to `FakeClock`, `MockHttpClient`, and `MockWebSocket` explaining their purpose and usage.
ttypic
force-pushed
the
AIT-767/live-object-ground-work
branch
from
c8f95df to
c9d79f2
Compare
| You are translating a UTS pseudocode test spec file into a runnable Kotlin test in the `uts` module. Follow these steps in order. | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
Should there be a reference to https://github.com/ably/specification/blob/main/uts/docs/writing-derived-tests.md ?
| ## Step 1 — Read the spec | ||
|
|
||
| Read the file at `$ARGUMENTS`. Identify: | ||
| - All test cases (each has an ID like `RTN4a`, `RSC1`, etc. and a description) |
There was a problem hiding this comment.
There's a structured ID for each test, eg realtime/unit/RSA4c2/callback-error-connecting-disconnected-0 (eg https://github.com/ably/specification/blob/main/uts/realtime/unit/auth/auth_callback_errors_test.md#rsa4c2---authcallback-error-during-connecting-transitions-to-disconnected)
There was a problem hiding this comment.
Yeah it's weird, because later in the doc it uses structured ID, updated
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
liveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.kt (1)
16-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope server-time offset per adapter/clock instead of globally.
ServerTimekeeps a single globalserverTimeOffset, but Line 25/Line 35 now use an adapter-derived clock. If different adapters (or tests using different clocks) call this, the cached offset from one clock can be reused with another, yielding incorrect server times.Also applies to: 24-35
🤖 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 `@liveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.kt` around lines 16 - 17, ServerTime currently caches a single global serverTimeOffset (private var serverTimeOffset) which must be scoped per adapter/clock; change the cache to map offsets keyed by the adapter/clock identity (e.g., Map<Clock, Long?> or Map<AdapterId, Long?>) and update all uses in ServerTime methods (the code paths around where serverTimeOffset is read/updated — lines ~24-35) to look up and set the offset using the current adapter/clock key instead of the global variable so each clock has its own cached offset.
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/rest/Auth.java (1)
926-927: ⚡ Quick winKeep nanoTime drift detection tied to system wall clock.
The
nanoTimeDeltacheck is meant to detect local wall-clock jumps. Basing it on injectedclock.currentTimeMillis()can trigger false drift when custom/fake clocks are used, causing avoidableably.time()refreshes.Suggested fix
- long currentNanoTimeDelta = clock.currentTimeMillis() - System.nanoTime()/(1000*1000); + long currentNanoTimeDelta = System.currentTimeMillis() - System.nanoTime()/(1000*1000); ... - this.nanoTimeDelta = clock.currentTimeMillis() - System.nanoTime()/(1000*1000); + this.nanoTimeDelta = System.currentTimeMillis() - System.nanoTime()/(1000*1000);Also applies to: 1056-1057
🤖 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 `@lib/src/main/java/io/ably/lib/rest/Auth.java` around lines 926 - 927, The nanoTime drift detection wrongly uses the injected clock (clock.currentTimeMillis()) which allows fake/custom clocks to mask or create false wall-clock jumps; change the computation of currentNanoTimeDelta to use the JVM wall-clock directly (System.currentTimeMillis()) together with System.nanoTime()/ (1000*1000) so the drift check is tied to the real system clock; update both occurrences (the currentNanoTimeDelta calculation around currentTimeMillis/System.nanoTime and the similar logic at the later occurrence around lines 1056-1057) to reference System.currentTimeMillis() instead of clock.currentTimeMillis() and keep the rest of the logic unchanged.uts/build.gradle.kts (1)
24-24: ⚡ Quick winScope forced test reruns instead of disabling up-to-date checks globally.
Line 24 forces every test task to rerun on every build, which can noticeably slow local iteration and CI. Gate this behind a property (or remove it) so it’s opt-in.
Proposed diff
tasks.withType<Test>().configureEach { @@ - outputs.upToDateWhen { false } + if (project.hasProperty("forceTestRerun")) { + outputs.upToDateWhen { false } + } }🤖 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 `@uts/build.gradle.kts` at line 24, The build currently forces all tests to rerun by unconditionally setting outputs.upToDateWhen { false }; change this to be opt-in by gating it behind a project property (e.g., "forceTestRerun"). Locate the block that sets outputs.upToDateWhen { false } (the test task configuration) and wrap it so it only executes when project.findProperty("forceTestRerun") == "true" (or similar), or remove the line altogether; ensure to document the property in the repo README or CI docs so callers can opt in when needed.
🤖 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 @.claude/skills/uts-to-kotlin/SKILL.md:
- Around line 42-50: Two fenced code blocks in SKILL.md (the one listing
test/mock files and the ASCII "Test fails" diagram) lack a language tag; add
"text" to both opening triple-backtick fences so they read ```text to satisfy
MD040. Locate the fenced block containing the list of test/mock files and the
fenced block containing the ASCII decision diagram and update their opening
fences to include the language tag without changing the block contents.
In `@lib/src/main/java/io/ably/lib/http/HttpScheduler.java`:
- Around line 291-297: The timeout loop in HttpScheduler.get(...) can hang when
using an injected clock that doesn't advance with real time because
wait(remaining) blocks by wall-clock while remaining is recomputed from
clock.currentTimeMillis(); change the wait to use a bounded sleep interval so
the loop re-checks the injected clock frequently. Specifically, in
HttpScheduler.get use wait(Math.min(remaining, <smallMillis>)) (e.g. 100ms)
instead of wait(remaining), keep recalculating remaining = deadline -
clock.currentTimeMillis(), and preserve the existing isDone check and
spurious-wakeup handling so the loop exits correctly when done or timed out.
In `@lib/src/main/java/io/ably/lib/util/SystemClock.java`:
- Around line 19-31: Replace per-call java.util.Timer creation in
SystemClock.newTimer() with a shared ScheduledExecutorService: add a private
static final ScheduledExecutorService (created with
Executors.newScheduledThreadPool(1, daemon ThreadFactory) or a small pool) and
in the NamedTimer implementation use scheduler.schedule(...) to schedule the
TimerTask (wrap TimerTask in a Runnable that calls run and catches/logs
throwables), return a TimerInstance that cancels the underlying ScheduledFuture,
and implement cancel() to cancel outstanding futures as appropriate; update
references to NamedTimer.schedule, TimerInstance and cancel to use
ScheduledFuture cancellation semantics to avoid one-thread-per-timer and to
handle exceptions safely.
In
`@uts/src/test/kotlin/io/ably/lib/realtime/unit/connection/ConnectionRecoveryTest.kt`:
- Around line 304-306: The test currently only asserts msgSerial when
RUN_DEVIATIONS is set; add an else branch to assert the default-path behavior by
checking currentRecoveryKey.msgSerial equals the SDK's expected default value.
In the else branch (i.e., when System.getenv("RUN_DEVIATIONS") == null) add
assertEquals(<SDK_DEFAULT_MSG_SERIAL>, currentRecoveryKey.msgSerial), using the
explicit SDK constant or literal used elsewhere in the codebase (or the test's
initial recorded value) so the non-deviation path cannot vacuously pass;
reference currentRecoveryKey.msgSerial and the RUN_DEVIATIONS env check to
locate where to add this assertion.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt`:
- Around line 20-31: The respondWith method must preserve the provided headers
and serialize non-binary bodies as JSON instead of calling toString(); update
DefaultPendingRequest.respondWith to pass the supplied headers into
HttpResponse.builder().headers(...) and, when body is not a ByteArray, serialize
it to UTF-8 JSON bytes (e.g., via your project's JSON serializer) and set the
content-type to "application/json" on HttpBody; ensure ByteArray bodies still
bypass serialization and preserve their original bytes and any provided headers.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt`:
- Around line 29-31: The advance() method mutates the timers map while iterating
(timers.values.forEach { ... }), which can throw ConcurrentModificationException
if a callback creates a new timer; change it to iterate over a snapshot of the
timer collection (e.g. make a list copy of timers.values) and call fireDue(time)
on that snapshot so callbacks can safely mutate the original timers map; update
the advance() implementation (and any similar iteration over timers) to use the
snapshot approach to avoid concurrent modification during Timer.fireDue calls.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/MockHttpClient.kt`:
- Around line 35-39: The trySend() calls in the onConnection/onRequest lambdas
can fail silently when channels are recreated in reset(), causing
awaitConnectionAttempt()/awaitRequest() to timeout; update the
_pendingConnections.trySend(conn) and _pendingRequests.trySend(pending)
invocations so their Result is checked by calling getOrThrow() (e.g.,
_pendingConnections.trySend(conn).getOrThrow()) to surface failures instead of
dropping events; keep existing handler fallback logic and ensure any thrown
exceptions propagate so tests fail fast.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocket.kt`:
- Around line 46-49: The events getter returns a snapshot from
Collections.synchronizedList without external synchronization, which can race
with concurrent writers; modify the events property (and any reset() that
iterates) to synchronize on _events when creating the snapshot (i.e. wrap the
_events.toList() call in synchronized(_events) { ... }) so iteration/read is
protected; reference the private val _events and the val events: List<MockEvent>
get() = ... and ensure any other callers that iterate _events do the same.
In `@uts/src/test/kotlin/io/ably/lib/Utils.kt`:
- Around line 29-35: The ConnectionStateListener added with
client.connection.on(listener) is only removed on cancellation; update the
success path to also remove it: when you call cont.resume(Unit) (both in the
listener callback and the immediate state-check), call
client.connection.off(listener) either before or immediately after resuming to
unregister the listener and avoid buildup. Modify the two occurrences (the
listener block where you check change.current == target and the immediate check
where you compare client.connection.state == target) to unregister the same
listener (listener) on successful resume, and keep the existing
cont.invokeOnCancellation { client.connection.off(listener) } to cover
cancellation.
---
Outside diff comments:
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.kt`:
- Around line 16-17: ServerTime currently caches a single global
serverTimeOffset (private var serverTimeOffset) which must be scoped per
adapter/clock; change the cache to map offsets keyed by the adapter/clock
identity (e.g., Map<Clock, Long?> or Map<AdapterId, Long?>) and update all uses
in ServerTime methods (the code paths around where serverTimeOffset is
read/updated — lines ~24-35) to look up and set the offset using the current
adapter/clock key instead of the global variable so each clock has its own
cached offset.
---
Nitpick comments:
In `@lib/src/main/java/io/ably/lib/rest/Auth.java`:
- Around line 926-927: The nanoTime drift detection wrongly uses the injected
clock (clock.currentTimeMillis()) which allows fake/custom clocks to mask or
create false wall-clock jumps; change the computation of currentNanoTimeDelta to
use the JVM wall-clock directly (System.currentTimeMillis()) together with
System.nanoTime()/ (1000*1000) so the drift check is tied to the real system
clock; update both occurrences (the currentNanoTimeDelta calculation around
currentTimeMillis/System.nanoTime and the similar logic at the later occurrence
around lines 1056-1057) to reference System.currentTimeMillis() instead of
clock.currentTimeMillis() and keep the rest of the logic unchanged.
In `@uts/build.gradle.kts`:
- Line 24: The build currently forces all tests to rerun by unconditionally
setting outputs.upToDateWhen { false }; change this to be opt-in by gating it
behind a project property (e.g., "forceTestRerun"). Locate the block that sets
outputs.upToDateWhen { false } (the test task configuration) and wrap it so it
only executes when project.findProperty("forceTestRerun") == "true" (or
similar), or remove the line altogether; ensure to document the property in the
repo README or CI docs so callers can opt in when needed.
🪄 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: 15dbe772-ae6e-4dca-810a-36af6ff01876
📒 Files selected for processing (37)
.claude/skills/uts-to-kotlin/SKILL.md.github/workflows/check.ymllib/src/main/java/io/ably/lib/debug/DebugOptions.javalib/src/main/java/io/ably/lib/http/HttpCore.javalib/src/main/java/io/ably/lib/http/HttpScheduler.javalib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/rest/Auth.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/transport/Hosts.javalib/src/main/java/io/ably/lib/transport/WebSocketTransport.javalib/src/main/java/io/ably/lib/util/Clock.javalib/src/main/java/io/ably/lib/util/NamedTimer.javalib/src/main/java/io/ably/lib/util/SystemClock.javalib/src/main/java/io/ably/lib/util/TimerInstance.javaliveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.ktsettings.gradle.ktsuts/build.gradle.ktsuts/src/test/kotlin/io/ably/lib/ClientFactories.ktuts/src/test/kotlin/io/ably/lib/Utils.ktuts/src/test/kotlin/io/ably/lib/realtime/unit/connection/ConnectionRecoveryTest.ktuts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingConnection.ktuts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.ktuts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockEvent.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockHttpClient.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockHttpEngine.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocket.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocketEngineFactory.ktuts/src/test/kotlin/io/ably/lib/test/mock/PendingConnection.ktuts/src/test/kotlin/io/ably/lib/test/mock/PendingRequest.ktuts/src/test/kotlin/io/ably/lib/types/Utils.kt
There was a problem hiding this comment.
Pull request overview
This PR lays groundwork for UTS-derived Kotlin tests and deterministic transport/time control by adding a Clock abstraction, mock HTTP/WebSocket transports, and a new uts Gradle module; it also updates parts of the SDK (core + liveobjects) to use injected time/engines via DebugOptions.
Changes:
- Introduces
Clock/NamedTimer/TimerInstance+SystemClock, and wires clock usage into transports, auth, scheduling, and liveobjects tombstoning/GC paths. - Adds mock WebSocket/HTTP implementations plus test helpers, and a new
utsmodule with initial UTS-derived connection recovery tests. - Adds CI wiring to run
:uts:testand includes a Claude skill doc for translating UTS specs to Kotlin tests.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| uts/src/test/kotlin/io/ably/lib/Utils.kt | Adds coroutine helpers to await connection/channel states in tests. |
| uts/src/test/kotlin/io/ably/lib/types/Utils.kt | Adds a Kotlin DSL-style initializer for ConnectionDetails in tests. |
| uts/src/test/kotlin/io/ably/lib/test/mock/PendingRequest.kt | Defines HTTP pending request interface for mock engine. |
| uts/src/test/kotlin/io/ably/lib/test/mock/PendingConnection.kt | Defines connection-attempt interface and query parsing helper. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocketEngineFactory.kt | Adds a mock WebSocketEngineFactory/engine/client for tests. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocket.kt | Implements a higher-level mock WebSocket with callbacks/await APIs and event log. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockHttpEngine.kt | Implements a mock HttpEngine/HttpCall that tests must resolve. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockHttpClient.kt | Wraps mock HTTP engine with callback/await APIs and install helper. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockEvent.kt | Adds an event model for mock transport event recording. |
| uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt | Adds a virtual clock for deterministic timer advancement in tests. |
| uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt | Implements PendingRequest response helpers for mock HTTP. |
| uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingConnection.kt | Implements PendingConnection responses for mock WebSocket. |
| uts/src/test/kotlin/io/ably/lib/realtime/unit/connection/ConnectionRecoveryTest.kt | Adds initial UTS-derived tests for recovery key/recover behavior. |
| uts/src/test/kotlin/io/ably/lib/ClientFactories.kt | Adds TestRealtimeClient/TestRestClient builders and mock/fake-timer installation hooks. |
| uts/build.gradle.kts | Introduces the new uts test module configuration. |
| settings.gradle.kts | Includes the uts module in the build. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt | Uses injected clock for tombstone timestamps when server timestamp missing. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt | Makes GC eligibility time source injectable via Clock. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt | Propagates clock into base object and uses it for GC decisions. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt | Propagates clock into base object. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt | Adds Clock to base object lifecycle/GC timestamp computations. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.kt | Uses injected clock to compute server time offset and derived server time. |
| lib/src/main/java/io/ably/lib/util/TimerInstance.java | Adds timer cancellation handle abstraction. |
| lib/src/main/java/io/ably/lib/util/SystemClock.java | Implements Clock using system time and java.util.Timer; provides clockFrom helper. |
| lib/src/main/java/io/ably/lib/util/NamedTimer.java | Adds named timer abstraction returning TimerInstance. |
| lib/src/main/java/io/ably/lib/util/Clock.java | Adds core clock interface used across SDK for time + timer creation. |
| lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java | Allows injecting WebSocket engine factory and uses Clock + NamedTimer for activity timers. |
| lib/src/main/java/io/ably/lib/transport/Hosts.java | Uses injected Clock for preferred-host expiry calculations. |
| lib/src/main/java/io/ably/lib/transport/ConnectionManager.java | Uses injected Clock for stale/suspend calculations. |
| lib/src/main/java/io/ably/lib/rest/Auth.java | Uses injected Clock for timestamping and time-change detection. |
| lib/src/main/java/io/ably/lib/realtime/Presence.java | Uses channel clock for timestamps generated by presence sync logic. |
| lib/src/main/java/io/ably/lib/realtime/ChannelBase.java | Replaces Timer with NamedTimer and uses injected clock to create timers. |
| lib/src/main/java/io/ably/lib/http/HttpScheduler.java | Uses injected clock for timeout deadline calculations in Future.get(timeout). |
| lib/src/main/java/io/ably/lib/http/HttpCore.java | Allows injecting an HttpEngine via DebugOptions. |
| lib/src/main/java/io/ably/lib/debug/DebugOptions.java | Adds debug-time injection points: HttpEngine, WebSocketEngineFactory, and Clock. |
| .github/workflows/check.yml | Updates CI to run :uts:test. |
| .claude/skills/uts-to-kotlin/SKILL.md | Adds guidance/skill doc for translating UTS specs to Kotlin tests. |
Comments suppressed due to low confidence (1)
uts/src/test/kotlin/io/ably/lib/Utils.kt:54
awaitChannelStateregisters aChannelStateListenerbut does not unregister it when the target state is reached (only on cancellation). This can leak listeners across awaits and potentially affect subsequent tests. Ensure the listener is removed once the coroutine resumes successfully.
suspendCancellableCoroutine { cont ->
val listener = ChannelStateListener { change ->
if (change.current == target && cont.isActive) cont.resume(Unit)
}
channel.on(listener)
if (channel.state == target && cont.isActive) cont.resume(Unit)
cont.invokeOnCancellation { channel.off(listener) }
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .code(status) | ||
| .message("") | ||
| .body(HttpBody("application/json", bytes)) | ||
| .headers(emptyMap()) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
lib/src/main/java/io/ably/lib/util/SystemClock.java (1)
18-31: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftEach
newTimer()call creates a dedicated daemon thread.Every
newTimer(name)invocation creates a newjava.util.Timerwith its own background thread. With multiple channels and transports, this scales thread count linearly and reduces reliability—if any timer task throws an unhandled exception, theTimer's thread terminates and subsequent schedules fail silently.Modern Java best practices recommend
ScheduledExecutorServiceoverTimerfor better resource management, exception handling, and scalability.🤖 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 `@lib/src/main/java/io/ably/lib/util/SystemClock.java` around lines 18 - 31, The newTimer() implementation creates a dedicated java.util.Timer per call which spawns a thread per timer; replace this with a shared ScheduledExecutorService and schedule tasks as ScheduledFutures to avoid one-thread-per-timer and silent failures. Update newTimer() to use a singleton ScheduledExecutorService (created with a ThreadFactory that sets threads as daemon) and in the returned NamedTimer.schedule(...) submit the TimerTask as a Runnable that wraps task.run() in try/catch to log/handle exceptions, returning a TimerInstance that cancels the underlying ScheduledFuture (future.cancel(false)). Also update NamedTimer.cancel() to no-op or cancel any outstanding futures, and ensure the executor is properly created once (e.g., static final) and not shutdown on each timer creation; use the existing symbols newTimer, NamedTimer, TimerInstance, schedule and cancel to locate and change the code.uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt (1)
20-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve response headers and serialize non-binary bodies as JSON.
respondWith(...)currently discards theheadersparameter (line 30) and usestoString()for non-ByteArray bodies (line 23), which produces invalid JSON and breaks consumers expecting proper JSON serialization.Proposed fix
+import io.ably.lib.util.Serialisation override fun respondWith(status: Int, body: Any, headers: Map<String, String>) { val bytes = when (body) { is ByteArray -> body - else -> body.toString().toByteArray(Charsets.UTF_8) + else -> Serialisation.gson.toJson(body).toByteArray(Charsets.UTF_8) } deferred.complete( HttpResponse.builder() .code(status) .message("") .body(HttpBody("application/json", bytes)) - .headers(emptyMap()) + .headers(headers.mapValues { (_, value) -> listOf(value) }) .build() ) }🤖 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 `@uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt` around lines 20 - 33, In DefaultPendingRequest.respondWith, preserve the passed headers instead of using emptyMap() and serialize non-ByteArray bodies to JSON (not via toString()) before converting to bytes; update the HttpResponse builder to use the provided headers parameter and ensure HttpBody is constructed with UTF-8 bytes of JSON-serialized body for non-binary inputs, completing the deferred with that response.lib/src/main/java/io/ably/lib/http/HttpScheduler.java (1)
291-296:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftTimeout loop can still hang with non-advancing injected clocks.
The timeout calculation uses
clock.currentTimeMillis()to computedeadlineandremaining, but whenFakeClock(or any non-wall-clock implementation) is injected,clock.waitOn(this, remaining)may complete immediately (without advancing virtual time), causingremainingto never decrease. This can makeget(timeout, unit)hang indefinitely.Proposed fix
Use
System.nanoTime()for the timeout deadline to ensure it advances with real wall-clock time, regardless of the injected clock:public T get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { - long remaining = unit.toMillis(timeout), deadline = clock.currentTimeMillis() + remaining; + final long deadlineNanos = System.nanoTime() + unit.toNanos(timeout); synchronized(this) { - while(remaining > 0) { - clock.waitOn(this, remaining); - if(isDone) { break; } - remaining = deadline - clock.currentTimeMillis(); + while(!isDone) { + long remainingNanos = deadlineNanos - System.nanoTime(); + if (remainingNanos <= 0) { + break; + } + long remainingMillis = TimeUnit.NANOSECONDS.toMillis(remainingNanos); + clock.waitOn(this, Math.max(1, remainingMillis)); } if(!isDone) { throw new TimeoutException(); }🤖 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 `@lib/src/main/java/io/ably/lib/http/HttpScheduler.java` around lines 291 - 296, Replace the wall-clock-dependent timeout math in HttpScheduler.get(timeout, unit): instead of using clock.currentTimeMillis() to compute deadline and remaining, compute a nano-based deadline with System.nanoTime() + unit.toNanos(timeout) and in the loop compute remainingNanos = deadline - System.nanoTime(); if remainingNanos <= 0 then break; convert remainingNanos to a millisecond value (e.g. Math.max(0, remainingNanos/1_000_000) or at least 1ms if you need non-zero waits) when calling clock.waitOn(this, remainingMillis). This ensures clock.waitOn/clock.currentTimeMillis (and injected FakeClock) cannot cause get(timeout, unit) to hang by preventing virtual-time from affecting the deadline calculation.uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt (1)
37-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent map mutation during timer firing.
Line 39 iterates
timers.valueswhile executingfireDue(time)callbacks. If a callback creates a new timer (viaclock.newTimer(...)), it mutatestimersand throwsConcurrentModificationException.Proposed fix
fun advance(ms: Long) { time += ms - timers.values.forEach { it.fireDue(time) } + timers.values.toList().forEach { it.fireDue(time) } val due = synchronized(waiters) {🤖 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 `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt` around lines 37 - 51, FakeClock.advance currently iterates timers.values while calling each timer.fireDue(time), which allows callbacks (e.g., clock.newTimer) to mutate the timers map and cause ConcurrentModificationException; fix this by iterating over a stable snapshot of the timers collection (e.g., copy timers.values to a list) before calling fireDue so mutations during callbacks don't affect the iterator; update the loop in FakeClock.advance (the timers.values.forEach -> snapshot.forEach) while keeping the subsequent waiters handling and notifyAll logic unchanged.
🤖 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 `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt`:
- Around line 28-34: The FakeClock.waitOn implementation currently calls (target
as Object).wait() without ensuring the caller holds target's monitor; update the
contract and implementation: add KDoc to Clock.waitOn stating callers MUST hold
the target's monitor (mention callers like HttpScheduler.get / AsyncRequest) and
enforce it in FakeClock.waitOn by checking Thread.holdsLock(target) and throwing
IllegalMonitorStateException with a clear message if not, then keep the existing
wait logic (synchronized(waiters) { waiters.add(Waiter(...)) } followed by
target.wait()) so the precondition is explicit and violations fail fast.
---
Duplicate comments:
In `@lib/src/main/java/io/ably/lib/http/HttpScheduler.java`:
- Around line 291-296: Replace the wall-clock-dependent timeout math in
HttpScheduler.get(timeout, unit): instead of using clock.currentTimeMillis() to
compute deadline and remaining, compute a nano-based deadline with
System.nanoTime() + unit.toNanos(timeout) and in the loop compute remainingNanos
= deadline - System.nanoTime(); if remainingNanos <= 0 then break; convert
remainingNanos to a millisecond value (e.g. Math.max(0,
remainingNanos/1_000_000) or at least 1ms if you need non-zero waits) when
calling clock.waitOn(this, remainingMillis). This ensures
clock.waitOn/clock.currentTimeMillis (and injected FakeClock) cannot cause
get(timeout, unit) to hang by preventing virtual-time from affecting the
deadline calculation.
In `@lib/src/main/java/io/ably/lib/util/SystemClock.java`:
- Around line 18-31: The newTimer() implementation creates a dedicated
java.util.Timer per call which spawns a thread per timer; replace this with a
shared ScheduledExecutorService and schedule tasks as ScheduledFutures to avoid
one-thread-per-timer and silent failures. Update newTimer() to use a singleton
ScheduledExecutorService (created with a ThreadFactory that sets threads as
daemon) and in the returned NamedTimer.schedule(...) submit the TimerTask as a
Runnable that wraps task.run() in try/catch to log/handle exceptions, returning
a TimerInstance that cancels the underlying ScheduledFuture
(future.cancel(false)). Also update NamedTimer.cancel() to no-op or cancel any
outstanding futures, and ensure the executor is properly created once (e.g.,
static final) and not shutdown on each timer creation; use the existing symbols
newTimer, NamedTimer, TimerInstance, schedule and cancel to locate and change
the code.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt`:
- Around line 20-33: In DefaultPendingRequest.respondWith, preserve the passed
headers instead of using emptyMap() and serialize non-ByteArray bodies to JSON
(not via toString()) before converting to bytes; update the HttpResponse builder
to use the provided headers parameter and ensure HttpBody is constructed with
UTF-8 bytes of JSON-serialized body for non-binary inputs, completing the
deferred with that response.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt`:
- Around line 37-51: FakeClock.advance currently iterates timers.values while
calling each timer.fireDue(time), which allows callbacks (e.g., clock.newTimer)
to mutate the timers map and cause ConcurrentModificationException; fix this by
iterating over a stable snapshot of the timers collection (e.g., copy
timers.values to a list) before calling fireDue so mutations during callbacks
don't affect the iterator; update the loop in FakeClock.advance (the
timers.values.forEach -> snapshot.forEach) while keeping the subsequent waiters
handling and notifyAll logic unchanged.
🪄 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: 282b2ebb-747b-4b16-aab0-dee45f605dee
📒 Files selected for processing (7)
lib/src/main/java/io/ably/lib/http/HttpScheduler.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/util/Clock.javalib/src/main/java/io/ably/lib/util/SystemClock.javauts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.ktuts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.ktuts/src/test/kotlin/io/ably/lib/test/mock/PendingRequest.kt
| @Suppress("PLATFORM_CLASS_MAPPED_TO_KOTLIN") | ||
| override fun waitOn(target: Any, timeout: Long) { | ||
| synchronized(waiters) { | ||
| waiters.add(Waiter(target as Object, time + timeout)) | ||
| } | ||
| (target as Object).wait() | ||
| } |
There was a problem hiding this comment.
waitOn violates Java monitor contract.
Line 33 calls (target as Object).wait() without holding the target's monitor. Java's Object.wait() requires the calling thread to own the object's monitor, or it throws IllegalMonitorStateException.
The caller (e.g., HttpScheduler.get()) holds the monitor of this (the AsyncRequest), not the monitor of target. Since FakeClock.waitOn is called with target = this (the AsyncRequest), the AsyncRequest's monitor is held by the caller's synchronized(this) block, so line 33 should succeed. However, the design is fragile because waitOn delegates to target.wait() assuming the caller already holds target's monitor, which is not enforced by the Clock interface contract.
Clarify the contract or add documentation
Add a precondition check or KDoc to Clock.waitOn stating that the caller MUST hold target's monitor before calling:
override fun waitOn(target: Any, timeout: Long) {
+ // Caller MUST hold target's monitor before calling this method
synchronized(waiters) {
waiters.add(Waiter(target as Object, time + timeout))
}
(target as Object).wait()
}Or enforce it with a check (though this adds overhead):
if (!Thread.holdsLock(target)) {
throw IllegalMonitorStateException("Caller must hold target's monitor")
}🤖 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 `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt` around lines 28 - 34,
The FakeClock.waitOn implementation currently calls (target as Object).wait()
without ensuring the caller holds target's monitor; update the contract and
implementation: add KDoc to Clock.waitOn stating callers MUST hold the target's
monitor (mention callers like HttpScheduler.get / AsyncRequest) and enforce it
in FakeClock.waitOn by checking Thread.holdsLock(target) and throwing
IllegalMonitorStateException with a clear message if not, then keep the existing
wait logic (synchronized(waiters) { waiters.add(Waiter(...)) } followed by
target.wait()) so the precondition is explicit and violations fail fast.
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Lead-dev review of the UTS groundwork (cross-checked against specification/uts and the SDK changes in this PR). The overall direction — Clock/NamedTimer SPI, DebugOptions injection points, mock WS+HTTP engines, fake clock, and a uts-to-kotlin skill — is sound, and the RTN16 translation faithfully matches the spec sections that exist. Comments below are correctness or robustness issues; a couple are blockers.
Strategic findings that don't belong on a single line:
deviations.mdis referenced but missing.ConnectionRecoveryTest.kt:301and the SKILL both point atuts/src/test/kotlin/io/ably/lib/deviations.md. The file doesn't exist. RTN16f is a real deviation that should be recorded there.- Spec coverage gap is upstream, not here. The spec header at
connection_recovery_test.mdlistsRTN16d/RTN16i/RTN16l, but onlyRTN16iis actually encoded (inline withinRTN16j);RTN16dandRTN16lhave no dedicated sections in the spec itself. The translation here is faithful — the gap should be filed againstably/specification. - No
@AfterEachcleanup.RTN16g2creates three clients and only closes the last one on the success path; if any earlier assertion throws, all three leak their internal Timer threads into subsequent tests. Recommend a tracked-client@AfterEach(or auseClient { … }helper). useJUnitPlatform()+ barekotlin("test")inuts/build.gradle.kts. Kotlin 2.1.10 + Gradle variant metadata should resolve this tokotlin-test-junit5automatically, but the failure mode if it doesn't is "0 tests executed, build green". Recommend pinning explicitly:testImplementation(kotlin("test-junit5")), then verify with./gradlew :uts:test -ithat tests actually execute.
| } | ||
|
|
||
| public static long timestamp() { return System.currentTimeMillis(); } | ||
| public long timestamp() { return clock.currentTimeMillis(); } |
There was a problem hiding this comment.
ABI break — timestamp() changed from static to instance method.
- public static long timestamp() { return System.currentTimeMillis(); }
+ public long timestamp() { return clock.currentTimeMillis(); }Auth is public class (see Auth.java:41), so this is part of the published API. Any external caller compiled against the previous version (downstream apps, ably-android, etc.) will get NoSuchMethodError at runtime — and source callers using Auth.timestamp() will fail to compile.
This change isn't required for the FakeClock test path either: FakeClock plugs in via DebugOptions.clock, which Auth already reads through SystemClock.clockFrom(options) for internal use.
Suggested fix — preserve the static API for backwards compatibility:
/** @deprecated Use the instance method on an Auth instance. Retained for ABI compatibility. */
@Deprecated
public static long timestamp() { return System.currentTimeMillis(); }
public long instanceTimestamp() { return clock.currentTimeMillis(); }Or, if you want to commit to the rename, audit downstream consumers (ably-android etc.) and call it out in the CHANGELOG as a breaking change.
There was a problem hiding this comment.
We can't take a chance on breaking public API changes
There was a problem hiding this comment.
It's not a public API, our users never should've used this and I just can't imagine situation where they decided to use it, we can have this weird api available, but I would prefer removing it
| Auth(AblyBase ably, ClientOptions options) throws AblyException { | ||
| this.ably = ably; | ||
| this.clock = SystemClock.clockFrom(options); | ||
| this.nanoTimeDelta = clock.currentTimeMillis() - System.nanoTime()/(1000*1000); |
There was a problem hiding this comment.
nanoTimeDelta mixes virtual Clock time with real System.nanoTime().
this.nanoTimeDelta = clock.currentTimeMillis() - System.nanoTime()/(1000*1000);When clock is a FakeClock initialised at t=0, this computes 0 - System.nanoTime()/1e6 — a large negative value. The drift-detection at line ~924 recomputes the same formula and checks Math.abs(currentNanoTimeDelta - oldNanoTimeDelta) > 500. Virtual time stays at 0 while real time advances, so the drift exceeds 500 ms quickly and Auth will incorrectly conclude the device clock changed.
Dormant for RTN16 tests (no token-query path), but the first auth UTS test using FakeClock will hit it.
Suggested fix: add a nanoTimeMillis() (or similar) method to the Clock SPI, implement consistently for both SystemClock (delegate to System.nanoTime()/1e6) and FakeClock (return current virtual time minus a constructor-captured baseline). Then change both call sites here to use it, so the two ends of the drift subtraction always come from the same clock.
this.nanoTimeDelta = clock.currentTimeMillis() - clock.nanoTimeMillis();There was a problem hiding this comment.
that's valid, I'll add
|
|
||
| override fun newTimer(name: String): NamedTimer { | ||
| val t = FakeNamedTimer(name) | ||
| timers[name] = t |
There was a problem hiding this comment.
Name-keyed map silently clobbers concurrent timers.
override fun newTimer(name: String): NamedTimer {
val t = FakeNamedTimer(name)
timers[name] = t // ← overwrites any existing timer with this name
return t
}The SDK creates a new timer per attach/reattach/detach attempt (ChannelBase.java:543,607,646). If two channels are attached concurrently, the second clock.newTimer("attach-timer") overwrites the first in this map. The SDK's own NamedTimer reference still works, but FakeClock.advance() only iterates timers.values — the orphaned timer's tasks are silently never advanced.
The current ConnectionRecoveryTest doesn't expose this because it only ever has one of each timer alive at a time, but the first multi-channel test will silently fail to advance some retry/timeout schedules.
Suggested fix — store timers in a list, not a map. The name is for diagnostic logging, not identity:
private val timers = mutableListOf<FakeNamedTimer>()
override fun newTimer(name: String): NamedTimer =
FakeNamedTimer(name).also { synchronized(timers) { timers += it } }
fun advance(ms: Long) {
time += ms
synchronized(timers) { timers.toList() }.forEach { it.fireDue(time) }
// ...
}There was a problem hiding this comment.
shouldn't be a problem, we never should create timers concurrently, if we do it's a bug, we need to fix it anyway
| synchronized(waiters) { | ||
| waiters.add(Waiter(target as Object, time + timeout)) | ||
| } | ||
| (target as Object).wait() |
There was a problem hiding this comment.
waitOn calls Object.wait() with no timeout — diverges from SystemClock.waitOn semantics.
override fun waitOn(target: Any, timeout: Long) {
synchronized(waiters) {
waiters.add(Waiter(target as Object, time + timeout))
}
(target as Object).wait() // ← no timeout passed
}SystemClock.waitOn does target.wait(timeout) (returns after timeout ms regardless of whether notify fires). This implementation waits indefinitely — if fakeClock.advance(...) is never called past the deadline, the SDK thread hangs forever. That's a behavioural divergence from SystemClock and a footgun for any test that uses a blocking Future.get(timeout) without driving the clock.
Suggested fix — preserve the timeout as a safety fallback so the contract matches SystemClock:
(target as Object).wait(timeout)The virtual notifyAll() from advance() will usually fire first in tests that drive the clock properly; the real-time fallback is a safety net only.
(Note: this is distinct from CodeRabbit's monitor-contract comment on line 34 — the caller HttpScheduler.AsyncRequest.get does invoke clock.waitOn(this, remaining) from inside synchronized(this), so the monitor is held. The issue here is the missing timeout argument.)
| } | ||
|
|
||
| fun fireDue(now: Long) { | ||
| val due = pending.filter { it.fireAt <= now } |
There was a problem hiding this comment.
Two latent issues in fireDue worth addressing together:
fun fireDue(now: Long) {
val due = pending.filter { it.fireAt <= now }
pending -= due.toSet()
due.forEach { it.task.run() }
}-
No chain-scheduling fan-out. If a fired task calls
schedule(_, 0)to defer-to-next-tick, the new task is added topendingafterduewas filtered — it sits until the nextadvance(). Realjava.util.Timerfires chain-scheduled tasks promptly within the same tick. -
Cancelled tasks still fire.
TimerTask.cancel()called directly on the SDK's reference doesn't remove the task frompending—fireDuewill still calltask.run()on it. RealTimerskips cancelled tasks at fire time.
Suggested fix:
fun fireDue(now: Long) {
while (true) {
val due = synchronized(this) {
val d = pending.filter { it.fireAt <= now && it.task.state == TimerTask.SCHEDULED }
pending -= d.toSet()
d
}
if (due.isEmpty()) return
due.forEach { it.task.run() } // may schedule more tasks; loop picks them up
}
}(TimerTask.state isn't public — substitute a cancelled: AtomicBoolean on the Scheduled wrapper that the TimerInstance.cancel lambda sets.)
| @Test | ||
| fun `RSA4c2 - callback error connecting disconnected`() = runTest { | ||
| // DEVIATION: see deviations.md | ||
| if (System.getenv("RUN_DEVIATIONS") != null) return@runTest |
There was a problem hiding this comment.
Polarity is backwards — this example skips when RUN_DEVIATIONS is set, not when it's unset.
if (System.getenv("RUN_DEVIATIONS") != null) return@runTestThe surrounding text on line 326 says "test contains spec-correct assertions but is skipped by default", which means:
- Default (
RUN_DEVIATIONSunset) → skip RUN_DEVIATIONS=1set → run the spec-correct assertions and prove the deviation exists
As written, this example does the opposite. A future translator following the SKILL verbatim will produce backwards deviation tests that pass by default because they're skipping the assertion when they should be running it.
Note: the actual test at ConnectionRecoveryTest.kt:304 gets it right (if (… != null) { assertEquals(...) } — assertion only runs when env var is set). Only the SKILL example is wrong.
Suggested fix:
if (System.getenv("RUN_DEVIATIONS") == null) return@runTestThere was a problem hiding this comment.
Yes, as per https://github.com/ably/specification/blob/main/uts/docs/writing-derived-tests.md#3-deviation-test-patterns, this makes sense.
There was a problem hiding this comment.
Can you double check if this was an intentional change?
There was a problem hiding this comment.
I mean, do we intend to correct this? Like spec https://github.com/ably/specification/blob/main/uts/docs/writing-derived-tests.md#3-deviation-test-patterns says to change it
| val currentRecoveryKey = RecoveryKeyContext.decode(client.connection.createRecoveryKey()!!) | ||
| assertNotNull(currentRecoveryKey) | ||
| if (System.getenv("RUN_DEVIATIONS") != null) { | ||
| assertEquals(42L, currentRecoveryKey.msgSerial) |
There was a problem hiding this comment.
RTN16f deviation assertion doesn't verify what the spec actually requires.
The spec (connection_recovery_test.md §RTN16f, lines 437-458) requires the test to publish a message after recovery, capture the outgoing MESSAGE frame, and assert:
ASSERT message_frame.message.msgSerial == 42
The current test only re-reads createRecoveryKey() and inspects the in-memory msgSerial field — it never exercises the publish flow. A future SDK fix that preserves the recovery-key msgSerial but still resets the outgoing-frame msgSerial would pass this test, which defeats the point of the deviation gate.
(This is related to but distinct from CodeRabbit's vacuous-pass comment on line 306 — that's about the default path; this is about the deviation path not asserting the right thing.)
Suggested fix — inside the RUN_DEVIATIONS gate, do the spec's publish + capture flow:
if (System.getenv("RUN_DEVIATIONS") != null) {
testChannel.publish("event", "data")
val msg = mock.awaitNextMessageFromClient()
assertEquals(ProtocolMessage.Action.message, msg.action)
assertEquals(42L, msg.msgSerial)
}Also record this in uts/src/test/kotlin/io/ably/lib/deviations.md — it's currently referenced (line 301) but the file doesn't exist.
| assertEquals(ConnectionState.connected, client.connection.state) | ||
| assertEquals("fresh-conn", client.connection.id) | ||
| assertEquals("fresh-key", client.connection.key) | ||
| assertNull(capturedQueryParams!!["recover"]) |
There was a problem hiding this comment.
RTN16f1 — missing log-handler assertion called for by the spec.
The spec has an explicit implementation note at connection_recovery_test.md:533-536:
Implementations should verify this by capturing log output (e.g., via a log handler) and asserting that an error-level log message was emitted mentioning the malformed recovery key.
The current test asserts the connection succeeds and that no recover / resume query param was sent, but doesn't verify the log emission. Confirmed RecoveryKeyContext.decode does Log.e(TAG, …) on JsonSyntaxException, so a handler attached via ClientOptions.logHandler would observe it.
Suggested fix — capture log events and assert one was emitted at error level mentioning the malformed key:
val logEvents = mutableListOf<Pair<Int, String>>()
val client = TestRealtimeClient {
autoConnect = false
recover = "this-is-not-valid-json!!!"
logHandler = Log.LogHandler { sev, _, msg, _ -> logEvents += sev to msg }
install(mock)
}
…
assertTrue(
logEvents.any { (sev, msg) -> sev >= Log.ERROR && msg.contains("recovery", ignoreCase = true) },
"Expected an error log mentioning the malformed recovery key"
)| awaitState(client, ConnectionState.connected) | ||
|
|
||
| mock.simulateDisconnect() | ||
| awaitState(client, ConnectionState.connected) |
There was a problem hiding this comment.
RTN16k — missing assertion on the SDK's post-reconnect state.
The mock returns connectionKey: "resumed-key" on the second CONNECTED (line 224). The test verifies the outgoing resume query param (line 250) but never checks what the SDK applied on receiving the second CONNECTED. A bug where the SDK keeps the old key (or fails to update connection.id) would pass this test silently.
The SDK updates these from connectionDetails at ConnectionManager.java:1319,1323:
connection.id = message.connectionId;
connection.key = connectionDetails.connectionKey; // RTN16dSuggested fix — add after the second awaitState(connected):
assertEquals("resumed-key", client.connection.key)
assertEquals("recovered-conn-id", client.connection.id)This also gives partial coverage of RTN16d (which has no dedicated section in the spec).
| awaitState(clientSuspended, ConnectionState.connected) | ||
|
|
||
| mockSuspended.simulateDisconnect() | ||
| awaitState(clientSuspended, ConnectionState.disconnected) |
There was a problem hiding this comment.
Three related issues in RTN16g2's SUSPENDED branch:
-
Transient-state observation.
awaitState(disconnected)on line 192 is exactly whatwriting-test-specs.md§"Verifying Transient States (Record-and-Verify Pattern)" calls out as an anti-pattern — DISCONNECTED is supposed to be transient (RTN15a immediate-reconnect). It's safe today only because FakeClock gates the SDK's retry timer (ConnectionManager.java:316,912); any future SDK change introducing an immediate-reconnect path on transport failure will make this race-y. Use record-and-verify instead. -
refuseJobis launched aftersimulateDisconnect()and afterawaitState(disconnected). Under FakeClock this is structurally clean (the SDK can't move until the test advances time), but moving the launch before the disconnect makes the ordering self-evident and removes the forward-compatibility hazard above. -
Client leaks. This test creates three clients (
client,clientFailed,clientSuspended) but only the last is closed on the success path. If any assertion above throws, all three leak their internal Timer threads into subsequent tests.
Suggested fix — tracked-list @AfterEach + record-and-verify pattern:
private val tracked = mutableListOf<AblyRealtime>()
@AfterEach fun tearDown() = tracked.forEach { runCatching { it.close() } }
// in the test:
tracked += clientSuspended
val stateChanges = mutableListOf<ConnectionState>()
clientSuspended.connection.on { stateChanges += it.current }
// start the refuse job FIRST
val refuseJob = launch { /* ... */ }
mockSuspended.simulateDisconnect()
awaitState(clientSuspended, ConnectionState.suspended)
refuseJob.cancel()
assertContainsInOrder(stateChanges,
ConnectionState.connected,
ConnectionState.disconnected,
ConnectionState.suspended)Apply the same tracked += pattern to client and clientFailed earlier in the test.
There was a problem hiding this comment.
Seems, we should explicitly call out these type of cases in .claude/skills/uts-to-kotlin/SKILL.md
Maybe, it's because writing-test-specs.md is not correctly loaded as a part of the skill.
|
|
||
| Translate the UTS pseudocode test spec at `$ARGUMENTS` into a runnable Kotlin test in the `uts` module. | ||
|
|
||
| Reference: [Writing Derived Tests](https://github.com/ably/specification/blob/main/uts/docs/writing-derived-tests.md) |
There was a problem hiding this comment.
| Reference: [Writing Derived Tests](https://github.com/ably/specification/blob/main/uts/docs/writing-derived-tests.md) | |
| Reference: [Writing Derived Tests](https://raw.githubusercontent.com/ably/specification/refs/heads/main/uts/docs/writing-derived-tests.md) |
It's easier for LLM to fetch/understand raw markdown than HTML page
There was a problem hiding this comment.
When skill is loaded, it doesn't necessarily load Reference markdown.
It's better we make fetch mandatory.
Note - Always fetch Writing Derived Tests as a part of SKILL load.
| allowed-tools: Bash, Read, Edit, Write | ||
| --- | ||
|
|
||
| Translate the UTS pseudocode test spec at `$ARGUMENTS` into a runnable Kotlin test in the `uts` module. |
There was a problem hiding this comment.
If $ARGUMENTS not provided, skill should either explicitly prompt for $ARGUMENTS with format of the give file.
There was a problem hiding this comment.
Also, if required/sufficient information is not available in $ARGUMENTS, then skill should log error and return early
| uts/src/test/kotlin/io/ably/lib/ClientFactories.kt | ||
| uts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocket.kt | ||
| uts/src/test/kotlin/io/ably/lib/test/mock/MockHttpClient.kt | ||
| uts/src/test/kotlin/io/ably/lib/test/mock/PendingRequest.kt | ||
| uts/src/test/kotlin/io/ably/lib/test/mock/PendingConnection.kt | ||
| uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt | ||
| uts/src/test/kotlin/io/ably/lib/test/mock/MockEvent.kt |
There was a problem hiding this comment.
Can't we put all of those in single uts.infra package? We need to update skill everytime infrastructure changes wdyt
There was a problem hiding this comment.
or atleast include uts.infra.*/ uts/src/test/kotlin/io/ably/lib/test/mock/*` package
| assertEquals(40160, error.errorInfo.code) | ||
| ``` | ||
|
|
||
| **Never use the accommodate-both pattern** (accept either spec or SDK behaviour). Every test must assert either spec behaviour or the SDK's actual behaviour — never both at once. |
There was a problem hiding this comment.
I don't like idea of accommodating SDK behaviour when we accept UTS spec as source of truth.
We should only accept UTS spec behaviour
Maybe this should explicitly go into deviations.md.
Not sure why we agreed on it here -> https://github.com/ably/specification/blob/main/uts/docs/writing-derived-tests.md#3-deviation-test-patterns:~:text=Avoid%20the%20accommodate%2Dboth%20pattern
|
|
||
| ### Deviations file | ||
|
|
||
| Append to `uts/src/test/kotlin/io/ably/lib/deviations.md`. Each entry needs: |
There was a problem hiding this comment.
| Append to `uts/src/test/kotlin/io/ably/lib/deviations.md`. Each entry needs: | |
| Deviation file should be created at `uts/src/test/kotlin/io/ably/lib/deviations.md` ( Required for `Evaluation` phase). Each entry is appended with: |
|
|
||
| import java.util.TimerTask; | ||
|
|
||
| public interface NamedTimer { |
There was a problem hiding this comment.
| public interface NamedTimer { | |
| public interface AblyTimer { |
Since, this is Ably SDK specific timer, would make sense to rename it.
This also conveys that it's a custom timer with ably requirement specific functionality.
| objectId: String, | ||
| private val realtimeObjects: DefaultRealtimeObjects, | ||
| ) : LiveCounter, BaseRealtimeObject(objectId, ObjectType.Counter) { | ||
| ) : LiveCounter, BaseRealtimeObject(objectId, ObjectType.Counter, SystemClock.clockFrom(realtimeObjects.adapter.clientOptions)) { |
There was a problem hiding this comment.
Can't we add extension method to SystemClock, that builds clock from given realtimeObjects.adapter.clientOptions. Currently, long method call is polluting the constructor
| private val realtimeObjects: DefaultRealtimeObjects, | ||
| internal val semantics: ObjectsMapSemantics = ObjectsMapSemantics.LWW | ||
| ) : LiveMap, BaseRealtimeObject(objectId, ObjectType.Map) { | ||
| ) : LiveMap, BaseRealtimeObject(objectId, ObjectType.Map, SystemClock.clockFrom(realtimeObjects.adapter.clientOptions)) { |
There was a problem hiding this comment.
Can't we add extension method to SystemClock, that builds clock from given realtimeObjects.adapter.clientOptions. Currently, long method call is polluting the constructor
This PR includes:
Summary by CodeRabbit
New Features
Documentation
Tests / Chores