Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR replaces the E2E test helper Changes
Review Notes
|
There was a problem hiding this comment.
Review: test(e2e): draw client IDs from a bounded pool
The change is well-motivated (cutting MAU from per-run UUID minting) and the implementation is clean. A few observations below.
test/helpers/e2e-test-helper.ts — pool correctness
The clientIdPoolCursors map intentionally persists across tests within the same vitest worker process. resetTestTracking() does not clear it, which is correct: if it reset on every beforeEach, a test needing two distinct clients would get the same ID on re-entry (cursor % 8 would always start at 0). The current design ensures successive calls within a test get different IDs while recycling across CI runs.
One subtle risk worth being aware of: createAblyClient() and createAblyRealtimeClient() both call getTestClientId() with the default prefix "cli-e2e-test", sharing the same cursor. If a test calls both in sequence, it gets IDs cli-e2e-test-0 and cli-e2e-test-1. That's fine for the current tests, but if future tests assert a specific client ID from these helpers, the cursor offset could cause surprises.
test/e2e/connections/connections.test.ts — Date.now() instead of UUID
The comment correctly explains why this test can't use the pool. However, Date.now() only has millisecond precision — two parallel CI runs starting at the same ms would get identical client IDs and the test could hide a failure in exactly the scenario the comment warns about. A randomUUID() here (imported from node:crypto, already available in the file) would eliminate the remaining risk without any other trade-off.
vitest.config.ts — ABLY_CLIENT_ID: "cli-e2e-default"
Correct placement: this applies only to the e2e project, and fileParallelism: false means CLI subprocesses spawned by different test files won't collide with each other within a single CI run. Works as intended.
test/e2e/auth/auth-tokens-e2e.test.ts — pooled ID in JWT
Using getTestClientId("e2e-revoke-key-client") for the JWT x-ably-clientId field is fine. Token revocation is keyed on the per-token x-ably-revocation-key (generated fresh each run), so two concurrent CI runs sharing e2e-revoke-key-client-0 cannot interfere with each other's revocation test.
Overall
The MAU cost problem is real and this fix is pragmatic. The only actionable change I'd suggest is replacing Date.now() with randomUUID() in connections.test.ts — the current comment correctly identifies the risk but Date.now() doesn't fully close it. Everything else looks good.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Update the documentation or migrate the remaining random-ID channel test.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR reduces E2E client-ID churn by using bounded ID pools and a fixed subprocess identity.
Changes:
- Adds pooled client-ID generation.
- Migrates reviewed Spaces, Rooms, Channels, and Auth tests.
- Configures
ABLY_CLIENT_IDand documents the convention. - Preserves a unique connection-lifecycle test ID.
One moderate issue remains: the documentation overlooks another channel test that still generates random client IDs.
| File | Summary |
|---|---|
vitest.config.ts |
Configures the fixed E2E subprocess identity. |
test/helpers/e2e-test-helper.ts |
Adds bounded client-ID generation. |
test/e2e/spaces/spaces-subscribe-e2e.test.ts |
Uses pooled IDs. |
test/e2e/spaces/spaces-occupancy-e2e.test.ts |
Uses pooled IDs. |
test/e2e/spaces/spaces-locations-e2e.test.ts |
Uses pooled IDs. |
test/e2e/spaces/spaces-e2e.test.ts |
Uses pooled IDs. |
test/e2e/spaces/spaces-crud-e2e.test.ts |
Uses pooled IDs. |
test/e2e/rooms/rooms-typing-e2e.test.ts |
Uses pooled IDs. |
test/e2e/rooms/rooms-reactions-e2e.test.ts |
Uses pooled IDs. |
test/e2e/rooms/rooms-presence-subscribe-e2e.test.ts |
Uses pooled IDs. |
test/e2e/rooms/rooms-presence-e2e.test.ts |
Uses pooled IDs. |
test/e2e/rooms/rooms-occupancy-e2e.test.ts |
Uses pooled IDs. |
test/e2e/rooms/rooms-messages-subscribe-e2e.test.ts |
Uses pooled IDs. |
test/e2e/rooms/rooms-messages-reactions-e2e.test.ts |
Uses pooled IDs. |
test/e2e/rooms/rooms-messages-e2e.test.ts |
Uses pooled IDs. |
test/e2e/rooms/rooms-e2e.test.ts |
Uses pooled IDs. |
test/e2e/connections/connections.test.ts |
Preserves and documents the unique lifecycle ID. |
test/e2e/channels/channel-presence-subscribe-e2e.test.ts |
Uses pooled IDs. |
test/e2e/auth/auth-tokens-e2e.test.ts |
Uses pooled token client IDs. |
docs/Testing.md |
Documents the bounded-ID convention, but omits another random-ID channel test. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| Run against real Ably services with real credentials (via env vars). These cover the entire journey — the CLI's interaction with the actual Ably service, end to end. Every command must have an E2E test. E2E tests should cover the happy path and major sad paths (e.g., invalid capabilities, nonexistent resources). They are slow and can incur costs, so use them deliberately. | ||
|
|
||
| Ably counts every distinct client ID it sees towards the account's MAU, so E2E tests never mint random client IDs: use `getTestClientId(prefix)` from `test/helpers/e2e-test-helper.ts`, which draws from a small fixed pool per prefix (successive calls return distinct IDs), and CLI subprocesses that don't pass `--client-id` act as the fixed `ABLY_CLIENT_ID` set for the `e2e` project in `vitest.config.ts`. |
Every E2E run minted fresh random client IDs against Ably's own test account, and every CLI subprocess acted as a newly generated default when its config dir was fresh, so each PR added new MAUs. - Replace `getUniqueClientId` with `getTestClientId`, which cycles through eight IDs per prefix: successive calls still return distinct IDs, so tests needing two clients get them, but runs reuse the same IDs. - Set a fixed ABLY_CLIENT_ID for the e2e project so subprocesses without --client-id share one identity. - Keep the connection-lifecycle test's ID unique on purpose: it finds its own connection by client ID, and a pooled ID could match a concurrent run's connection and hide a failure. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
b3cfc84 to
fd833ea
Compare
umair-ably
left a comment
There was a problem hiding this comment.
looks good but worth looking into #464 (comment)

Every E2E run created fresh random client IDs against Ably's own test
account, and every CLI subprocess acted as a newly generated default
when its config dir was fresh.
getUniqueClientIdwithgetTestClientId, which cyclesthrough eight IDs per prefix: successive calls still return distinct
IDs, so tests needing two clients get them, but runs reuse the same
IDs.
--client-id share one identity.
its own connection by client ID, and a pooled ID could match a
concurrent run's connection and hide a failure.