Conversation
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) <[email protected]>
|
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.
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.