Add UTS (Universal Test Spec) suite for REST API#2191
Open
paddybyers wants to merge 9 commits into
Open
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tighten weakened assertions, add missing test coverage, and improve test infrastructure: - Fix accommodate-both patterns in annotations, token_renewal, batch_presence, and publish tests to assert spec-correct behavior - Add fallback host tests: status code variants, cache expiry, endpoint routing, option conflict detection - Add batch_presence, request_endpoint, and additional push/presence tests - Add trackClient safety net for automatic client cleanup - Fix FakeClock: non-zero initial time, setTimeout yield in tickAsync - Update deviations.md with all documented non-conformances - Add msgpack test stubs (pending mock infrastructure support) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughCentralizes timing primitives under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 84-85: package.json currently depends on a tsx release that
requires Node >=18 and breaks the test:uts script (which uses "mocha --require
tsx/cjs"); update the "tsx" dependency in package.json to a version compatible
with Node 16 (downgrade/pin to a 3.x tsx release that supports Node 16) so the
test:uts script continues to work on supported Node >=16, then run the test:uts
script to verify mocha --require tsx/cjs loads correctly.
In `@test/uts/deviations.md`:
- Around line 205-212: The deviation note for RSC22d (batch_publish) is out of
sync with the current test: update the text so it accurately reflects the test
behavior—either state that the RSC22d case in
test/uts/rest/batch_publish.test.ts is currently skipped but still expects
generated ids, or update the test to match the deviation; specifically mention
the functions/symbols involved (batchPublish, RestChannel.publish,
allEmptyIds(), idempotentRestPublishing) so readers know this concerns id
generation differences, and replace the sentence "asserts messages lack `id`
property" with a sentence that the test is skipped and currently asserts
generated IDs (or vice versa) to eliminate the mismatch.
In `@test/uts/helpers.ts`:
- Around line 13-17: The saved-value checks for restoring mocks are using
truthiness of variables like _savedHttp and _savedWebSocket which fails when the
original value is falsy; add explicit boolean installation flags (e.g.,
_httpInstalled, _webSocketInstalled, _setTimeoutInstalled,
_clearTimeoutInstalled, _nowInstalled) set when you replace Platform.Http,
Platform.Config.WebSocket, setTimeout, clearTimeout, and now, and use those
flags in the restore/uninstall logic to decide whether to restore the
corresponding saved value (refer to the existing _savedHttp, _savedWebSocket,
_savedSetTimeout, _savedClearTimeout, _savedNow variables to locate the
replacements and the restore code).
In `@test/uts/mock_http.ts`:
- Around line 177-201: The timeout handlers in await_connection_attempt and
await_request currently reject but leave the waiter callback in
_connectionWaiters/_requestWaiters causing stale resolution; modify both
functions so the timeout path also removes the corresponding waiter from the
queue before rejecting, and ensure the waiter callback clears the timeout when
invoked; locate await_connection_attempt and await_request and update their
Promise executors to push a named watcher function (so it can be removed) and on
timeout call something like removeFromArray(this._connectionWaiters or
this._requestWaiters, watcher) then reject, and in the watcher clear the timer
and resolve.
In `@test/uts/README.md`:
- Line 151: The fenced directory-tree block in test/uts/README.md is missing a
language tag (causing markdownlint MD040); update the opening fence from ``` to
include a tag (e.g., change the block start to ```text) so the directory tree is
treated as plain text and the linter warning is resolved.
In `@test/uts/rest/auth/auth_scheme.test.ts`:
- Around line 74-75: The tests are incorrectly base64-encoding Bearer tokens;
update the assertions that build expectedAuth to use the raw token strings
instead of Buffer.from(...).toString('base64'). Locate the occurrences where
expectedAuth is defined (e.g., const expectedAuth = 'Bearer ' +
Buffer.from('explicit-token-string').toString('base64')) and replace them to
concatenate the plain token (e.g., 'Bearer ' + 'explicit-token-string') for all
seven assertions in auth_scheme.test.ts (references: expectedAuth variable and
captured[0].headers.authorization comparisons).
In `@test/uts/rest/channel/annotations.test.ts`:
- Around line 240-243: The test currently uses a substring check for the
annotation endpoint which can hide malformed paths; change the path assertions
to exact equality: replace
expect(captured[0].path).to.include('/messages/msg-serial-1/annotations') with
expect(captured[0].path).to.equal('/messages/msg-serial-1/annotations') (and
make the same replacement for the second occurrence around lines 285-288). Keep
the surrounding assertions (expect(captured).to.have.length(1) and
expect(captured[0].method).to.equal('post')) unchanged so the test verifies both
method and the exact endpoint path.
In `@test/uts/rest/channel/idempotency.test.ts`:
- Around line 67-69: The test currently allows non-URL-safe base64 chars; update
the assertions that validate the generated ID base part (see expect(parts[0])
usages in idempotency.test.ts) to only permit URL-safe base64 characters and
optional padding: replace the regex /^[A-Za-z0-9+/=_-]+$/ with one that excludes
'+' and '/' and allows '-' and '_' and optional '=' padding (e.g.
/^[A-Za-z0-9\-_]+=*$/), keep the length check
expect(parts[0].length).to.be.at.least(12), and apply this change to both
occurrences (around the earlier assertion and the similar assertion near line
263).
In `@test/uts/rest/channel/publish.test.ts`:
- Around line 411-430: The test "RSL1e - both name and data null" has no
assertion verifying the published message's name/data are null or omitted;
update the assertions after parsing captured[0].body (variable body) to inspect
the first message object (e.g., body[0]) and assert that either message.name and
message.data are strictly null or that the name and data properties are not
present, matching the spec; modify the expectations in this test
(publish.test.ts) to explicitly check message.name and message.data on the
object extracted from body so the test fails if the fields are present with
unexpected values.
In `@test/uts/rest/channel/rest_channel_attributes.test.ts`:
- Around line 105-106: The test currently uses
expect(captured[0].path).to.contain(encodeURIComponent('namespace:my channel'))
which can pass even if the route shape is wrong; change the assertion to compare
the full encoded route string exactly by asserting that captured[0].path equals
`/channels/${encodeURIComponent(channelName)}` (or the literal
`/channels/${encodeURIComponent('namespace:my channel')}`) so the test validates
the exact endpoint shape; update references to captured, captured[0].path,
encodeURIComponent and channelName in rest_channel_attributes.test.ts
accordingly.
In `@test/uts/rest/channel/update_delete_message.test.ts`:
- Around line 363-365: The test currently checks that captured[0].path includes
encodeURIComponent('serial/special:chars'), which can miss wrong
prefixes/suffixes or double-encoding; change the assertion to compare the full
path exactly by constructing the expected path string using the channel name and
the encoded serial (e.g. "/channels/<name>/messages/<encodedSerial>") and assert
expect(captured[0].path).to.equal(expectedPath) while keeping the existing
length check on captured; locate this change around the assertions referencing
captured and encodeURIComponent in update_delete_message.test.ts.
In `@test/uts/rest/presence/rest_presence.test.ts`:
- Around line 515-553: The test for RSP pagination should assert the actual
request target used when page1.next() is called: in the MockHttpClient onRequest
handler (the one that increments reqCount), add an explicit assertion when
reqCount === 2 that the incoming request's path/query matches the Link target
(e.g. contains cursor=page2 and limit=1 or the expected '/presence' path), so
the test verifies page2 was fetched from the Link header rather than some other
URL; reference the existing MockHttpClient onRequest, reqCount counter, and the
page1.next() call to locate where to add this check.
In `@test/uts/rest/request.test.ts`:
- Around line 420-425: The test around client.request('GET', '/test', 3)
currently catches the expect.fail() call as part of the same try/catch, creating
a false-positive when client.request does not throw; update the test to use an
explicit thrown flag (e.g., let thrown = false; set thrown = true in the try
after await client.request if it should throw, or set thrown = true in the catch
when the client actually throws) then assert thrown is true after the try/catch,
or alternatively rethrow assertion errors inside the catch to avoid swallowing
them; target the test block containing client.request('GET', '/test', 3) in
request.test.ts and ensure you reference the thrown flag or rethrow logic so the
assertion fails when client.request does not throw.
🪄 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: 0cb4ee70-8929-4419-a0f9-dd77e107f502
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
package.jsonsrc/common/lib/client/auth.tssrc/common/lib/client/baseclient.tssrc/common/lib/client/realtimechannel.tssrc/common/lib/client/realtimepresence.tssrc/common/lib/client/rest.tssrc/common/lib/transport/connectionmanager.tssrc/common/lib/transport/transport.tssrc/common/lib/util/logger.tssrc/common/lib/util/utils.tssrc/common/types/IPlatformConfig.d.tssrc/common/types/http.tssrc/platform/nativescript/config.jssrc/platform/nodejs/config.tssrc/platform/react-native/config.tssrc/platform/web/config.tstest/uts/README.mdtest/uts/deviations.mdtest/uts/helpers.tstest/uts/mock_http.tstest/uts/realtime/time.test.tstest/uts/rest/auth/auth_callback.test.tstest/uts/rest/auth/auth_scheme.test.tstest/uts/rest/auth/authorize.test.tstest/uts/rest/auth/client_id.test.tstest/uts/rest/auth/revoke_tokens.test.tstest/uts/rest/auth/token_details.test.tstest/uts/rest/auth/token_renewal.test.tstest/uts/rest/auth/token_request_params.test.tstest/uts/rest/batch_presence.test.tstest/uts/rest/batch_publish.test.tstest/uts/rest/channel/annotations.test.tstest/uts/rest/channel/get_message.test.tstest/uts/rest/channel/history.test.tstest/uts/rest/channel/idempotency.test.tstest/uts/rest/channel/message_versions.test.tstest/uts/rest/channel/publish.test.tstest/uts/rest/channel/publish_result.test.tstest/uts/rest/channel/rest_channel_attributes.test.tstest/uts/rest/channel/update_delete_message.test.tstest/uts/rest/channels_collection.test.tstest/uts/rest/encoding/message_encoding.test.tstest/uts/rest/fallback.test.tstest/uts/rest/logging.test.tstest/uts/rest/presence/rest_presence.test.tstest/uts/rest/push/push_admin_publish.test.tstest/uts/rest/push/push_channel_subscriptions.test.tstest/uts/rest/push/push_device_registrations.test.tstest/uts/rest/request.test.tstest/uts/rest/request_endpoint.test.tstest/uts/rest/rest_client.test.tstest/uts/rest/stats.test.tstest/uts/rest/time.test.tstest/uts/rest/types/error_types.test.tstest/uts/rest/types/message_types.test.tstest/uts/rest/types/mutable_message_types.test.tstest/uts/rest/types/options_types.test.tstest/uts/rest/types/paginated_result.test.tstest/uts/rest/types/presence_message_types.test.tstest/uts/rest/types/token_types.test.ts
Comment on lines
+84
to
85
| "@types/mocha": "^10.0.10", | ||
| "@types/node": "^18.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo constraints:"
jq -r '.engines.node, .devDependencies.mocha, .devDependencies["@types/mocha"], .devDependencies.tsx' package.json
echo "tsx@4.21.0 engines:"
curl -s https://registry.npmjs.org/tsx/4.21.0 | jq -r '.engines.node // "no engines field"'
echo "Mocha runtime/types versions:"
curl -s https://registry.npmjs.org/mocha/8.1.3 | jq -r '.version'
curl -s https://registry.npmjs.org/@types%2Fmocha/10.0.10 | jq -r '.version'Repository: ably/ably-js
Length of output: 176
🏁 Script executed:
# Check the lines mentioned in the review (84-85, 133-133, 163-163)
head -90 package.json | tail -10
echo "---"
sed -n '130,136p' package.json
echo "---"
sed -n '160,166p' package.jsonRepository: ably/ably-js
Length of output: 987
🌐 Web query:
@types/mocha 10 compatibility with mocha 8
💡 Result:
@types/mocha version 10 is compatible with Mocha 8. Type definitions packages like @types/mocha do not have strict peer dependencies on specific runtime versions of the library they type. They evolve independently to support TypeScript improvements and add types for newer library features while maintaining backward compatibility with older library versions where possible. @types/mocha v8 was released specifically for Mocha 8 (July 2020), v9 for Mocha 9 (July 2021), and v10 starting September 2022 (after Mocha 10). Projects commonly use newer @types/mocha versions with older Mocha runtimes without issues, as core APIs (describe, it, before, etc.) remain stable across Mocha major versions. No sources report incompatibility between @types/mocha 10 and Mocha 8. Type errors in v10.0.2 (fixed in later patches like v10.0.10) were internal TypeScript compilation issues, not runtime incompatibilities with Mocha versions. Modern projects use @types/mocha ^10 with Mocha ^10, but cross-version usage (e.g., Mocha 8 + @types/mocha 10) works for type checking test files.
Citations:
- 1: https://www.npmjs.com/package/@types/mocha?activeTab=versions
- 2: @types/mocha 10.0.2 has type errors DefinitelyTyped/DefinitelyTyped#66934
- 3: [@types/mocha] update types to match v9.0.1 DefinitelyTyped/DefinitelyTyped#54208
- 4: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46017/files
- 5: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/mocha/mocha-tests.ts
Fix tsx version to support Node 16.
tsx@^4.21.0 requires Node >=18, breaking the test:uts command on Node 16 since it uses mocha --require tsx/cjs. This is incompatible with the repo's declared support of node >=16.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 84 - 85, package.json currently depends on a tsx
release that requires Node >=18 and breaks the test:uts script (which uses
"mocha --require tsx/cjs"); update the "tsx" dependency in package.json to a
version compatible with Node 16 (downgrade/pin to a 3.x tsx release that
supports Node 16) so the test:uts script continues to work on supported Node
>=16, then run the test:uts script to verify mocha --require tsx/cjs loads
correctly.
Comment on lines
+13
to
+17
| let _savedHttp: any = null; | ||
| let _savedWebSocket: any = null; | ||
| let _savedSetTimeout: any = null; | ||
| let _savedClearTimeout: any = null; | ||
| let _savedNow: any = null; |
There was a problem hiding this comment.
Use explicit installation state tracking for mock restoration.
Line 36 and Line 56 restore only on truthy saved values. If the original Platform.Http or Platform.Config.WebSocket is falsy, uninstall won’t restore and the mock can leak into later tests.
Suggested fix
+const NOT_INSTALLED = Symbol('not_installed');
-let _savedHttp: any = null;
-let _savedWebSocket: any = null;
+let _savedHttp: any = NOT_INSTALLED;
+let _savedWebSocket: any = NOT_INSTALLED;
function installMockHttp(mockHttpClient: { asPlatformHttp(): any }): void {
- if (_savedHttp) throw new Error('Mock HTTP already installed — call uninstallMockHttp() first');
+ if (_savedHttp !== NOT_INSTALLED) throw new Error('Mock HTTP already installed — call uninstallMockHttp() first');
_savedHttp = Platform.Http;
Platform.Http = mockHttpClient.asPlatformHttp();
}
function uninstallMockHttp(): void {
- if (_savedHttp) {
+ if (_savedHttp !== NOT_INSTALLED) {
Platform.Http = _savedHttp;
- _savedHttp = null;
+ _savedHttp = NOT_INSTALLED;
}
}
function installMockWebSocket(mockWsConstructor: any): void {
- if (_savedWebSocket) throw new Error('Mock WebSocket already installed');
+ if (_savedWebSocket !== NOT_INSTALLED) throw new Error('Mock WebSocket already installed');
_savedWebSocket = Platform.Config.WebSocket;
Platform.Config.WebSocket = mockWsConstructor;
}
function uninstallMockWebSocket(): void {
- if (_savedWebSocket) {
+ if (_savedWebSocket !== NOT_INSTALLED) {
Platform.Config.WebSocket = _savedWebSocket;
- _savedWebSocket = null;
+ _savedWebSocket = NOT_INSTALLED;
}
}Also applies to: 26-40, 46-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/helpers.ts` around lines 13 - 17, The saved-value checks for
restoring mocks are using truthiness of variables like _savedHttp and
_savedWebSocket which fails when the original value is falsy; add explicit
boolean installation flags (e.g., _httpInstalled, _webSocketInstalled,
_setTimeoutInstalled, _clearTimeoutInstalled, _nowInstalled) set when you
replace Platform.Http, Platform.Config.WebSocket, setTimeout, clearTimeout, and
now, and use those flags in the restore/uninstall logic to decide whether to
restore the corresponding saved value (refer to the existing _savedHttp,
_savedWebSocket, _savedSetTimeout, _savedClearTimeout, _savedNow variables to
locate the replacements and the restore code).
Comment on lines
+177
to
+201
| /** Wait for the next connection attempt */ | ||
| await_connection_attempt(timeout?: number): Promise<PendingConnection> { | ||
| return new Promise((resolve, reject) => { | ||
| const timer = timeout | ||
| ? setTimeout(() => reject(new Error('Timeout waiting for connection attempt')), timeout) | ||
| : null; | ||
| this._connectionWaiters.push((conn) => { | ||
| if (timer) clearTimeout(timer); | ||
| resolve(conn); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| /** Wait for the next HTTP request (after connection succeeds) */ | ||
| await_request(timeout?: number): Promise<PendingRequest> { | ||
| return new Promise((resolve, reject) => { | ||
| const timer = timeout | ||
| ? setTimeout(() => reject(new Error('Timeout waiting for request')), timeout) | ||
| : null; | ||
| this._requestWaiters.push((req) => { | ||
| if (timer) clearTimeout(timer); | ||
| resolve(req); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Remove timed-out waiters from queues to avoid flaky cross-test behavior.
If timeout fires, the waiter callback is left in _connectionWaiters/_requestWaiters. A later event can resolve the stale waiter and steal events from active callers.
Suggested fix
await_connection_attempt(timeout?: number): Promise<PendingConnection> {
return new Promise((resolve, reject) => {
- const timer = timeout
- ? setTimeout(() => reject(new Error('Timeout waiting for connection attempt')), timeout)
- : null;
- this._connectionWaiters.push((conn) => {
+ const waiter = (conn: PendingConnection) => {
if (timer) clearTimeout(timer);
resolve(conn);
- });
+ };
+ const timer = timeout
+ ? setTimeout(() => {
+ const i = this._connectionWaiters.indexOf(waiter);
+ if (i >= 0) this._connectionWaiters.splice(i, 1);
+ reject(new Error('Timeout waiting for connection attempt'));
+ }, timeout)
+ : null;
+ this._connectionWaiters.push(waiter);
});
}
await_request(timeout?: number): Promise<PendingRequest> {
return new Promise((resolve, reject) => {
- const timer = timeout
- ? setTimeout(() => reject(new Error('Timeout waiting for request')), timeout)
- : null;
- this._requestWaiters.push((req) => {
+ const waiter = (req: PendingRequest) => {
if (timer) clearTimeout(timer);
resolve(req);
- });
+ };
+ const timer = timeout
+ ? setTimeout(() => {
+ const i = this._requestWaiters.indexOf(waiter);
+ if (i >= 0) this._requestWaiters.splice(i, 1);
+ reject(new Error('Timeout waiting for request'));
+ }, timeout)
+ : null;
+ this._requestWaiters.push(waiter);
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/mock_http.ts` around lines 177 - 201, The timeout handlers in
await_connection_attempt and await_request currently reject but leave the waiter
callback in _connectionWaiters/_requestWaiters causing stale resolution; modify
both functions so the timeout path also removes the corresponding waiter from
the queue before rejecting, and ensure the waiter callback clears the timeout
when invoked; locate await_connection_attempt and await_request and update their
Promise executors to push a named watcher function (so it can be removed) and on
timeout call something like removeFromArray(this._connectionWaiters or
this._requestWaiters, watcher) then reject, and in the watcher clear the timer
and resolve.
|
|
||
| ## Directory structure | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced directory-tree block.
Line 151 opens a fenced block without a language, which triggers markdownlint MD040 and can break docs lint gating.
Suggested patch
-```
+```text
test/uts/
README.md # This file
helpers.ts # install/uninstall, FakeClock, Ably re-export
@@
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/README.md` at line 151, The fenced directory-tree block in
test/uts/README.md is missing a language tag (causing markdownlint MD040);
update the opening fence from ``` to include a tag (e.g., change the block start
to ```text) so the directory tree is treated as plain text and the linter
warning is resolved.
Comment on lines
+411
to
+430
| it('RSL1e - both name and data null', async function () { | ||
| const captured: any[] = []; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| captured.push(req); | ||
| req.respond_with(201, {}); | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret', useBinaryProtocol: false }); | ||
| await client.channels.get('test').publish(null, null); | ||
|
|
||
| expect(captured).to.have.length(1); | ||
| const body = JSON.parse(captured[0].body); | ||
| expect(body).to.be.an('array'); | ||
| expect(body).to.have.length(1); | ||
| // The message should be essentially empty (name and data are null/missing) | ||
| }); |
There was a problem hiding this comment.
RSL1e - both name and data null currently has no behavioral assertion.
On Line 429, the comment states intent, but the test never asserts name/data are null or omitted. That makes this case a false positive risk in spec-conformance coverage.
Proposed fix
expect(captured).to.have.length(1);
const body = JSON.parse(captured[0].body);
expect(body).to.be.an('array');
expect(body).to.have.length(1);
- // The message should be essentially empty (name and data are null/missing)
+ const message = body[0];
+ expect('name' in message ? message.name : null).to.equal(null);
+ expect('data' in message ? message.data : null).to.equal(null);📝 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
| it('RSL1e - both name and data null', async function () { | |
| const captured: any[] = []; | |
| const mock = new MockHttpClient({ | |
| onConnectionAttempt: (conn) => conn.respond_with_success(), | |
| onRequest: (req) => { | |
| captured.push(req); | |
| req.respond_with(201, {}); | |
| }, | |
| }); | |
| installMockHttp(mock); | |
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret', useBinaryProtocol: false }); | |
| await client.channels.get('test').publish(null, null); | |
| expect(captured).to.have.length(1); | |
| const body = JSON.parse(captured[0].body); | |
| expect(body).to.be.an('array'); | |
| expect(body).to.have.length(1); | |
| // The message should be essentially empty (name and data are null/missing) | |
| }); | |
| it('RSL1e - both name and data null', async function () { | |
| const captured: any[] = []; | |
| const mock = new MockHttpClient({ | |
| onConnectionAttempt: (conn) => conn.respond_with_success(), | |
| onRequest: (req) => { | |
| captured.push(req); | |
| req.respond_with(201, {}); | |
| }, | |
| }); | |
| installMockHttp(mock); | |
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret', useBinaryProtocol: false }); | |
| await client.channels.get('test').publish(null, null); | |
| expect(captured).to.have.length(1); | |
| const body = JSON.parse(captured[0].body); | |
| expect(body).to.be.an('array'); | |
| expect(body).to.have.length(1); | |
| const message = body[0]; | |
| expect('name' in message ? message.name : null).to.equal(null); | |
| expect('data' in message ? message.data : null).to.equal(null); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/channel/publish.test.ts` around lines 411 - 430, The test
"RSL1e - both name and data null" has no assertion verifying the published
message's name/data are null or omitted; update the assertions after parsing
captured[0].body (variable body) to inspect the first message object (e.g.,
body[0]) and assert that either message.name and message.data are strictly null
or that the name and data properties are not present, matching the spec; modify
the expectations in this test (publish.test.ts) to explicitly check message.name
and message.data on the object extracted from body so the test fails if the
fields are present with unexpected values.
Comment on lines
+105
to
+106
| expect(captured).to.have.length(1); | ||
| expect(captured[0].path).to.contain(encodeURIComponent('namespace:my channel')); |
There was a problem hiding this comment.
Assert the full encoded status path here.
contain(encodeURIComponent(...)) still passes if the route is wrong or the name is encoded in the wrong place. Since this spec is validating the endpoint shape, compare against the exact /channels/${encodeURIComponent(channelName)} path instead.
Proposed fix
- expect(captured[0].path).to.contain(encodeURIComponent('namespace:my channel'));
+ expect(captured[0].path).to.equal('/channels/' + encodeURIComponent('namespace:my channel'));📝 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
| expect(captured).to.have.length(1); | |
| expect(captured[0].path).to.contain(encodeURIComponent('namespace:my channel')); | |
| expect(captured).to.have.length(1); | |
| expect(captured[0].path).to.equal('/channels/' + encodeURIComponent('namespace:my channel')); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/channel/rest_channel_attributes.test.ts` around lines 105 -
106, The test currently uses
expect(captured[0].path).to.contain(encodeURIComponent('namespace:my channel'))
which can pass even if the route shape is wrong; change the assertion to compare
the full encoded route string exactly by asserting that captured[0].path equals
`/channels/${encodeURIComponent(channelName)}` (or the literal
`/channels/${encodeURIComponent('namespace:my channel')}`) so the test validates
the exact endpoint shape; update references to captured, captured[0].path,
encodeURIComponent and channelName in rest_channel_attributes.test.ts
accordingly.
Comment on lines
+363
to
+365
| expect(captured).to.have.length(1); | ||
| // The path should contain the URL-encoded serial | ||
| expect(captured[0].path).to.include(encodeURIComponent('serial/special:chars')); |
There was a problem hiding this comment.
Use exact path equality for the encoded serial check.
include(encodeURIComponent(...)) won't catch a wrong route prefix/suffix or accidental double-encoding. This test is stronger if it asserts the full /channels/<name>/messages/<serial> path.
Proposed fix
- expect(captured[0].path).to.include(encodeURIComponent('serial/special:chars'));
+ expect(captured[0].path).to.equal(
+ '/channels/test-channel/messages/' + encodeURIComponent('serial/special:chars'),
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/channel/update_delete_message.test.ts` around lines 363 - 365,
The test currently checks that captured[0].path includes
encodeURIComponent('serial/special:chars'), which can miss wrong
prefixes/suffixes or double-encoding; change the assertion to compare the full
path exactly by constructing the expected path string using the channel name and
the encoded serial (e.g. "/channels/<name>/messages/<encodedSerial>") and assert
expect(captured[0].path).to.equal(expectedPath) while keeping the existing
length check on captured; locate this change around the assertions referencing
captured and encodeURIComponent in update_delete_message.test.ts.
Comment on lines
+515
to
+553
| it('RSP pagination - history() navigates pages via next()', async function () { | ||
| let reqCount = 0; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| reqCount++; | ||
| if (reqCount === 1) { | ||
| req.respond_with(200, [ | ||
| { action: 2, clientId: 'alice', timestamp: 1609459200000 }, | ||
| ], { | ||
| 'Link': '<./presence?cursor=page2&limit=1>; rel="next"', | ||
| }); | ||
| } else { | ||
| req.respond_with(200, [ | ||
| { action: 3, clientId: 'bob', timestamp: 1609459100000 }, | ||
| ]); | ||
| } | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret', useBinaryProtocol: false }); | ||
| const channel = client.channels.get('test'); | ||
|
|
||
| // First page | ||
| const page1 = await channel.presence.history({ limit: 1 }); | ||
| expect(page1.items).to.have.length(1); | ||
| expect(page1.items[0].action).to.equal('enter'); | ||
| expect(page1.items[0].clientId).to.equal('alice'); | ||
| expect(page1.hasNext()).to.be.true; | ||
|
|
||
| // Second page | ||
| const page2 = await page1.next(); | ||
| expect(page2.items).to.have.length(1); | ||
| expect(page2.items[0].action).to.equal('leave'); | ||
| expect(page2.items[0].clientId).to.equal('bob'); | ||
| expect(page2.hasNext()).to.be.false; | ||
| expect(page2.isLast()).to.be.true; | ||
| }); |
There was a problem hiding this comment.
Assert the actual next() request target in the history pagination test.
At Line 525 the Link points to ./presence?..., and the test never verifies where Line 547 actually requests page 2. This can pass even if pagination follows the wrong URL. Please assert the second request path/query explicitly.
Suggested hardening
it('RSP pagination - history() navigates pages via next()', async function () {
let reqCount = 0;
+ const captured: any[] = [];
const mock = new MockHttpClient({
onConnectionAttempt: (conn) => conn.respond_with_success(),
onRequest: (req) => {
+ captured.push(req);
reqCount++;
if (reqCount === 1) {
req.respond_with(200, [
{ action: 2, clientId: 'alice', timestamp: 1609459200000 },
], {
- 'Link': '<./presence?cursor=page2&limit=1>; rel="next"',
+ 'Link': '</channels/test/presence/history?cursor=page2&limit=1>; rel="next"',
});
} else {
req.respond_with(200, [
{ action: 3, clientId: 'bob', timestamp: 1609459100000 },
]);
}
},
});
@@
const page2 = await page1.next();
+ expect(captured).to.have.length(2);
+ expect(captured[1].path).to.equal('/channels/test/presence/history');
+ expect(captured[1].url.searchParams.get('cursor')).to.equal('page2');
expect(page2.items).to.have.length(1);📝 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
| it('RSP pagination - history() navigates pages via next()', async function () { | |
| let reqCount = 0; | |
| const mock = new MockHttpClient({ | |
| onConnectionAttempt: (conn) => conn.respond_with_success(), | |
| onRequest: (req) => { | |
| reqCount++; | |
| if (reqCount === 1) { | |
| req.respond_with(200, [ | |
| { action: 2, clientId: 'alice', timestamp: 1609459200000 }, | |
| ], { | |
| 'Link': '<./presence?cursor=page2&limit=1>; rel="next"', | |
| }); | |
| } else { | |
| req.respond_with(200, [ | |
| { action: 3, clientId: 'bob', timestamp: 1609459100000 }, | |
| ]); | |
| } | |
| }, | |
| }); | |
| installMockHttp(mock); | |
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret', useBinaryProtocol: false }); | |
| const channel = client.channels.get('test'); | |
| // First page | |
| const page1 = await channel.presence.history({ limit: 1 }); | |
| expect(page1.items).to.have.length(1); | |
| expect(page1.items[0].action).to.equal('enter'); | |
| expect(page1.items[0].clientId).to.equal('alice'); | |
| expect(page1.hasNext()).to.be.true; | |
| // Second page | |
| const page2 = await page1.next(); | |
| expect(page2.items).to.have.length(1); | |
| expect(page2.items[0].action).to.equal('leave'); | |
| expect(page2.items[0].clientId).to.equal('bob'); | |
| expect(page2.hasNext()).to.be.false; | |
| expect(page2.isLast()).to.be.true; | |
| }); | |
| it('RSP pagination - history() navigates pages via next()', async function () { | |
| let reqCount = 0; | |
| const captured: any[] = []; | |
| const mock = new MockHttpClient({ | |
| onConnectionAttempt: (conn) => conn.respond_with_success(), | |
| onRequest: (req) => { | |
| captured.push(req); | |
| reqCount++; | |
| if (reqCount === 1) { | |
| req.respond_with(200, [ | |
| { action: 2, clientId: 'alice', timestamp: 1609459200000 }, | |
| ], { | |
| 'Link': '</channels/test/presence/history?cursor=page2&limit=1>; rel="next"', | |
| }); | |
| } else { | |
| req.respond_with(200, [ | |
| { action: 3, clientId: 'bob', timestamp: 1609459100000 }, | |
| ]); | |
| } | |
| }, | |
| }); | |
| installMockHttp(mock); | |
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret', useBinaryProtocol: false }); | |
| const channel = client.channels.get('test'); | |
| // First page | |
| const page1 = await channel.presence.history({ limit: 1 }); | |
| expect(page1.items).to.have.length(1); | |
| expect(page1.items[0].action).to.equal('enter'); | |
| expect(page1.items[0].clientId).to.equal('alice'); | |
| expect(page1.hasNext()).to.be.true; | |
| // Second page | |
| const page2 = await page1.next(); | |
| expect(captured).to.have.length(2); | |
| expect(captured[1].path).to.equal('/channels/test/presence/history'); | |
| expect(captured[1].url.searchParams.get('cursor')).to.equal('page2'); | |
| expect(page2.items).to.have.length(1); | |
| expect(page2.items[0].action).to.equal('leave'); | |
| expect(page2.items[0].clientId).to.equal('bob'); | |
| expect(page2.hasNext()).to.be.false; | |
| expect(page2.isLast()).to.be.true; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/presence/rest_presence.test.ts` around lines 515 - 553, The
test for RSP pagination should assert the actual request target used when
page1.next() is called: in the MockHttpClient onRequest handler (the one that
increments reqCount), add an explicit assertion when reqCount === 2 that the
incoming request's path/query matches the Link target (e.g. contains
cursor=page2 and limit=1 or the expected '/presence' path), so the test verifies
page2 was fetched from the Link header rather than some other URL; reference the
existing MockHttpClient onRequest, reqCount counter, and the page1.next() call
to locate where to add this check.
Comment on lines
+420
to
+425
| try { | ||
| await client.request('GET', '/test', 3); | ||
| expect.fail('Expected request to throw on connection refused'); | ||
| } catch (error: any) { | ||
| expect(error).to.exist; | ||
| } |
There was a problem hiding this comment.
Fix false-positive path in the connection-refused test.
Line 420–Line 425 can pass even when client.request() does not throw, because expect.fail(...) is caught by the same catch. Please assert an explicit thrown flag (or rethrow assertion errors).
Suggested patch
- try {
- await client.request('GET', '/test', 3);
- expect.fail('Expected request to throw on connection refused');
- } catch (error: any) {
- expect(error).to.exist;
- }
+ let threw = false;
+ try {
+ await client.request('GET', '/test', 3);
+ } catch (error: any) {
+ threw = true;
+ expect(error).to.exist;
+ }
+ expect(threw).to.be.true;📝 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
| try { | |
| await client.request('GET', '/test', 3); | |
| expect.fail('Expected request to throw on connection refused'); | |
| } catch (error: any) { | |
| expect(error).to.exist; | |
| } | |
| let threw = false; | |
| try { | |
| await client.request('GET', '/test', 3); | |
| } catch (error: any) { | |
| threw = true; | |
| expect(error).to.exist; | |
| } | |
| expect(threw).to.be.true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/request.test.ts` around lines 420 - 425, The test around
client.request('GET', '/test', 3) currently catches the expect.fail() call as
part of the same try/catch, creating a false-positive when client.request does
not throw; update the test to use an explicit thrown flag (e.g., let thrown =
false; set thrown = true in the try after await client.request if it should
throw, or set thrown = true in the catch when the client actually throws) then
assert thrown is true after the try/catch, or alternatively rethrow assertion
errors inside the catch to avoid swallowing them; target the test block
containing client.request('GET', '/test', 3) in request.test.ts and ensure you
reference the thrown flag or rethrow logic so the assertion fails when
client.request does not throw.
Import Ably from internal source types instead of untyped require() so
the test suite compiles cleanly under strict TypeScript checking.
- Replace require('../../build/ably-node') with internal source imports
(DefaultRest, DefaultRealtime, ErrorInfo, ProtocolMessage)
- Add explicit type annotations to eliminate noImplicitAny errors
(captured arrays, callback parameters, catch clauses)
- Add non-null assertions and as-any casts for test mock patterns
- Fix Platform.Config.clearTimeout casts in source files to use
ReturnType<typeof setTimeout> instead of number/NodeJS.Timeout
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
temporarily deployed
to
staging/pull/2191/bundle-report
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
temporarily deployed
to
staging/pull/2191/bundle-report
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 (3)
src/common/lib/transport/connectionmanager.ts (2)
868-874:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat
0as a valid timestamp here.
Platform.Config.now()can return0under deterministic timers, so!this.lastActivitywill skip the freshness check entirely. Use an explicit null/undefined check instead.Suggested fix
- if (!this.lastActivity || !this.connectionId) { + if (this.lastActivity == null || !this.connectionId) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/lib/transport/connectionmanager.ts` around lines 868 - 874, The freshness check in checkConnectionStateFreshness treats 0 as falsy and skips the check; change the guard to explicitly check for null/undefined for lastActivity (e.g., use this.lastActivity == null or this.lastActivity === null || this.lastActivity === undefined) while keeping the existing check for connectionId, so that a timestamp of 0 is considered valid and the sinceLast calculation runs as intended.
1215-1235:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNull-check the reconnect timestamp before computing backoff.
The
&&expression collapses a0timestamp to falsy, so a fake clock starting at epoch 0 bypasses the 1s reconnect delay. That makes the new timer-based tests flaky.Suggested fix
- const sinceLast = this.lastAutoReconnectAttempt && Platform.Config.now() - this.lastAutoReconnectAttempt + 1; - if (sinceLast && sinceLast < 1000) { + const sinceLast = + this.lastAutoReconnectAttempt == null + ? null + : Platform.Config.now() - this.lastAutoReconnectAttempt + 1; + if (sinceLast !== null && sinceLast < 1000) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/lib/transport/connectionmanager.ts` around lines 1215 - 1235, The backoff calculation uses "this.lastAutoReconnectAttempt && Platform.Config.now() - this.lastAutoReconnectAttempt + 1" which treats 0 as falsy and bypasses the delay; update the logic in ConnectionManager.notifyState to explicitly null-check lastAutoReconnectAttempt (e.g., test against null/undefined or use Number.isFinite) before computing sinceLast so a timestamp of 0 is respected, then only branch into the "sinceLast < 1000" path when sinceLast is a valid number; adjust the computation and subsequent Platform.Config.setTimeout call accordingly (refer to lastAutoReconnectAttempt, Platform.Config.now(), sinceLast and autoReconnect).test/uts/rest/push/push_device_registrations.test.ts (1)
1-556:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun Prettier before merge.
format:checkis still failing on this file, so CI will stay red until the formatting drift is fixed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/push/push_device_registrations.test.ts` around lines 1 - 556, The file under the test suite describe('uts/rest/push/push_device_registrations') has formatting drift; run your project's formatter (e.g. Prettier) to reformat the file, then stage and commit the changes so format:check passes in CI; specifically reformat the test file containing the it(...) cases like "RSH1b3 - save sends PUT to /push/deviceRegistrations/{id}" and "RSH1b1 - get returns device object" and ensure no lint/format scripts report changes before pushing.
♻️ Duplicate comments (1)
test/uts/rest/channel/rest_channel_attributes.test.ts (1)
105-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the exact encoded path.
contain(...)can still pass if the encoded channel name shows up in the wrong place. Compare against the full/channels/...path so this test actually validates the endpoint shape.Suggested fix
- expect(captured[0].path).to.contain(encodeURIComponent('namespace:my channel')); + expect(captured[0].path).to.equal('/channels/' + encodeURIComponent('namespace:my channel'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/channel/rest_channel_attributes.test.ts` around lines 105 - 106, The test currently uses expect(captured[0].path).to.contain(encodeURIComponent('namespace:my channel')) which can match the encoded name appearing anywhere; update the assertion to compare the full endpoint shape by asserting equality against the exact path string '/channels/' + encodeURIComponent('namespace:my channel') so that captured[0].path exactly equals the expected '/channels/{encodedChannel}' value (referencing the captured variable and the test assertion on captured[0].path).
🤖 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/mock_http.ts`:
- Around line 294-303: The mock implementation of shouldFallback in mock_http.ts
is missing platform error codes EHOSTDOWN and ESOCKETTIMEDOUT; update the
shouldFallback(error: any) function to treat error.code === 'EHOSTDOWN' and
error.code === 'ESOCKETTIMEDOUT' as fallbacks alongside the existing codes
(ECONNREFUSED, ENETUNREACH, EHOSTUNREACH, ETIMEDOUT, ECONNRESET, ENOTFOUND), and
keep the existing statusCode >= 500 && statusCode <= 504 check (guarding against
undefined statusCode as currently done).
In `@test/uts/rest/auth/token_renewal.test.ts`:
- Around line 1-396: The file under the describe block
"uts/rest/auth/token_renewal" is failing format checks; run your project's
formatter (e.g. Prettier) over this test file and re-commit the changes so
format:check passes; specifically format and save the file containing the
"it('RSA4b - renewal on 40142 error')" and other it() tests, then run the repo's
formatting command (e.g. npm run format or npx prettier --write .) and include
the formatted file in your PR before merge.
In `@test/uts/rest/channel/get_message.test.ts`:
- Around line 116-120: The test currently only checks substring presence of the
encoded serial in captured[0].path which can miss route shape errors; change the
assertion to compare the entire path string: build the exact expected path using
the channel name and the encoded serial (e.g.
`/channels/${channelName}/messages/${encodeURIComponent(serial)}`) and assert
captured[0].path equals that expectedPath (replace the current
include/not.include checks for 'serial/with:special+chars' with an equality
check against the full `/channels/<name>/messages/<encodedSerial>` value).
In `@test/uts/rest/channel/update_delete_message.test.ts`:
- Around line 1-367: Prettier formatting has drifted in the test suite
describe('uts/rest/channel/update_delete_message') (contains tests like "RSL15b
- updateMessage sends PATCH" and helper msg()), so run the project's formatter
(Prettier) to reformat this test file (or run the repo's format script) and
commit the changes so format:check passes; no code changes needed—only apply
Prettier formatting to the file containing those tests.
In `@test/uts/rest/encoding/message_encoding.test.ts`:
- Around line 1-359: The test file describing
"uts/rest/encoding/message_encoding" has formatting drift causing format:check
to fail; fix by running your project's formatter (e.g. prettier --write) on the
file (or run the repo's format script like npm run format / yarn format), stage
the resulting changes, and commit only formatting edits so the tests
(describe('uts/rest/encoding/message_encoding', ...) and its it() cases such as
'RSL4a - string data has no encoding') remain unchanged semantically; re-run
format:check locally before pushing to clear the CI failure.
In `@test/uts/rest/push/push_admin_publish.test.ts`:
- Around line 1-235: The file fails the repository formatter; run Prettier (or
the project's format script) to reformat this test file so it passes
format:check—e.g., format the file containing the
describe('uts/rest/push/push_admin_publish', ...) block and its it(...) tests
(such as the "RSH1a - publish sends POST to /push/publish" and "RSH1 -
client.push.admin exposes PushAdmin" tests) and commit the changes; ensure the
updated formatting is included before merging.
In `@test/uts/rest/rest_client.test.ts`:
- Around line 1-259: The test file formatting has drifted (format:check
failing); run the repository formatter (e.g., run the project's format script
such as npm run format or yarn format, or run prettier --write on the tests) to
reformat the test suite including the top-level describe('uts/rest/rest_client')
and its it(...) blocks, re-run format:check (or CI) to verify it passes, and
commit the formatted changes before merging.
In `@test/uts/rest/types/presence_message_types.test.ts`:
- Around line 1-259: The file test/uts/rest/types/presence_message_types.test.ts
fails the project's formatting check; run the project's Prettier command (e.g.,
npm run format or npx prettier --write) on this file (the
describe('uts/rest/types/presence_message_types' ...) and its contained it(...)
blocks such as 'TP2 - PresenceAction values' and 'TP3 - deserialization from
wire via fromEncoded') to fix formatting drift, re-run format:check, and commit
the reformatted file so CI passes.
In `@test/uts/rest/types/token_types.test.ts`:
- Around line 1-321: This file has formatting drift causing format:check to
fail; run the project's formatter (e.g., Prettier via the repo's format script
or prettier --write) over the test file containing simpleMock and the
describe('uts/rest/types/token_types', ...) / it(...) blocks, fix
whitespace/indentation and any trailing commas or semicolon issues, re-run
format:check, and commit the reformatted file so CI passes.
---
Outside diff comments:
In `@src/common/lib/transport/connectionmanager.ts`:
- Around line 868-874: The freshness check in checkConnectionStateFreshness
treats 0 as falsy and skips the check; change the guard to explicitly check for
null/undefined for lastActivity (e.g., use this.lastActivity == null or
this.lastActivity === null || this.lastActivity === undefined) while keeping the
existing check for connectionId, so that a timestamp of 0 is considered valid
and the sinceLast calculation runs as intended.
- Around line 1215-1235: The backoff calculation uses
"this.lastAutoReconnectAttempt && Platform.Config.now() -
this.lastAutoReconnectAttempt + 1" which treats 0 as falsy and bypasses the
delay; update the logic in ConnectionManager.notifyState to explicitly
null-check lastAutoReconnectAttempt (e.g., test against null/undefined or use
Number.isFinite) before computing sinceLast so a timestamp of 0 is respected,
then only branch into the "sinceLast < 1000" path when sinceLast is a valid
number; adjust the computation and subsequent Platform.Config.setTimeout call
accordingly (refer to lastAutoReconnectAttempt, Platform.Config.now(), sinceLast
and autoReconnect).
In `@test/uts/rest/push/push_device_registrations.test.ts`:
- Around line 1-556: The file under the test suite
describe('uts/rest/push/push_device_registrations') has formatting drift; run
your project's formatter (e.g. Prettier) to reformat the file, then stage and
commit the changes so format:check passes in CI; specifically reformat the test
file containing the it(...) cases like "RSH1b3 - save sends PUT to
/push/deviceRegistrations/{id}" and "RSH1b1 - get returns device object" and
ensure no lint/format scripts report changes before pushing.
---
Duplicate comments:
In `@test/uts/rest/channel/rest_channel_attributes.test.ts`:
- Around line 105-106: The test currently uses
expect(captured[0].path).to.contain(encodeURIComponent('namespace:my channel'))
which can match the encoded name appearing anywhere; update the assertion to
compare the full endpoint shape by asserting equality against the exact path
string '/channels/' + encodeURIComponent('namespace:my channel') so that
captured[0].path exactly equals the expected '/channels/{encodedChannel}' value
(referencing the captured variable and the test assertion on captured[0].path).
🪄 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: c0679ac5-8f7a-48a5-b593-a90b1c4bc899
📒 Files selected for processing (43)
src/common/lib/client/realtimechannel.tssrc/common/lib/transport/connectionmanager.tssrc/common/lib/transport/transport.tstest/uts/helpers.tstest/uts/mock_http.tstest/uts/realtime/time.test.tstest/uts/rest/auth/auth_callback.test.tstest/uts/rest/auth/auth_scheme.test.tstest/uts/rest/auth/authorize.test.tstest/uts/rest/auth/client_id.test.tstest/uts/rest/auth/revoke_tokens.test.tstest/uts/rest/auth/token_details.test.tstest/uts/rest/auth/token_renewal.test.tstest/uts/rest/auth/token_request_params.test.tstest/uts/rest/batch_presence.test.tstest/uts/rest/batch_publish.test.tstest/uts/rest/channel/annotations.test.tstest/uts/rest/channel/get_message.test.tstest/uts/rest/channel/history.test.tstest/uts/rest/channel/idempotency.test.tstest/uts/rest/channel/message_versions.test.tstest/uts/rest/channel/publish.test.tstest/uts/rest/channel/publish_result.test.tstest/uts/rest/channel/rest_channel_attributes.test.tstest/uts/rest/channel/update_delete_message.test.tstest/uts/rest/encoding/message_encoding.test.tstest/uts/rest/fallback.test.tstest/uts/rest/logging.test.tstest/uts/rest/presence/rest_presence.test.tstest/uts/rest/push/push_admin_publish.test.tstest/uts/rest/push/push_channel_subscriptions.test.tstest/uts/rest/push/push_device_registrations.test.tstest/uts/rest/request.test.tstest/uts/rest/request_endpoint.test.tstest/uts/rest/rest_client.test.tstest/uts/rest/stats.test.tstest/uts/rest/time.test.tstest/uts/rest/types/error_types.test.tstest/uts/rest/types/message_types.test.tstest/uts/rest/types/mutable_message_types.test.tstest/uts/rest/types/paginated_result.test.tstest/uts/rest/types/presence_message_types.test.tstest/uts/rest/types/token_types.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/uts/rest/batch_presence.test.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- test/uts/rest/auth/auth_scheme.test.ts
- test/uts/rest/types/message_types.test.ts
- test/uts/rest/types/error_types.test.ts
- test/uts/rest/channel/publish.test.ts
- test/uts/rest/channel/idempotency.test.ts
- test/uts/rest/request_endpoint.test.ts
- test/uts/realtime/time.test.ts
- test/uts/rest/stats.test.ts
- test/uts/rest/request.test.ts
- test/uts/rest/time.test.ts
- test/uts/rest/channel/message_versions.test.ts
- test/uts/rest/types/paginated_result.test.ts
- src/common/lib/transport/transport.ts
- test/uts/rest/auth/authorize.test.ts
- test/uts/rest/logging.test.ts
- test/uts/rest/types/mutable_message_types.test.ts
- test/uts/rest/auth/revoke_tokens.test.ts
- test/uts/rest/auth/token_request_params.test.ts
Comment on lines
+294
to
+303
| shouldFallback(error: any): boolean { | ||
| if (!error) return false; | ||
| const code = error.code; | ||
| const statusCode = error.statusCode; | ||
| if (code === 'ECONNREFUSED' || code === 'ENETUNREACH' || code === 'EHOSTUNREACH' || | ||
| code === 'ETIMEDOUT' || code === 'ECONNRESET' || code === 'ENOTFOUND') { | ||
| return true; | ||
| } | ||
| return statusCode >= 500 && statusCode <= 504; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify fallback-code parity between node platform HTTP and UTS mock.
sed -n '129,150p' src/platform/nodejs/lib/util/http.ts
echo "-----"
sed -n '294,304p' test/uts/mock_http.tsRepository: ably/ably-js
Length of output: 1167
shouldFallback() mock is missing platform error codes.
The UTS mock omits EHOSTDOWN and ESOCKETTIMEDOUT, which are present in the node platform implementation (src/platform/nodejs/lib/util/http.ts). This divergence could cause test behavior to differ from production when these error codes are encountered.
Suggested fix
shouldFallback(error: any): boolean {
if (!error) return false;
const code = error.code;
const statusCode = error.statusCode;
- if (code === 'ECONNREFUSED' || code === 'ENETUNREACH' || code === 'EHOSTUNREACH' ||
- code === 'ETIMEDOUT' || code === 'ECONNRESET' || code === 'ENOTFOUND') {
+ if (
+ code === 'ECONNREFUSED' ||
+ code === 'ENETUNREACH' ||
+ code === 'EHOSTUNREACH' ||
+ code === 'EHOSTDOWN' ||
+ code === 'ETIMEDOUT' ||
+ code === 'ESOCKETTIMEDOUT' ||
+ code === 'ECONNRESET' ||
+ code === 'ENOTFOUND'
+ ) {
return true;
}
return statusCode >= 500 && statusCode <= 504;
}📝 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
| shouldFallback(error: any): boolean { | |
| if (!error) return false; | |
| const code = error.code; | |
| const statusCode = error.statusCode; | |
| if (code === 'ECONNREFUSED' || code === 'ENETUNREACH' || code === 'EHOSTUNREACH' || | |
| code === 'ETIMEDOUT' || code === 'ECONNRESET' || code === 'ENOTFOUND') { | |
| return true; | |
| } | |
| return statusCode >= 500 && statusCode <= 504; | |
| } | |
| shouldFallback(error: any): boolean { | |
| if (!error) return false; | |
| const code = error.code; | |
| const statusCode = error.statusCode; | |
| if ( | |
| code === 'ECONNREFUSED' || | |
| code === 'ENETUNREACH' || | |
| code === 'EHOSTUNREACH' || | |
| code === 'EHOSTDOWN' || | |
| code === 'ETIMEDOUT' || | |
| code === 'ESOCKETTIMEDOUT' || | |
| code === 'ECONNRESET' || | |
| code === 'ENOTFOUND' | |
| ) { | |
| return true; | |
| } | |
| return statusCode >= 500 && statusCode <= 504; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/mock_http.ts` around lines 294 - 303, The mock implementation of
shouldFallback in mock_http.ts is missing platform error codes EHOSTDOWN and
ESOCKETTIMEDOUT; update the shouldFallback(error: any) function to treat
error.code === 'EHOSTDOWN' and error.code === 'ESOCKETTIMEDOUT' as fallbacks
alongside the existing codes (ECONNREFUSED, ENETUNREACH, EHOSTUNREACH,
ETIMEDOUT, ECONNRESET, ENOTFOUND), and keep the existing statusCode >= 500 &&
statusCode <= 504 check (guarding against undefined statusCode as currently
done).
Comment on lines
+116
to
+120
| expect(captured).to.have.length(1); | ||
| expect(captured[0].method).to.equal('get'); | ||
| // The serial must be URL-encoded in the path | ||
| expect(captured[0].path).to.include(encodeURIComponent('serial/with:special+chars')); | ||
| expect(captured[0].path).to.not.include('serial/with:special+chars'); |
There was a problem hiding this comment.
Check the full message path, not just substring presence.
This assertion can still pass if the encoded serial appears in the wrong segment. Compare against the exact /channels/<name>/messages/<serial> path so the route shape is validated too.
Suggested fix
- expect(captured[0].path).to.include(encodeURIComponent('serial/with:special+chars'));
- expect(captured[0].path).to.not.include('serial/with:special+chars');
+ expect(captured[0].path).to.equal('/channels/test/messages/' + encodeURIComponent('serial/with:special+chars'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/channel/get_message.test.ts` around lines 116 - 120, The test
currently only checks substring presence of the encoded serial in
captured[0].path which can miss route shape errors; change the assertion to
compare the entire path string: build the exact expected path using the channel
name and the encoded serial (e.g.
`/channels/${channelName}/messages/${encodeURIComponent(serial)}`) and assert
captured[0].path equals that expectedPath (replace the current
include/not.include checks for 'serial/with:special+chars' with an equality
check against the full `/channels/<name>/messages/<encodedSerial>` value).
Comment on lines
+1
to
+259
| /** | ||
| * UTS: REST Client Tests | ||
| * | ||
| * Spec points: RSC5, RSC7, RSC7c, RSC7d, RSC7e, RSC8a-c, RSC17, RSC18 | ||
| * Source: uts/test/rest/unit/rest_client.md | ||
| */ | ||
|
|
||
| import { expect } from 'chai'; | ||
| import { MockHttpClient } from '../mock_http'; | ||
| import { Ably, installMockHttp, restoreAll } from '../helpers'; | ||
|
|
||
| describe('uts/rest/rest_client', function () { | ||
| afterEach(function () { | ||
| restoreAll(); | ||
| }); | ||
|
|
||
| /** | ||
| * RSC5 - Auth attribute accessible | ||
| */ | ||
| it('RSC5 - client.auth is accessible', function () { | ||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret' }); | ||
| expect(client.auth).to.not.be.null; | ||
| expect(client.auth).to.not.be.undefined; | ||
| }); | ||
|
|
||
| /** | ||
| * RSC7e - X-Ably-Version header | ||
| * | ||
| * All REST requests must include the X-Ably-Version header with a version string. | ||
| */ | ||
| it('RSC7e - X-Ably-Version header is sent', async function () { | ||
| const captured: any[] = []; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| captured.push(req); | ||
| req.respond_with(200, [1234567890000]); | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret' }); | ||
| await client.time(); | ||
|
|
||
| expect(captured).to.have.length(1); | ||
| // ably-js sends headers with their original casing | ||
| expect(captured[0].headers).to.have.property('X-Ably-Version'); | ||
| expect(captured[0].headers['X-Ably-Version']).to.match(/[0-9.]+/); | ||
| }); | ||
|
|
||
| /** | ||
| * RSC7d - Ably-Agent header | ||
| * | ||
| * All REST requests must include the Ably-Agent header identifying the library. | ||
| */ | ||
| it('RSC7d - Ably-Agent header is sent', async function () { | ||
| const captured: any[] = []; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| captured.push(req); | ||
| req.respond_with(200, [1234567890000]); | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret' }); | ||
| await client.time(); | ||
|
|
||
| expect(captured).to.have.length(1); | ||
| expect(captured[0].headers).to.have.property('Ably-Agent'); | ||
| expect(captured[0].headers['Ably-Agent']).to.match(/ably-js\/[0-9]+\.[0-9]+/); | ||
| }); | ||
|
|
||
| /** | ||
| * RSC7c - Request ID when addRequestIds enabled | ||
| * | ||
| * When addRequestIds is true, all requests must include a request_id query parameter. | ||
| */ | ||
| /** | ||
| * NOTE: ably-js accepts addRequestIds option but does not implement it. | ||
| * The option is stored but no request_id parameter is added to requests. | ||
| * See deviations.md. | ||
| */ | ||
| it('RSC7c - request_id query param when addRequestIds is true', async function () { | ||
| // DEVIATION: see deviations.md | ||
| this.skip(); | ||
| const captured: any[] = []; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| captured.push(req); | ||
| req.respond_with(200, [1234567890000]); | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret', addRequestIds: true } as any); | ||
| await client.time(); | ||
|
|
||
| expect(captured).to.have.length(1); | ||
| const requestId = captured[0].url.searchParams.get('request_id'); | ||
| expect(requestId).to.be.a('string'); | ||
| expect(requestId.length).to.be.at.least(12); | ||
| }); | ||
|
|
||
| /** | ||
| * RSC8a/RSC8b - Protocol content type | ||
| * | ||
| * With useBinaryProtocol: false, Content-Type should be application/json. | ||
| */ | ||
| it('RSC8a/RSC8b - JSON content type when useBinaryProtocol is false', async function () { | ||
| const captured: any[] = []; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| captured.push(req); | ||
| req.respond_with(201, { serials: ['s1'] }); | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret', useBinaryProtocol: false }); | ||
| await client.channels.get('test').publish('e', 'd'); | ||
|
|
||
| expect(captured).to.have.length(1); | ||
| expect(captured[0].headers['content-type']).to.include('application/json'); | ||
| }); | ||
|
|
||
| /** | ||
| * RSC8c - Accept header | ||
| * | ||
| * Accept header must match the configured protocol. | ||
| */ | ||
| it('RSC8c - Accept header is application/json', async function () { | ||
| const captured: any[] = []; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| captured.push(req); | ||
| req.respond_with(201, { serials: ['s1'] }); | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret', useBinaryProtocol: false }); | ||
| await client.channels.get('test').publish('e', 'd'); | ||
|
|
||
| expect(captured).to.have.length(1); | ||
| expect(captured[0].headers['accept']).to.include('application/json'); | ||
| }); | ||
|
|
||
| /** | ||
| * RSC17 - clientId attribute | ||
| * | ||
| * When clientId is set in ClientOptions, Auth#clientId reflects it. | ||
| */ | ||
| it('RSC17 - clientId from options is accessible via auth.clientId', function () { | ||
| const client = new Ably.Rest({ | ||
| key: 'appId.keyId:keySecret', | ||
| clientId: 'explicit-client', | ||
| }); | ||
| expect(client.auth.clientId).to.equal('explicit-client'); | ||
| }); | ||
|
|
||
| /** | ||
| * RSC18 - TLS: true uses HTTPS (default) | ||
| */ | ||
| it('RSC18 - default TLS uses HTTPS', async function () { | ||
| const captured: any[] = []; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| captured.push(req); | ||
| req.respond_with(200, [1234567890000]); | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret' }); | ||
| await client.time(); | ||
|
|
||
| expect(captured).to.have.length(1); | ||
| expect(captured[0].url.protocol).to.equal('https:'); | ||
| }); | ||
|
|
||
| /** | ||
| * RSC18 - TLS: false uses HTTP | ||
| */ | ||
| it('RSC18 - tls:false uses HTTP', async function () { | ||
| const captured: any[] = []; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| captured.push(req); | ||
| req.respond_with(200, [1234567890000]); | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ token: 'tok', tls: false }); | ||
| await client.time(); | ||
|
|
||
| expect(captured).to.have.length(1); | ||
| expect(captured[0].url.protocol).to.equal('http:'); | ||
| }); | ||
|
|
||
| /** | ||
| * RSC6 - stats() basic request | ||
| * | ||
| * Verify that stats() sends a GET request to /stats. | ||
| */ | ||
| it('RSC6 - stats() sends GET /stats', async function () { | ||
| const captured: any[] = []; | ||
| const mock = new MockHttpClient({ | ||
| onConnectionAttempt: (conn) => conn.respond_with_success(), | ||
| onRequest: (req) => { | ||
| captured.push(req); | ||
| req.respond_with(200, []); | ||
| }, | ||
| }); | ||
| installMockHttp(mock); | ||
|
|
||
| const client = new Ably.Rest({ key: 'appId.keyId:keySecret' }); | ||
| try { | ||
| await client.stats({} as any); | ||
| } catch (e) { | ||
| // Response parsing may fail — we only care about the request | ||
| } | ||
|
|
||
| expect(captured).to.have.length.at.least(1); | ||
| expect(captured[0].method.toUpperCase()).to.equal('GET'); | ||
| expect(captured[0].path).to.equal('/stats'); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // MsgPack tests — PENDING (mock HTTP does not support msgpack encoding) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| it('RSC8a - default msgpack protocol Content-Type', function () { | ||
| // PENDING: Requires mock msgpack encoding support. See deviations.md. | ||
| this.skip(); | ||
| }); | ||
|
|
||
| it('RSC8d - mismatched Content-Type response decoded', function () { | ||
| // PENDING: Requires mock msgpack encoding support. See deviations.md. | ||
| this.skip(); | ||
| }); | ||
|
|
||
| it('RSC8e - unsupported Content-Type response error', function () { | ||
| // PENDING: Requires mock msgpack encoding support. See deviations.md. | ||
| this.skip(); | ||
| }); | ||
|
|
||
| it('RSC8 - msgpack error response decoded', function () { | ||
| // PENDING: Requires mock msgpack encoding support. See deviations.md. | ||
| this.skip(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Run Prettier before merge.
format:check is still failing on this file, so CI will stay red until the formatting drift is fixed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/rest_client.test.ts` around lines 1 - 259, The test file
formatting has drifted (format:check failing); run the repository formatter
(e.g., run the project's format script such as npm run format or yarn format, or
run prettier --write on the tests) to reformat the test suite including the
top-level describe('uts/rest/rest_client') and its it(...) blocks, re-run
format:check (or CI) to verify it passes, and commit the formatted changes
before merging.
Comment on lines
+1
to
+259
| /** | ||
| * UTS: PresenceMessage Type Tests | ||
| * | ||
| * Spec points: TP1, TP2, TP3, TP3a-TP3i, TP4, TP5 | ||
| * Source: uts/test/rest/unit/types/presence_message_types.md | ||
| */ | ||
|
|
||
| import { expect } from 'chai'; | ||
| import { Ably } from '../../helpers'; | ||
|
|
||
| describe('uts/rest/types/presence_message_types', function () { | ||
| /** | ||
| * TP2 - PresenceAction values | ||
| * | ||
| * PresenceAction enum: absent (0), present (1), enter (2), leave (3), update (4). | ||
| * In ably-js, application code uses string actions. | ||
| */ | ||
| it('TP2 - PresenceAction values', function () { | ||
| const actionStrings = ['absent', 'present', 'enter', 'leave', 'update']; | ||
|
|
||
| actionStrings.forEach(function (actionStr) { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ action: actionStr }); | ||
| expect(pm.action).to.equal(actionStr); | ||
| }); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3a - id attribute | ||
| */ | ||
| it('TP3a - id attribute', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ id: 'pm-1' }); | ||
| expect(pm.id).to.equal('pm-1'); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3b - action attribute | ||
| */ | ||
| it('TP3b - action attribute', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ action: 'enter' }); | ||
| expect(pm.action).to.equal('enter'); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3c - clientId attribute | ||
| */ | ||
| it('TP3c - clientId attribute', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ clientId: 'user-1' }); | ||
| expect(pm.clientId).to.equal('user-1'); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3d - connectionId attribute | ||
| */ | ||
| it('TP3d - connectionId attribute', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ connectionId: 'conn-1' }); | ||
| expect(pm.connectionId).to.equal('conn-1'); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3e - data attribute (string) | ||
| */ | ||
| it('TP3e - data attribute (string)', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ data: 'hello' }); | ||
| expect(pm.data).to.equal('hello'); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3e - data attribute (object) | ||
| */ | ||
| it('TP3e - data attribute (object)', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ data: { key: 'val' } }); | ||
| expect(pm.data).to.deep.equal({ key: 'val' }); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3f - encoding attribute | ||
| */ | ||
| it('TP3f - encoding attribute', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ encoding: 'json' }); | ||
| expect(pm.encoding).to.equal('json'); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3g - timestamp attribute | ||
| */ | ||
| it('TP3g - timestamp attribute', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ timestamp: 1234567890000 }); | ||
| expect(pm.timestamp).to.equal(1234567890000); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3i - extras attribute | ||
| */ | ||
| it('TP3i - extras attribute', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ | ||
| extras: { headers: { 'x-custom': 'value' } }, | ||
| }); | ||
| expect(pm.extras.headers['x-custom']).to.equal('value'); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3h - memberKey combines connectionId and clientId | ||
| * | ||
| * Per spec, memberKey is a "string function that combines the connectionId | ||
| * and clientId ensuring multiple connected clients with the same clientId | ||
| * are uniquely identifiable." | ||
| */ | ||
| it('TP3h - memberKey format', function () { | ||
| // DEVIATION: see deviations.md | ||
| this.skip(); | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ | ||
| connectionId: 'conn-1', | ||
| clientId: 'client-1', | ||
| }); | ||
|
|
||
| expect(typeof (pm as any).memberKey).to.equal('string'); | ||
| expect((pm as any).memberKey).to.equal('conn-1:client-1'); | ||
|
|
||
| const pm2 = Ably.Rest.PresenceMessage.fromValues({ | ||
| connectionId: 'conn-2', | ||
| clientId: 'client-1', | ||
| }); | ||
|
|
||
| expect((pm2 as any).memberKey).to.equal('conn-2:client-1'); | ||
| expect((pm as any).memberKey).to.not.equal((pm2 as any).memberKey); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3 - deserialization from wire format via fromEncoded | ||
| * | ||
| * Wire format uses numeric action (2 = enter). fromEncoded decodes to string action. | ||
| */ | ||
| it('TP3 - deserialization from wire via fromEncoded', async function () { | ||
| const pm = await Ably.Rest.PresenceMessage.fromEncoded({ | ||
| action: 2, | ||
| clientId: 'test', | ||
| data: 'hi', | ||
| }); | ||
|
|
||
| expect(pm.action).to.equal('enter'); | ||
| expect(pm.clientId).to.equal('test'); | ||
| expect(pm.data).to.equal('hi'); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3 - wire numeric actions decode to correct strings | ||
| */ | ||
| it('TP3 - all wire action values decode correctly', async function () { | ||
| const expected = [ | ||
| { wire: 0, str: 'absent' }, | ||
| { wire: 1, str: 'present' }, | ||
| { wire: 2, str: 'enter' }, | ||
| { wire: 3, str: 'leave' }, | ||
| { wire: 4, str: 'update' }, | ||
| ]; | ||
|
|
||
| for (const tc of expected) { | ||
| const pm = await Ably.Rest.PresenceMessage.fromEncoded({ | ||
| action: tc.wire, | ||
| clientId: 'user', | ||
| }); | ||
| expect(pm.action).to.equal(tc.str, 'wire action ' + tc.wire + ' should decode to ' + tc.str); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * TP4 - fromEncoded with JSON-encoded data | ||
| * | ||
| * fromEncoded decodes data based on the encoding field. | ||
| */ | ||
| it('TP4 - fromEncoded decodes json-encoded data', async function () { | ||
| const pm = await Ably.Rest.PresenceMessage.fromEncoded({ | ||
| action: 2, | ||
| clientId: 'user-1', | ||
| data: '{"status":"online"}', | ||
| encoding: 'json', | ||
| }); | ||
|
|
||
| expect(pm.data).to.deep.equal({ status: 'online' }); | ||
| // Encoding should be consumed after decoding | ||
| expect(pm.encoding).to.be.null; | ||
| }); | ||
|
|
||
| /** | ||
| * TP4 - fromEncodedArray | ||
| * | ||
| * Decodes an array of wire-format presence messages. | ||
| */ | ||
| it('TP4 - fromEncodedArray', async function () { | ||
| const messages = await Ably.Rest.PresenceMessage.fromEncodedArray([ | ||
| { action: 2, clientId: 'alice', data: 'hello' }, | ||
| { action: 2, clientId: 'bob', data: 'world' }, | ||
| ]); | ||
|
|
||
| expect(messages).to.have.lengthOf(2); | ||
| expect(messages[0].clientId).to.equal('alice'); | ||
| expect(messages[0].data).to.equal('hello'); | ||
| expect(messages[1].clientId).to.equal('bob'); | ||
| expect(messages[1].data).to.equal('world'); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3 - null/missing attributes are undefined | ||
| * | ||
| * When fromEncoded receives a minimal presence message (only action), | ||
| * unspecified attributes should be null or undefined. | ||
| */ | ||
| it('TP3 - null/missing attributes are undefined', async function () { | ||
| const pm = await Ably.Rest.PresenceMessage.fromEncoded({ action: 1 }); | ||
|
|
||
| expect(pm.action).to.equal('present'); | ||
| // clientId, connectionId, data should be null or undefined | ||
| expect(pm.clientId).to.satisfy((v: any) => v === null || v === undefined); | ||
| expect(pm.connectionId).to.satisfy((v: any) => v === null || v === undefined); | ||
| expect(pm.data).to.satisfy((v: any) => v === null || v === undefined); | ||
| }); | ||
|
|
||
| /** | ||
| * TP3 - timestamp as number | ||
| * | ||
| * When fromEncoded receives a presence message with a numeric timestamp, | ||
| * it should be preserved as-is. | ||
| */ | ||
| it('TP3 - timestamp as number', async function () { | ||
| const pm = await Ably.Rest.PresenceMessage.fromEncoded({ | ||
| action: 1, | ||
| timestamp: 1700000000000, | ||
| }); | ||
|
|
||
| expect(pm.action).to.equal('present'); | ||
| expect(pm.timestamp).to.equal(1700000000000); | ||
| }); | ||
|
|
||
| /** | ||
| * TP - presence message with data exists as complete object | ||
| * | ||
| * Construct a PresenceMessage with data and verify it has all | ||
| * the expected properties of a complete presence message. | ||
| */ | ||
| it('TP - presence message with data is a complete object', function () { | ||
| const pm = Ably.Rest.PresenceMessage.fromValues({ | ||
| action: 'enter', | ||
| clientId: 'user-1', | ||
| connectionId: 'conn-1', | ||
| data: { status: 'online', role: 'admin' }, | ||
| timestamp: 1700000000000, | ||
| id: 'pm-full', | ||
| encoding: null, | ||
| }); | ||
|
|
||
| expect(pm).to.be.an('object'); | ||
| expect(pm.action).to.equal('enter'); | ||
| expect(pm.clientId).to.equal('user-1'); | ||
| expect(pm.connectionId).to.equal('conn-1'); | ||
| expect(pm.data).to.deep.equal({ status: 'online', role: 'admin' }); | ||
| expect(pm.timestamp).to.equal(1700000000000); | ||
| expect(pm.id).to.equal('pm-full'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Run Prettier before merge.
format:check is still failing on this file, so CI will stay red until the formatting drift is fixed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/types/presence_message_types.test.ts` around lines 1 - 259, The
file test/uts/rest/types/presence_message_types.test.ts fails the project's
formatting check; run the project's Prettier command (e.g., npm run format or
npx prettier --write) on this file (the
describe('uts/rest/types/presence_message_types' ...) and its contained it(...)
blocks such as 'TP2 - PresenceAction values' and 'TP3 - deserialization from
wire via fromEncoded') to fix formatting drift, re-run format:check, and commit
the reformatted file so CI passes.
Replace unconditional this.skip() in 27 deviation tests with `if (!process.env.RUN_DEVIATIONS) this.skip()` so that each deviation can be reproduced on demand: RUN_DEVIATIONS=1 npx mocha --grep "RSA7b" test/uts/rest/auth/client_id.test.ts Normal test runs are unchanged (464 passing, 37 pending). With RUN_DEVIATIONS=1: 464 passing, 10 pending, 27 failing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
test/uts/mock_http.ts (2)
313-320:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
shouldFallback()is missing platform fallback codes.Line 313-320 omits
EHOSTDOWNandESOCKETTIMEDOUT, so mock behavior can diverge from runtime fallback logic.Suggested fix
if ( code === 'ECONNREFUSED' || code === 'ENETUNREACH' || code === 'EHOSTUNREACH' || + code === 'EHOSTDOWN' || code === 'ETIMEDOUT' || + code === 'ESOCKETTIMEDOUT' || code === 'ECONNRESET' || code === 'ENOTFOUND' ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/mock_http.ts` around lines 313 - 320, The platform fallback check in shouldFallback currently tests for error codes 'ECONNREFUSED','ENETUNREACH','EHOSTUNREACH','ETIMEDOUT','ECONNRESET','ENOTFOUND' but omits two runtime fallback codes; update the conditional in shouldFallback (in mock_http.ts) to also include 'EHOSTDOWN' and 'ESOCKETTIMEDOUT' so the mock matches runtime fallback behavior.
181-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTimeout path leaks waiters and can cause cross-test event theft.
On Line 184 and Line 196, timeout rejection leaves the corresponding waiter in queue; a later event can resolve that stale waiter instead of the active one.
Suggested fix
await_connection_attempt(timeout?: number): Promise<PendingConnection> { return new Promise((resolve, reject) => { - const timer = timeout - ? setTimeout(() => reject(new Error('Timeout waiting for connection attempt')), timeout) - : null; - this._connectionWaiters.push((conn) => { + const waiter = (conn: PendingConnection) => { if (timer) clearTimeout(timer); resolve(conn); - }); + }; + const timer = timeout + ? setTimeout(() => { + const i = this._connectionWaiters.indexOf(waiter); + if (i >= 0) this._connectionWaiters.splice(i, 1); + reject(new Error('Timeout waiting for connection attempt')); + }, timeout) + : null; + this._connectionWaiters.push(waiter); }); } await_request(timeout?: number): Promise<PendingRequest> { return new Promise((resolve, reject) => { - const timer = timeout ? setTimeout(() => reject(new Error('Timeout waiting for request')), timeout) : null; - this._requestWaiters.push((req) => { + const waiter = (req: PendingRequest) => { if (timer) clearTimeout(timer); resolve(req); - }); + }; + const timer = timeout + ? setTimeout(() => { + const i = this._requestWaiters.indexOf(waiter); + if (i >= 0) this._requestWaiters.splice(i, 1); + reject(new Error('Timeout waiting for request')); + }, timeout) + : null; + this._requestWaiters.push(waiter); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/mock_http.ts` around lines 181 - 201, The timeout rejection paths in await_connection_attempt and await_request leave their waiter callbacks in _connectionWaiters/_requestWaiters, allowing later events to resolve stale waiters; change both methods to register a removable wrapper (or capture the index) when pushing into the waiters array and, in the timeout handler, remove that specific wrapper from the array before rejecting, and likewise when the wrapper is invoked to resolve, clear the timeout and remove any remaining reference; update logic in await_connection_attempt and await_request to ensure the waiter is always removed on either resolve or reject.test/uts/rest/channel/publish.test.ts (1)
425-430:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
RSL1e - both name and data nullstill lacks behavioral assertions.Line 429 states intent but does not assert
name/datanull-or-omitted semantics, so this test can pass without validating the spec behavior.Suggested fix
expect(captured).to.have.length(1); const body = JSON.parse(captured[0].body); expect(body).to.be.an('array'); expect(body).to.have.length(1); - // The message should be essentially empty (name and data are null/missing) + const message = body[0]; + expect('name' in message ? message.name : null).to.equal(null); + expect('data' in message ? message.data : null).to.equal(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/channel/publish.test.ts` around lines 425 - 430, The test currently parses captured[0].body into body and checks its length but doesn't assert the RSL1e semantics for the message's name/data; update the test (the block handling captured and body in this spec) to inspect body[0] and assert that both name and data are either missing or null by checking each property on body[0] with assertions that allow either absence or a null value (e.g., use expect(body[0]).to.not.have.property('name') || expect(body[0].name).to.be.null and similarly for data), thereby enforcing the intended null-or-omitted behavior for the message in the RSL1e test.test/uts/rest/channel/update_delete_message.test.ts (1)
360-362:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse exact path equality for the encoded serial check.
Line 362 uses
include(...), which won’t catch wrong path prefix/suffix or accidental double-encoding.Suggested fix
- // The path should contain the URL-encoded serial - expect(captured[0].path).to.include(encodeURIComponent('serial/special:chars')); + expect(captured[0].path).to.equal( + '/channels/test-channel/messages/' + encodeURIComponent('serial/special:chars'), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/channel/update_delete_message.test.ts` around lines 360 - 362, Replace the loose "include" assertion with an exact equality check: compute the expected path string using encodeURIComponent('serial/special:chars') and assert expect(captured[0].path).to.equal(expectedPath) instead of expect(...).to.include(...); update the test around the captured variable so expectedPath uses the same request prefix used elsewhere in the test and concatenates the encoded serial exactly (e.g., const expectedPath = `<requestPrefix>/${encodeURIComponent('serial/special:chars')}`) and then assert equality on captured[0].path.test/uts/README.md (1)
152-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the directory-tree fence.
The bare fence still triggers MD040, so docs lint will keep failing until the block is marked as plain text. This is the same issue called out previously.
Suggested patch
-``` +```text test/uts/ README.md # This file helpers.ts # install/uninstall, FakeClock, Ably re-export @@ -``` +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/README.md` at line 152, The fenced directory-tree block in test/uts/README.md is missing a language tag and triggers MD040; update the opening fence for that block to include a language tag (e.g., change ``` to ```text) so the snippet is treated as plain text and the docs lint stops failing.test/uts/rest/channel/annotations.test.ts (1)
242-243:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse exact path equality for annotation endpoints.
.include('/messages/.../annotations')can still pass on malformed paths, so these checks should pin the full endpoint. This is the same regression noted in the earlier review.Suggested patch
- expect(captured[0].path).to.include('/messages/msg-serial-1/annotations'); + expect(captured[0].path).to.equal('/channels/test/messages/msg-serial-1/annotations');- expect(captured[0].path).to.include('/messages/msg-serial-1/annotations'); + expect(captured[0].path).to.equal('/channels/test/messages/msg-serial-1/annotations');Also applies to: 287-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/channel/annotations.test.ts` around lines 242 - 243, The test is using a loose substring assertion for the annotation endpoint; change the assertions to check exact equality of the full path to avoid false positives. Replace the `.include('/messages/msg-serial-1/annotations')` assertion on `captured[0].path` with an equality assertion against the exact expected string (e.g., `expect(captured[0].path).to.equal('/messages/msg-serial-1/annotations')`), and make the same change for the other occurrence referenced (the assertion around lines 287-288, likely `captured[1].path` or similar) so both annotation endpoint checks assert full-path equality instead of using `.include`.test/uts/rest/presence/rest_presence.test.ts (1)
511-545:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert the actual
next()target.
page1.next()is only checked indirectly, so a wrong Link target could still slip through. Capture the second request and pin its path/query to the Link URL. This repeats the earlier pagination concern.Suggested hardening
it('RSP pagination - history() navigates pages via next()', async function () { let reqCount = 0; + const captured: any[] = []; const mock = new MockHttpClient({ onConnectionAttempt: (conn) => conn.respond_with_success(), onRequest: (req) => { + captured.push(req); reqCount++; if (reqCount === 1) { - req.respond_with(200, [{ action: 2, clientId: 'alice', timestamp: 1609459200000 }], { - Link: '<./presence?cursor=page2&limit=1>; rel="next"', - }); + req.respond_with(200, [{ action: 2, clientId: 'alice', timestamp: 1609459200000 }], { + Link: '</channels/test/presence/history?cursor=page2&limit=1>; rel="next"', + }); } else { req.respond_with(200, [{ action: 3, clientId: 'bob', timestamp: 1609459100000 }]); } }, }); @@ // Second page const page2 = await page1.next(); + expect(captured).to.have.length(2); + expect(captured[1].path).to.equal('/channels/test/presence/history'); + expect(captured[1].url.searchParams.get('cursor')).to.equal('page2'); + expect(captured[1].url.searchParams.get('limit')).to.equal('1'); expect(page2!.items).to.have.length(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/presence/rest_presence.test.ts` around lines 511 - 545, The test should explicitly assert that the second HTTP request uses the Link target from the first response instead of relying only on page behavior; modify the MockHttpClient onRequest handler (the onRequest callback) to capture the request details for reqCount === 2 and assert the request path/query equals the Link value from the first response (e.g., the expected "/presence?cursor=page2&limit=1"), or only respond with the second page when that exact URL is requested; reference MockHttpClient.onRequest, the Link header set in the first response, and page1.next() to locate where to add this check.test/uts/rest/request.test.ts (1)
442-447:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the connection-refused assertion.
expect.fail()is inside the sametry, so it gets swallowed by thecatchand the test can pass even whenclient.request()does not throw. This is the same false-positive pattern flagged before.Suggested patch
- try { - await client.request('GET', '/test', 3, null as any, null as any, null as any); - expect.fail('Expected request to throw on connection refused'); - } catch (error: any) { - expect(error).to.exist; - } + let threw = false; + try { + await client.request('GET', '/test', 3, null as any, null as any, null as any); + } catch (error: any) { + threw = true; + expect(error).to.exist; + } + expect(threw).to.be.true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uts/rest/request.test.ts` around lines 442 - 447, The current try/catch around client.request in request.test.ts allows expect.fail() to be swallowed by the catch; replace this pattern by asserting the promise is rejected directly (e.g., use await expect(client.request('GET', '/test', 3, null as any, null as any, null as any)).to.be.rejected() or to.be.rejectedWith(Error)) so the test fails if client.request does not throw; target the client.request call and remove the try/catch/expect.fail pattern.
🤖 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/rest/fallback.test.ts`:
- Around line 523-530: The test currently only checks the hostname of the last
captured request but doesn't prove the cached fallback was used immediately;
change the assertion to verify the request-count delta and that the specific new
capture after calling client.time() used the fallback host. Concretely: record
requestCount as countBefore, call await client.time(), assert requestCount ===
countBefore + 1 (or that requestCount increased by one), and assert that
captured[countBefore] (or captured[captured.length - 1] if you adjust indexing
to the new arrival) has url.hostname === fallbackHost so the new request itself
is verified to hit the cached fallback rather than succeeding after a primary
retry.
---
Duplicate comments:
In `@test/uts/mock_http.ts`:
- Around line 313-320: The platform fallback check in shouldFallback currently
tests for error codes
'ECONNREFUSED','ENETUNREACH','EHOSTUNREACH','ETIMEDOUT','ECONNRESET','ENOTFOUND'
but omits two runtime fallback codes; update the conditional in shouldFallback
(in mock_http.ts) to also include 'EHOSTDOWN' and 'ESOCKETTIMEDOUT' so the mock
matches runtime fallback behavior.
- Around line 181-201: The timeout rejection paths in await_connection_attempt
and await_request leave their waiter callbacks in
_connectionWaiters/_requestWaiters, allowing later events to resolve stale
waiters; change both methods to register a removable wrapper (or capture the
index) when pushing into the waiters array and, in the timeout handler, remove
that specific wrapper from the array before rejecting, and likewise when the
wrapper is invoked to resolve, clear the timeout and remove any remaining
reference; update logic in await_connection_attempt and await_request to ensure
the waiter is always removed on either resolve or reject.
In `@test/uts/README.md`:
- Line 152: The fenced directory-tree block in test/uts/README.md is missing a
language tag and triggers MD040; update the opening fence for that block to
include a language tag (e.g., change ``` to ```text) so the snippet is treated
as plain text and the docs lint stops failing.
In `@test/uts/rest/channel/annotations.test.ts`:
- Around line 242-243: The test is using a loose substring assertion for the
annotation endpoint; change the assertions to check exact equality of the full
path to avoid false positives. Replace the
`.include('/messages/msg-serial-1/annotations')` assertion on `captured[0].path`
with an equality assertion against the exact expected string (e.g.,
`expect(captured[0].path).to.equal('/messages/msg-serial-1/annotations')`), and
make the same change for the other occurrence referenced (the assertion around
lines 287-288, likely `captured[1].path` or similar) so both annotation endpoint
checks assert full-path equality instead of using `.include`.
In `@test/uts/rest/channel/publish.test.ts`:
- Around line 425-430: The test currently parses captured[0].body into body and
checks its length but doesn't assert the RSL1e semantics for the message's
name/data; update the test (the block handling captured and body in this spec)
to inspect body[0] and assert that both name and data are either missing or null
by checking each property on body[0] with assertions that allow either absence
or a null value (e.g., use expect(body[0]).to.not.have.property('name') ||
expect(body[0].name).to.be.null and similarly for data), thereby enforcing the
intended null-or-omitted behavior for the message in the RSL1e test.
In `@test/uts/rest/channel/update_delete_message.test.ts`:
- Around line 360-362: Replace the loose "include" assertion with an exact
equality check: compute the expected path string using
encodeURIComponent('serial/special:chars') and assert
expect(captured[0].path).to.equal(expectedPath) instead of
expect(...).to.include(...); update the test around the captured variable so
expectedPath uses the same request prefix used elsewhere in the test and
concatenates the encoded serial exactly (e.g., const expectedPath =
`<requestPrefix>/${encodeURIComponent('serial/special:chars')}`) and then assert
equality on captured[0].path.
In `@test/uts/rest/presence/rest_presence.test.ts`:
- Around line 511-545: The test should explicitly assert that the second HTTP
request uses the Link target from the first response instead of relying only on
page behavior; modify the MockHttpClient onRequest handler (the onRequest
callback) to capture the request details for reqCount === 2 and assert the
request path/query equals the Link value from the first response (e.g., the
expected "/presence?cursor=page2&limit=1"), or only respond with the second page
when that exact URL is requested; reference MockHttpClient.onRequest, the Link
header set in the first response, and page1.next() to locate where to add this
check.
In `@test/uts/rest/request.test.ts`:
- Around line 442-447: The current try/catch around client.request in
request.test.ts allows expect.fail() to be swallowed by the catch; replace this
pattern by asserting the promise is rejected directly (e.g., use await
expect(client.request('GET', '/test', 3, null as any, null as any, null as
any)).to.be.rejected() or to.be.rejectedWith(Error)) so the test fails if
client.request does not throw; target the client.request call and remove the
try/catch/expect.fail pattern.
🪄 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: bb93a023-ff9a-475e-879e-df0453fbeeef
📒 Files selected for processing (30)
src/common/lib/util/utils.tstest/uts/README.mdtest/uts/deviations.mdtest/uts/mock_http.tstest/uts/rest/auth/auth_callback.test.tstest/uts/rest/auth/auth_scheme.test.tstest/uts/rest/auth/authorize.test.tstest/uts/rest/auth/client_id.test.tstest/uts/rest/auth/revoke_tokens.test.tstest/uts/rest/auth/token_details.test.tstest/uts/rest/auth/token_renewal.test.tstest/uts/rest/auth/token_request_params.test.tstest/uts/rest/batch_presence.test.tstest/uts/rest/batch_publish.test.tstest/uts/rest/channel/annotations.test.tstest/uts/rest/channel/history.test.tstest/uts/rest/channel/publish.test.tstest/uts/rest/channel/update_delete_message.test.tstest/uts/rest/encoding/message_encoding.test.tstest/uts/rest/fallback.test.tstest/uts/rest/presence/rest_presence.test.tstest/uts/rest/push/push_admin_publish.test.tstest/uts/rest/push/push_channel_subscriptions.test.tstest/uts/rest/request.test.tstest/uts/rest/rest_client.test.tstest/uts/rest/stats.test.tstest/uts/rest/types/options_types.test.tstest/uts/rest/types/paginated_result.test.tstest/uts/rest/types/presence_message_types.test.tstest/uts/rest/types/token_types.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/common/lib/util/utils.ts
- test/uts/deviations.md
🚧 Files skipped from review as they are similar to previous changes (12)
- test/uts/rest/auth/token_request_params.test.ts
- test/uts/rest/auth/token_details.test.ts
- test/uts/rest/types/paginated_result.test.ts
- test/uts/rest/push/push_channel_subscriptions.test.ts
- test/uts/rest/batch_presence.test.ts
- test/uts/rest/auth/client_id.test.ts
- test/uts/rest/rest_client.test.ts
- test/uts/rest/auth/token_renewal.test.ts
- test/uts/rest/auth/revoke_tokens.test.ts
- test/uts/rest/batch_publish.test.ts
- test/uts/rest/encoding/message_encoding.test.ts
- test/uts/rest/channel/history.test.ts
Comment on lines
+523
to
+530
| // Second request: should go to cached fallback host, not primary | ||
| const countBefore = requestCount; | ||
| await client.time(); | ||
|
|
||
| // The second request should use the cached fallback host | ||
| const secondRequestHost = captured[captured.length - 1].url.hostname; | ||
| expect(secondRequestHost).to.equal(fallbackHost); | ||
| }); |
There was a problem hiding this comment.
Assert the cached fallback path, not just the final host.
As written, the second time() call can still pass after a primary retry, so the cache behavior is not actually verified. Use the request-count delta (or a separate capture) to prove the cached fallback is taken immediately.
Suggested hardening
// Second request within cache window: should go to cached fallback
const countBefore = requestCount;
await client.time();
- // The second request should use the cached fallback host
+ expect(requestCount - countBefore).to.equal(1);
const secondRequestHost = captured[captured.length - 1].url.hostname;
expect(secondRequestHost).to.equal(fallbackHost);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/uts/rest/fallback.test.ts` around lines 523 - 530, The test currently
only checks the hostname of the last captured request but doesn't prove the
cached fallback was used immediately; change the assertion to verify the
request-count delta and that the specific new capture after calling
client.time() used the fallback host. Concretely: record requestCount as
countBefore, call await client.time(), assert requestCount === countBefore + 1
(or that requestCount increased by one), and assert that captured[countBefore]
(or captured[captured.length - 1] if you adjust indexing to the new arrival) has
url.hostname === fallbackHost so the new request itself is verified to hit the
cached fallback rather than succeeding after a primary retry.
This was referenced May 1, 2026
This was referenced May 1, 2026
Member
|
@paddybyers you should add a github workflow for each of these PRs so we have the new test suite running in CI |
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
2 participants
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 work is primarily intended as a validation step for the UTS test suite. This step is scoped to unit tests for the REST API only, and the corresponding UTS tests are in ably/specification#456.
37 tests that were generated are skipped here because ably-js fails them, and I think the tests are a faithful reflection of the spec. There will be separate issues opened for them.
Later phases will add:
Please review in particular:
I'm not expecting a line-by-line review of all of the generated tests.
PR summary by Claude
Platform.Configto enable deterministic fake-timer testingmock_http.ts,helpers.ts) and test runner configurationdeviations.mdfallbackHostsUseDefault, etc.)Tests are derived from the portable UTS pseudocode specs in ably/specification#456.
Test plan
npm run test:uts— 464 passing, 37 pending, 0 failing🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation
Chores