Skip to content

Commit e1ed6cd

Browse files
committed
test(uts): address review feedback — harden shared infra, fix test-resource leaks, correct docs
Review-driven fixes on the UTS refactoring (PR #1228 threads): - uts infra: awaitState/awaitChannelState could double-resume their continuation (the state listener and the immediate check race on different threads; check-then-resume is not atomic). Use the atomic single-winner tryResume/completeResume pair (@InternalCoroutinesApi). - liveobjects tests: dispose ObjectsPool in teardown where it leaked a GC coroutine + adapter subscription (LiveObjectTombstoneTest, DefaultRealtimeObjectAsyncTest), matching the documented S-4 contract; align ValueTypesTest to the dispose-before-unmockkAll ordering. - liveobjects integration: retain the provisioned SandboxApp and delete it in tearDownAfterClass (best-effort, matching the :java suites). - InternalLiveMapApiTest: replace the stale note claiming a real GET /time fires — setupSyncedChannel installs a MockHttpClient that answers /time locally; the unit tier is hermetic. - deviations.md (:java): restructure into the canonical four-section format from writing-derived-tests.md (UTS Spec Errors / Failing Tests / Adapted Tests / Mock Infrastructure Limitations), preserving all recorded deviations; fix the RTN16g2 heading to describe the workaround rather than restate it as a requirement; drop the stale moved-pointer. - uts/README.md: replace the invalid-Kotlin ellipsis with the real systemProperty expression from uts/build.gradle.kts. - uts-to-kotlin skill: point the deviation checklist at the tier's module deviations file; spell out the full TestHelpers.kt path. The ObjectsSyncTracker channelSerial regex finding is intentionally not fixed here — tracked cross-SDK in ably/specification#520. Verified: liveobjects unit 389/0; :java/:uts UTS unit 6/0, 3/0; integration 29/0, 5/0, 4/0; codenarc + checkstyle green.
1 parent f9492ab commit e1ed6cd

11 files changed

Lines changed: 104 additions & 79 deletions

File tree

‎.claude/skills/uts-to-kotlin/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -645,7 +645,7 @@ For each test case, verify:
645645
Deviations are discovered by running, so this check applies in evaluate mode. For any place where the
646646
generated test diverges from the spec pseudocode (adapted assertion, env-gated skip, or omitted step):
647647
- [ ] A `// DEVIATION:` comment explains why
648-
- [ ] The deviation is recorded in `lib/src/test/kotlin/io/ably/lib/uts/deviations.md`
648+
- [ ] The deviation is recorded in the tier's module deviations file (see "Deviations file" above: `lib/src/test/kotlin/io/ably/lib/uts/deviations.md` for realtime/rest tiers, `liveobjects/src/test/kotlin/io/ably/lib/liveobjects/uts/deviations.md` for objects tiers)
649649

650650
If you find gaps during this review, fix them, then **re-run the audit script** until `missingInKotlin` /
651651
`orphanInKotlin` are empty and every `perTest` entry reconciles, and re-run Step 5 (compile) — and, in

‎.claude/skills/uts-to-kotlin/references/objects-mapping.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -726,7 +726,7 @@ reflection.
726726

727727
The internal classes have `private constructor`s; each pairs with an `internal` companion factory that
728728
needs a `DefaultRealtimeObject`. Build one from the mocked adapter (helper already exists in
729-
`liveobjects/src/test/.../unit/TestHelpers.kt`):
729+
`liveobjects/src/test/kotlin/io/ably/lib/liveobjects/unit/TestHelpers.kt`):
730730

731731
```kotlin
732732
val ro = DefaultRealtimeObject("test", getMockAblyClientAdapter())
Lines changed: 56 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,19 @@
1-
# SDK Deviations
2-
3-
Deviations from the Ably spec identified during UTS test translation. Each entry records the spec point, what the spec requires, what the SDK actually does, and which test contains the deviation gate.
4-
5-
**Scope:** this file now lives alongside the realtime UTS suites in the `:java` module
6-
(`lib/src/test/kotlin/io/ably/lib/uts/`) and holds deviations for the **realtime/rest tiers** it
7-
hosts. All **objects** tiers (unit, integration and proxy) moved to `:liveobjects`'s own test source
8-
set alongside the tests they document; their deviations (the typed-SDK / language adaptations, the
9-
intentional RTO18d entry, and any objects integration/proxy entries) are in
10-
`liveobjects/src/test/kotlin/io/ably/lib/liveobjects/uts/deviations.md`. For the shared UTS infra these
11-
suites consume, the tier smoke examples, and the `RUN_DEVIATIONS` mechanism, see `uts/README.md`.
12-
13-
Entries are grouped by actionability (shared taxonomy across both files; only groups with entries in
14-
this file appear as sections below):
15-
16-
| Group | Meaning | Action |
17-
|---|---|---|
18-
| **1) Genuine SDK bugs — open** | runtime behaviour differs from the spec; ably-js is compliant | fix the SDK |
19-
| **2) Shared gap — open in both SDKs** | ably-java and ably-js deviate the same way | optional joint fix (spec is ahead of both) |
20-
| **3) Expected — typed-SDK / language adaptations** | not bugs: RTTS API partitioning, compile-time guarantees, internal-wire visibility | none — correct as documented |
21-
| **4) Intentional deviation** | deliberate SDK design choice; the spec point itself is questioned | none unless the spec is revised |
1+
# Deviations — UTS realtime/rest suites (`io.ably.lib.uts.*`)
2+
3+
> **Scope:** this file lives alongside the realtime/rest UTS suites in the `:java` module
4+
> (`lib/src/test/kotlin/io/ably/lib/uts/`) and holds deviations for the **realtime/rest tiers** it
5+
> hosts. All **objects** tiers (unit, integration and proxy) moved to `:liveobjects`'s own test source
6+
> set alongside the tests they document; their deviations (the typed-SDK / language adaptations, the
7+
> intentional RTO18d entry, and any objects integration/proxy entries) are in
8+
> `liveobjects/src/test/kotlin/io/ably/lib/liveobjects/uts/deviations.md`. For the shared UTS infra these
9+
> suites consume, the tier smoke examples, and the `RUN_DEVIATIONS` mechanism, see `uts/README.md`.
10+
>
11+
> Records every place a generated test deviates from its UTS spec, using the manual's **Recording
12+
> deviations** entry format: each entry records the spec point, what the spec requires, what the SDK
13+
> actually does, and which test carries the deviation gate. Every deviation recorded here is a
14+
> **genuine SDK bug** (open; ably-js is compliant — fix the SDK), stated per entry under **Status**; the
15+
> other actionability groups of the shared taxonomy (shared cross-SDK gap, typed-SDK / language
16+
> adaptations, intentional design divergence) only have entries in the objects file.
2217
2318
> **Recently fixed and removed from this file:** RTO23e (`get()` now re-attaches a DETACHED channel —
2419
> mode-only check + ensure-active-channel) and RTO20e/RTO20e1 (event-driven `once(SYNCED)` waiters +
@@ -32,16 +27,48 @@ this file appear as sections below):
3227
3328
---
3429

35-
# 1) Genuine SDK bugs — open (realtime module)
30+
## UTS Spec Errors
31+
32+
*(none)*
33+
34+
## Failing Tests
3635

37-
*Runtime behaviour differs from the spec and ably-js is compliant — real bugs, pending an SDK fix.*
36+
*SDK non-compliance where the spec-correct assertion is present but skipped (env-gated behind `RUN_DEVIATIONS`). These are the primary output — each maps to a potential issue to file.*
3837

3938
> ⚠ **RTL13b / RTL13c:** the channel-state UTS tests these two entries cite are **not currently part of the
4039
> uts suite** (no `RTL13*` tests or gates exist in `unit/realtime/` — only `ConnectionRecoveryTest.kt` is
4140
> translated). The entries are retained as confirmed SDK gaps (cross-checked against ably-js in
4241
> `ABLY-JS-JAVA-DEVIATIONS-COMPARISON.md`); re-verify them when the channels module translation lands.
42+
> (RTL13b is recorded under **Adapted Tests** — it adapts the test setup rather than env-gating an assertion.)
43+
44+
### RTL13c — channelRetryTimeout not cancelled when connection leaves CONNECTED
45+
46+
**Spec point:** RTL13c
47+
**What the spec requires:** When the connection is no longer CONNECTED, any pending automatic channel reattach timer (channelRetryTimeout) must be cancelled. The channel should remain SUSPENDED without attempting to reattach until the connection is restored.
48+
**What the SDK does:** The channelRetryTimeout fires regardless of connection state. When it fires while disconnected, the channel transitions to ATTACHING even though there is no active connection, and no ATTACH message can be sent.
49+
**Tests affected:**
50+
- `RTL13c - automatic retry cancelled when connection is no longer CONNECTED` (RTL13c/retry-cancelled-disconnected-0) — the assertions `assertEquals(attachCountAfterDisconnect, attachCount)` and `assertEquals(ChannelState.suspended, channel.state)` are gated behind `RUN_DEVIATIONS`.
51+
52+
**Status:** Open SDK bug — ably-js is compliant; fix the SDK.
53+
54+
### RTN16f — msgSerial not initialised from recovery key on connect
55+
56+
**Spec point:** RTN16f
57+
**What the spec requires:** When instantiated with the `recover` option, the SDK initialises its internal `msgSerial` counter to the value stored in the recovery key, so the first published message carries that serial.
58+
**What the SDK does:** `ConnectionManager.onConnected()` resets `msgSerial` to 0 whenever `connection.id` is null on the fresh client (line 1316), even when the `recover` option is set. The recovered serial is discarded.
59+
**Workaround in tests:** The spec-correct assertion (`assertEquals(42L, msgSerial)`) is gated behind `RUN_DEVIATIONS`. A regression guard assertion (`assertEquals(0L, msgSerial)`) runs by default to catch any unintentional change to the SDK's actual behaviour.
60+
**Tests affected:**
61+
- `RTN16f - recover option initializes msgSerial from recoveryKey` (RTN16f/recover-initializes-msgserial-0) — `assertEquals(42L, ...)` gated; `assertEquals(0L, ...)` added as regression guard.
62+
63+
**Status:** Open SDK bug — ably-js is compliant; fix the SDK.
64+
65+
## Adapted Tests
4366

44-
## RTL13b — ATTACHING → SUSPENDED via `realtimeRequestTimeout` not implemented
67+
*SDK non-compliance where the test asserts the SDK's actual behaviour (or adapts its setup/stimulus to the SDK's reality) instead of using the spec's unreachable path. It passes, guards against regressions, and documents the deviation.*
68+
69+
> ⚠ Not currently part of the uts suite — see the shared **RTL13b / RTL13c** caveat under **Failing Tests**.
70+
71+
### RTL13b — ATTACHING → SUSPENDED via realtimeRequestTimeout not implemented
4572

4673
**Spec point:** RTL13b
4774
**What the spec requires:** If a channel's reattach request (triggered by RTL13a) does not receive a response within `realtimeRequestTimeout`, the channel must transition from ATTACHING to SUSPENDED and schedule a retry after `channelRetryTimeout`.
@@ -53,19 +80,9 @@ this file appear as sections below):
5380
- `RTL13b - repeated failures cycle SUSPENDED to ATTACHING indefinitely` (RTL13b/repeated-failure-cycle-2) — mock sends DETACHED instead of withholding response
5481
- `RTL13c - automatic retry cancelled when connection is no longer CONNECTED` (RTL13c/retry-cancelled-disconnected-0) — setup path changed
5582

56-
---
57-
58-
## RTL13c — channelRetryTimeout not cancelled when connection leaves CONNECTED
59-
60-
**Spec point:** RTL13c
61-
**What the spec requires:** When the connection is no longer CONNECTED, any pending automatic channel reattach timer (channelRetryTimeout) must be cancelled. The channel should remain SUSPENDED without attempting to reattach until the connection is restored.
62-
**What the SDK does:** The channelRetryTimeout fires regardless of connection state. When it fires while disconnected, the channel transitions to ATTACHING even though there is no active connection, and no ATTACH message can be sent.
63-
**Tests affected:**
64-
- `RTL13c - automatic retry cancelled when connection is no longer CONNECTED` (RTL13c/retry-cancelled-disconnected-0) — the assertions `assertEquals(attachCountAfterDisconnect, attachCount)` and `assertEquals(ChannelState.suspended, channel.state)` are gated behind `RUN_DEVIATIONS`.
65-
66-
---
83+
**Status:** Open SDK bug — ably-js is compliant; fix the SDK.
6784

68-
## RTN16g2 — Fatal ERROR must be sent without closing the transport
85+
### RTN16g2 — the spec's fatal-ERROR-plus-close trigger can't drive this SDK to FAILED
6986

7087
**Spec point:** RTN16g2
7188
**What the spec requires:** Trigger FAILED state by sending a fatal ERROR message followed by closing the WebSocket (`send_to_client_and_close`), using error code 50000/statusCode 500.
@@ -76,24 +93,8 @@ this file appear as sections below):
7693
**Tests affected:**
7794
- `RTN16g2 - createRecoveryKey returns null in inactive states and before first connect` (RTN16g2/recovery-key-null-inactive-0) — error code and send method changed.
7895

79-
---
80-
81-
## RTN16f — msgSerial not initialised from recovery key on connect
82-
83-
**Spec point:** RTN16f
84-
**What the spec requires:** When instantiated with the `recover` option, the SDK initialises its internal `msgSerial` counter to the value stored in the recovery key, so the first published message carries that serial.
85-
**What the SDK does:** `ConnectionManager.onConnected()` resets `msgSerial` to 0 whenever `connection.id` is null on the fresh client (line 1316), even when the `recover` option is set. The recovered serial is discarded.
86-
**Workaround in tests:** The spec-correct assertion (`assertEquals(42L, msgSerial)`) is gated behind `RUN_DEVIATIONS`. A regression guard assertion (`assertEquals(0L, msgSerial)`) runs by default to catch any unintentional change to the SDK's actual behaviour.
87-
**Tests affected:**
88-
- `RTN16f - recover option initializes msgSerial from recoveryKey` (RTN16f/recover-initializes-msgserial-0) — `assertEquals(42L, ...)` gated; `assertEquals(0L, ...)` added as regression guard.
89-
90-
---
91-
96+
**Status:** Open SDK bug — ably-js is compliant; fix the SDK.
9297

93-
# Objects deviations — moved
98+
## Mock Infrastructure Limitations
9499

95-
All objects tiers (**unit, integration and proxy**) live in `:liveobjects`'s own test source set,
96-
and their deviation records (the typed-SDK / language adaptations, the former groups 3 & 4 as they
97-
applied to objects, and any objects integration/proxy entries) are in
98-
`liveobjects/src/test/kotlin/io/ably/lib/liveobjects/uts/deviations.md`. This file keeps only the
99-
deviations for the realtime/rest tiers hosted in `:java`.
100+
*(none)*

‎liveobjects/src/test/kotlin/io/ably/lib/liveobjects/integration/setup/IntegrationTest.kt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,8 @@ abstract class IntegrationTest {
9191
@JvmStatic
9292
@AfterClass
9393
@Throws(Exception::class)
94-
fun tearDownAfterClass() {
94+
fun tearDownAfterClass(): Unit = runBlocking {
95+
if (::sandbox.isInitialized) sandbox.delete()
9596
}
9697
}
9798
}

‎liveobjects/src/test/kotlin/io/ably/lib/liveobjects/integration/setup/Sandbox.kt‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,17 @@ import kotlinx.coroutines.CompletableDeferred
1515
* canonical `test-app-setup.json` app spec); this type just carries the fields the local
1616
* client-factory extensions need.
1717
*/
18-
class Sandbox private constructor(val appId: String, val apiKey: String) {
18+
class Sandbox private constructor(private val app: SandboxApp, val appId: String, val apiKey: String) {
1919
companion object {
2020
internal suspend fun createInstance(): Sandbox {
2121
val app = SandboxApp.create()
2222
// defaultKey is the full-capability "appId.keyId:keySecret" key (index 0 of the app spec)
23-
return Sandbox(appId = app.appId, apiKey = app.defaultKey)
23+
return Sandbox(app = app, appId = app.appId, apiKey = app.defaultKey)
2424
}
2525
}
26+
27+
/** Best-effort teardown of the provisioned sandbox app (see [SandboxApp.delete]). */
28+
internal suspend fun delete() = app.delete()
2629
}
2730

2831
internal fun Sandbox.createRealtimeClient(options: ClientOptions.() -> Unit): AblyRealtime {

‎liveobjects/src/test/kotlin/io/ably/lib/liveobjects/unit/DefaultRealtimeObjectAsyncTest.kt‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,18 @@ import kotlin.test.assertFailsWith
2020
*/
2121
class DefaultRealtimeObjectAsyncTest {
2222

23+
private val created = mutableListOf<DefaultRealtimeObject>()
24+
2325
private fun newObject(): DefaultRealtimeObject =
24-
DefaultRealtimeObject("ch", getMockAblyClientAdapter())
26+
DefaultRealtimeObject("ch", getMockAblyClientAdapter()).also { created += it }
2527

2628
private fun boom() = AblyException.fromErrorInfo(ErrorInfo("boom", 400, 40000))
2729

2830
@After
29-
fun tearDown() = unmockkAll() // getMockAblyClientAdapter uses mockkStatic - clean up global state
31+
fun tearDown() {
32+
created.forEach { it.objectsPool.dispose() } // the pool init starts a real GC coroutine
33+
unmockkAll() // getMockAblyClientAdapter uses mockkStatic - clean up global state
34+
}
3035

3136
@Test
3237
fun asyncFutureCompletesWithResult() {

‎liveobjects/src/test/kotlin/io/ably/lib/liveobjects/unit/LiveObjectTombstoneTest.kt‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,21 @@ import org.junit.Test
2929
*/
3030
class LiveObjectTombstoneTest {
3131

32+
private lateinit var realtimeObject: DefaultRealtimeObject
33+
3234
private fun rootMapWithNameEntry(): InternalLiveMap {
33-
val realtimeObject = DefaultRealtimeObject("test", getMockAblyClientAdapter())
35+
realtimeObject = DefaultRealtimeObject("test", getMockAblyClientAdapter())
3436
val map = InternalLiveMap.zeroValue("root", realtimeObject)
3537
map.data["name"] = LiveMapEntry(timeserial = "01", data = WireObjectData(string = "Alice"))
3638
map.siteTimeserials["site1"] = "00"
3739
return map
3840
}
3941

4042
@After
41-
fun tearDown() = unmockkAll() // getMockAblyClientAdapter uses mockkStatic - clean up global state
43+
fun tearDown() {
44+
realtimeObject.objectsPool.dispose() // the pool init starts a real GC coroutine
45+
unmockkAll() // getMockAblyClientAdapter uses mockkStatic - clean up global state
46+
}
4247

4348
/**
4449
* @UTS objects/unit/RTLO4e10/object-delete-root-noop-0

‎liveobjects/src/test/kotlin/io/ably/lib/liveobjects/uts/unit/InternalLiveMapApiTest.kt‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,10 @@ import kotlin.test.assertTrue
2929
* mock's recorded `MessageFromClient` event log as the spec's `captured_messages` — see
3030
* [capturedObjectMessages].
3131
*
32-
* Note: evaluating a `LiveCounter`/`LiveMap` value type generates its objectId from server
33-
* time (RTO16), which the SDK fetches once per JVM via REST `GET /time` — the mock transport
34-
* does not intercept HTTP, so the first `*_CREATE` test in a run performs that single
35-
* unauthenticated request against the real endpoint.
32+
* Note: evaluating a LiveCounter/LiveMap value type derives its objectId from server time
33+
* (RTO16), which the SDK fetches via REST GET /time. The shared setupSyncedChannel installs a
34+
* MockHttpClient (Helpers.kt) that answers /time locally, so these unit tests stay hermetic —
35+
* no real network request is made, per the UTS unit-tier no-network contract.
3636
*/
3737
class InternalLiveMapApiTest {
3838

‎liveobjects/src/test/kotlin/io/ably/lib/liveobjects/uts/unit/ValueTypesTest.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,8 @@ class ValueTypesTest {
5050

5151
@AfterTest
5252
fun tearDown() {
53-
unmockkAll()
5453
ro.objectsPool.dispose()
54+
unmockkAll()
5555
}
5656

5757
/**

‎uts/README.md‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,12 @@ tasks.withType<Test>().configureEach {
234234
jvmArgs("--add-opens", "java.base/java.lang=ALL-UNNAMED")
235235
// Propagate a local proxy-build override (see ProxyManager): -Duts.proxy.localPath=… or
236236
// $UTS_PROXY_LOCAL_PATH.
237-
systemProperty("uts.proxy.localPath", …)
237+
systemProperty(
238+
"uts.proxy.localPath",
239+
providers.systemProperty("uts.proxy.localPath")
240+
.orElse(providers.environmentVariable("UTS_PROXY_LOCAL_PATH"))
241+
.getOrElse(""),
242+
)
238243
}
239244

240245
tasks.register<Test>("runUtsUnitTests") { filter { includeTestsMatching("io.ably.lib.uts.unit.*") } }

0 commit comments

Comments
 (0)