Add UTS (Universal Test Spec) suite for Realtime API#2211
Open
paddybyers wants to merge 6 commits intouts-restfrom
Open
Add UTS (Universal Test Spec) suite for Realtime API#2211paddybyers wants to merge 6 commits intouts-restfrom
paddybyers wants to merge 6 commits intouts-restfrom
Conversation
Add MockWebSocket infrastructure and realtime client test suite: - mock_websocket.js: MockWebSocket, PendingWSConnection, MockWSInstance - client_options.test.js: RSC1, RSC1a, RSC1b, RSC1c, RTC12 (9 tests) - realtime_client.test.js: RTC1a-f, RTC2-4, RTC13, RTC15-17 (22 tests) - realtime_stats.test.js: RTC5, RTC5a, RTC5b (3 tests) - realtime_request.test.js: RTC9 (5 tests) - realtime_timeouts.test.js: RTC7 (4 tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… RTN25) Add 10 new test files covering connection ping, open failures, resume/retry, fallback hosts, heartbeat idle timer, error reason, auto connect, connection ID/key, server-initiated reauth, update events, and whenState. Fix FakeClock zero-time bug (JS falsy 0 broke ably-js idle timer check) and add checkConnectivity() to mock HTTP for fallback host tests. 516 tests passing across full UTS suite. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Apply the same type annotation fixes as the REST tests: - Add `as any` casts to partial connectionDetails objects - Add `as any` casts to Ably.Realtime constructor options - Cast `(Ably as any).RealtimeAnnotations` for plugin access - Add `!` non-null assertions for errorReason property access - Add `: any` type annotations to callback parameters - Type array declarations as `any[]` - Fix prettier formatting across all 36 files Build passes (0 errors), 749 tests passing, 39 pending. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
925 passing, 52 pending (skipped deviations), 0 failing. Adds tests for all realtime UTS specs including presence, auth, channels, connection, and delta decoding. Updates deviations.md with correct classification of all skipped and failing tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace all real setTimeout/setInterval delays with flushAsync() — a setImmediate-based event-loop yield. Test suite drops from 16s to 2s. - Add flushAsync() helper to helpers.ts - Replace ~190 await setTimeout patterns across 28 test files - Convert callback-based "prove a negative" tests to async - Replace delayed mock responses with setImmediate - Track all Platform.Config timers and cancel orphans in restoreAll() to work around ably-js connectWs() timer handle overwrite bug - Skip 5 remaining failing deviation tests (RTC1a, RTL5k, RTN24, RTP17e, TM4) with RUN_DEVIATIONS guard 925 passing, 52 pending, 0 failing in 2s. Process exits cleanly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThis PR introduces comprehensive UTS (Unified Test Suite) infrastructure and test coverage for the Ably Realtime client library. It adds a mock WebSocket implementation, timer/async helpers for testing, and 40+ test files covering connection authentication, channel operations, client behavior, and connection lifecycle scenarios using a mocking framework with deterministic timer control. ChangesTest Infrastructure & UTS Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (2)
test/uts/realtime/client/client_options.test.ts (1)
48-53: ⚡ Quick winUse an obviously fake JWT fixture here.
This literal is realistic enough to trip secret scanners, but the test only needs the three-segment shape. Replacing it with a clearly synthetic value keeps the coverage and avoids noisy security findings.
Suggested change
- const jwt = - 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dozjgNryP4J3jVmNHl0w5N_XgL0n3I9PlFUP0THsR8U'; + const jwt = 'header.payload.signature';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/realtime/client/client_options.test.ts` around lines 48 - 53, The test uses a realistic-looking JWT literal in the 'RSC1c - token string (JWT format)' test: replace the current jwt variable value with an obviously fake three-segment token (e.g. "aaa.bbb.ccc" or "header.payload.signature") so it preserves the three-dot JWT shape used by the Ably.Realtime instantiation and the assertion on client.options.token but avoids using a realistic secret-like string; update the jwt constant in that test and leave the Ably.Realtime({ token: jwt, autoConnect: false }) and expect(client.options.token).to.equal(jwt) lines unchanged.test/uts/realtime/channels/channel_subscribe.test.ts (1)
283-307: ⚡ Quick winRemove or merge duplicate RTL7f echo URL test.
The same scenario/title (
RTL7f - echoMessages false sets echo param in URL) is defined twice. Keeping one avoids duplicate execution and ambiguous mocha output.Also applies to: 878-902
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/realtime/channels/channel_subscribe.test.ts` around lines 283 - 307, The test "RTL7f - echoMessages false sets echo param in URL" is duplicated; remove the redundant duplicate or merge their expectations so the scenario is only defined once. Locate the duplicate 'it' block with the exact title "RTL7f - echoMessages false sets echo param in URL" in the channel_subscribe.test.ts test file, delete one of the two identical test cases (or combine any differing assertions into a single 'it' block) and ensure the remaining test still creates MockWebSocket, constructs the Ably.Realtime client with echoMessages: false, waits for connection, asserts connUrl.searchParams.get('echo') === 'false', and closes the client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/uts/helpers.ts`:
- Around line 41-50: The Platform.Config wrapper functions currently diverge
from IPlatformConfig and use unsafe casts; update the wrappers for
Platform.Config.setTimeout and Platform.Config.clearTimeout to match the
interface by removing the variadic ...args and using arrow functions so they
inherit the correct lexical this, call
_origPlatformSetTimeout/_origPlatformClearTimeout with the expected (handler,
timeout) signature, add proper parameter types (handler: () => void, timeout?:
number) and ensure clearTimeout does not return a value (void) while still
deleting the timer from _allPlatformTimers; keep references to
_origPlatformSetTimeout, _origPlatformClearTimeout and _allPlatformTimers to
maintain behavior.
In `@test/uts/mock_websocket.ts`:
- Around line 294-320: The mock never records the pending connection into
this.active_connection, so send_to_client() can’t find an active connection;
when creating a PendingWSConnection in _onNewWebSocket (and the other similar
block at lines ~340-344) assign the new PendingWSConnection to
this.active_connection (e.g., this.active_connection = conn) after constructing
conn so tests using send_to_client() will see the active connection; keep
existing notifications to onConnectionAttempt/_connectionWaiters unchanged.
- Around line 109-115: _fireMessage currently ignores the socket closed state
and will emit messages even after close, which diverges from real WebSocket
behavior; modify _fireMessage to check the internal _closed flag (and/or the
close()/_fireClose() state) before calling this.onmessage and bail out if
_closed is true so no messages are delivered after close; reference the
_fireMessage method and the _closed property (and existing close()/_fireClose()
handlers) when adding the early-return guard to prevent delivering messages
post-close.
In `@test/uts/realtime-audit.md`:
- Around line 163-173: The opening sentence in the "Resolution Status" section
inaccurately claims "All findings have been addressed"; update it to accurately
reflect that RTN15h2 remains out of scope (e.g., "Most findings have been
addressed; RTN15h2 remains out of scope") so the summary matches the table
entries for RTL4g, RTN15c7, RTN14g, RTN15h2 and RTL17 in
test/uts/realtime-audit.md.
In `@test/uts/realtime/auth/connection_auth.test.ts`:
- Around line 106-116: Both terminal listeners ('disconnected' and 'failed' on
client.connection) call done(), which can cause multiple completions; add a
local boolean guard (e.g., doneCalled or terminalHandled) in the test scope and
check/set it at the start of each callback so done() is executed only once, and
optionally remove the other listener or perform any cleanup after setting the
guard; reference the callbacks attached to
client.connection.once('disconnected', ...) and client.connection.once('failed',
...) and the existing connectionAttempted variable when implementing the guard.
In `@test/uts/realtime/channels/channel_annotations.test.ts`:
- Around line 14-15: The test imports Ably but not the annotations plugin, so
(Ably as any).RealtimeAnnotations is undefined; update the imports to explicitly
import the annotations plugin (the same way installMockWebSocket/restoreAll are
imported) and ensure when constructing clients in this file you pass the plugin
(e.g., supply RealtimeAnnotations to the Ably client constructor where currently
using (Ably as any).RealtimeAnnotations); apply the same explicit import/use
replacement for the other client constructions around lines 64-69 as noted so
every client in channel_annotations.test.ts receives the actual
RealtimeAnnotations plugin.
In `@test/uts/realtime/channels/channel_attributes.test.ts`:
- Around line 75-86: Register the 'failed' event waiter on the Channel instance
before sending the ERROR frame: move the channel.once('failed', resolve) (or
equivalent listener registration) to occur prior to calling
mock.active_connection!.send_to_client(...) so the test will not miss a
synchronously-emitted 'failed' event; apply the same change for the later
FAILED-path test (the block around lines 267-277) and ensure client.close()
remains after awaiting the resolved promise.
In `@test/uts/realtime/channels/channel_detach.test.ts`:
- Around line 506-513: The test starts a pending promise via channel.attach()
(attachPromise) but doesn't await or handle its rejection before calling
channel.detach() and client.close(), which leads to an unhandled rejection;
update the test to handle attachPromise by awaiting it or adding a .catch()
(e.g., await attachPromise.catch(() => {}) or assert the expected rejection)
after calling channel.detach() so the pending attach promise is settled and
won't produce unhandled rejections; ensure you reference the existing
attachPromise variable, the channel.attach() call, the channel.detach() call,
and client.close() in your fix.
In `@test/uts/realtime/channels/channel_options.test.ts`:
- Around line 427-442: The test leaves channel.attach() pending which can cause
unhandled promise rejections; update the test to capture and handle that promise
by either awaiting channel.attach() (or storing its return and adding .catch(()
=> {})/await with try/catch) before calling client.close(), ensuring any
rejection is caught; apply this change around the channel.attach() call used
with client.channels.get('test-RTS3c1-attaching', { modes: [...] }) and ensure
the attach promise is resolved/rejected (and cleaned up) prior to
client.close().
In `@test/uts/realtime/channels/channel_server_initiated_detach.test.ts`:
- Around line 433-463: After the server sends DETACHED for channel 'test-RTL13c'
you must drive the channel from attaching into suspended before disconnecting so
the suspended retry timer is created and can be tested; after
mock.active_connection!.send_to_client(...) complete the attach attempt by
advancing the fake clock enough to trigger the attach timeout/suspension (e.g.,
call clock.tickAsync or repeated clock.tick until the channel transitions to
suspended), then capture attachCount, then call
mock.active_connection!.simulate_disconnect() and advance time to ensure no
retry occurs; update the flow around attachCount/attachCountAfterDetach to
perform the suspend transition between the DETACHED send and the
simulate_disconnect call so the retry-timer path is exercised.
In `@test/uts/realtime/channels/channel_subscribe.test.ts`:
- Around line 797-803: The test calls channel.subscribe(...) which returns a
Promise that will reject when the implicit attach fails, causing an unhandled
rejection; update the test to handle that Promise (either await the subscribe
call inside a try/catch or append .catch(() => {}) to it) before waiting for the
channel to enter 'failed' (references: channel.subscribe, channel.state,
channel.once('failed')). Ensure the rejection is consumed so the test waits for
the failed event without producing unhandled rejections.
In `@test/uts/realtime/channels/channel_when_state.test.ts`:
- Around line 154-177: The test currently measures one-shot behavior via
channel.once('attached') (attachCount) instead of the whenState API; change the
test so the counter is incremented from whenState's resolution instead of
channel.once. Specifically, replace the attachCount increment in the
channel.once handler with increment inside whenStatePromise.then(...), assert
that that whenState-derived counter increments exactly once after the first
attach, then perform detach and re-attach and assert the whenState-derived
counter remains 1 (and optionally verify there are no leftover 'attached'
listeners on channel after whenState resolves by checking
channel.listeners('attached').length). Use the existing symbols whenState,
whenStatePromise, channel.attach, channel.detach and channel.listeners to locate
and update the code.
In `@test/uts/realtime/channels/channels_collection.test.ts`:
- Around line 213-218: The test attaches the 'detached' listener after calling
client.channels.release('test-RTS4a-attached'), which can miss a synchronous
detach; to fix, register the listener on channel (channel.once('detached', ...))
before invoking client.channels.release(...) so the event cannot be missed, then
call release and await the promise/flush as before; ensure the promise used to
await the detach is the same one created prior to calling release.
In `@test/uts/realtime/client/realtime_request.test.ts`:
- Around line 49-71: Update the RTC9 test to assert the actual request body was
sent: after capturing the request in the onRequest handler (variable captured),
add an assertion that captured[0].body deeply equals the payload passed to
Ably.Realtime.request (the object { name: 'test' }) so the test verifies the
POST payload was not dropped or mangled when calling request().
In `@test/uts/realtime/connection/connection_failures.test.ts`:
- Around line 70-76: Change the final done-based event handlers from using
client.connection.on('connected', ...) to client.connection.once('connected',
...) so the completion callback cannot fire multiple times; locate each
occurrence of client.connection.on('connected', ...) in the test file (the final
assertion blocks that call done()) and replace with
client.connection.once('connected', ...), keeping the same assertion logic
(checking client.connection.state, client.connection.id, connectionAttemptCount,
sawDisconnected, etc.) to avoid flaky tests.
- Around line 250-262: The helper respond_with_connected in
test/uts/mock_websocket.ts currently types its payload as Partial<typeof
DEFAULT_CONNECTED> which excludes the optional error property and causes TS2345;
update its signature to accept a payload type compatible with ProtocolMessage's
CONNECTED shape (e.g., Partial<typeof DEFAULT_CONNECTED> & { error?: ErrorInfo }
or directly ProtocolMessage['CONNECTED'] as partial) so tests can pass an error
field without casting, and adjust any callers or imports to use ErrorInfo and
ProtocolMessage symbols to ensure typing consistency.
In `@test/uts/realtime/connection/connection_open_failures.test.ts`:
- Around line 439-448: The test installs a persistent client.connection.on
handler that may call done() multiple times when the connection enters
'disconnected' repeatedly; change the handler to be single-shot by using the
one-time subscription (e.g., client.connection.once) or unsubscribe/remove the
listener after it fires (or guard with a local boolean like handled) so that
when change.current === 'disconnected' and authCallbackCount >= 2 you assert
client.connection.state and errorReason and call done() exactly once; target the
handler registered in the test (the client.connection.on callback that pushes to
stateChanges and checks authCallbackCount/done()).
In `@test/uts/realtime/connection/connection_ping.test.ts`:
- Around line 54-60: The test uses an async callback registered with
client.connection.once('connected', async () => { ... done(); }) which can
detach promise rejections from Mocha and cause timeouts; change the test to an
async function (remove done) and explicitly await the connection state before
calling client.connection.ping(), then perform assertions and call
client.close() afterwards. Replace the event-listener pattern around
client.connection.once('connected', ...) with an awaited helper or promise that
resolves when the connection is ready, then await client.connection.ping(),
assert the numeric duration (expect(...).to.be.a('number') and
.to.be.at.least(0)), and return from the async test; apply the same change to
the other occurrences that use client.connection.once with done.
In `@test/uts/realtime/connection/fallback_hosts.test.ts`:
- Around line 290-335: The test must assert ordering of events: modify the
MockWebSocket onConnectionAttempt and MockHttpClient handlers to push ordered
markers into a shared events array (e.g. push `ws:<hostname>` from the WebSocket
handler when each attempt occurs and push `http:internet-up` from the HTTP
handler when the connectivity check request is seen), then after connected
verify that events contains the first `ws:<primary-host>` entry, then an
`http:internet-up` entry, then a `ws:<fallback-host>` entry; use the existing
connectionHosts array and httpMock.captured_requests to derive expected host
names and assert the index ordering (first ws attempt index < http index < first
fallback ws index) to prove the connectivity check ran before fallback.
In `@test/uts/realtime/connection/heartbeat.test.ts`:
- Around line 42-51: The test mutates the global flag
Platform.Config.useProtocolHeartbeats and only restores it on the success path;
wrap the onConnectionAttempt body in a try/finally so savedUseProtocolHeartbeats
is always restored: capture the original value into savedUseProtocolHeartbeats,
then in onConnectionAttempt (the callback passed to new MockWebSocket), perform
the assertion and conn.respond_with_connected() inside a try block and put
Platform.Config.useProtocolHeartbeats = savedUseProtocolHeartbeats in the
finally block (call done() after restore) to guarantee cleanup even if the
expect() fails.
---
Nitpick comments:
In `@test/uts/realtime/channels/channel_subscribe.test.ts`:
- Around line 283-307: The test "RTL7f - echoMessages false sets echo param in
URL" is duplicated; remove the redundant duplicate or merge their expectations
so the scenario is only defined once. Locate the duplicate 'it' block with the
exact title "RTL7f - echoMessages false sets echo param in URL" in the
channel_subscribe.test.ts test file, delete one of the two identical test cases
(or combine any differing assertions into a single 'it' block) and ensure the
remaining test still creates MockWebSocket, constructs the Ably.Realtime client
with echoMessages: false, waits for connection, asserts
connUrl.searchParams.get('echo') === 'false', and closes the client.
In `@test/uts/realtime/client/client_options.test.ts`:
- Around line 48-53: The test uses a realistic-looking JWT literal in the 'RSC1c
- token string (JWT format)' test: replace the current jwt variable value with
an obviously fake three-segment token (e.g. "aaa.bbb.ccc" or
"header.payload.signature") so it preserves the three-dot JWT shape used by the
Ably.Realtime instantiation and the assertion on client.options.token but avoids
using a realistic secret-like string; update the jwt constant in that test and
leave the Ably.Realtime({ token: jwt, autoConnect: false }) and
expect(client.options.token).to.equal(jwt) lines 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: 460e0ff7-fb3b-4c57-96c7-58b8e8d7a55b
📒 Files selected for processing (55)
test/uts/helpers.tstest/uts/mock_websocket.tstest/uts/realtime-audit.mdtest/uts/realtime/auth/connection_auth.test.tstest/uts/realtime/auth/realtime_authorize.test.tstest/uts/realtime/channels/channel_additional_attached.test.tstest/uts/realtime/channels/channel_annotations.test.tstest/uts/realtime/channels/channel_attach.test.tstest/uts/realtime/channels/channel_attributes.test.tstest/uts/realtime/channels/channel_connection_state.test.tstest/uts/realtime/channels/channel_delta_decoding.test.tstest/uts/realtime/channels/channel_detach.test.tstest/uts/realtime/channels/channel_error.test.tstest/uts/realtime/channels/channel_get_message.test.tstest/uts/realtime/channels/channel_history.test.tstest/uts/realtime/channels/channel_message_versions.test.tstest/uts/realtime/channels/channel_options.test.tstest/uts/realtime/channels/channel_properties.test.tstest/uts/realtime/channels/channel_publish.test.tstest/uts/realtime/channels/channel_server_initiated_detach.test.tstest/uts/realtime/channels/channel_state_events.test.tstest/uts/realtime/channels/channel_subscribe.test.tstest/uts/realtime/channels/channel_update_delete_message.test.tstest/uts/realtime/channels/channel_when_state.test.tstest/uts/realtime/channels/channels_collection.test.tstest/uts/realtime/channels/message_field_population.test.tstest/uts/realtime/client/client_options.test.tstest/uts/realtime/client/realtime_client.test.tstest/uts/realtime/client/realtime_request.test.tstest/uts/realtime/client/realtime_stats.test.tstest/uts/realtime/client/realtime_time.test.tstest/uts/realtime/client/realtime_timeouts.test.tstest/uts/realtime/connection/auto_connect.test.tstest/uts/realtime/connection/connection_failures.test.tstest/uts/realtime/connection/connection_id_key.test.tstest/uts/realtime/connection/connection_open_failures.test.tstest/uts/realtime/connection/connection_ping.test.tstest/uts/realtime/connection/error_reason.test.tstest/uts/realtime/connection/fallback_hosts.test.tstest/uts/realtime/connection/heartbeat.test.tstest/uts/realtime/connection/server_initiated_reauth.test.tstest/uts/realtime/connection/update_events.test.tstest/uts/realtime/connection/when_state.test.tstest/uts/realtime/presence/local_presence_map.test.tstest/uts/realtime/presence/presence_map.test.tstest/uts/realtime/presence/presence_sync.test.tstest/uts/realtime/presence/realtime_presence_channel_state.test.tstest/uts/realtime/presence/realtime_presence_enter.test.tstest/uts/realtime/presence/realtime_presence_get.test.tstest/uts/realtime/presence/realtime_presence_history.test.tstest/uts/realtime/presence/realtime_presence_reentry.test.tstest/uts/realtime/presence/realtime_presence_subscribe.test.tstest/uts/realtime/time.test.tstest/uts/rest/auth/revoke_tokens.test.tstest/uts/rest/types/message_types.test.ts
Comment on lines
+41
to
+50
| const _origPlatformSetTimeout = Platform.Config.setTimeout; | ||
| const _origPlatformClearTimeout = Platform.Config.clearTimeout; | ||
| Platform.Config.setTimeout = function (fn: any, ms?: number, ...args: any[]) { | ||
| const timer = _origPlatformSetTimeout.call(this, fn, ms, ...args); | ||
| _allPlatformTimers.add(timer); | ||
| return timer; | ||
| } as any; | ||
| Platform.Config.clearTimeout = function (timer: any) { | ||
| _allPlatformTimers.delete(timer); | ||
| return _origPlatformClearTimeout.call(this, timer); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the test/uts/helpers.ts file and read the relevant lines
if [ -f "test/uts/helpers.ts" ]; then
echo "=== File: test/uts/helpers.ts (lines 35-55) ==="
sed -n '35,55p' test/uts/helpers.ts | cat -n
else
echo "File not found at test/uts/helpers.ts"
git ls-files | grep -i helpers.ts
fiRepository: ably/ably-js
Length of output: 1182
🏁 Script executed:
#!/bin/bash
# Find where Platform.Config is defined
echo "=== Searching for Platform.Config definition ==="
rg "Platform\.Config" -A 5 -B 5 --type ts --type tsx | head -100Repository: ably/ably-js
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Look for IPlatformConfig or similar type definitions
echo "=== Searching for Platform type definitions ==="
rg "interface.*Platform|type.*Platform" --type ts --type tsx | head -30Repository: ably/ably-js
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Try searching for Platform config without specifying ts/tsx
echo "=== Searching for Platform.Config definition ==="
rg "Platform" -t ts -A 3 | grep -E "(Config|setTimeout|clearTimeout)" | head -50Repository: ably/ably-js
Length of output: 4588
🏁 Script executed:
#!/bin/bash
# Search for interface or type definitions containing setTimeout
echo "=== Searching for setTimeout definitions ==="
rg "setTimeout|clearTimeout" -t ts -B 3 -A 1 | grep -E "(interface|type|setTimeout|clearTimeout|\s+:)" | head -80Repository: ably/ably-js
Length of output: 8166
🏁 Script executed:
#!/bin/bash
# Look for tsconfig.json or compilation setup
echo "=== Looking for TypeScript configuration ==="
find . -name "tsconfig.json" -o -name "tsc" | head -10Repository: ably/ably-js
Length of output: 507
🏁 Script executed:
#!/bin/bash
# Check if there's a dist/es directory or build output that shows compilation errors
echo "=== Checking for compilation errors or issue tracking ==="
rg "TS2683|TS2556" -t ts -B 5 -A 5 | head -50Repository: ably/ably-js
Length of output: 114
🏁 Script executed:
#!/bin/bash
cat -n src/common/types/IPlatformConfig.d.ts | head -50Repository: ably/ably-js
Length of output: 2183
Fix the Platform.Config wrapper signatures to match IPlatformConfig.
The wrapper functions use as any to bypass type errors. The interface defines setTimeout: (handler: () => void, timeout?: number) => ... and clearTimeout: (id: ... | null | undefined) => void, but the wrapper adds a variadic ...args: any[], uses untyped this context, and returns a value from clearTimeout. Remove the variadic parameters, switch to arrow functions, and eliminate the unnecessary return in clearTimeout.
Proposed fix
-Platform.Config.setTimeout = function (fn: any, ms?: number, ...args: any[]) {
- const timer = _origPlatformSetTimeout.call(this, fn, ms, ...args);
+Platform.Config.setTimeout = ((fn: () => void, ms?: number) => {
+ const timer = _origPlatformSetTimeout(fn, ms);
_allPlatformTimers.add(timer);
return timer;
-} as any;
-Platform.Config.clearTimeout = function (timer: any) {
+}) as typeof Platform.Config.setTimeout;
+Platform.Config.clearTimeout = ((timer) => {
_allPlatformTimers.delete(timer);
- return _origPlatformClearTimeout.call(this, timer);
-} as any;
+ _origPlatformClearTimeout(timer);
+}) as typeof Platform.Config.clearTimeout;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const _origPlatformSetTimeout = Platform.Config.setTimeout; | |
| const _origPlatformClearTimeout = Platform.Config.clearTimeout; | |
| Platform.Config.setTimeout = function (fn: any, ms?: number, ...args: any[]) { | |
| const timer = _origPlatformSetTimeout.call(this, fn, ms, ...args); | |
| _allPlatformTimers.add(timer); | |
| return timer; | |
| } as any; | |
| Platform.Config.clearTimeout = function (timer: any) { | |
| _allPlatformTimers.delete(timer); | |
| return _origPlatformClearTimeout.call(this, timer); | |
| const _origPlatformSetTimeout = Platform.Config.setTimeout; | |
| const _origPlatformClearTimeout = Platform.Config.clearTimeout; | |
| Platform.Config.setTimeout = ((fn: () => void, ms?: number) => { | |
| const timer = _origPlatformSetTimeout(fn, ms); | |
| _allPlatformTimers.add(timer); | |
| return timer; | |
| }) as typeof Platform.Config.setTimeout; | |
| Platform.Config.clearTimeout = ((timer) => { | |
| _allPlatformTimers.delete(timer); | |
| _origPlatformClearTimeout(timer); | |
| }) as typeof Platform.Config.clearTimeout; |
🧰 Tools
🪛 GitHub Actions: API Reference
[error] 44-44: TypeScript compilation error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
[error] 44-44: TypeScript compilation error TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.
[error] 50-50: TypeScript compilation error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
🪛 GitHub Actions: Bundle size report
[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.
[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
🪛 GitHub Actions: Lint
[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.
[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
🪛 GitHub Actions: Spec coverage report
[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.
[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
🪛 GitHub Actions: Test (react hooks)
[error] 44-44: TypeScript TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
[error] 44-44: TypeScript TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.
[error] 50-50: TypeScript TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
🪛 GitHub Actions: Test NodeJS
[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.
[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
🪛 GitHub Actions: Test NPM package
[error] 44-44: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
[error] 44-44: TS2556: A spread argument must either have a tuple type or be passed to a rest parameter.
[error] 50-50: TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/helpers.ts` around lines 41 - 50, The Platform.Config wrapper
functions currently diverge from IPlatformConfig and use unsafe casts; update
the wrappers for Platform.Config.setTimeout and Platform.Config.clearTimeout to
match the interface by removing the variadic ...args and using arrow functions
so they inherit the correct lexical this, call
_origPlatformSetTimeout/_origPlatformClearTimeout with the expected (handler,
timeout) signature, add proper parameter types (handler: () => void, timeout?:
number) and ensure clearTimeout does not return a value (void) while still
deleting the timer from _allPlatformTimers; keep references to
_origPlatformSetTimeout, _origPlatformClearTimeout and _allPlatformTimers to
maintain behavior.
Comment on lines
+109
to
+115
| _fireMessage(protocolMessage: any): void { | ||
| process.nextTick(() => { | ||
| if (this.onmessage) { | ||
| this.onmessage({ data: JSON.stringify(protocolMessage) }); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Don't deliver messages after the socket is closed.
_fireMessage() ignores _closed, so tests can still inject protocol frames after close()/_fireClose(). That diverges from real WebSocket behavior and can mask connection-lifecycle bugs.
Proposed fix
_fireMessage(protocolMessage: any): void {
process.nextTick(() => {
- if (this.onmessage) {
+ if (!this._closed && this.onmessage) {
this.onmessage({ data: JSON.stringify(protocolMessage) });
}
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _fireMessage(protocolMessage: any): void { | |
| process.nextTick(() => { | |
| if (this.onmessage) { | |
| this.onmessage({ data: JSON.stringify(protocolMessage) }); | |
| } | |
| }); | |
| } | |
| _fireMessage(protocolMessage: any): void { | |
| process.nextTick(() => { | |
| if (!this._closed && this.onmessage) { | |
| this.onmessage({ data: JSON.stringify(protocolMessage) }); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/mock_websocket.ts` around lines 109 - 115, _fireMessage currently
ignores the socket closed state and will emit messages even after close, which
diverges from real WebSocket behavior; modify _fireMessage to check the internal
_closed flag (and/or the close()/_fireClose() state) before calling
this.onmessage and bail out if _closed is true so no messages are delivered
after close; reference the _fireMessage method and the _closed property (and
existing close()/_fireClose() handlers) when adding the early-return guard to
prevent delivering messages post-close.
Comment on lines
+294
to
+320
| _onNewWebSocket(url: string): MockWSInstance { | ||
| // Handle connectivity checker — auto-respond without involving test handlers | ||
| if (url.includes(WS_CONNECTIVITY_CHECK)) { | ||
| const ws = new MockWSInstance(url, this); | ||
| process.nextTick(() => { | ||
| if (ws.onopen) ws.onopen(); | ||
| process.nextTick(() => { | ||
| if (ws.onclose) ws.onclose({ code: 1000, wasClean: true }); | ||
| }); | ||
| }); | ||
| return ws; | ||
| } | ||
|
|
||
| const ws = new MockWSInstance(url, this); | ||
| const parsedUrl = new URL(url); | ||
| const conn = new PendingWSConnection(ws, parsedUrl); | ||
| this.connect_attempts.push(conn); | ||
|
|
||
| // Notify handler or waiter | ||
| if (this.onConnectionAttempt) { | ||
| this.onConnectionAttempt(conn); | ||
| } else if (this._connectionWaiters.length > 0) { | ||
| this._connectionWaiters.shift()!(conn); | ||
| } | ||
| // If neither handler nor waiter, the connection hangs until test responds | ||
|
|
||
| return ws; |
There was a problem hiding this comment.
Track the active socket inside the mock instead of relying on each test to do it.
send_to_client() depends on this.active_connection, but _onNewWebSocket() never assigns it. Any suite that uses the helper directly will hit "No active connection" despite having an open mock socket.
Proposed fix
const ws = new MockWSInstance(url, this);
const parsedUrl = new URL(url);
const conn = new PendingWSConnection(ws, parsedUrl);
this.connect_attempts.push(conn);
+ this.active_connection = conn;
@@
_onClientClose(_ws: MockWSInstance, code?: number, reason?: string): void {
+ if (this.active_connection?.ws === _ws) {
+ this.active_connection = null;
+ }
if (this._closeWaiters.length > 0) {
this._closeWaiters.shift()!({ code, reason });
}
}Also applies to: 340-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/mock_websocket.ts` around lines 294 - 320, The mock never records
the pending connection into this.active_connection, so send_to_client() can’t
find an active connection; when creating a PendingWSConnection in
_onNewWebSocket (and the other similar block at lines ~340-344) assign the new
PendingWSConnection to this.active_connection (e.g., this.active_connection =
conn) after constructing conn so tests using send_to_client() will see the
active connection; keep existing notifications to
onConnectionAttempt/_connectionWaiters unchanged.
Comment on lines
+163
to
+173
| ## Resolution Status | ||
|
|
||
| All findings have been addressed. UTS specs fixed, ably-js tests updated. Results: | ||
|
|
||
| | # | Finding | Resolution | ably-js | | ||
| |---|---------|-----------|---------| | ||
| | 1 | RTL4g mislabeled | Test fixed to assert spec behavior (errorReason cleared) | **FAILS** — ably-js does not clear errorReason on re-attach from FAILED | | ||
| | 2 | RTN15c7 missing assertions | Added error field to mock + errorReason/event assertions | **PASSES** | | ||
| | 3 | RTN14g wrong scenario | Restructured to send ERROR during connection opening | **PASSES** | | ||
| | 4 | RTN15h2 failure sub-case | Not added (out of scope for this fix round) | — | | ||
| | 5 | RTL17 missing | Test added | **PASSES** — ably-js correctly drops messages when not ATTACHED | |
There was a problem hiding this comment.
The resolution summary contradicts the table.
Line 165 says all findings were addressed, but Line 172 says the RTN15h2 sub-case was not added and remains out of scope. Please soften the opening sentence so the status reads accurately.
Suggested wording
-## Resolution Status
-
-All findings have been addressed. UTS specs fixed, ably-js tests updated. Results:
+## Resolution Status
+
+Most findings in this audit have been addressed. UTS specs fixed, ably-js tests updated. Results:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Resolution Status | |
| All findings have been addressed. UTS specs fixed, ably-js tests updated. Results: | |
| | # | Finding | Resolution | ably-js | | |
| |---|---------|-----------|---------| | |
| | 1 | RTL4g mislabeled | Test fixed to assert spec behavior (errorReason cleared) | **FAILS** — ably-js does not clear errorReason on re-attach from FAILED | | |
| | 2 | RTN15c7 missing assertions | Added error field to mock + errorReason/event assertions | **PASSES** | | |
| | 3 | RTN14g wrong scenario | Restructured to send ERROR during connection opening | **PASSES** | | |
| | 4 | RTN15h2 failure sub-case | Not added (out of scope for this fix round) | — | | |
| | 5 | RTL17 missing | Test added | **PASSES** — ably-js correctly drops messages when not ATTACHED | | |
| ## Resolution Status | |
| Most findings in this audit have been addressed. UTS specs fixed, ably-js tests updated. Results: | |
| | # | Finding | Resolution | ably-js | | |
| |---|---------|-----------|---------| | |
| | 1 | RTL4g mislabeled | Test fixed to assert spec behavior (errorReason cleared) | **FAILS** — ably-js does not clear errorReason on re-attach from FAILED | | |
| | 2 | RTN15c7 missing assertions | Added error field to mock + errorReason/event assertions | **PASSES** | | |
| | 3 | RTN14g wrong scenario | Restructured to send ERROR during connection opening | **PASSES** | | |
| | 4 | RTN15h2 failure sub-case | Not added (out of scope for this fix round) | — | | |
| | 5 | RTL17 missing | Test added | **PASSES** — ably-js correctly drops messages when not ATTACHED | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime-audit.md` around lines 163 - 173, The opening sentence in
the "Resolution Status" section inaccurately claims "All findings have been
addressed"; update it to accurately reflect that RTN15h2 remains out of scope
(e.g., "Most findings have been addressed; RTN15h2 remains out of scope") so the
summary matches the table entries for RTL4g, RTN15c7, RTN14g, RTN15h2 and RTL17
in test/uts/realtime-audit.md.
Comment on lines
+106
to
+116
| client.connection.once('disconnected', () => { | ||
| expect(connectionAttempted).to.be.false; | ||
| expect(client.connection.errorReason).to.not.be.null; | ||
| done(); | ||
| }); | ||
|
|
||
| client.connection.once('failed', () => { | ||
| expect(connectionAttempted).to.be.false; | ||
| expect(client.connection.errorReason).to.not.be.null; | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
Guard against double done() across terminal listeners
Line 106 and Line 112 both call done(). If both states occur across retries, this test can fail nondeterministically due to multiple completions.
Suggested fix
- client.connection.once('disconnected', () => {
- expect(connectionAttempted).to.be.false;
- expect(client.connection.errorReason).to.not.be.null;
- done();
- });
-
- client.connection.once('failed', () => {
- expect(connectionAttempted).to.be.false;
- expect(client.connection.errorReason).to.not.be.null;
- done();
- });
+ let finished = false;
+ const assertAndDone = () => {
+ if (finished) return;
+ finished = true;
+ expect(connectionAttempted).to.be.false;
+ expect(client.connection.errorReason).to.not.be.null;
+ done();
+ };
+ client.connection.once('disconnected', assertAndDone);
+ client.connection.once('failed', assertAndDone);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| client.connection.once('disconnected', () => { | |
| expect(connectionAttempted).to.be.false; | |
| expect(client.connection.errorReason).to.not.be.null; | |
| done(); | |
| }); | |
| client.connection.once('failed', () => { | |
| expect(connectionAttempted).to.be.false; | |
| expect(client.connection.errorReason).to.not.be.null; | |
| done(); | |
| }); | |
| let finished = false; | |
| const assertAndDone = () => { | |
| if (finished) return; | |
| finished = true; | |
| expect(connectionAttempted).to.be.false; | |
| expect(client.connection.errorReason).to.not.be.null; | |
| done(); | |
| }; | |
| client.connection.once('disconnected', assertAndDone); | |
| client.connection.once('failed', assertAndDone); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/auth/connection_auth.test.ts` around lines 106 - 116, Both
terminal listeners ('disconnected' and 'failed' on client.connection) call
done(), which can cause multiple completions; add a local boolean guard (e.g.,
doneCalled or terminalHandled) in the test scope and check/set it at the start
of each callback so done() is executed only once, and optionally remove the
other listener or perform any cleanup after setting the guard; reference the
callbacks attached to client.connection.once('disconnected', ...) and
client.connection.once('failed', ...) and the existing connectionAttempted
variable when implementing the guard.
Comment on lines
+250
to
+262
| conn.respond_with_connected({ | ||
| connectionId: 'connection-2', | ||
| connectionDetails: { | ||
| connectionKey: 'key-2', | ||
| maxIdleInterval: 15000, | ||
| connectionStateTtl: 120000, | ||
| } as any, | ||
| error: { | ||
| code: 80008, | ||
| statusCode: 400, | ||
| message: 'Unable to recover connection', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify helper signature and failing call shape
rg -n "respond_with_connected\\(" test/uts/mock_websocket.ts -C4
rg -n "respond_with_connected\\(\\{" test/uts/realtime/connection/connection_failures.test.ts -C4Repository: ably/ably-js
Length of output: 7801
🏁 Script executed:
# Get full content of lines 240-265 to see complete code block
sed -n '240,265p' test/uts/realtime/connection/connection_failures.test.ts | cat -nRepository: ably/ably-js
Length of output: 1022
🏁 Script executed:
# Find DEFAULT_CONNECTED definition to check if error property is allowed
rg -n "DEFAULT_CONNECTED\s*=" test/uts/mock_websocket.ts -A 20Repository: ably/ably-js
Length of output: 739
🏁 Script executed:
# Check if there are TypeScript errors in the actual type definitions
rg -n "interface.*CONNECTED|type.*CONNECTED" test/uts/mock_websocket.ts -B2 -A5Repository: ably/ably-js
Length of output: 413
🏁 Script executed:
# Search for protocol message types and CONNECTED definition
rg -n "type.*CONNECTED|interface.*Message" test/uts/ --type=ts -A3 | head -50Repository: ably/ably-js
Length of output: 395
🏁 Script executed:
# Look for any other references to error in CONNECTED context
rg -n "error.*code.*80008|80008" . --type=tsRepository: ably/ably-js
Length of output: 398
🏁 Script executed:
# Check the actual Ably protocol or message type definitions
rg -n "CONNECTED.*=.*4|action.*4" . --type=ts -B2 -A2 | head -40Repository: ably/ably-js
Length of output: 3946
🏁 Script executed:
# Look for Ably protocol message definitions or type definitions
rg -n "Protocol|ProtocolMessage|CONNECTED" src/ --type=ts | grep -i "type\|interface" | head -20Repository: ably/ably-js
Length of output: 2281
🏁 Script executed:
# Check if there are any type definitions for error in connection context
rg -n "error.*code.*statusCode|statusCode.*code.*error" . --type=ts -B2 -A2 | head -40Repository: ably/ably-js
Length of output: 3116
🏁 Script executed:
# Look for the actual message protocol schema or definition
fd -e ts -e d.ts . | xargs grep -l "CONNECTED\|ProtocolMessage" | head -5Repository: ably/ably-js
Length of output: 228
🏁 Script executed:
# Check ProtocolMessage class definition to see if error is a property
sed -n '154,207p' src/common/lib/types/protocolmessage.ts | cat -nRepository: ably/ably-js
Length of output: 1955
🏁 Script executed:
# Also check the protocolmessagecommon.ts for CONNECTED constant
cat -n src/common/lib/types/protocolmessagecommon.tsRepository: ably/ably-js
Length of output: 2125
Fix compile-blocking respond_with_connected payload typing to allow error field.
The test at lines 250-262 passes an error property to respond_with_connected(), but the helper's type signature restricts the payload to Partial<typeof DEFAULT_CONNECTED>, which excludes error. While ProtocolMessage itself supports error?: ErrorInfo (a valid CONNECTED protocol field), the mock helper's type definition doesn't allow it, causing TS2345.
Workaround: cast to any
- conn.respond_with_connected({
+ conn.respond_with_connected({
connectionId: 'connection-2',
connectionDetails: {
connectionKey: 'key-2',
maxIdleInterval: 15000,
connectionStateTtl: 120000,
} as any,
error: {
code: 80008,
statusCode: 400,
message: 'Unable to recover connection',
},
- });
+ } as any);Better fix: Update respond_with_connected signature in test/uts/mock_websocket.ts to include error in the allowed payload shape.
🧰 Tools
🪛 GitHub Actions: API Reference
[error] 257-257: TypeScript compilation error TS2345: Argument object has unknown property 'error' (not in expected type).
🪛 GitHub Actions: Bundle size report
[error] 257-261: TS2345: Argument object includes property 'error' which does not exist on the expected parameter type.
🪛 GitHub Actions: Lint
[error] 257-261: TS2345: Argument object contains unknown property 'error'; 'error' does not exist in the expected parameter type.
🪛 GitHub Actions: Spec coverage report
[error] 257-257: TS2345: Argument object includes unknown property 'error'; 'error' does not exist in the expected parameter type.
🪛 GitHub Actions: Test (react hooks)
[error] 257-261: TypeScript TS2345: Object literal includes unknown property 'error'; 'error' does not exist on the expected parameter type.
🪛 GitHub Actions: Test NodeJS
[error] 257-257: TS2345: Argument object includes unknown property 'error' that does not exist in the expected parameter type.
🪛 GitHub Actions: Test NPM package
[error] 257-257: TS2345: Object literal has unknown property 'error' when passed to a function expecting Partial<...> & { ... }.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/connection/connection_failures.test.ts` around lines 250 -
262, The helper respond_with_connected in test/uts/mock_websocket.ts currently
types its payload as Partial<typeof DEFAULT_CONNECTED> which excludes the
optional error property and causes TS2345; update its signature to accept a
payload type compatible with ProtocolMessage's CONNECTED shape (e.g.,
Partial<typeof DEFAULT_CONNECTED> & { error?: ErrorInfo } or directly
ProtocolMessage['CONNECTED'] as partial) so tests can pass an error field
without casting, and adjust any callers or imports to use ErrorInfo and
ProtocolMessage symbols to ensure typing consistency.
Comment on lines
+439
to
+448
| const stateChanges: string[] = []; | ||
| client.connection.on((change: any) => { | ||
| stateChanges.push(change.current); | ||
|
|
||
| if (change.current === 'disconnected' && authCallbackCount >= 2) { | ||
| expect(client.connection.state).to.equal('disconnected'); | ||
| expect(client.connection.errorReason).to.not.be.null; | ||
| done(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Make completion single-shot in renewal-failure state tracking
Line 440 uses a persistent connection.on handler that can hit the Line 446 done() branch more than once on subsequent disconnected transitions.
Suggested fix
- const stateChanges: string[] = [];
- client.connection.on((change: any) => {
+ const stateChanges: string[] = [];
+ let finished = false;
+ const onChange = (change: any) => {
stateChanges.push(change.current);
- if (change.current === 'disconnected' && authCallbackCount >= 2) {
+ if (!finished && change.current === 'disconnected' && authCallbackCount >= 2) {
+ finished = true;
+ client.connection.off(onChange);
expect(client.connection.state).to.equal('disconnected');
expect(client.connection.errorReason).to.not.be.null;
done();
}
- });
+ };
+ client.connection.on(onChange);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const stateChanges: string[] = []; | |
| client.connection.on((change: any) => { | |
| stateChanges.push(change.current); | |
| if (change.current === 'disconnected' && authCallbackCount >= 2) { | |
| expect(client.connection.state).to.equal('disconnected'); | |
| expect(client.connection.errorReason).to.not.be.null; | |
| done(); | |
| } | |
| }); | |
| const stateChanges: string[] = []; | |
| let finished = false; | |
| const onChange = (change: any) => { | |
| stateChanges.push(change.current); | |
| if (!finished && change.current === 'disconnected' && authCallbackCount >= 2) { | |
| finished = true; | |
| client.connection.off(onChange); | |
| expect(client.connection.state).to.equal('disconnected'); | |
| expect(client.connection.errorReason).to.not.be.null; | |
| done(); | |
| } | |
| }; | |
| client.connection.on(onChange); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/connection/connection_open_failures.test.ts` around lines
439 - 448, The test installs a persistent client.connection.on handler that may
call done() multiple times when the connection enters 'disconnected' repeatedly;
change the handler to be single-shot by using the one-time subscription (e.g.,
client.connection.once) or unsubscribe/remove the listener after it fires (or
guard with a local boolean like handled) so that when change.current ===
'disconnected' and authCallbackCount >= 2 you assert client.connection.state and
errorReason and call done() exactly once; target the handler registered in the
test (the client.connection.on callback that pushes to stateChanges and checks
authCallbackCount/done()).
Comment on lines
+54
to
+60
| client.connection.once('connected', async () => { | ||
| const duration = await client.connection.ping(); | ||
| expect(duration).to.be.a('number'); | ||
| expect(duration).to.be.at.least(0); | ||
| client.close(); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/uts/realtime/connection/connection_ping.test.ts | head -100Repository: ably/ably-js
Length of output: 3752
🏁 Script executed:
cat -n test/uts/realtime/connection/connection_ping.test.ts | sed -n '95,330p'Repository: ably/ably-js
Length of output: 7906
Avoid done callback tests with async event listeners.
If ping() rejects or an assertion throws inside the async listener, the returned promise is detached from Mocha and done() is never called, degrading into a timeout failure that's harder to diagnose. Convert to async function () tests and await the connection state explicitly instead of using event listeners with done().
Suggested pattern
-it('RTN13a - ping sends HEARTBEAT and returns duration', function (done) {
+it('RTN13a - ping sends HEARTBEAT and returns duration', async function () {
const mock = new MockWebSocket({
onConnectionAttempt: (conn) => {
mock.active_connection = conn;
conn.respond_with_connected();
},
@@
const client = new Ably.Realtime({
key: 'appId.keyId:keySecret',
autoConnect: false,
useBinaryProtocol: false,
});
trackClient(client);
- client.connection.once('connected', async () => {
- const duration = await client.connection.ping();
- expect(duration).to.be.a('number');
- expect(duration).to.be.at.least(0);
- client.close();
- done();
- });
-
client.connect();
+ await new Promise<void>((resolve) => client.connection.once('connected', resolve));
+ const duration = await client.connection.ping();
+ expect(duration).to.be.a('number');
+ expect(duration).to.be.at.least(0);
+ client.close();
});(Note: Tests on lines 192 and 237 have already been correctly refactored to this pattern.)
Also applies to: 95-102, 135-141, 174-184, 281-289, 318-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/connection/connection_ping.test.ts` around lines 54 - 60,
The test uses an async callback registered with
client.connection.once('connected', async () => { ... done(); }) which can
detach promise rejections from Mocha and cause timeouts; change the test to an
async function (remove done) and explicitly await the connection state before
calling client.connection.ping(), then perform assertions and call
client.close() afterwards. Replace the event-listener pattern around
client.connection.once('connected', ...) with an awaited helper or promise that
resolves when the connection is ready, then await client.connection.ping(),
assert the numeric duration (expect(...).to.be.a('number') and
.to.be.at.least(0)), and return from the async test; apply the same change to
the other occurrences that use client.connection.once with done.
Comment on lines
+290
to
+335
| it('RTN17j - connectivity check performed before fallback', function (done) { | ||
| const connectionHosts: string[] = []; | ||
| const mock = new MockWebSocket({ | ||
| onConnectionAttempt: (conn) => { | ||
| connectionHosts.push(conn.url.hostname); | ||
| mock.active_connection = conn; | ||
|
|
||
| if (connectionHosts.length === 1) { | ||
| conn.respond_with_refused(); | ||
| } else { | ||
| conn.respond_with_connected({ | ||
| connectionId: 'connection-id', | ||
| connectionDetails: { | ||
| connectionKey: 'connection-key', | ||
| maxIdleInterval: 15000, | ||
| connectionStateTtl: 120000, | ||
| } as any, | ||
| }); | ||
| } | ||
| }, | ||
| }); | ||
| installMockWebSocket(mock.constructorFn); | ||
|
|
||
| const httpMock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => req.respond_with(200, 'yes'), | ||
| }); | ||
| installMockHttp(httpMock); | ||
|
|
||
| const client = new Ably.Realtime({ | ||
| key: 'appId.keyId:keySecret', | ||
| autoConnect: false, | ||
| useBinaryProtocol: false, | ||
| }); | ||
| trackClient(client); | ||
|
|
||
| client.connection.once('connected', () => { | ||
| // Connectivity check was performed via HTTP mock | ||
| const connectivityChecks = httpMock.captured_requests.filter((req) => req.url.href.includes('internet-up')); | ||
| expect(connectivityChecks.length).to.be.at.least(1); | ||
|
|
||
| // Connection proceeded to fallback after check | ||
| expect(connectionHosts.length).to.be.at.least(2); | ||
| client.close(); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
This doesn't verify the connectivity check happens before fallback.
Right now the test only proves that an internet-up request happened and that a fallback host was eventually used. It would still pass if the HTTP check moved after the fallback attempt. Record both websocket attempts and HTTP calls in one ordered log, then assert the internet-up entry lands between the failed primary attempt and the first fallback attempt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/connection/fallback_hosts.test.ts` around lines 290 - 335,
The test must assert ordering of events: modify the MockWebSocket
onConnectionAttempt and MockHttpClient handlers to push ordered markers into a
shared events array (e.g. push `ws:<hostname>` from the WebSocket handler when
each attempt occurs and push `http:internet-up` from the HTTP handler when the
connectivity check request is seen), then after connected verify that events
contains the first `ws:<primary-host>` entry, then an `http:internet-up` entry,
then a `ws:<fallback-host>` entry; use the existing connectionHosts array and
httpMock.captured_requests to derive expected host names and assert the index
ordering (first ws attempt index < http index < first fallback ws index) to
prove the connectivity check ran before fallback.
Comment on lines
+42
to
+51
| const savedUseProtocolHeartbeats = Platform.Config.useProtocolHeartbeats; | ||
| Platform.Config.useProtocolHeartbeats = true; | ||
|
|
||
| const mock = new MockWebSocket({ | ||
| onConnectionAttempt: (conn) => { | ||
| const heartbeats = conn.url.searchParams.get('heartbeats'); | ||
| expect(heartbeats).to.equal('true'); | ||
| conn.respond_with_connected(); | ||
| Platform.Config.useProtocolHeartbeats = savedUseProtocolHeartbeats; | ||
| done(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and check the file
fd -t f heartbeat.test.tsRepository: ably/ably-js
Length of output: 103
🏁 Script executed:
# Get the file content around the specified lines, with context
cat -n test/uts/realtime/connection/heartbeat.test.ts | sed -n '30,65p'Repository: ably/ably-js
Length of output: 1435
🏁 Script executed:
# Get a broader view of the test structure to understand test dependencies
cat -n test/uts/realtime/connection/heartbeat.test.ts | head -100Repository: ably/ably-js
Length of output: 4059
🏁 Script executed:
# Check what restoreAll() does - it's imported from helpers
fd -t f helpers.ts helpers.jsRepository: ably/ably-js
Length of output: 154
🏁 Script executed:
# Look for restoreAll function definition
rg -A 10 "export.*restoreAll|function restoreAll" --type ts --type jsRepository: ably/ably-js
Length of output: 678
🏁 Script executed:
# Get the full restoreAll function from helpers.ts
rg -A 30 "^function restoreAll" test/uts/helpers.tsRepository: ably/ably-js
Length of output: 1079
🏁 Script executed:
# Also check if Platform.Config.useProtocolHeartbeats is ever restored globally in the helper
rg "useProtocolHeartbeats" test/uts/helpers.tsRepository: ably/ably-js
Length of output: 38
🏁 Script executed:
# Check what the default value of useProtocolHeartbeats should be
rg "useProtocolHeartbeats.*=" test/uts/ -B 2 -A 2 | head -50Repository: ably/ably-js
Length of output: 1422
🏁 Script executed:
# Verify that RTN23b test expects the default behavior and would be affected
cat -n test/uts/realtime/connection/heartbeat.test.ts | sed -n '74,82p'Repository: ably/ably-js
Length of output: 451
🏁 Script executed:
# Check how many tests follow RTN23a in this suite and if any depend on default heartbeat behavior
cat -n test/uts/realtime/connection/heartbeat.test.ts | sed -n '95,200p'Repository: ably/ably-js
Length of output: 4294
🏁 Script executed:
# Also verify what the actual default value of useProtocolHeartbeats should be in Platform.Config
rg "Config.useProtocolHeartbeats\s*=" --type ts --type js -B 3 -A 1 | head -30Repository: ably/ably-js
Length of output: 1009
🏁 Script executed:
# Verify the RTN23a test is the one that mutates the global flag
# and check if there are any other places where useProtocolHeartbeats is modified
rg "useProtocolHeartbeats\s*=" test/uts/realtime/connection/heartbeat.test.tsRepository: ably/ably-js
Length of output: 250
🏁 Script executed:
# Check the full context of the afterEach hook and verify it doesn't restore useProtocolHeartbeats
cat -n test/uts/helpers.ts | sed -n '1,100p' | grep -A 50 "restoreAll"Repository: ably/ably-js
Length of output: 2240
Ensure global Platform.Config.useProtocolHeartbeats is always restored.
At line 43, a process-wide flag is mutated, but restoration at line 50 only happens in the success path. If the assertion at line 48 fails, the flag remains true and leaks into subsequent tests, causing cascading failures (e.g., RTN23b expects heartbeats=false). The afterEach hook's restoreAll() does not restore this flag, so the test itself must guarantee cleanup.
Use a try-finally block to ensure restoration regardless of assertion outcome:
Suggested fix
it('RTN23a - heartbeats=true in connection URL when ping frames not observable', function (done) {
const savedUseProtocolHeartbeats = Platform.Config.useProtocolHeartbeats;
Platform.Config.useProtocolHeartbeats = true;
const mock = new MockWebSocket({
onConnectionAttempt: (conn) => {
- const heartbeats = conn.url.searchParams.get('heartbeats');
- expect(heartbeats).to.equal('true');
- conn.respond_with_connected();
- Platform.Config.useProtocolHeartbeats = savedUseProtocolHeartbeats;
- done();
+ try {
+ const heartbeats = conn.url.searchParams.get('heartbeats');
+ expect(heartbeats).to.equal('true');
+ conn.respond_with_connected();
+ done();
+ } finally {
+ Platform.Config.useProtocolHeartbeats = savedUseProtocolHeartbeats;
+ }
},
});
installMockWebSocket(mock.constructorFn);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/realtime/connection/heartbeat.test.ts` around lines 42 - 51, The
test mutates the global flag Platform.Config.useProtocolHeartbeats and only
restores it on the success path; wrap the onConnectionAttempt body in a
try/finally so savedUseProtocolHeartbeats is always restored: capture the
original value into savedUseProtocolHeartbeats, then in onConnectionAttempt (the
callback passed to new MockWebSocket), perform the assertion and
conn.respond_with_connected() inside a try block and put
Platform.Config.useProtocolHeartbeats = savedUseProtocolHeartbeats in the
finally block (call done() after restore) to guarantee cleanup even if the
expect() fails.
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
1 participant
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
This extends #2191 with realtime unit tests. Again this is primarily intended as a validation step for the UTS test suite. This step is scoped to unit tests for the Realtime API only, and the corresponding UTS tests are in ably/specification#460.
Some tests that were generated are skipped here because ably-js fails them, and I think the tests are a faithful reflection of the spec. Issues have been created for these differences between ably-js behaviour and the spec: https://github.com/ably/ably-js/issues?q=is%3Aissue%20state%3Aopen%20label%3Auts-issue.
As before, I'm not expecting a line-by-line review of all of the generated tests.
Summary by CodeRabbit