[DX-947] Expand unit test coverage and add TTY test infrastructure#162
Merged
umair-ably merged 5 commits intomainfrom Mar 12, 2026
Merged
[DX-947] Expand unit test coverage and add TTY test infrastructure#162umair-ably merged 5 commits intomainfrom
umair-ably merged 5 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces TTY-dependent test infrastructure for interactive terminal testing, updates build and test documentation to reflect a new README regeneration step, expands the README with --json and --client-id flag examples across many commands, relocates flaky TTY-specific tests from unit tests to a dedicated TTY test suite, and adds extensive test coverage for help output and flag validation across numerous CLI command tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/commands/logs/connection-lifecycle/subscribe.test.ts (1)
64-75:⚠️ Potential issue | 🟡 MinorAvoid
stdoutassertions for streamed subscribe output.These cases are still verifying real-time subscribe output via the resolved
stdoutstring, which is brittle for long-running commands and can missthis.log()emissions that happen before process teardown. Please switch these human-readable cases to the samevi.spyOn(console, "log")capture pattern used in other streaming tests, and keepcaptureJsonLogs()for the JSON path. Based on learnings: In tests for streaming commands under the ably-cli repo, replace stdout-based assertions with spying on console.log to capture real-time this.log() outputs, and parse the captured lines using parseLogLines() from test/helpers/ndjson.ts.Also applies to: 129-170, 201-208, 239-245, 276-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/logs/connection-lifecycle/subscribe.test.ts` around lines 64 - 75, The test currently asserts streamed subscribe output by inspecting the resolved stdout from runCommand (see uses of runCommand, stdout, expect(channel.subscribe) and expect(stdout). Replace the human-readable stdout assertions with a console.log spy: use vi.spyOn(console, "log") to capture real-time this.log() emissions for the non-JSON path, then parse the captured lines with parseLogLines from test/helpers/ndjson.ts and assert the expected human-readable messages (e.g., "Subscribed to connection lifecycle logs"); keep captureJsonLogs() and existing JSON-path assertions for the JSON route. Apply the same replacement pattern to the other blocks noted (lines ~129-170, 201-208, 239-245, 276-282).
🟡 Minor comments (20)
test/unit/commands/support.test.ts-100-119 (1)
100-119:⚠️ Potential issue | 🟡 MinorDuplicate test — remove or consolidate.
This test is functionally identical to the existing test at lines 41-63 in the
"unknown subcommand"describe block. Both tests:
- Set
ABLY_CLI_NON_INTERACTIVEto"true"- Run
["support", "unknowncommand"]- Assert the output matches
/not found|not.*command|available/i- Restore the environment variable
Remove this duplicate test block to avoid redundancy.
Proposed fix
- describe("error handling", () => { - it("should handle unknown subcommand gracefully", async () => { - const originalEnv = process.env.ABLY_CLI_NON_INTERACTIVE; - process.env.ABLY_CLI_NON_INTERACTIVE = "true"; - - try { - const { stdout, stderr } = await runCommand( - ["support", "unknowncommand"], - import.meta.url, - ); - const output = stdout + (stderr || ""); - expect(output).toMatch(/not found|not.*command|available/i); - } finally { - if (originalEnv === undefined) { - delete process.env.ABLY_CLI_NON_INTERACTIVE; - } else { - process.env.ABLY_CLI_NON_INTERACTIVE = originalEnv; - } - } - }); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/support.test.ts` around lines 100 - 119, Remove the duplicate test case "should handle unknown subcommand gracefully" inside the "error handling" describe block: either delete this it(...) block or consolidate its assertions into the existing "unknown subcommand" test (the test with the same runCommand(["support", "unknowncommand"]) call). Ensure any environment setup/teardown for process.env.ABLY_CLI_NON_INTERACTIVE is preserved (move the try/finally cleanup into the remaining test if needed) so no test relies on leftover env changes.test/unit/commands/channels/presence/enter.test.ts-215-229 (1)
215-229:⚠️ Potential issue | 🟡 MinorAssert the surfaced error message here.
expect(error).toBeDefined()passes for any command failure, so this does not prove the rejectedpresence.enter()error is what users actually see. Please assert onerror?.messageas well.Suggested tightening
const { error } = await runCommand( ["channels:presence:enter", "test-channel"], import.meta.url, ); expect(error).toBeDefined(); + expect(error?.message).toContain("Presence enter failed");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/channels/presence/enter.test.ts` around lines 215 - 229, The test currently only checks that an error exists; update the assertion to verify the surfaced message matches the mocked rejection from channel.presence.enter so we confirm the real error is propagated. After calling runCommand([...], import.meta.url) and obtaining error, assert on error?.message (e.g., contains or equals "Presence enter failed") instead of only expect(error).toBeDefined(); this ensures the mocked Error("Presence enter failed") is what users see.test/unit/commands/support/contact.test.ts-132-139 (1)
132-139:⚠️ Potential issue | 🟡 MinorAssert the specific unknown-flag failure.
expect(error).toBeDefined()will pass for any thrown error, including unrelated command failures. Tighten this to verify the parser rejected--unknown-flag-xyzso the test actually protects the intended behavior.Proposed tightening
describe("error handling", () => { it("should handle errors gracefully", async () => { const { error } = await runCommand( ["support:contact", "--unknown-flag-xyz"], import.meta.url, ); - expect(error).toBeDefined(); + expect(error).toBeDefined(); + expect(error?.message).toContain("--unknown-flag-xyz"); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/support/contact.test.ts` around lines 132 - 139, The test currently only checks that an error exists which is too broad; update the assertion in the "should handle errors gracefully" test that calls runCommand(["support:contact", "--unknown-flag-xyz"], import.meta.url) to assert the error specifically indicates the unknown-flag rejection (e.g. inspect error.message or parser error code returned by runCommand) and assert it contains the string "--unknown-flag-xyz" or the parser’s standard unknown-option text (e.g. /unknown (flag|option)|unrecognized option/), so the test verifies the parser rejected that specific flag rather than any generic failure.test/unit/commands/logs/push/subscribe.test.ts-68-69 (1)
68-69:⚠️ Potential issue | 🟡 MinorTighten the error-message assertion.
/No mock|client/ipasses on any unrelated failure that happens to mentionclient, so this test can go green without actually exercising the missing-mock path.🎯 Suggested fix
expect(error).toBeDefined(); - expect(error?.message).toMatch(/No mock|client/i); + expect(error?.message).toMatch(/no mock/i); + expect(error?.message).toMatch(/client/i);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/logs/push/subscribe.test.ts` around lines 68 - 69, The current assertion expect(error?.message).toMatch(/No mock|client/i) is too permissive; tighten it to assert the specific missing-mock error produced by the code under test (the failure path exercised in this test). Update the assertion in subscribe.test.ts to match the exact error text emitted when a mock client is absent (e.g., use expect(error?.message).toMatch(/No mock client configured/i) or expect(error?.message).toContain('No mock client configured')) so the test only passes for the intended missing-mock path rather than any message containing "client"..claude/skills/ably-new-command/SKILL.md-364-370 (1)
364-370:⚠️ Potential issue | 🟡 MinorKeep this validation workflow aligned with the repo’s required sequence.
This now teaches a different flow from the repository guidance by splitting README generation out of
pnpm prepare. Please reconcile this block with the actual required workflow, and update the checklist below to match the same source of truth.As per coding guidelines,
Run mandatory workflow IN ORDER for every change: pnpm prepare (build + update manifest/README), pnpm exec eslint . (lint, MUST be 0 errors), pnpm test:unit (test at minimum), and update docs if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/ably-new-command/SKILL.md around lines 364 - 370, The checklist shows a different command sequence (splitting README generation into a separate pnpm exec oclif readme step) which diverges from the repo’s required workflow; update the block to match the canonical sequence used by the repo: ensure the instructions list the exact ordered steps for every change — pnpm prepare (which performs build + updates manifest and README), then pnpm exec eslint . (lint must be 0 errors), then pnpm test:unit — and remove or fold the separate pnpm exec oclif readme line so the README generation is covered by pnpm prepare and the checklist wording matches that single-source-of-truth sequence.test/unit/commands/spaces/cursors/subscribe.test.ts-200-211 (1)
200-211:⚠️ Potential issue | 🟡 MinorMake the failure assertion specific.
This only proves that the command failed somehow. If the command starts failing earlier for an unrelated reason, this test still passes. Please assert the propagated message and that the subscription path was not reached.
Suggested tightening
const { error } = await runCommand( ["spaces:cursors:subscribe", "test-space"], import.meta.url, ); expect(error).toBeDefined(); + expect(error!.message).toContain("Connection failed"); + expect(space.cursors.subscribe).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/cursors/subscribe.test.ts` around lines 200 - 211, The test "should handle space entry failure" currently only checks that an error occurred; update it to assert the exact propagated error message and that the subscription code path was not executed: after mocking space.enter.mockRejectedValue(new Error("Connection failed")), call runCommand as before and assert error.message === "Connection failed" (or that error contains that string) and also assert that the space subscription mock (the mock on the space object used for subscribing, e.g., space.subscribe or spacesMock._getSpace(...).subscribe) was not called to guarantee the subscription path was skipped.test/unit/commands/spaces/locations/get-all.test.ts-123-136 (1)
123-136:⚠️ Potential issue | 🟡 MinorAssert the expected failure, not just any failure.
Right now this passes for any exception path. Since the test is named around
getAllrejection, it should verify that the surfaced error matches that rejection.Suggested tightening
const { error } = await runCommand( ["spaces:locations:get-all", "test-space"], import.meta.url, ); expect(error).toBeDefined(); + expect(error!.message).toContain("Failed to get locations");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/locations/get-all.test.ts` around lines 123 - 136, The test currently only checks that some error is returned; change the assertion to verify the specific rejection from space.locations.getAll by asserting the surfaced error message matches "Failed to get locations" (e.g., replace expect(error).toBeDefined() with an assertion on error.message or error string). Update the test that uses getMockAblySpaces(), space.locations.getAll.mockRejectedValue(new Error("Failed to get locations")), and runCommand(...) to assert the exact error text so the test ensures the getAll rejection is what was propagated.test/unit/commands/integrations/delete.test.ts-289-296 (1)
289-296:⚠️ Potential issue | 🟡 MinorDuplicate test: this scenario is already covered at lines 96-104.
The "error handling" describe block already contains the exact same test case:
it("should require ruleId argument", ...)with identical command invocation and assertion pattern. Remove this duplicate to avoid test redundancy.🗑️ Suggested removal
- describe("argument validation", () => { - it("should require ruleId argument", async () => { - const { error } = await runCommand( - ["integrations:delete", "--force"], - import.meta.url, - ); - expect(error?.message).toMatch(/required|Missing/i); - }); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/integrations/delete.test.ts` around lines 289 - 296, Remove the duplicate test case that asserts the required ruleId argument: inside the "argument validation" describe in delete.test.ts remove the it("should require ruleId argument", ...) block (the one invoking runCommand(["integrations:delete", "--force"], import.meta.url) and expecting error?.message toMatch /required|Missing/i) because the same scenario is already covered in the "error handling" describe; keep one canonical test (the existing one at lines ~96-104) and delete this redundant one to avoid duplicate assertions.CONTRIBUTING.md-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorKeep
pnpm test:unitexplicit as the minimum required suite.The current wording reads like contributors can pick any relevant test command. Please make
pnpm test:unitmandatory first, then list the extra suites as conditional follow-ups.Suggested wording
-3. **Test:** Run relevant tests (`pnpm test:unit`, `pnpm test:integration`, `pnpm test:e2e`, `pnpm test:playwright`, or specific files) and ensure they pass. For interactive mode changes, also run `pnpm run test:tty` (requires a real terminal, not run in CI). Add new tests or update existing ones as needed. +3. **Test:** Run `pnpm test:unit` at minimum, then run any additional relevant suites (`pnpm test:integration`, `pnpm test:e2e`, `pnpm test:playwright`, or specific files) as needed. For interactive mode changes, also run `pnpm run test:tty` (requires a real terminal, not run in CI). Add new tests or update existing ones as needed.As per coding guidelines
**/*.{ts,tsx,js,mjs}: "Run mandatory workflow IN ORDER for every change: pnpm prepare ... pnpm exec eslint . ... pnpm test:unit (test at minimum), and update docs if needed."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 13, Update the "3. **Test:**" bullet in CONTRIBUTING.md so that "pnpm test:unit" is required first and explicitly mandatory, then list the other test commands (`pnpm test:integration`, `pnpm test:e2e`, `pnpm test:playwright`, `pnpm run test:tty`) as optional/conditional follow-ups; specifically edit the sentence starting with "3. **Test:** Run relevant tests..." to require "pnpm test:unit" as the minimum mandatory suite before mentioning the additional suites and the tty note.test/unit/commands/spaces/locks/get-all.test.ts-123-134 (1)
123-134:⚠️ Potential issue | 🟡 MinorAssert the expected failure message.
Right now this will pass for any thrown error. Since the mock is explicitly rejecting with
"Failed to get locks", assert on that message so the test validates the intended error path.🔎 Tighten the assertion
const { error } = await runCommand( ["spaces:locks:get-all", "test-space"], import.meta.url, ); - expect(error).toBeDefined(); + expect(error).toBeDefined(); + expect(error?.message).toContain("Failed to get locks");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/locks/get-all.test.ts` around lines 123 - 134, The test currently only checks that an error was thrown; tighten it to assert the specific failure message from the mock. Update the "error handling" spec (the test invoking runCommand) to assert that the returned error's message equals or matches "Failed to get locks" (the value the mock space.locks.getAll.mockRejectedValue was given) instead of just using expect(error).toBeDefined(); reference the runCommand invocation and the space.locks.getAll mock to locate the assertion to change.test/unit/commands/spaces/list.test.ts-135-142 (1)
135-142:⚠️ Potential issue | 🟡 MinorMake the error-path assertion specific.
expect(error).toBeDefined()is too permissive here. The mock sets a concrete"API error"message, so assert on it to avoid this test passing on an unrelated failure.🔎 Tighten the assertion
const { error } = await runCommand(["spaces:list"], import.meta.url); - expect(error).toBeDefined(); + expect(error).toBeDefined(); + expect(error?.message).toContain("API error");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/list.test.ts` around lines 135 - 142, The test's error-path assertion is too broad; replace the permissive expect(error).toBeDefined() with a specific assertion that checks the mocked message from mock.request.mockRejectedValue(new Error("API error")). Update the assertion after runCommand(["spaces:list"], import.meta.url) to assert the error's message (for example expect(error?.message).toEqual("API error") or expect(error?.message).toContain("API error")), so the test verifies the exact failure produced by the mocked getMockAblyRest request.test/unit/commands/spaces/locks/get.test.ts-126-137 (1)
126-137:⚠️ Potential issue | 🟡 MinorAssert the propagated error text here.
This currently passes on any failure path, including unrelated setup regressions. Since the mock already injects
"Failed to get lock", assert that message so the test proves the command surfaces the intended error.🔎 Tighten the assertion
const { error } = await runCommand( ["spaces:locks:get", "test-space", "my-lock"], import.meta.url, ); - expect(error).toBeDefined(); + expect(error).toBeDefined(); + expect(error?.message).toContain("Failed to get lock");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/locks/get.test.ts` around lines 126 - 137, The test's final assertion is too loose; update the assertion that currently reads expect(error).toBeDefined() to assert the propagated error text from the mocked rejection (space.locks.get.mockRejectedValue(new Error("Failed to get lock"))). Locate the test in get.test.ts that calls runCommand(["spaces:locks:get", "test-space", "my-lock"], import.meta.url) and replace the broad check with an assertion that the returned error object's message contains or equals "Failed to get lock" (e.g., expect(error).toHaveProperty("message", "Failed to get lock") or expect(error?.message).toContain("Failed to get lock")) so the test verifies the intended error is surfaced.AGENTS.md-8-13 (1)
8-13:⚠️ Potential issue | 🟡 MinorDon’t require
pnpm test:ttyfor every change.This makes a local-only, CI-skipped suite part of the mandatory workflow for all contributors, which conflicts with the note on the same line and raises the bar for unrelated changes. Keep
pnpm test:unitas the required baseline and documentpnpm test:ttyas an extra step when touching TTY-specific behavior. As per coding guidelines, "Run mandatory workflow IN ORDER for every change: pnpm prepare (build + update manifest/README), pnpm exec eslint . (lint, MUST be 0 errors), pnpm test:unit (test at minimum), and update docs if needed."📝 Suggested wording
pnpm prepare # 1. Build + update manifest pnpm exec oclif readme # 2. Regenerate README.md from command metadata pnpm exec eslint . # 3. Lint (MUST be 0 errors) pnpm test:unit # 4. Test (at minimum) -pnpm test:tty # 5. TTY tests (local only, skip in CI) - # 6. Update docs if needed + # 5. Update docs if needed + +# Run when touching TTY-specific behavior or debugging interactive flows: +# pnpm test:tty🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 8 - 13, Update AGENTS.md to remove "pnpm test:tty" from the mandatory workflow list and instead document it as an optional/extra step for TTY-specific changes; keep the required sequence as "pnpm prepare", "pnpm exec eslint .", "pnpm test:unit" and then "update docs if needed", and add a short note stating "Run pnpm test:tty locally when touching TTY-specific behavior (CI skips this)". Use the exact tokens from the diff—pnpm prepare, pnpm exec eslint ., pnpm test:unit, pnpm test:tty—to locate and edit the list and the explanatory comment.README.md-3575-3588 (1)
3575-3588:⚠️ Potential issue | 🟡 MinorAdd a language to this fenced block.
The changed fence is still bare ``` and trips MD040 on this section. Use a neutral language like
textif this is generated command output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 3575 - 3588, The fenced code block in the README showing the "USAGE/DESCRIPTION/EXAMPLES" output uses a bare ``` fence which triggers MD040; change the opening fence to specify a neutral language like backtick fence with "text" (i.e., replace ``` with ```text) so the block is annotated as plain text; update the block around the Ably Spaces location commands (the USAGE/EXAMPLES section) accordingly.test/unit/commands/apps/switch.test.ts-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorRemove the unused
getMockConfigManagerimport.It is never read in this file, so this will fail the repo’s zero-error lint pass. As per coding guidelines
**/*.{ts,tsx,js,mjs}: "pnpm exec eslint . (lint, MUST be 0 errors)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/apps/switch.test.ts` at line 4, Remove the unused import getMockConfigManager from the test file to satisfy lint rules; locate the import statement importing getMockConfigManager in the top of the test module (the line with "import { getMockConfigManager }") and delete that named import (or remove the entire import statement if it only referenced getMockConfigManager) so the symbol is no longer imported but unused.README.md-3856-3861 (1)
3856-3861:⚠️ Potential issue | 🟡 MinorRemove the
get-allexample fromsrc/commands/spaces/members/index.tsor implement the missing command.The
spaces members get-allexample is listed in the source code (line 12 ofsrc/commands/spaces/members/index.ts) and generates into README, but there is no correspondingget-all.tsimplementation file. Other spaces subcommands (cursors, locations, locks) all haveget-all.tsfiles; only members is missing one. Either remove the example from the index or create the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 3856 - 3861, The README lists a "spaces members get-all" example but there is no corresponding implementation; either remove that example entry from the members index or add the missing get-all command file. To fix by implementing, create a new get-all.ts alongside the other member subcommands exporting the same command shape (export const command, description, and an async handler) and implement the handler to call the members listing API (mirror how the other subcommands like cursors/locations/locks implement get-all), then ensure the members index (index.ts) imports/exports the new get-all command so it appears in the CLI help and README generation; alternatively, simply delete the "get-all" example line from the members index so the README no longer references a non-existent command.vitest.config.ts-91-105 (1)
91-105:⚠️ Potential issue | 🟡 MinorDefault
testcommand will attempt TTY tests — consider excluding from shared projects or documenting the requirement.The CI workflows are properly guarded with explicit
--projectflags (test:unit,test:integration,test:e2e,test:hooks), so CI safety is addressed. However, the defaultpnpm testscript (vitest runwithout a project filter) andpnpm test:coverage(vitest run --coverage) will still attempt to run all projects includingtty, which requires a real TTY and will fail in non-TTY environments.Either remove
ttyfrom the sharedprojectsarray and only include it when explicitly requested (keeping it opt-in via CLI), or document thatpnpm testrequires TTY support and developers should use specific scripts likepnpm test:unitfor headless CI/environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.ts` around lines 91 - 105, The "tty" shared project in the Vitest projects array will be picked up by default runs and fails in non-TTY environments; remove the project object whose test.name is "tty" from the common projects array (or guard its inclusion behind an opt-in condition such as an environment variable like RUN_TTY_TESTS) so that default "vitest run" and coverage runs do not attempt TTY tests; update README or scripts to document how to run the tty suite explicitly (e.g., via a dedicated npm script that passes --project=tty or sets RUN_TTY_TESTS).test/tty/commands/interactive-sigint.test.ts-89-113 (1)
89-113:⚠️ Potential issue | 🟡 MinorTest intent and assertion conflict on expected exit code.
The title says code
130, but the assertion accepts[0, 42, 130]. Please either tighten the assertion to130or rename the test to reflect multiple acceptable outcomes.Possible rename-only fix
- "should exit cleanly with code 130 when Ctrl+C is pressed during command execution", + "should interrupt command and allow clean exit after Ctrl+C during command execution",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tty/commands/interactive-sigint.test.ts` around lines 89 - 113, The test title asserts that Ctrl+C should produce exit code 130 but the assertion allows [0, 42, 130]; update the test to make intent consistent by changing the expectation to only 130: replace the expect([0, 42, 130]).toContain(code) with expect(code).toBe(130) (in the test whose description string is "should exit cleanly with code 130 when Ctrl+C is pressed during command execution"), ensure any comment about acceptable codes is removed or updated, and keep the existing helpers spawnTty, waitForOutput, writeTty, sendCtrlC and proc.exitPromise logic intact.test/unit/commands/rooms/messages/history.test.ts-154-176 (1)
154-176:⚠️ Potential issue | 🟡 MinorThe
--orderpart of this test is currently unverified.The test name says it covers both
limitandorder, but it only assertslimit. If the command stops forwarding--order, this still passes. Please assert the mapped history option for the requested ordering as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/messages/history.test.ts` around lines 154 - 176, The test currently only asserts the limit but not the ordering; update the test to also assert that room.messages.history was called with the correct mapped ordering for the CLI flag. After locating the mapping in the command implementation (the code that translates the "--order oldestFirst" arg), add an assertion like expect(room.messages.history).toHaveBeenCalledWith(expect.objectContaining({ order: <mappedValueForOldestFirst> })), using the exact mapped value used by the command, so both limit and order are verified when runCommand is invoked.test/unit/commands/bench/publisher.test.ts-68-91 (1)
68-91:⚠️ Potential issue | 🟡 MinorAssert the requested send count explicitly.
With
--messages 2, this still passes if the command reports the wrong total because it only checks thatmessagesSentexists. Please assert the exact value so the benchmark result contract is actually covered.Suggested assertion tightening
const result = JSON.parse(stdout); expect(result).toHaveProperty("type", "result"); expect(result).toHaveProperty("command", "bench:publisher"); expect(result).toHaveProperty("success", true); - expect(result).toHaveProperty("messagesSent"); + expect(result).toHaveProperty("messagesSent", 2); expect(result).toHaveProperty("testId"); expect(result).toHaveProperty("transport");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/bench/publisher.test.ts` around lines 68 - 91, The test currently only checks that messagesSent exists; update the assertion after parsing stdout from runCommand for "bench:publisher" to assert the exact requested send count (from the "--messages 2" flag) by asserting result.messagesSent === 2 (use expect(result.messagesSent).toBe(2) or equivalent) so the benchmark contract is enforced; locate this in the test using runCommand and the parsed result variable `result`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 880b8242-3b10-454a-8320-d39251c1903b
📒 Files selected for processing (111)
.claude/skills/ably-new-command/SKILL.mdAGENTS.mdCONTRIBUTING.mdREADME.mddocs/Project-Structure.mddocs/Testing.mdpackage.jsontest/README.mdtest/tty/commands/interactive-sigint.test.tstest/tty/tty-test-helper.tstest/unit/commands/accounts/current.test.tstest/unit/commands/accounts/list.test.tstest/unit/commands/accounts/login.test.tstest/unit/commands/accounts/logout.test.tstest/unit/commands/accounts/switch.test.tstest/unit/commands/apps/channel-rules/create.test.tstest/unit/commands/apps/channel-rules/delete.test.tstest/unit/commands/apps/channel-rules/list.test.tstest/unit/commands/apps/channel-rules/update.test.tstest/unit/commands/apps/create.test.tstest/unit/commands/apps/current.test.tstest/unit/commands/apps/delete.test.tstest/unit/commands/apps/list.test.tstest/unit/commands/apps/set-apns-p12.test.tstest/unit/commands/apps/switch.test.tstest/unit/commands/apps/update.test.tstest/unit/commands/auth/issue-ably-token.test.tstest/unit/commands/auth/issue-jwt-token.test.tstest/unit/commands/auth/keys/create.test.tstest/unit/commands/auth/keys/current.test.tstest/unit/commands/auth/keys/get.test.tstest/unit/commands/auth/keys/list.test.tstest/unit/commands/auth/keys/revoke.test.tstest/unit/commands/auth/keys/switch.test.tstest/unit/commands/auth/keys/update.test.tstest/unit/commands/bench/bench.test.tstest/unit/commands/bench/publisher.test.tstest/unit/commands/bench/subscriber.test.tstest/unit/commands/channel-rule/create.test.tstest/unit/commands/channel-rule/delete.test.tstest/unit/commands/channel-rule/list.test.tstest/unit/commands/channel-rule/update.test.tstest/unit/commands/channels/inspect.test.tstest/unit/commands/channels/list.test.tstest/unit/commands/channels/occupancy/get.test.tstest/unit/commands/channels/occupancy/subscribe.test.tstest/unit/commands/channels/presence/enter.test.tstest/unit/commands/channels/presence/subscribe.test.tstest/unit/commands/channels/publish.test.tstest/unit/commands/channels/subscribe.test.tstest/unit/commands/config/path.test.tstest/unit/commands/config/show.test.tstest/unit/commands/connections/test.test.tstest/unit/commands/integrations/create.test.tstest/unit/commands/integrations/delete.test.tstest/unit/commands/integrations/get.test.tstest/unit/commands/integrations/list.test.tstest/unit/commands/integrations/update.test.tstest/unit/commands/interactive-sigint.test.tstest/unit/commands/interactive.test.tstest/unit/commands/login.test.tstest/unit/commands/logs/channel-lifecycle/subscribe.test.tstest/unit/commands/logs/connection-lifecycle/history.test.tstest/unit/commands/logs/connection-lifecycle/subscribe.test.tstest/unit/commands/logs/history.test.tstest/unit/commands/logs/push/history.test.tstest/unit/commands/logs/push/subscribe.test.tstest/unit/commands/logs/subscribe.test.tstest/unit/commands/queues/create.test.tstest/unit/commands/queues/delete.test.tstest/unit/commands/queues/list.test.tstest/unit/commands/rooms/features.test.tstest/unit/commands/rooms/list.test.tstest/unit/commands/rooms/messages.test.tstest/unit/commands/rooms/messages/history.test.tstest/unit/commands/rooms/messages/reactions/remove.test.tstest/unit/commands/rooms/messages/reactions/send.test.tstest/unit/commands/rooms/messages/reactions/subscribe.test.tstest/unit/commands/rooms/messages/send.test.tstest/unit/commands/rooms/messages/subscribe.test.tstest/unit/commands/rooms/occupancy/get.test.tstest/unit/commands/rooms/occupancy/subscribe.test.tstest/unit/commands/rooms/presence/enter.test.tstest/unit/commands/rooms/presence/subscribe.test.tstest/unit/commands/rooms/reactions/send.test.tstest/unit/commands/rooms/reactions/subscribe.test.tstest/unit/commands/rooms/typing/keystroke.test.tstest/unit/commands/rooms/typing/subscribe.test.tstest/unit/commands/spaces/cursors/get-all.test.tstest/unit/commands/spaces/cursors/set.test.tstest/unit/commands/spaces/cursors/subscribe.test.tstest/unit/commands/spaces/list.test.tstest/unit/commands/spaces/locations/get-all.test.tstest/unit/commands/spaces/locations/set.test.tstest/unit/commands/spaces/locations/subscribe.test.tstest/unit/commands/spaces/locks/acquire.test.tstest/unit/commands/spaces/locks/get-all.test.tstest/unit/commands/spaces/locks/get.test.tstest/unit/commands/spaces/locks/subscribe.test.tstest/unit/commands/spaces/members/enter.test.tstest/unit/commands/spaces/members/subscribe.test.tstest/unit/commands/spaces/spaces.test.tstest/unit/commands/stats/account.test.tstest/unit/commands/stats/app.test.tstest/unit/commands/status.test.tstest/unit/commands/support.test.tstest/unit/commands/support/ask.test.tstest/unit/commands/support/contact.test.tstest/unit/commands/test/wait.test.tstest/unit/commands/version.test.tsvitest.config.ts
There was a problem hiding this comment.
Pull request overview
This PR expands CLI test coverage across the command suite and introduces a new Vitest “tty” project (using node-pty) to reliably test interactive-mode SIGINT behavior in a real pseudo-terminal, plus related documentation and contributor workflow updates.
Changes:
- Adds a new
test/tty/tier andpnpm test:ttyscript for PTY-backed interactive SIGINT tests. - Expands/standardizes unit test coverage across many commands (help output, flag visibility, argument validation, basic error handling).
- Updates docs and contributor guidance to reflect the new testing tier and README regeneration steps.
Reviewed changes
Copilot reviewed 111 out of 111 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adds a new Vitest tty project configuration. |
| package.json | Adds test:tty script to run the new project. |
| test/tty/tty-test-helper.ts | Introduces PTY spawn/write/wait utilities for TTY tests. |
| test/tty/commands/interactive-sigint.test.ts | Adds SIGINT/Ctrl+C behavior tests using real PTYs. |
| test/README.md | Documents the new TTY test tier and how to run it. |
| docs/Testing.md | Documents TTY tests purpose, constraints, and commands. |
| docs/Project-Structure.md | Adds test/tty/ to project structure docs. |
| CONTRIBUTING.md | Updates required workflow to include README regeneration + TTY tests for interactive changes. |
| AGENTS.md | Updates “run in order” checklist and test structure to include TTY tests and README regeneration. |
| .claude/skills/ably-new-command/SKILL.md | Updates contributor automation guidance to regenerate README separately. |
| README.md | Regenerates CLI help/docs output with new flags/examples reflected. |
| test/unit/commands/version.test.ts | Adds unit tests for version help/flags/JSON output. |
| test/unit/commands/test/wait.test.ts | Adds unit tests for test:wait help/flags/required duration. |
| test/unit/commands/support/contact.test.ts | Adds help/flags/error-handling checks. |
| test/unit/commands/support/ask.test.ts | Adds new unit tests covering help/args/JSON output and API error cases. |
| test/unit/commands/support.test.ts | Adds help output assertions and unknown subcommand handling. |
| test/unit/commands/status.test.ts | Adds --json flag visibility test. |
| test/unit/commands/stats/app.test.ts | Adds help/flags/error-handling checks. |
| test/unit/commands/stats/account.test.ts | Adds help/flags/error-handling checks. |
| test/unit/commands/spaces/spaces.test.ts | Adds help output checks and unknown-flag error case. |
| test/unit/commands/spaces/members/subscribe.test.ts | Adds help/flags/error-handling and ensures duration-bounded runs. |
| test/unit/commands/spaces/members/enter.test.ts | Adds help/flags checks. |
| test/unit/commands/spaces/locks/subscribe.test.ts | Adds help checks. |
| test/unit/commands/spaces/locks/get.test.ts | Adds help and error-handling checks. |
| test/unit/commands/spaces/locks/get-all.test.ts | Adds help and error-handling checks. |
| test/unit/commands/spaces/locks/acquire.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/spaces/locations/subscribe.test.ts | Adds help checks. |
| test/unit/commands/spaces/locations/set.test.ts | Adds help/flags checks. |
| test/unit/commands/spaces/locations/get-all.test.ts | Adds help and error-handling checks. |
| test/unit/commands/spaces/list.test.ts | Adds help/flags/error-handling checks. |
| test/unit/commands/spaces/cursors/subscribe.test.ts | Adds help and error-handling checks. |
| test/unit/commands/spaces/cursors/set.test.ts | Adds help/flags checks. |
| test/unit/commands/spaces/cursors/get-all.test.ts | Adds help checks. |
| test/unit/commands/rooms/typing/subscribe.test.ts | Adds duration-bounded runs + help/flags/error-handling checks. |
| test/unit/commands/rooms/typing/keystroke.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/rooms/reactions/subscribe.test.ts | Adds duration-bounded runs + help/flags/error-handling checks. |
| test/unit/commands/rooms/reactions/send.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/rooms/presence/subscribe.test.ts | Adds duration-bounded runs + help/flags/error-handling checks. |
| test/unit/commands/rooms/presence/enter.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/rooms/occupancy/subscribe.test.ts | Adds help/arg/flags/error-handling checks. |
| test/unit/commands/rooms/occupancy/get.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/rooms/messages/subscribe.test.ts | Adds new unit tests for subscribe behavior + JSON envelope assertions. |
| test/unit/commands/rooms/messages/send.test.ts | Adds new unit tests for send behavior, interpolation, delay enforcement, JSON output. |
| test/unit/commands/rooms/messages/history.test.ts | Adds new unit tests for history retrieval, time flags, JSON output, validation. |
| test/unit/commands/rooms/messages/reactions/subscribe.test.ts | Adds duration-bounded runs + help/flags/error-handling checks. |
| test/unit/commands/rooms/messages/reactions/send.test.ts | Adds help checks. |
| test/unit/commands/rooms/messages/reactions/remove.test.ts | Adds help checks. |
| test/unit/commands/rooms/messages/reactions/*.test.ts | Adds help coverage for message reactions subcommands. |
| test/unit/commands/rooms/messages.test.ts | Adds help/args/flags checks for rooms message subcommands. |
| test/unit/commands/rooms/list.test.ts | Adds help/flags checks. |
| test/unit/commands/rooms/features.test.ts | Adds help/args/flags/error-handling checks for several rooms feature commands. |
| test/unit/commands/queues/list.test.ts | Adds help/flags checks. |
| test/unit/commands/queues/delete.test.ts | Improves confirmation prompt tests + adds help/flags checks. |
| test/unit/commands/queues/create.test.ts | Adds help checks. |
| test/unit/commands/logs/subscribe.test.ts | Adds help checks. |
| test/unit/commands/logs/push/subscribe.test.ts | Adds help/flags/error-handling checks. |
| test/unit/commands/logs/push/history.test.ts | Adds help and error-handling checks. |
| test/unit/commands/logs/history.test.ts | Adds help/flags/error-handling checks. |
| test/unit/commands/logs/connection-lifecycle/subscribe.test.ts | Adds help/flags checks + duration-bounded runs. |
| test/unit/commands/logs/connection-lifecycle/history.test.ts | Adds help and error-handling checks. |
| test/unit/commands/logs/channel-lifecycle/subscribe.test.ts | Adds help checks + duration-bounded runs and flag coverage. |
| test/unit/commands/login.test.ts | Adds new unit tests for login alias command help/flags/validation. |
| test/unit/commands/interactive.test.ts | Removes flaky non-TTY Ctrl+C integration tests; points to TTY suite. |
| test/unit/commands/interactive-sigint.test.ts | Removes skipped non-TTY SIGINT tests; points to TTY suite. |
| test/unit/commands/integrations/update.test.ts | Adds help checks. |
| test/unit/commands/integrations/list.test.ts | Adds new unit tests for listing integrations, JSON output, error cases. |
| test/unit/commands/integrations/get.test.ts | Adds help checks. |
| test/unit/commands/integrations/delete.test.ts | Adds help/arg validation checks. |
| test/unit/commands/integrations/create.test.ts | Adds help/flags checks. |
| test/unit/commands/connections/test.test.ts | Adds help/flags/error-handling checks. |
| test/unit/commands/config/show.test.ts | Adds help checks. |
| test/unit/commands/config/path.test.ts | Adds help and error-handling checks. |
| test/unit/commands/channels/subscribe.test.ts | Adds duration-bounded runs + error-handling for missing mocks. |
| test/unit/commands/channels/publish.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/channels/presence/subscribe.test.ts | Adds arg validation + duration-bounded runs + error-handling. |
| test/unit/commands/channels/presence/enter.test.ts | Adds arg validation + error-handling checks. |
| test/unit/commands/channels/occupancy/subscribe.test.ts | Adds duration-bounded runs + help/flags coverage. |
| test/unit/commands/channels/occupancy/get.test.ts | Adds help/arg/flags/error-handling checks. |
| test/unit/commands/channels/list.test.ts | Adds network-error handling test. |
| test/unit/commands/channels/inspect.test.ts | Adds arg validation + --json flag visibility test. |
| test/unit/commands/channel-rule/update.test.ts | Adds help/flags/error-handling checks for alias command. |
| test/unit/commands/channel-rule/list.test.ts | Adds help/flags/error-handling checks for alias command. |
| test/unit/commands/channel-rule/delete.test.ts | Adds help/flags/error-handling checks for alias command. |
| test/unit/commands/channel-rule/create.test.ts | Adds help/flags/error-handling checks for alias command. |
| test/unit/commands/bench/subscriber.test.ts | Adds new unit tests for subscriber benchmark help/args/flags. |
| test/unit/commands/bench/publisher.test.ts | Adds new unit tests for publisher benchmark help/args/flags/JSON output. |
| test/unit/commands/bench/bench.test.ts | Adds help/flags/error-handling checks around bench publisher coverage. |
| test/unit/commands/auth/keys/update.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/auth/keys/switch.test.ts | Adds new unit tests for switching API keys + JSON output + errors. |
| test/unit/commands/auth/keys/revoke.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/auth/keys/list.test.ts | Adds help/flags checks. |
| test/unit/commands/auth/keys/get.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/auth/keys/current.test.ts | Adds help/flags checks. |
| test/unit/commands/auth/keys/create.test.ts | Adds help/flags checks. |
| test/unit/commands/auth/issue-jwt-token.test.ts | Adds help checks. |
| test/unit/commands/auth/issue-ably-token.test.ts | Adds help checks. |
| test/unit/commands/apps/update.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/apps/switch.test.ts | Adds new unit tests for switching apps + JSON output + errors. |
| test/unit/commands/apps/set-apns-p12.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/apps/list.test.ts | Adds help/flags checks. |
| test/unit/commands/apps/delete.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/apps/current.test.ts | Adds help/flags checks. |
| test/unit/commands/apps/create.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/apps/channel-rules/update.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/apps/channel-rules/list.test.ts | Adds help/flags checks. |
| test/unit/commands/apps/channel-rules/delete.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/apps/channel-rules/create.test.ts | Adds help/arg/flags checks. |
| test/unit/commands/accounts/switch.test.ts | Adds help/flags/error-handling checks. |
| test/unit/commands/accounts/logout.test.ts | Adds --json flag visibility check. |
| test/unit/commands/accounts/login.test.ts | Adds --json flag visibility check. |
| test/unit/commands/accounts/list.test.ts | Adds help/flags/error-handling checks. |
| test/unit/commands/accounts/current.test.ts | Adds help/flags checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
umair-ably
force-pushed
the
DX-947-tests
branch
from
03f71c8 to
a9c829f
Compare
umair-ably
force-pushed
the
DX-947-tests
branch
from
a9c829f to
f8505ce
Compare
umair-ably
force-pushed
the
DX-947-tests
branch
from
f8505ce to
e1723c5
Compare
sacOO7
reviewed
Mar 11, 2026
Contributor
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Okay, so I validated PR for
- A structured review covering:
- Infrastructure changes
- Test architecture
- Conventions compliance
- Inconsistencies across files
- Potential bugs or fragile tests
- Missing or incomplete test coverage
- Documentation alignment with implementation
- Analyze the codebase for opportunities to improve maintainability, including:
- Extracting repeated setup/teardown logic
- Introducing shared test utilities
- Creating base test classes or helpers
- Reducing duplicated assertions or fixtures
- Consolidating repeated describe/test structures
- Improving test organization
Will share actionable findings for the same 👍
sacOO7
approved these changes
Mar 12, 2026
Contributor
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Thanks for addressing feedback, LGTM 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
standardHelpTests(),standardFlagTests(),standardArgValidationTests(),standardControlApiErrorTests()) to enforce the 5 required describe blocks and eliminate boilerplatetest/helpers/standard-tests.ts,test/helpers/control-api-test-helpers.ts,test/fixtures/control-api.ts(mock factories for apps, keys, rules)apps switch,auth keys switch,bench publisher/subscriber,integrations list,login,rooms messages (send/subscribe/history),rooms list,spaces list,channels occupancy get,logs connection-lifecycle subscribe,support ask,test wait,versiontest/tty/test tier withnode-ptyfor testing interactive mode SIGINT handling in real pseudo-terminals (migrated from flaky piped-stdio unit tests)COMMANDSheaders that get garbled by xterm escape codesdocs/Testing.mdandtest/README.md; updatesCLAUDE.md,docs/Project-Structure.mdStats
127 files changed, ~6,500 insertions, ~4,050 deletions (net ~2,450 lines — most reduction is boilerplate removal)
Review strategy
This is a large PR but most changes are mechanical and repetitive. Suggested review approach:
test/helpers/standard-tests.ts,test/helpers/control-api-test-helpers.ts,test/fixtures/control-api.tstest/tty/tty-test-helper.ts,test/tty/commands/interactive-sigint.test.ts,vitest.config.ts,package.jsontest/unit/commands/apps/switch.test.ts,test/unit/commands/rooms/messages/send.test.ts, etc.test/unit/commands/status.test.tstest/unit/commands/apps/create.test.tstest/unit/commands/channels/subscribe.test.tstest/unit/commands/rooms/presence/subscribe.test.tstest/e2e/web-cli/web-cli.test.tsand siblings — changed assertions from styled headers to stable contentdocs/Testing.md,test/README.mdTest plan
pnpm test:unitpassespnpm test:ttypasses (local only, requires real terminal)pnpm exec eslint .shows 0 errorspnpm preparesucceeds🤖 Generated with Claude Code