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 |
| "Maintaining typing status...", | ||
| ); | ||
| // If auto-type is enabled, keep the command running to maintain typing state | ||
| if (flags["auto-type"]) { |
There was a problem hiding this comment.
This should be in the if block above
| } | ||
|
|
||
| // Fall back to ABLY_API_KEY environment variable (for CI/scripting) | ||
| const envApiKey = process.env.ABLY_API_KEY; |
There was a problem hiding this comment.
apiKey will be defined by this point as getApiKey looks at the env var
| * a message update on a temporary channel. Returns false if error 93002 | ||
| * (mutableMessages not enabled) is returned. | ||
| */ | ||
| export async function checkMutableMessagesSupport(): Promise<boolean> { |
There was a problem hiding this comment.
This has resulted in the CI run just skipping the message-ops tests.
Mutable message "support" is universal, but any app that hasn't had a rule created for the namespace mutable-probe is going to fail. So unless we plan on creating this rule prior to the test (we don't prior to this call), this check will never be true.
Any tests needing to exercise mutable messages should create the channel rule for the known-channel, do the tests, then clean it up afterwards.
3 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