Skip to content

test(e2e): draw client IDs from a bounded pool - #464

Open
ttypic wants to merge 1 commit into
integration/mau-docsfrom
integration/mau-ci-fix
Open

ttypic wants to merge 1 commit into
integration/mau-docsfrom
integration/mau-ci-fix

Conversation

@ttypic

@ttypic ttypic commented Oct 1, 2026 •

Copy link
Copy Markdown

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.

  • 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.

@vercel

vercel Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
cli-web-cli Ready Ready Preview Oct 5, 2026 6:57pm UTC

Request Review

@claude-code-ably-assistant

Copy link
Copy Markdown

Walkthrough

This PR replaces the E2E test helper getUniqueClientId (which generated a fresh random UUID on every call) with getTestClientId, which cycles through a fixed pool of eight IDs per prefix. The motivation is MAU billing: Ably counts every distinct client ID it sees toward the account's monthly active user quota, so random IDs were accruing new MAUs on every CI run. A fixed ABLY_CLIENT_ID is also set in the Vitest E2E project config so CLI subprocesses that don't receive --client-id reuse a shared identity.

Changes

Area Files Summary
Tests – Helper test/helpers/e2e-test-helper.ts Adds getTestClientId(prefix) that draws from a pool of 8 deterministic IDs per prefix; deprecates getUniqueClientId
Tests – E2E (Rooms) rooms-e2e, rooms-messages, rooms-messages-reactions, rooms-messages-subscribe, rooms-occupancy, rooms-presence, rooms-presence-subscribe, rooms-reactions, rooms-typing (9 files) Replaces all getUniqueClientId calls with getTestClientId
Tests – E2E (Spaces) spaces-crud, spaces-e2e, spaces-locations, spaces-occupancy, spaces-subscribe (5 files) Same replacement
Tests – E2E (Other) auth-tokens-e2e, channel-presence-subscribe-e2e Same replacement
Tests – E2E (Connections) connections.test.ts Keeps a timestamp-based unique ID with an explanatory comment — intentional exception
Config vitest.config.ts Adds ABLY_CLIENT_ID fixed env var to the E2E project so subprocesses share one identity
Docs docs/Testing.md Documents the MAU constraint and the getTestClientId / ABLY_CLIENT_ID convention

Review Notes

  • Behavioral subtlety – pool size is 8: getTestClientId cycles modulo 8. If a single test file calls it more than 8 times with the same prefix (unlikely today but possible), successive calls wrap and return already-used IDs in the same test run. Worth keeping in mind when new tests are added.
  • Intentional exception in connections test: connections.test.ts deliberately keeps a Date.now()-based unique ID. The test locates its own live connection in a log by matching client ID; reusing a pooled ID could match a connection from a concurrent parallel run and produce a false positive. The comment in the diff explains this — reviewers should verify the reasoning holds.
  • No production code changes: all changes are in test/ helpers, vitest.config.ts, and docs. There is zero risk to CLI command behavior.
  • Old getUniqueClientId export: check whether e2e-test-helper.ts still exports getUniqueClientId (for back-compat) or removes it — if it's still exported but unused, it should be removed to avoid drift.
  • Parallel CI runs: pooled IDs reduce MAU cost but mean two simultaneous CI runs (e.g., two open PRs running E2E at the same time) could share client IDs and potentially interfere in presence/occupancy tests. The trade-off appears intentional given the MAU concern, but worth noting.

@claude-code-ably-assistant claude-code-ably-assistant Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@umair-ably
umair-ably added this pull request to stack #465 October 1, 2026 14:14
@sacOO7
sacOO7 requested a lite review from Copilot October 5, 2026 09:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Update the documentation or migrate the remaining random-ID channel test.

Review effort: Lite
Findings: 1 Medium severity

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_ID and 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.

Comment thread docs/Testing.md

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>

@umair-ably umair-ably left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good but worth looking into #464 (comment)

This branch was successfully deployed

1 active deployment
Preview — fd833ea3 Deployed Oct 5, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants