Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
umair-ably
changed the title
umair-ably
force-pushed
the
jsonFixes
branch
3 times, most recently
from
4a75d10 to
572225b
Compare
WalkthroughThis PR adds 26 new E2E test files covering happy-path scenarios for channels, rooms, spaces, auth, logs, config, integrations, and more — bringing overall E2E coverage from ~60% to ~81% of leaf commands. Alongside the tests, three small production fixes were made to unblock the new test scenarios. Changes
Review Notes
|
![claude-code-ably-assistant[bot]](https://avatars.githubusercontent.com/in/2109912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
| File | Status | Issues |
|---|---|---|
src/base-command.ts |
OK | — |
src/commands/logs/subscribe.ts |
OK | — |
src/commands/rooms/typing/keystroke.ts |
1 issue | Unhandled promise rejection |
test/helpers/e2e-test-helper.ts |
OK | — |
test/helpers/e2e-mutable-messages.ts |
OK | — |
test/unit/base/base-command.test.ts |
OK | — |
| 26 new E2E test files | OK | — |
src/commands/rooms/typing/keystroke.ts
[Bug] Unhandled promise rejection when --auto-type is false
setupRoomStatusHandler creates a failurePromise that rejects if the room transitions to RoomStatus.Failed. When --auto-type is not set, failurePromise is created (line 96) but never awaited — only the auto-type branch awaits it (line 175). If the room happens to fail between the keystroke call and process exit, the rejection has no handler and Node.js will emit an unhandled rejection warning (or error, depending on Node version).
The fix: either move setupRoomStatusHandler inside the if (flags["auto-type"]) block (it only serves the hold path), or attach a no-op catch in the else branch:
const { failurePromise } = this.setupRoomStatusHandler(room, flags, { roomName });
// ... send keystroke ...
if (flags["auto-type"]) {
await Promise.race([
this.waitAndTrackCleanup(flags, "typing", flags.duration),
failurePromise,
]);
} else {
// Normal cleanup (detach/close) won't trigger RoomStatus.Failed, but
// suppress the rejection to avoid an unhandled rejection warning.
failurePromise.catch(() => {});
}Other changes look correct
-
base-command.ts: The newABLY_API_KEYenv-var fallback inensureAppAndKey()(before the interactive/Control-API path) mirrors the existing web-CLI mode behaviour and unblocks E2E scenarios where no config file is present. The unit test update accurately reflects the new behaviour. -
logs/subscribe.ts: The removed block fetchedappConfigbut never used the result — dead code. Correct removal;createAblyRealtimeClient()handles auth independently. -
E2E tests: Auth via env vars,
--durationon subscribe runners, and--json+parseNdjsonLinesfor structured assertions are all correct patterns. ThecheckMutableMessagesSupportprobe handles eventual consistency and the 93002 feature-flag error sensibly.
One non-blocking observation: every new beforeEach block calls resetTestTracking() then immediately repeats testOutputFiles.clear(); testCommands.length = 0 — but those two lines are exactly what resetTestTracking() already does internally. Harmless, but worth a follow-up cleanup.
|
Reviewed PR with following points in mind
Shared review for the same |
2 participants
Summary
Adds 26 new E2E test files and updates the E2E test helper, covering commands across channels, rooms, spaces, auth, logs, config, integrations, and more. These are all happy-path tests verifying commands succeed and produce expected output against real Ably infrastructure.
New test files (26)
Current E2E coverage after this PR
Remaining gaps are mostly commands requiring interactive OAuth / account config (
accounts login/switch,apps current/switch,auth keys current/switch,channels inspect), subscribe variants for spaces sub-resources, and some push channel subscription CRUD.Test plan
pnpm exec eslint test/e2e/passes with 0 errorspnpm test:e2epasses against real Ably infrastructure🤖 Generated with Claude Code