Initial test coverage #8
Conversation
Failures were logged to stderr even in test environments
Such as `ably channels publis`, will now suggest `channels publish`, but then show the argument issues and high level usage info to make DX more pleasurable and intuitive.
Prevent CI lock up
Auto-generated updates
- Create shared version utilities in src/utils/version.ts for consistent behavior - Update version-flag hook to use the shared utilities for JSON formatting - Make the version command hidden to encourage use of --version flag - Reinstate commented-out test in did-you-mean.test.ts that verifies command help behavior - Fix test to match actual hook behavior with missing arguments
…mmands The CLI's "did you mean" feature was not correctly handling arguments when a command had a typo. For example, "ably channels publis foo bar" would not suggest "channels publish" and pass the arguments correctly. This fix: - Updates the command matching to consider progressively shorter prefixes of commands - Properly extracts arguments from mistyped commands (using colon separators in actual CLI usage) - Ensures extracted arguments are passed to the suggested command - Updates tests to match actual CLI behaviour Now when users mistype a command with arguments, the CLI will suggest the correct command and preserve all arguments if the user confirms.
The error message shown when a command is not found incorrectly suggested using 'ably help' instead of the correct 'ably --help' syntax. This commit updates all occurrences of this message in code and tests.
- Includes new base integration and end-to-end tests - standardise how Rest clients are instanced for better testing - Improves a few small issues along the way surfaced by tests
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces extensive enhancements to the CLI's testing infrastructure, developer documentation, and codebase maintainability. Major changes include the addition of comprehensive end-to-end, integration, and unit test suites for core commands and services, alongside new utilities for test mocking and environment management. The CLI now supports a dedicated test mode for mocking Ably clients, with corresponding changes in command implementations to centralize client instantiation. New utility modules for version formatting and string distance calculations were added. Developer and project documentation was expanded, and scripts for running categorized tests with timeouts were introduced. Several commands and services were refactored for improved error handling, configurability, and testability. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CLI as CLI Command
participant Base as AblyBaseCommand
participant Ably as Ably SDK / Mock
participant Test as Test Runner / Script
Dev->>Test: Run test script (e.g., pnpm test:e2e)
Test->>CLI: Spawn CLI process with test mode env
CLI->>Base: Execute command (e.g., channels:list)
Base->>Base: isTestMode() checks env
alt Test mode
Base->>Ably: getMockAblyRest()
else Normal mode
Base->>Ably: Instantiate real client
end
CLI->>Ably: Perform operation (list, publish, etc.)
Ably-->>CLI: Return result (mocked or real)
CLI->>Test: Output/log result
sequenceDiagram
participant User as CLI User
participant CLI as CLI
participant Hook as did-you-mean Hook
participant Utils as String Distance Utils
User->>CLI: Enter unknown command
CLI->>Hook: Trigger command_not_found
Hook->>Utils: Find closest command match
Utils-->>Hook: Return suggestion (if any)
alt Suggestion found
Hook->>User: Prompt for confirmation (skip in test)
User->>Hook: Accept suggestion
Hook->>CLI: Run suggested command with args
CLI-->>Hook: Return result or error
Hook->>User: Output result or help/error
else No suggestion
Hook->>User: Show generic error/help
end
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/commands/logs/push/history.ts (1)
33-46: 💡 Verification agent🧩 Analysis chain
Consider adding a test for the push logs history command.
Since one of the main goals of this PR is to introduce testing, ensure that this command is covered by appropriate tests that verify:
- Proper client creation
- Correct API calls to retrieve push logs
- Proper output formatting in different formats (standard, JSON)
🏁 Script executed:
#!/bin/bash # Check for test coverage of the push logs history command echo "Checking for unit tests..." # Check for unit tests specific to push logs history find test/unit -type f -name "*.test.ts" -exec grep -l "LogsPushHistory" {} \; echo "Checking for integration tests..." # Check for integration tests that might cover this command find test/integration -type f -name "*.test.ts" -exec grep -l "logs push history" {} \; echo "Checking for e2e tests..." # Check for e2e tests that might exercise this command find test/e2e -type f -name "*.test.ts" -exec grep -l "logs push history" {} \;Length of output: 460
🏁 Script executed:
#!/bin/bash # Search for .spec.ts test files covering the LogsPushHistory command echo "Listing all .spec.ts files in the test directory..." find test -type f -name "*.spec.ts" echo echo "Searching for 'LogsPushHistory' references in test files..." rg -n "LogsPushHistory" -g "test/**/*.ts" echo echo "Searching for 'push history' references in test files..." rg -n "push history" -g "test/**/*.ts"Length of output: 513
Add tests for the
LogsPushHistorycommand
No existing tests coversrc/commands/logs/push/history.ts, so please add unit (and if applicable integration) tests to verify:
- Proper REST client creation via
getClientOptions/createAblyRestClient- Correct API calls to fetch push log history
- Output formatting for both standard and JSON flags
Suggested locations/naming:
test/unit/commands/logs/push/history.test.ts- (Optional)
test/integration/commands/logs/push/history.test.ts
🧹 Nitpick comments (31)
src/utils/string-distance.ts (1)
39-43: Consider using named exports exclusively for consistency.You're exporting the
closestfunction both as a named export and as a default export. While this provides flexibility, it can lead to inconsistent import styles across the codebase.Consider standardizing on named exports throughout the codebase for consistency, making it easier to import multiple functions from the same file when needed.
src/commands/auth/revoke-token.ts (1)
69-69: Centralized client creation for consistent testingThis change adopts the standardized approach for Ably REST client creation. Note that the variable is prefixed with an underscore (
_rest), suggesting it might not be used in the current implementation. Consider either removing the unused variable or documenting why it's being created but not used.-const _rest = this.createAblyRestClient(this.getClientOptions(flags)); +// Create Ably REST client for future use or remove if not needed +const rest = this.createAblyRestClient(this.getClientOptions(flags));test/integration/commands/minimal-test.test.ts (1)
14-16: Consider adding a basic assertion.While the purpose of this test is minimal verification that execution doesn't throw errors, adding at least one basic assertion would improve its value.
.command(['help']) .it('should show help', (_ctx) => { // Assertion is just that the command executed without error + // Add a basic assertion that help output contains expected text + expect(_ctx.stdout).to.include('Usage:'); });scripts/run-tests.sh (1)
28-29: Consider making the outer timeout configurable.The script sets a hardcoded outer timeout of 120 seconds. For flexibility, especially with E2E tests that might need more time, consider making this configurable via an environment variable or an additional command line parameter.
- # Set an outer timeout (in seconds) - default 120s or 2 minutes - OUTER_TIMEOUT=120 + # Set an outer timeout (in seconds) - use environment variable or default to 120s (2 minutes) + OUTER_TIMEOUT=${TEST_OUTER_TIMEOUT:-120}test/README.md (1)
61-76: Consider adding information about test timeouts.The documentation on running tests is clear, but it could benefit from mentioning the timeout behavior implemented in the
run-tests.shscript. This would help developers understand how hanging tests are handled.Run all tests: ```bash pnpm test
+Each test suite has a specific timeout configured in package.json. Additionally,
+an outer timeout of 120 seconds is enforced by the run-tests.sh script to prevent
+hanging tests from blocking the test pipeline indefinitely.</blockquote></details> <details> <summary>test/integration/commands/channels.test.ts (2)</summary><blockquote> `20-49`: **Consider enhancing the mock implementation for more complex scenarios.** The mock client implementation is minimal and always returns the same static data. For more comprehensive testing, consider parametrizing the mock to handle different test cases. ```diff - const simpleMockClient = { + function createMockClient(config = { channelId: "test-channel-1", messageData: { text: "Test message 1" } }) { + return { request: () => { // Return channel list response return { statusCode: 200, - items: [{ channelId: "test-channel-1" }] + items: [{ channelId: config.channelId }] }; }, channels: { get: () => ({ history: () => { // Return channel history response return { - items: [{ name: "event1", data: { text: "Test message 1" } }] + items: [{ name: "event1", data: config.messageData }] }; } }) }, // Add connection for Realtime compatibility connection: { once: (event: string, callback: () => void) => { if (event === 'connected') { setTimeout(callback, 0); } } }, close: () => { // Mock close method } + }; } + + const simpleMockClient = createMockClient();
64-65: Avoid using the delete operator for performance reasons.The
deleteoperator is flagged by static analysis tools for potential performance impact. While not critical in test code, you could use direct assignment toundefinedinstead.- delete globalThis.__TEST_MOCKS__; - delete process.env.ABLY_CLI_TEST_MODE; + globalThis.__TEST_MOCKS__ = undefined; + process.env.ABLY_CLI_TEST_MODE = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 65-65: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/base-command.ts (2)
126-146: Simplify error handling for missing mock client.There's redundant error handling when a mock client is missing in test mode. The code both calls
this.error()and throws a new Error, which is unnecessary.if (mockAblyRest) { return mockAblyRest as Ably.Rest; } - this.error('No mock Ably Rest client available in test mode'); throw new Error('Missing mock Ably Rest client in test mode');
186-197: Document potential limitations of realtime client mocking.When using a REST mock client as a Realtime client, there's a potential risk if code tries to use Realtime-specific features not present in the REST mock. Consider documenting these limitations or enhancing the mock to better simulate Realtime functionality.
// If in test mode, skip connection and use mock if (this.isTestMode()) { this.debug('Running in test mode, using mock Ably client'); const mockAblyRest = this.getMockAblyRest(); if (mockAblyRest) { // Wrap the mock in a Promise that resolves immediately + // Note: This REST mock is being used as a Realtime client, which may not + // support all Realtime-specific methods beyond what's explicitly mocked return mockAblyRest as unknown as Ably.Realtime; } - this.error('No mock Ably client available in test mode'); return null; }src/commands/channels/publish.ts (2)
165-180: Consider passing a typedchannelNameinstead of the genericargsobject
logFinalSummary()only needs the channel string, but it now receives the entireargsobject, typed asRecord<string, unknown>.
This loses compile‑time safety and advertises a broader surface than required.- results: Array<Record<string, unknown>>, - args: Record<string, unknown>, + results: Array<Record<string, unknown>>, + channelName: string, ... - channel: args.channel, + channel: channelName,Call sites would then pass
args.channel as string, giving the compiler something useful to check and avoiding accidental misuse of unrelated properties.
If you prefer keeping the whole object, at least tighten the type:Args.Definition<typeof ChannelsPublish.args>.
190-199: Minor duplication in success messagingLines 194–197 duplicate the literal channel success message that already exists in the event log block (lines 313–324). Consider extracting a helper or re‑using the existing string to avoid drift if you tweak the wording later.
test/unit/services/interactive-helper.test.ts (1)
209-243: Assert the returned value in “unnamed keys” test for completenessThe test currently verifies that
"Unnamed key"appears in the prompt choices but does not check thatselectKey()actually returns the selected key. Adding the assertion future‑proofs the test against accidental behavioural regressions.- await interactiveHelper.selectKey(controlApiStub, "app1"); + const result = await interactiveHelper.selectKey(controlApiStub, "app1"); + + expect(result).to.equal(keys[0]);src/hooks/command_not_found/did-you-mean.ts (3)
90-108: Environment flag name could be more idiomatic
SKIP_CONFIRMATIONis perfectly functional, but elsewhere the project usesABLY_CLI_TEST_MODE. Aligning the flag names avoids proliferation of ad‑hoc env vars:-const skipConfirmation = process.env.SKIP_CONFIRMATION === 'true'; +const skipConfirmation = + process.env.SKIP_CONFIRMATION === 'true' || + process.env.ABLY_CLI_TEST_MODE === 'true';
119-138: Use optional‑chaining to silence edge‑case crashes
err.message?.includes(...)already guards, buterr.messageitself may beundefined.
err.message?.includes('Missing')is fine; the risk iserr.message.split('\n')on 162.
Applying an optional chain prevents an unhandled exception whenmessageis falsy.-const errorMsg = err.message || ''; +const errorMsg = err.message ?? '';(and likewise on line 162).
This also addresses the Biome “useOptionalChain” lint hint.
🧰 Tools
🪛 Biome (1.9.4)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
174-176: Swallow‑and‑ignore block hides helpful diagnosticsAn empty
catch {}(lines 173‑175) masks problems in the help‑display path, making future debugging harder. At minimum, log the error in verbose mode:-} catch { - // If something goes wrong showing help, just show the original error +} catch (e) { + /* istanbul ignore next */ + this.debug?.(`did-you-mean help rendering failed: ${(e as Error).message}`); // If something goes wrong showing help, just show the original error }test/integration/test-utils.ts (3)
8-19: Consider using more specific types instead of anyWhile the eslint-disable comments explain why
anyis being used, consider using more specific types where possible to improve type safety and code clarity.interface AblyRestMock { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - request: () => any; + request: () => unknown; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - channels: { get: () => any }; + channels: { get: () => unknown }; close: () => void; connection?: { once: (event: string, cb: () => void) => void }; // eslint-disable-next-line @typescript-eslint/no-explicit-any [key: string]: any; }
55-66: Consider enhancing the default mock implementationThe default mock implementation returns empty objects for methods. Consider using sinon stubs or jest mock functions instead to make verification and behavior customization easier in tests.
export function registerMock<T>(key: string, mock: T): void { if (!globalThis.__TEST_MOCKS__) { globalThis.__TEST_MOCKS__ = { ablyRestMock: { - request: () => ({}), - channels: { get: () => ({}) }, - close: () => {}, + request: sinon.stub().returns({}), + channels: { get: sinon.stub().returns({}) }, + close: sinon.stub(), } }; } globalThis.__TEST_MOCKS__[key] = mock; }
72-77: Enhance error message for missing mocksThe error message could be more helpful by suggesting how to fix the issue when a mock is not found.
export function getMock<T>(key: string): T { if (!isTestMode() || !globalThis.__TEST_MOCKS__ || !globalThis.__TEST_MOCKS__[key]) { - throw new Error(`Mock not found for key: ${key}`); + throw new Error(`Mock not found for key: ${key}. Make sure registerMock('${key}', mock) is called before attempting to retrieve it.`); } return globalThis.__TEST_MOCKS__[key] as T; }test/e2e/core/basic-cli.test.ts (2)
14-42: Improve robustness of the JSON parsing helperThe current implementation may be fragile in certain cases, such as nested JSON structures or malformed outputs.
Consider enhancing the JSON parsing logic to be more robust:
const _parseJsonFromOutput = (output: string): any => { // Strip ANSI color codes first const strippedOutput = stripAnsi(output); + // Try to find a valid JSON object or array within the output + const jsonRegex = /({[\s\S]*}|\[[\s\S]*\])/; + const match = strippedOutput.match(jsonRegex); + + if (match) { + try { + return JSON.parse(match[0]); + } catch (error) { + console.error("JSON parsing failed:", error); + throw new Error(`Failed to parse JSON from matched content.`); + } + } const jsonStart = strippedOutput.lastIndexOf('{'); const arrayStart = strippedOutput.lastIndexOf('['); let startIndex = -1; if (jsonStart === -1 && arrayStart === -1) { console.error("No JSON start character ({ or [) found."); throw new Error(`No JSON object or array found in output.`); } if (jsonStart !== -1 && arrayStart !== -1) { startIndex = Math.max(jsonStart, arrayStart); // Use the later starting character } else if (jsonStart === -1) { startIndex = arrayStart; } else { startIndex = jsonStart; } const jsonString = strippedOutput.slice(Math.max(0, startIndex)); try { return JSON.parse(jsonString); } catch (error) { console.error("JSON parsing failed:", error); throw new Error(`Failed to parse JSON from output substring.`); } };
127-137: Consider making the non-existent help command test more specificThe test correctly verifies that requesting help for a non-existent command fails, but the assertion on the error message could be more specific.
it("should fail when attempting to get help for a non-existent command", async function() { // Use the `--help` flag pattern on a non-existent command with non-interactive flag const result = await execa("node", ["bin/run.js", "help", "doesnotexist", "--non-interactive"], { ...execaOptions, timeout: 5000 // Shorter timeout for this specific test }); expect(result.failed).to.be.true; + expect(result.exitCode).not.equal(0); // In non-interactive mode, it shows a warning that "help" command is not found expect(result.stderr).to.include("help is not an ably command"); + expect(result.stderr).to.include("doesnotexist"); });test/unit/services/control-api.test.ts (5)
166-186: Consider adding error case test for updateAppWhile the success case is tested, there's no test for error handling in the updateApp method, unlike other methods in this file.
Add an error case test similar to other methods:
it("should handle error when updating app", async function() { const appId = "test-app-id"; const updateData = { name: "Invalid App Name" }; // Set up nock to intercept request with error nock(`https://${controlHost}`) .patch(`/v1/apps/${appId}`, updateData) .reply(400, { message: "Invalid app name" }); // Intercept any calls to /me nock(`https://${controlHost}`) .get("/v1/me") .reply(200, { account: { id: "test-account-id" } }); try { await api.updateApp(appId, updateData); expect.fail("Should have thrown an error"); } catch (error) { expect(error).to.be.an.instanceOf(Error); expect((error as Error).message).to.include("Invalid app name"); } });
188-205: Consider adding error case test for deleteAppSimilar to updateApp, there's no test for error handling in the deleteApp method.
Add an error case test:
it("should handle error when deleting app", async function() { const appId = "test-app-id"; // Set up nock to intercept request with error nock(`https://${controlHost}`) .delete(`/v1/apps/${appId}`) .reply(403, { message: "Forbidden: Cannot delete this app" }); // Intercept any calls to /me nock(`https://${controlHost}`) .get("/v1/me") .reply(200, { account: { id: "test-account-id" } }); try { await api.deleteApp(appId); expect.fail("Should have thrown an error"); } catch (error) { expect(error).to.be.an.instanceOf(Error); expect((error as Error).message).to.include("Forbidden"); } });
259-275: Consider adding error case test for getMeWhile the success case is tested, there's no test for error handling in the getMe method.
Add an error case test:
it("should handle error when getting user info", async function() { // Set up nock to intercept request with error nock(`https://${controlHost}`) .get("/v1/me") .reply(401, { message: "Unauthorized: Invalid token" }); try { await api.getMe(); expect.fail("Should have thrown an error"); } catch (error) { expect(error).to.be.an.instanceOf(Error); expect((error as Error).message).to.include("Unauthorized"); } });
353-396: Consider adding success case test for revokeKeyWhile error handling for revokeKey is tested, there's no test for the success case.
Add a success case test:
it("should revoke an API key", async function() { const appId = "test-app-id"; const keyId = "test-key-id"; // Set up nock to intercept request nock(`https://${controlHost}`) .delete(`/v1/apps/${appId}/keys/${keyId}`) .reply(204); // Intercept any calls to /me nock(`https://${controlHost}`) .get("/v1/me") .reply(200, { account: { id: "test-account-id" } }); await api.revokeKey(appId, keyId); // No assertion needed as the test would fail if the promise was rejected });
397-424: Consider adding success case test for updateKeySimilar to revokeKey, there's no test for the success case of updateKey.
Add a success case test:
it("should update an API key", async function() { const appId = "test-app-id"; const keyId = "test-key-id"; const updateData = { name: "Updated Key Name", capability: { "channel:*": ["publish", "subscribe"] }, }; const expectedKey = { id: keyId, name: "Updated Key Name", capability: { "channel:*": ["publish", "subscribe"] }, }; // Set up nock to intercept request nock(`https://${controlHost}`) .patch(`/v1/apps/${appId}/keys/${keyId}`, updateData) .reply(200, expectedKey); // Intercept any calls to /me nock(`https://${controlHost}`) .get("/v1/me") .reply(200, { account: { id: "test-account-id" } }); const key = await api.updateKey(appId, keyId, updateData); expect(key).to.deep.equal(expectedKey); });test/unit/services/config-manager.test.ts (1)
269-284: Missing test for getKeyName methodWhile the
getKeyNamemethod is used instoreAppKeytests, there are no dedicated tests for this method like there are forgetApiKeyandgetAppName.Consider adding specific tests for the
getKeyNamemethod:describe("#getKeyName", function() { it("should return the key name for the current app", function() { expect(configManager.getKeyName()).to.equal("Test Key"); }); it("should return the key name for a specific app", function() { expect(configManager.getKeyName("testappid")).to.equal("Test Key"); }); it("should return undefined if app doesn't exist", function() { expect(configManager.getKeyName("nonexistentappid")).to.be.undefined; }); });test/unit/base/base-command.test.ts (1)
272-288: Appropriate fallback behavior testing.The test verifies that
ensureAppAndKeyreturns null when no authentication sources are available, allowing commands to handle this case appropriately.However, there's a minor performance issue with using
delete:Consider using
undefinedassignment instead of thedeleteoperator for better performance:- delete process.env.ABLY_API_KEY; + process.env.ABLY_API_KEY = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 282-282: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
test/hooks/command_not_found/did-you-mean.test.ts (4)
7-8: Remove unused import to keep the test bundle lean
_stringDistanceis imported but never referenced in the file. Aside from failingno‑unused‑vars/import/no‑duplicatesESLint rules, this needlessly pulls extra code into the Jest/Oclif runner.-import * as _stringDistance from "../../../src/utils/string-distance.js";
15-17: Scope theSKIP_CONFIRMATIONflag to individual testsSetting the env‑var at module load time makes it bleed into any other test suites executed in the same process, creating hidden coupling. Prefer setting it inside
setupTestContext.do()(and unsetting it in the globalafterEach) so each spec is hermetic.-// Set environment variable to skip confirmation in tests -process.env.SKIP_CONFIRMATION = "true";Move to:
.do(() => { process.env.SKIP_CONFIRMATION = "true"; }) // ... afterEach(() => { delete process.env.SKIP_CONFIRMATION; sinon.restore(); });
181-187: Return a resolved promise from the defaultrunCommandstub
hook.applyawaitsrunCommand. With the current stub no behaviour is specified, so it returnsundefined, which is fine forawaitbut breaks the Contract: the real method isasyncand always returns aPromise<void>. Explicitly.resolves()keeps the typings and intent clear.-runCommand: sinon - .stub(Config.prototype, "runCommand") +runCommand: sinon + .stub(Config.prototype, "runCommand") + .resolves()
419-428: Use array helpers for readabilityTraversing the stub calls with an imperative
forloop is fine, butsome/everycommunicates the intent more clearly and is less error‑prone:-usageCall = true; -// … -helpCall = true; +usageCall = ctx.stubs.log.args.some(([msg]) => msg === "\nUSAGE"); +helpCall = ctx.stubs.log.args.some(([msg]) => typeof msg === "string" && + msg.includes("See more help with:"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
.cursor/rules/development.mdc(2 hunks).cursor/rules/project.mdc(2 hunks).env.example(1 hunks)README.md(2 hunks)docs/Project-Structure.md(1 hunks)docs/TODO.md(6 hunks)package.json(4 hunks)scripts/run-tests.sh(1 hunks)src/base-command.ts(2 hunks)src/commands/apps/logs/history.ts(1 hunks)src/commands/auth/issue-ably-token.ts(1 hunks)src/commands/auth/revoke-token.ts(1 hunks)src/commands/channels/batch-publish.ts(1 hunks)src/commands/channels/history.ts(1 hunks)src/commands/channels/list.ts(2 hunks)src/commands/channels/publish.ts(5 hunks)src/commands/connections/stats.ts(1 hunks)src/commands/logs/app/history.ts(1 hunks)src/commands/logs/connection-lifecycle/history.ts(1 hunks)src/commands/logs/push/history.ts(1 hunks)src/commands/rooms/list.ts(2 hunks)src/commands/spaces/list.ts(4 hunks)src/commands/version.ts(1 hunks)src/hooks/command_not_found/did-you-mean.ts(1 hunks)src/hooks/init/version-flag.ts(1 hunks)src/mcp/mcp-server.ts(2 hunks)src/services/config-manager.ts(1 hunks)src/services/control-api.ts(3 hunks)src/services/interactive-helper.ts(4 hunks)src/utils/string-distance.ts(1 hunks)src/utils/version.ts(1 hunks)test/README.md(1 hunks)test/e2e/channels/channels-e2e.test.ts(1 hunks)test/e2e/core/basic-cli-minimal.test.ts(1 hunks)test/e2e/core/basic-cli.test.ts(1 hunks)test/hooks/command_not_found/did-you-mean.test.ts(4 hunks)test/integration/commands/channels.test.ts(1 hunks)test/integration/commands/minimal-test.test.ts(1 hunks)test/integration/core/help.test.ts(1 hunks)test/integration/test-utils.ts(1 hunks)test/setup.ts(1 hunks)test/unit/base/base-command.test.ts(1 hunks)test/unit/commands/channels/history.test.ts(1 hunks)test/unit/commands/channels/list.test.ts(1 hunks)test/unit/services/config-manager.test.ts(1 hunks)test/unit/services/control-api.test.ts(1 hunks)test/unit/services/interactive-helper.test.ts(1 hunks)tsconfig.eslint.json(1 hunks)tsconfig.test.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
src/hooks/init/version-flag.ts (1)
src/utils/version.ts (2)
getVersionInfo(9-23)formatVersionJson(35-48)
src/services/interactive-helper.ts (1)
src/services/config-manager.ts (1)
ConfigManager(41-489)
src/hooks/command_not_found/did-you-mean.ts (1)
test/hooks/command_not_found/did-you-mean.test.ts (2)
error(146-161)error(194-208)
src/commands/version.ts (1)
src/utils/version.ts (2)
getVersionInfo(9-23)formatVersionString(28-30)
test/integration/test-utils.ts (1)
src/base-command.ts (2)
isTestMode(106-108)getMockAblyRest(114-119)
src/base-command.ts (1)
src/types/cli.ts (1)
BaseFlags(7-24)
test/unit/services/control-api.test.ts (1)
src/services/control-api.ts (1)
ControlApi(164-567)
test/unit/services/config-manager.test.ts (1)
src/services/config-manager.ts (1)
ConfigManager(41-489)
test/unit/base/base-command.test.ts (2)
src/services/config-manager.ts (1)
ConfigManager(41-489)src/services/interactive-helper.ts (1)
InteractiveHelper(9-151)
test/unit/commands/channels/list.test.ts (1)
src/commands/channels/list.ts (1)
ChannelsList(30-178)
🪛 Biome (1.9.4)
tsconfig.eslint.json
[error] 5-5: Expected an array, an object, or a literal but instead found '// Include all test files'.
Expected an array, an object, or a literal here.
(parse)
src/hooks/command_not_found/did-you-mean.ts
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 179-179: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/integration/commands/channels.test.ts
[error] 64-64: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 65-65: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
test/unit/base/base-command.test.ts
[error] 14-14: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 282-282: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 LanguageTool
test/README.md
[misspelling] ~28-~28: Did you mean “too”?
Context: ...n test/e2e/, these use real API calls to Ably. They verify: - Real-world behavi...
(TO_TOO)
🔇 Additional comments (144)
.cursor/rules/project.mdc (1)
1-17: Format improvements look good.The removal of trailing spaces after the YAML front matter keys and consistency improvements align well with the code formatting standards.
.env.example (1)
1-6: Well-structured environment variable example for E2E tests.This is a helpful addition that clearly documents the required environment variable format for running E2E tests. Including comments about copying the file to
.envand providing the expected API key format makes it easy for developers to set up their test environment.tsconfig.test.json (1)
1-12: Good TypeScript configuration for test files.The test-specific TypeScript configuration correctly:
- Extends the base configuration
- Focuses only on test files
- Includes appropriate type definitions for testing libraries
- Sets necessary compiler options for testing context
This dedicated configuration will help ensure proper type checking and compilation behavior for test files.
src/services/config-manager.ts (1)
49-60: Good implementation of configurable config directory.The addition of
ABLY_CLI_CONFIG_DIRenvironment variable support provides flexibility that's particularly valuable for testing scenarios. This allows tests to use isolated configuration directories without affecting the user's actual configuration.The implementation maintains backward compatibility and has a clear separation of concerns:
- Determine the config directory (from env or default)
- Set the config file path
- Ensure the directory exists and load configuration
src/utils/string-distance.ts (3)
1-2: Good package choice for Levenshtein distance calculation.Fast-levenshtein is a good choice for string distance calculations, offering solid performance for this use case.
4-11: Well-documented function with clear purpose.The JSDoc comments clearly explain the function's purpose and parameters, which helps with maintainability and understanding.
12-37: Excellent implementation with smart threshold calculation.The function is well-implemented with:
- Proper input normalization by replacing spaces with colons
- Smart threshold calculation based on word length with a minimum value of 1
- A fixed maximum distance threshold as a safety measure
- Proper sorting and return value handling
This will be useful for the command suggestion feature, providing a good balance between finding close matches without suggesting unrelated commands.
src/services/control-api.ts (3)
6-6: Good addition of configurable error logging.Adding the
logErrorsoption allows for suppressing error output during tests, which improves testability.
167-172: Well-implemented default for logErrors.The implementation correctly defaults to
trueunless explicitly set tofalse, maintaining backward compatibility with existing code.
533-540: Improved error logging with conditional output.The conditional error logging is well-implemented:
- Logs are only shown when
this.logErrorsis true- The logged error information is simplified to only include essential details
- This allows suppressing noisy error logs during tests
This change supports the broader effort to improve testing coverage while maintaining good developer experience.
src/utils/version.ts (3)
1-24: Well-structured version info utility.The
getVersionInfofunction provides a standardized way to collect version information including runtime details. This approach makes version reporting consistent across the CLI.
25-30: Clean version string formatting.The
formatVersionStringproduces a concise, standard-formatted string that follows common CLI version output conventions.
32-48: Robust JSON formatting with error handling.The
formatVersionJsonfunction:
- Handles pretty-printing with color when requested
- Includes proper error handling when
colorJsonfails- Falls back to standard JSON formatting
- Uses proper TypeScript typing with
ReturnType<typeof getVersionInfo>This is a well-implemented utility that will make version reporting consistent across different output formats.
src/commands/logs/push/history.ts (1)
43-45: Good refactoring for testability.Replacing direct Ably client instantiation with
this.createAblyRestClient(options)improves testability by centralizing client creation. This change supports the PR's goal of improving test coverage by making it easier to mock the Ably client during tests.src/commands/channels/batch-publish.ts (1)
102-102: Refactored client creation using centralized methodThis change replaces direct instantiation with the centralized
createAblyRestClientmethod, which enhances testability by enabling mock clients in test mode. This modification is aligned with the PR's objective of improving test coverage.src/commands/connections/stats.ts (1)
98-98: Improved testability through centralized client creationGood change replacing direct Ably client instantiation with the centralized
createAblyRestClientmethod. This enables proper mocking during tests and maintains consistency with other command implementations.src/mcp/mcp-server.ts (1)
557-558: Well-documented exception to the refactoring patternExcellent explanatory comment clarifying why this particular client instantiation doesn't use the centralized method. This documentation is valuable for future maintenance and helps other developers understand the architectural constraints.
src/commands/logs/connection-lifecycle/history.ts (1)
45-45: Consistent client creation refactoringThis change follows the pattern of centralizing Ably client creation, which supports the PR's testing objectives. The consistent approach across all command files makes the codebase more maintainable and testable.
src/commands/apps/logs/history.ts (1)
58-58: Refactoring improves testability through centralized client creationThis change replaces direct instantiation of Ably REST client with a call to the base class method
createAblyRestClient(), which supports test mode by returning mock clients whenABLY_CLI_TEST_MODEis set. This centralization allows for easier testing and consistent client configuration across commands.src/commands/auth/issue-ably-token.ts (1)
96-96: Good standardization of Ably client creationThis change follows the consistent pattern of centralizing Ably REST client creation through the base class method, making it easier to mock during tests and ensuring uniform client configuration across the CLI.
src/commands/logs/app/history.ts (1)
45-45: Improved testability with centralized client creationThe replacement of direct client instantiation with
createAblyRestClient()aligns with the project-wide refactoring to support test mode. This makes it possible to inject mock clients during testing, which is critical for implementing the comprehensive test suite mentioned in the PR objectives.tsconfig.eslint.json (1)
3-5: Improved test file coverage for lintingThe change to include all TypeScript files in the test directory (
test/**/*.ts) is a good enhancement that ensures consistent code quality across all test files, including the newly added unit, integration, and e2e tests.🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: Expected an array, an object, or a literal but instead found '// Include all test files'.
Expected an array, an object, or a literal here.
(parse)
src/commands/channels/history.ts (1)
66-67: Enhanced testability through centralized client creationThe refactoring to use
createAblyRestClient(flags)instead of direct Ably client instantiation supports mock clients in testing mode, which is essential for proper unit and integration testing without making real API calls.This change is part of a consistent pattern applied across multiple command files to improve testability.
src/commands/channels/list.ts (2)
3-3: Improved import organizationMoving the chalk import to the top of the file with other imports improves code organization and readability.
62-63: Enhanced testability through centralized client creationReplacing direct client instantiation with
createAblyRestClient(flags)enables test mocking and aligns with the systematic refactoring approach seen in other command files.The comment clearly explains the purpose of this change, making the code more maintainable.
src/commands/rooms/list.ts (2)
3-3: Improved import organizationMoving the chalk import to the top of the file with other imports improves code organization.
64-64: Enhanced testability through centralized client creationUsing
createAblyRestClient(this.getClientOptions(flags))improves testability by enabling the injection of mock clients during tests.However, note that this implementation differs slightly from the other commands as it passes
this.getClientOptions(flags)instead of justflags. This might be intentional due toRoomsListextendingChatBaseCommandinstead ofAblyBaseCommand.README.md (2)
14-18: Approve formatting change: TOC list markers updated.The table of contents entries have been switched from hyphens (
-) to asterisks (*). This is a non‑functional, stylistic update. Ensure that this aligns with any existing Markdown style guidelines for the repository (e.g., consistency with other docs).
59-66: Confirm list marker consistency for Commands section.The Commands list now uses
*markers instead of-. Please verify this matches the preferred list style across all documentation, or update accordingly to maintain uniform Markdown formatting.src/hooks/init/version-flag.ts (1)
1-33: Well-structured version flag interceptor implementation.This hook elegantly intercepts the
--versionflag to provide JSON output while preserving oclif's default behavior when needed. The implementation properly differentiates between standalone version flags and the actual "version" command.A few notes about the implementation:
- It intelligently inspects raw arguments to catch flags before oclif processes them
- The conditional logic correctly handles both
--versionand-vflags- JSON output is handled with proper formatting options
- The early exit pattern is appropriate to prevent further CLI processing
src/commands/spaces/list.ts (2)
2-2: Good refactoring to improve testability with centralized client creation.The code has been updated to extend from
SpacesBaseCommandinstead ofChatBaseCommand, which is more appropriate for the functionality. This refactoring aligns with the broader effort to centralize client creation and enable mocking for tests.Also applies to: 26-26, 38-38, 58-58
53-54:Details
✅ Verification successful
Verify the relationship between the two Ably clients.
The command creates two different Ably clients:
- A client with
this.createAblyClient()on line 53- A REST client with
this.createAblyRestClient()on line 58Make sure both clients are necessary and properly closed at the end of the command execution.
Also applies to: 58-59
🏁 Script executed:
#!/bin/bash # Search for other commands that use both client types to confirm pattern # Find all command files that use both client creation methods echo "Files that use both client types:" rg -l "createAblyClient" src/commands/ | xargs rg -l "createAblyRestClient" # Check if other commands follow the same pattern of only closing one client echo "\nCommands that close the client:" rg "client\.close\(\)" src/commands/ -A 1 -B 10Length of output: 26002
🏁 Script executed:
#!/bin/bash # Show where Ably clients are created in spaces/list.ts rg -n "createAbly" src/commands/spaces/list.ts -A 5 -B 5 # Show how restClient is used and whether it's closed rg -n "restClient" src/commands/spaces/list.ts -A 5 -B 5Length of output: 660
Both Ably clients are required and cleanup is correct
- The realtime client returned by
this.createAblyClient(flags)is used for publishing CLI events and is properly closed in thefinallyblock (client.close()).- The REST client from
this.createAblyRestClient(...)is a stateless HTTP client and does not implement or require a.close()method.No further action needed.
docs/Project-Structure.md (1)
69-78: Great documentation of the test structure.The updated documentation provides a clear organization of the test directories, making it easier for contributors to understand where different types of tests should be placed. This aligns with the PR objective of introducing comprehensive testing to the CLI project.
The hierarchy is well-structured with:
- End-to-end tests that run CLI commands in real environments
- Integration tests for command flows
- Unit tests for isolated components
test/integration/commands/minimal-test.test.ts (1)
1-18: Good minimal test to validate test infrastructure.This minimal test provides a basic sanity check for the CLI framework and test setup. I appreciate how it:
- Uses a short timeout to fail fast
- Has clear comments explaining its purpose
- Targets the most reliable command ('help')
For minimal tests like this, it's reasonable to focus on execution without errors rather than specific assertions.
package.json (3)
15-21: Well-structured test scripts with appropriate timeouts.The addition of multiple test scripts with different patterns and timeouts is excellent. This approach allows developers to run targeted test suites rather than the entire test suite, which improves development efficiency. The script hierarchy is logical:
- General tests with 30s timeout
- Integration tests with 30s timeout
- E2E tests with 60s timeout, with further subsets for specific modules
The pretest build step ensures TypeScript is compiled before tests run, preventing test failures due to outdated compiled code.
47-50: Good addition of version-flag hook to init array.Converting the init hook from a string to an array and adding the version-flag hook is a good approach. This allows for multiple initialization hooks to run sequentially, maintaining the existing alias-command functionality while adding version flag support. This structure follows the OCLIF plugin pattern correctly.
90-90: Appropriate test dependency additions.The added dependencies all serve clear purposes for the enhanced testing infrastructure:
@types/fs-extra&fs-extra: For enhanced file system operations in testsdotenv: For test environment variable managementexeca: For running CLI commands programmatically in testsnock: For HTTP request mockingThese are all standard, well-maintained packages appropriate for testing a CLI application.
Also applies to: 102-102, 110-111, 114-114
src/commands/version.ts (4)
1-3: Good use of existing utilities and proper imports.The command correctly imports the base command class and reuses the version utility functions, following the DRY principle and maintaining consistency with other parts of the codebase.
4-15: Well-designed command class structure.The command class follows OCLIF best practices:
- Clear description
- Useful examples with template variables
- Setting the command as hidden (since
--versionflag is preferred)- Reusing global flags from the base command
This implementation aligns well with the rest of the codebase's command structure.
16-30: Clean implementation with proper JSON output handling.The
runmethod implementation is concise and effective:
- Parses flags using OCLIF's built-in parse method
- Gets version info from the shared utility
- Correctly handles JSON output based on flags
- Uses the appropriate formatting for each output type
The implementation properly leverages the base command's JSON output handling, ensuring consistent behavior across the CLI.
1-31:Details
❓ Verification inconclusive
Verify version command works with the new hook.
Ensure this new version command works correctly with the version-flag hook added to package.json. The hook should intercept
--versionflags and use this command's functionality.
🏁 Script executed:
#!/bin/bash # Test both the version command and version flag echo "Testing 'ably version' command:" ./bin/run.js version echo -e "\nTesting 'ably --version' flag:" ./bin/run.js --version echo -e "\nTesting JSON output:" ./bin/run.js version --json ./bin/run.js --version --jsonLength of output: 3400
Manual verification needed: Version command compatibility with the version‑flag hook
The automated test in the sandbox failed due to missing dependencies (
@oclif/core), so please ensure your environment is set up and manually verify that the new hook correctly intercepts both invocation styles:• Run
ably versionand confirm it prints the version info
• Runably --versionand confirm it routes through the same logic
• Test JSON output in both forms:
–ably version --json
–ably --version --jsonOnce you’ve confirmed these work as expected with the hook in
package.json, this command is ready.docs/TODO.md (4)
69-92: Comprehensive core CLI test coverage progress.Excellent progress marking off key test areas for core CLI functionality. The checked items cover critical aspects like base command functionality, flag handling, error flows, and core services. The remaining items like interpolation and client ID behavior are properly identified for future work.
123-135: Good progress on Channels (Pub/Sub) testing.The checked-off items demonstrate good coverage of the pub/sub functionality, including message encoding/decoding, channel reuse, and batch publishing. Important remaining items like encryption testing and presence/occupancy tests are clearly identified.
212-215: Solid integration test coverage for Channels.The completion of integration tests for Channels demonstrates thorough testing of the publish-subscribe functionality, including message delivery and history verification. This forms a critical foundation for the CLI's core functionality.
238-242: Good E2E test coverage of core features.Marking version output verification as complete ensures the CLI's most basic functionality works end-to-end. Similarly, completing the Channels pub/sub E2E tests verifies the most critical real-world functionality of the CLI. This provides confidence in the tool's basic reliability.
Also applies to: 251-254
.cursor/rules/development.mdc (3)
12-25: Excellent addition of mandatory development steps.This new section provides clear and detailed guidance for developers to ensure code quality and consistency. The four mandatory steps cover the essential aspects of quality assurance:
- Building and verifying compilation
- Ensuring code meets linting standards
- Running relevant tests to prevent regressions
- Updating documentation to keep it in sync with code changes
The explicit statement that "Failure to complete these steps means the work is not finished" sets clear expectations.
28-28: Clarified build instructions.The clarification about different build commands is very helpful:
pnpm build: Only compiles TypeScriptpnpm prepare: Compiles TypeScript, updates manifest, and regenerates READMEThis detail helps developers choose the right command for their specific needs.
34-34: Improved library installation guidance.The refinement of the library installation guidance is subtle but important. The addition of "Your training data has a cut-off date" acknowledges the limitations of knowledge and reinforces the importance of using the most current packages.
src/services/interactive-helper.ts (5)
5-7: Good interface design for configurable error logging.The
InteractiveHelperOptionsinterface with an optionallogErrorsproperty provides a clean way to control error output behavior. This change improves testability by allowing errors to be suppressed during test execution.
11-11: LGTM - Properly defined private field.The private
logErrorsfield correctly encapsulates the error logging behavior within the class.
13-19: Constructor properly handles optional parameter.The constructor correctly implements the default behavior (logging enabled) while allowing it to be overridden. The default value assignment using
options.logErrors !== falseis a good pattern to ensure logging is on by default.
77-79: Error handling improvement enhances test compatibility.Conditionally logging errors only when
logErrorsis true improves test output by reducing noise while maintaining error handling logic. This pattern is consistently applied across all three interactive methods.
112-114: Consistent error handling across all methods.The error handling pattern is consistently applied in all three methods (
selectAccount,selectApp, andselectKey), which is a good practice for code maintainability and predictability.Also applies to: 145-147
test/setup.ts (6)
7-15: Excellent safety mechanism to prevent hanging tests.Setting a maximum test runtime with
setTimeoutand usingunref()on the timer are excellent practices to prevent tests from hanging indefinitely while ensuring the timer doesn't keep the process alive unnecessarily.
17-23: Good signal handling for clean test termination.The signal handling for
SIGINTandSIGTERMensures that the test process can exit cleanly when interrupted, which is especially important for CI/CD pipelines and local development.
25-34: Sensible error handling strategy for tests.The error handling strategy is well thought out:
- Exiting on uncaught exceptions avoids hanging tests
- Logging but not exiting on unhandled rejections prevents false positives while still surfacing issues
This is a good balance between robustness and avoiding false failures.
36-39: Useful global helper for force exiting tests.The
forceTestExitglobal function provides a convenient way for tests to exit the process when needed, particularly useful for end-to-end tests that might otherwise hang.
41-55: Environment configuration setup is comprehensive and resilient.The environment loading logic:
- Correctly resolves the
.envpath relative to the project root- Checks for file existence before attempting to load
- Provides informative logs about the loading status
- Gracefully handles missing files and errors
This creates a robust setup for test environment configuration.
56-60: Robust error handling in setup.Catching errors at the top level ensures that setup problems don't prevent tests from running. The lack of a forced exit here is a good choice as it allows tests to run with default settings even if the custom setup fails.
test/e2e/core/basic-cli-minimal.test.ts (5)
4-9: Well-configured execution environment for CLI tests.The
execaOptionsconfiguration:
- Prevents Node debugger interference by clearing
NODE_OPTIONS- Sets a reasonable timeout to avoid hanging tests
- Configures non-rejecting promises for better error handling in tests
This creates a clean, isolated environment for running CLI commands during tests.
11-14: Clear test structure with appropriate timeout.The describe block has a clear, descriptive name and sets an appropriate timeout (15 seconds) that balances giving commands enough time to run while not waiting too long if something hangs.
16-22: Effective version output verification.The version test correctly:
- Runs the CLI with the
--versionflag- Verifies successful execution
- Uses a regex pattern to validate the version format
This ensures the version command works and produces correctly formatted output.
24-31: Good JSON format verification.The JSON version test properly:
- Runs the CLI with both
--versionand--jsonflags- Verifies the command succeeds
- Parses and validates the JSON structure
This ensures the JSON output option works correctly.
33-40: Well-structured help text verification.The help test effectively:
- Runs the CLI with the
helpcommand- Verifies successful execution
- Checks for key expected content in the output
This ensures the help command produces useful, correctly formatted output.
test/integration/core/help.test.ts (5)
7-9: Effective temporary directory creation for test isolation.The
getTestConfigDirhelper function creates unique temporary directories using timestamps, ensuring each test run has isolated configuration.
10-17: Well-structured options factory for CLI execution.The
createExecaOptionsfactory function:
- Clears
NODE_OPTIONSto prevent debugger interference- Sets a custom config directory for isolation
- Configures non-rejecting promises for better test assertions
This creates a clean, isolated environment for each test.
20-29: Good test setup with proper isolation.The test suite correctly:
- Creates a fresh config directory before each test
- Configures the execution options with the new directory
- Uses Mocha's
beforeEachhook for consistent setupThis ensures tests don't interfere with each other or with the user's actual config.
31-34: Proper cleanup in test teardown.The
afterEachhook correctly removes the temporary directory after each test, preventing test pollution and cleaning up resources.
36-48: Comprehensive help command testing.The test for the root help command:
- Verifies the command executes successfully
- Checks that stderr is empty (no errors)
- Validates the presence of expected content including usage and command topics
- Uses trailing spaces in topic checks to ensure complete command names are found
This provides good coverage of the help command's basic functionality.
scripts/run-tests.sh (2)
33-35: Node.js flags are well-configured for test isolation.Good use of Node.js options to disable debugging and enforce strict unhandled rejection behavior. The setup with the TypeScript ESM loader is also well-configured for test execution.
45-53: Robust test process monitoring.The monitoring loop is well-implemented to forcefully terminate hanging test processes. The 1-second check interval is reasonable, and the error handling with
2>/dev/null || trueensures the script continues even if the process already terminated.test/README.md (3)
7-16: Well-structured test documentation.The unit test section clearly explains what aspects of the codebase are covered, providing a good guide for developers to understand the testing strategy.
35-47: Clearly documented E2E test requirements.Good explanation of the E2E test requirements, including the necessary environment variable and what happens if it's not provided. This helps developers quickly understand how to run these tests correctly.
51-59: Excellent E2E test best practices.The E2E test design principles section provides valuable guidance on creating robust, isolated tests with unique channel names and proper error handling. These practices will help ensure reliable and maintainable test suites.
test/integration/commands/channels.test.ts (3)
5-17: Well-structured global mock type declaration.The global type declaration for test mocks is well-defined and provides good type safety for the mock objects used in tests.
68-75: Good basic test for listing channels.The test correctly verifies that the channels list command outputs the expected channel name. This serves as a good baseline integration test.
77-85: Good test for JSON output format.The test appropriately verifies that the JSON output has the expected structure. This ensures the command's output format flexibility works correctly.
src/base-command.ts (3)
106-108: Clear method for detecting test mode.Good implementation of a helper method to check if the CLI is running in test mode. The clear naming and documentation make the purpose obvious.
114-119: Well-structured mock retrieval method.The
getMockAblyRest()method correctly retrieves the mock Ably REST client from the global context only when in test mode, helping to centralize test dependency management.
140-145: Robust client options handling.The code correctly handles converting flags to client options when needed, which makes the method more flexible and reusable across different command implementations.
src/commands/channels/publish.ts (3)
312-324: 👍 Including the channel in per‑message logs improves debuggabilityExplicitly logging
"Message N published successfully to channel XYZ"is a nice usability enhancement, especially when running concurrent tests that operate on multiple channels.
428-431: Great to seecreateAblyRestClient()used consistentlySwitching from
new Ably.Rest()to the shared factory keeps test‑mode mocking and option handling in one place. No issues spotted here.
353-355: Race‑condition safeguard
clearProgressIndicator()is invoked beforelogFinalSummary(), ensuring the interval is stopped even iflogFinalSummary()throws. Good catch.test/unit/services/interactive-helper.test.ts (1)
14-22: Good isolation of external dependenciesStubbing both
inquirer.promptandconsole.logkeeps the tests side‑effect free and fast.sinon.restore()inafterEachguarantees clean state. Nicely done.src/hooks/command_not_found/did-you-mean.ts (1)
24-33: Threshold calculation may reject intuitive matches
Math.min(threshold, 3)caps acceptable Levenshtein distance at 3, meaning a 12‑character typo with 4 errors will offer no suggestion. Consider scaling the cap with length, e.g.Math.max(threshold, 3)or a percentage‑based limit.Would you like empirical data on common misspellings to fine‑tune this heuristic?
test/integration/test-utils.ts (4)
1-5: Well-structured documentation headerThe header clearly explains the purpose of the utility module, which helps developers understand its role in the integration testing framework.
27-31: Good practice defining global type declarationProperly extending the global namespace with a typed variable ensures type safety when accessing the global mocks throughout the test suite.
36-38: Consistent test mode detection with base-command.tsThis implementation correctly matches the
isTestMode()method insrc/base-command.ts, ensuring consistency between test utilities and application code.
43-48: Good error handling for missing mocksThe function properly checks prerequisites and provides a clear error message when attempting to access mocks outside of test mode.
test/e2e/core/basic-cli.test.ts (6)
5-10: Good practices for CLI E2E test setupThe options properly configure the test environment to:
- Clear NODE_OPTIONS to prevent debugger interference
- Disable interactive prompts
- Capture errors without rejecting promises
- Set reasonable timeouts
This ensures consistent, non-interactive test execution across environments.
46-53: Well-structured version command testThis test correctly verifies that the CLI outputs a valid version string in the expected format, which is a fundamental check for CLI functionality.
56-93: Comprehensive JSON output flag testsThese tests thoroughly validate both
--jsonand--pretty-jsonflags, checking for:
- Valid execution without errors
- Parseable JSON output
- Expected JSON structure with version, name, and platform
This ensures the JSON output functionality works correctly across different formatting options.
95-104: Good test for mutually exclusive flagsThis test properly verifies that the CLI correctly handles the constraint that
--jsonand--pretty-jsonflags cannot be used together, checking both the exit code and error message.
108-125: Comprehensive help command validationThis test thoroughly checks the output of the root help command, verifying that all major topics and subcommands are listed, providing confidence that the help system is working correctly.
140-190: Thorough command-not-found handling testsThese tests comprehensively validate the CLI's behavior when unknown or mistyped commands are entered, including:
- Typos with colon separators
- Typos with space separators
- Completely unknown commands
- Topic typos with subcommands
This ensures the CLI provides helpful feedback to users in all error cases.
test/unit/services/control-api.test.ts (6)
5-21: Well-structured test setup with proper cleanupThe test setup correctly:
- Creates a fresh instance for each test
- Disables error logging during tests
- Cleans up nock interceptors before and after each test
This ensures tests don't interfere with each other and makes failures easier to debug.
23-55: Good test coverage for constructor optionsThese tests thoroughly verify that both custom and default control hosts work correctly by:
- Creating instances with different configurations
- Setting up nock to intercept requests to the expected host
- Making actual requests to verify the correct host is used
This ensures the service properly respects configuration options.
57-118: Comprehensive testing of the listApps methodThese tests cover all key scenarios for the listApps method:
- Successfully retrieving a list of apps
- Handling empty responses
- Error handling with appropriate validation
The tests properly set up mock responses for both the getMe call (which happens first) and the actual listApps call.
121-164: Thorough testing of the createApp methodThe tests properly verify:
- Successful app creation with correct request body and response handling
- Error scenarios with appropriate error message validation
This ensures robust behavior in both success and failure cases.
207-256: Good test coverage for getApp methodThese tests thoroughly validate both success and failure scenarios for the getApp method:
- Finding an app by ID from the list of apps
- Appropriate error handling when the app is not found
The tests correctly mock the listApps response which is used internally by getApp.
277-352: Good test coverage for API key operations (part 1)These tests properly verify:
- Listing API keys for an app
- Creating a new API key
- Error handling for both operations
The tests correctly mock the backend responses and validate both success and error cases.
test/unit/services/config-manager.test.ts (8)
7-45: Well-structured test setup with comprehensive stubsThe test setup correctly:
- Stubs all filesystem operations to isolate tests from the actual filesystem
- Provides a realistic mock configuration for testing
- Creates a fresh ConfigManager instance for each test
- Properly restores stubs after each test
This approach ensures tests are reliable, isolated, and represent realistic scenarios.
51-72: Good testing of constructor behaviorThese tests verify two important aspects of the constructor:
- Directory creation when the config directory doesn't exist
- Loading of an existing configuration file
This ensures the ConfigManager correctly initializes regardless of the filesystem state.
74-140: Thorough testing of config retrieval methodsThese tests comprehensively verify the account and app ID retrieval methods:
getCurrentAccountAliasgetCurrentAccountgetCurrentAppIdEach method is tested for both success cases and when no current account/app is configured, ensuring robust behavior in all scenarios.
142-164: Good coverage of getApiKey and getAppName methodsThese tests properly verify:
- Getting API keys for both current and specific apps
- Getting app names
- Handling non-existent apps appropriately
This ensures the config retrieval functions work as expected in all scenarios.
166-202: Comprehensive testing of storeAccount methodThese tests verify critical aspects of account storage:
- Storing a new account with full metadata
- Setting the first account as current
- Updating the internal state correctly
This ensures account management functionality works as expected.
204-242: Thorough testing of storeAppKey methodThese tests cover important aspects of API key storage:
- Storing keys with metadata
- Storing keys for specific accounts
- Error handling for non-existent accounts
This ensures robust API key management functionality.
244-267: Good testing of removeAccount methodThese tests properly verify:
- Successfully removing an account
- Handling non-existent accounts
- Clearing the current account reference when removing the current account
This ensures account removal functionality is robust and handles edge cases correctly.
269-284: Good testing of switchAccount methodThese tests verify key aspects of account switching:
- Successfully switching to another account
- Handling attempts to switch to non-existent accounts
- Updating the current account reference correctly
This ensures the account switching functionality works reliably.
test/unit/commands/channels/history.test.ts (8)
1-5: Appropriate test imports and dependencies.The imports look good, including necessary testing libraries (chai, sinon) and the command class being tested.
6-165: Well-structured testable class implementation.The
TestableChannelsHistoryclass effectively makes the command testable by:
- Overriding protected methods to control test flow
- Providing setters to inject mock behaviors
- Simulating the command's execution path
This approach allows for comprehensive testing of the command's behavior without relying on external dependencies.
167-187: Good mock data structure.The mock history response includes a comprehensive set of message properties that match real-world Ably messages, which is important for realistic testing.
189-225: Thorough test setup.The test setup properly initializes the command instance and stubs necessary methods, ensuring tests run in isolation.
226-264: Comprehensive happy path testing.The test for successful history retrieval correctly verifies:
- That history was called with the expected parameters
- That the output is logged appropriately
- That the found messages message is displayed
This ensures the core functionality works as expected.
265-274: Good error handling test.The test correctly verifies that API errors are caught and properly reported to the user.
275-294: Thorough flag parameter testing.Tests verify that direction, start, and end flags are respected and passed correctly to the history API call, ensuring the command correctly handles all input parameters.
Also applies to: 295-324
327-351: Complete JSON output testing.The test verifies that:
- JSON output is generated when requested
- The output has the expected structure
- The output contains the correct message data
This ensures the JSON formatting functionality works correctly.
test/unit/commands/channels/list.test.ts (9)
1-5: Appropriate test imports and dependencies.The imports correctly include the necessary testing libraries and the command class being tested.
6-197: Well-structured testable command implementation.The
TestableChannelsListclass makes the command testable by:
- Overriding protected methods like
parse(),createAblyClient(), and JSON formatting methods- Providing mock implementations of REST client and response handling
- Simulating the command's execution flow including error paths
This allows for comprehensive testing without external dependencies.
199-230: Realistic mock channel data.The mock channel response includes both channel identifiers and detailed occupancy metrics, which is important for testing the display logic in the command.
232-282: Complete test setup with proper cleanup.The test setup correctly:
- Creates a mock configuration
- Initializes the command instance
- Stubs logging and error methods
- Creates and configures request and client stubs
- Ensures proper cleanup after each test
This provides a clean, isolated environment for each test.
284-309: Comprehensive happy path testing.The test for successful channel listing verifies:
- The REST client is called with the correct parameters
- Channel information is logged appropriately
- The client is properly closed afterward
This ensures the core functionality works as expected.
311-331: Empty response handling test.The test properly verifies the command handles empty channel lists and displays an appropriate message to the user.
333-349: API error handling test.The test verifies that API errors (such as non-200 responses) are caught and properly reported to the user.
351-398: Flag parameter testing.The tests verify that limit and prefix flags are respected and correctly passed to the API requests, ensuring the command handles all input parameters properly.
401-431: JSON output format verification.The test thoroughly verifies JSON output functionality:
- Enabling JSON output mode
- Checking the structure of the output JSON
- Verifying all required fields are present and correctly formatted
This ensures the command's formatting capabilities work correctly.
test/unit/base/base-command.test.ts (6)
1-9: Appropriate test imports and dependencies.The imports include necessary testing libraries and the command class being tested, along with dependencies needed for mocking.
63-111: Thorough test setup with environment isolation.The setup code:
- Preserves and restores the environment between tests
- Stubs filesystem operations to prevent actual file access
- Creates mock instances of dependencies
- Properly cleans up after tests
This provides a clean, isolated environment for each test case.
113-154: Comprehensive web CLI restriction testing.The tests verify:
- Normal operation when not in web CLI mode
- Restriction enforcement when in web CLI mode with restricted commands
- Allowance of permitted commands in web CLI mode
This ensures the command restriction system works correctly.
156-176: Complete JSON output detection testing.The tests verify that the JSON output detection works correctly with all possible flag combinations:
--jsonflag--pretty-jsonflag--format=jsonflag- No JSON-related flags
This ensures commands consistently detect output format preferences.
178-206: Thorough API key parsing tests.The tests verify the API key parsing logic correctly:
- Handles valid API keys in the expected format
- Rejects keys missing a colon (separator between key ID and secret)
- Rejects keys missing a period (separator between app ID and key ID)
- Handles empty keys
This ensures robust validation of API key formats.
208-271: Comprehensive authentication resolution testing.The tests verify that the
ensureAppAndKeymethod correctly:
- Uses app ID and API key from flags when provided
- Falls back to configuration when flags aren't provided
- Uses environment variables when needed
- Handles web CLI mode appropriately
This ensures the command can authenticate in all expected scenarios.
test/e2e/channels/channels-e2e.test.ts (10)
1-11: Good setup for unique test data generation.The helper function for generating unique channel names will prevent test collision issues and ensure test isolation, which is crucial for reliable E2E tests.
12-22: Proper client creation with environment check.The
createAblyClienthelper correctly:
- Verifies the required environment variable is present
- Creates an Ably client with appropriate configuration
- Uses a unique client ID for traceability
This ensures tests fail early with clear errors if prerequisites aren't met.
24-46: Useful SDK interaction helpers.The helper functions encapsulate common operations:
- Publishing test messages
- Retrieving channel history
- Listing available channels
These abstracted helpers make the tests more readable and maintainable.
48-71: Robust retry mechanism for asynchronous operations.The
retryUntilSuccesshelper:
- Supports polling for conditions that may not be immediately satisfied
- Accepts a custom validator function for flexible checking
- Has configurable timeout and interval parameters
- Returns the last result for diagnostic purposes
This is essential for testing eventually-consistent systems like Ably.
73-88: Clean test skipping logic with force exit handling.The code:
- Handles graceful process termination to prevent hanging tests
- Conditionally skips tests when prerequisites aren't available
- Uses Mocha's native skipping mechanism for proper reporting
This ensures the test suite works properly in all environments.
89-119: Proper test setup with unique resources.The before/after hooks:
- Create unique channel names for each test purpose
- Set up test data needed by the tests
- Handle cleanup of signal handlers
- Gracefully continue if setup fails, allowing individual tests to fail explicitly
This supports test isolation and diagnostics.
121-170: Comprehensive channel listing tests with verification.The tests:
- Verify the CLI command produces expected output
- Double-check results using the SDK directly
- Use the retry mechanism to handle eventual consistency
- Test both standard and JSON output formats
This provides confidence that the command works correctly end-to-end.
172-211: End-to-end message publishing test.The test verifies the full publishing workflow:
- Publishes a message using the CLI
- Verifies the CLI reports success
- Retrieves history using the CLI to confirm the message was persisted
This validates both publishing and history retrieval working together.
213-265: Thorough history retrieval testing.The tests:
- Verify both standard and JSON formatted history retrieval
- Check the CLI output contains expected messages
- Validate results using the SDK directly
- Parse JSON responses to verify structure
This ensures history retrieval works correctly in all formats.
267-352: Advanced publishing feature tests.These tests verify more complex publishing scenarios:
- Batch publishing to multiple channels
- Publishing multiple messages with count parameter and templating
- Verification via history to confirm all messages were published
This provides coverage of advanced CLI features beyond basic operations.
test/hooks/command_not_found/did-you-mean.test.ts (2)
96-101: Verify that mutatingconfig.commandIDsis safe
Config.load()buildscommandIDsfrom discovered plugins and marks the array as non‑writable in some oclif versions (it’s a getter in v3). Directly pushing into it risks breaking compatibility across oclif updates.
Consider stubbingconfig.findCommand()instead, or extend the testConfigwith your own property rather than mutating the internal one.
110-111: AvoidreplaceAllfor regex to keep Node 14 compatibility
String.prototype.replaceAllwith a regex literal landed in Node 15+. The CLI still advertises Node 14 LTS support. A single globalreplaceachieves the same result:-const stripAnsi = (str: string) => str.replaceAll(/\u001B\[(?:\d*;)*\d*m/g, ""); +const stripAnsi = (str: string) => str.replace(/\u001B\[(?:\d*;)*\d*m/g, "");🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
| const customConfigDir = process.env.ABLY_CLI_CONFIG_DIR; | ||
| this.configDir = customConfigDir || path.join(os.homedir(), ".ably"); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding tests for this new environment variable configuration.
Since you've added the ability to configure the config directory via an environment variable, ensure you have test coverage for this new feature. Verify both when the variable is set and when it's not.
🏁 Script executed:
#!/bin/bash
# Check if tests exist for the ABLY_CLI_CONFIG_DIR environment variable functionality
# Look for tests that might be testing this functionality
rg -l "ABLY_CLI_CONFIG_DIR" test/Length of output: 68
🏁 Script executed:
#!/bin/bash
# Show context around the ABLY_CLI_CONFIG_DIR reference in the integration test
rg -n "ABLY_CLI_CONFIG_DIR" -A3 -B3 test/integration/core/help.test.tsLength of output: 376
Add tests for default config directory fallback
- You already have a test in
test/integration/core/help.test.ts(lines 11–15) that verifies the custom directory viaABLY_CLI_CONFIG_DIR. - Please add a complementary test where
ABLY_CLI_CONFIG_DIRis unset, and assert thatconfigManagerfalls back topath.join(os.homedir(), ".ably").
| "include": [ | ||
| "src/**/*", | ||
| "test/**/*.test.ts", // Ensure test files use .test.ts convention | ||
| "test/**/*.ts", // Include all test files |
There was a problem hiding this comment.
JSON syntax issue in ESLint configuration
The line has a trailing comment which is not valid in JSON. While this might work in some environments that support JSONC (JSON with Comments), it could cause issues in stricter JSON parsers.
Apply this diff to fix the JSON syntax error:
- "test/**/*.ts", // Include all test files
+ "test/**/*.ts",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "test/**/*.ts", // Include all test files | |
| "test/**/*.ts", |
🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: Expected an array, an object, or a literal but instead found '// Include all test files'.
Expected an array, an object, or a literal here.
(parse)
| // Create a testable implementation of the abstract AblyBaseCommand | ||
| class TestCommand extends AblyBaseCommand { | ||
| // Expose protected methods for testing | ||
| public testCheckWebCliRestrictions(): void { | ||
| return this.checkWebCliRestrictions(); | ||
| } | ||
|
|
||
| public testIsAllowedInWebCliMode(command?: string): boolean { | ||
| return this.isAllowedInWebCliMode(command); | ||
| } | ||
|
|
||
| public testShouldOutputJson(flags: BaseFlags): boolean { | ||
| return this.shouldOutputJson(flags); | ||
| } | ||
|
|
||
| public testParseApiKey(apiKey: string) { | ||
| return this.parseApiKey(apiKey); | ||
| } | ||
|
|
||
| public testEnsureAppAndKey(flags: BaseFlags): Promise<{ apiKey: string; appId: string } | null> { | ||
| return this.ensureAppAndKey(flags); | ||
| } | ||
|
|
||
| // Make protected properties accessible for testing | ||
| public get testConfigManager(): ConfigManager { | ||
| return this.configManager; | ||
| } | ||
|
|
||
| public set testConfigManager(value: ConfigManager) { | ||
| this.configManager = value; | ||
| } | ||
|
|
||
| public get testInteractiveHelper(): InteractiveHelper { | ||
| return this.interactiveHelper; | ||
| } | ||
|
|
||
| public set testInteractiveHelper(value: InteractiveHelper) { | ||
| this.interactiveHelper = value; | ||
| } | ||
|
|
||
| public get testIsWebCliMode(): boolean { | ||
| return this.isWebCliMode; | ||
| } | ||
|
|
||
| public set testIsWebCliMode(value: boolean) { | ||
| this.isWebCliMode = value; | ||
| } | ||
|
|
||
| async run(): Promise<void> { | ||
| // Empty implementation | ||
| } | ||
| } |
There was a problem hiding this comment.
Effective approach for testing abstract class.
The TestCommand class provides a concrete implementation of the abstract AblyBaseCommand, exposing protected methods and properties for testing. This is an excellent pattern for testing abstract classes.
However, there's an issue in the testCheckWebCliRestrictions method:
The method is declared with void return type but returns a value. Apply this fix:
- public testCheckWebCliRestrictions(): void {
- return this.checkWebCliRestrictions();
+ public testCheckWebCliRestrictions(): void {
+ this.checkWebCliRestrictions();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create a testable implementation of the abstract AblyBaseCommand | |
| class TestCommand extends AblyBaseCommand { | |
| // Expose protected methods for testing | |
| public testCheckWebCliRestrictions(): void { | |
| return this.checkWebCliRestrictions(); | |
| } | |
| public testIsAllowedInWebCliMode(command?: string): boolean { | |
| return this.isAllowedInWebCliMode(command); | |
| } | |
| public testShouldOutputJson(flags: BaseFlags): boolean { | |
| return this.shouldOutputJson(flags); | |
| } | |
| public testParseApiKey(apiKey: string) { | |
| return this.parseApiKey(apiKey); | |
| } | |
| public testEnsureAppAndKey(flags: BaseFlags): Promise<{ apiKey: string; appId: string } | null> { | |
| return this.ensureAppAndKey(flags); | |
| } | |
| // Make protected properties accessible for testing | |
| public get testConfigManager(): ConfigManager { | |
| return this.configManager; | |
| } | |
| public set testConfigManager(value: ConfigManager) { | |
| this.configManager = value; | |
| } | |
| public get testInteractiveHelper(): InteractiveHelper { | |
| return this.interactiveHelper; | |
| } | |
| public set testInteractiveHelper(value: InteractiveHelper) { | |
| this.interactiveHelper = value; | |
| } | |
| public get testIsWebCliMode(): boolean { | |
| return this.isWebCliMode; | |
| } | |
| public set testIsWebCliMode(value: boolean) { | |
| this.isWebCliMode = value; | |
| } | |
| async run(): Promise<void> { | |
| // Empty implementation | |
| } | |
| } | |
| // Expose protected methods for testing | |
| public testCheckWebCliRestrictions(): void { | |
| this.checkWebCliRestrictions(); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
Previously, account information was being displayed even when shouldHideAccountInfo() returned true. This fix ensures that: 1. Account info is not displayed when explicit auth credentials are provided (API key, token, access token) or when using environment variables 2. No output is shown when there are no parts to display Added comprehensive test coverage for displayAuthInfo() to prevent future regressions, specifically testing all conditions where account info should be hidden and ensuring empty output isn't displayed.
- Split channel e2e tests into separate files based on command structure: - channels-e2e.test.ts: Root-level commands (list, publish, history) - channel-presence-e2e.test.ts: Presence-specific commands - channel-occupancy-e2e.test.ts: Occupancy-specific commands - channel-subscribe-e2e.test.ts: Subscribe functionality - channel-history-e2e.test.ts: Additional history tests - Update helper imports to correct path - Remove unused imports to fix lint errors - Improve type safety in e2e-test-helper.ts - Update Testing.md with guidance on e2e test organization
- Make filesystem read-only with controlled write access via tmpfs mounts - Add resource limits (processes, memory, CPU) to prevent container abuse - Implement session timeout for both inactivity and maximum duration - Create Security-Hardening.md documentation - Update TODO.md to reflect completed security tasks
This commit implements multiple container security hardening measures and adds comprehensive documentation: - Fixed Docker image name consistency (using 'ably-cli-sandbox') - Updated allowed domains to include both *.ably.io and *.ably.com - Resolved duplicate properties in container configuration - Added infrastructure for network restriction implementation - Created Security-Hardening.md with current and planned security measures - Updated TODO.md to track completed security tasks - Fixed linter errors and removed unused imports
- Prevent shell command injection via enhanced shell script that sanitizes input and validates commands, fixing issue with operators like & and | - Enhance network security: - Add *.ably.net to allowed domains in network-security.sh - Implement network filtering with iptables and TCP wrappers - Explicitly block raw socket access - Apply network restrictions via network-security.sh entrypoint - Implement system call filtering with custom seccomp profile: - Add docker/seccomp-profile.json with allowed syscalls - Mount and apply seccomp profile to containers - Restrict potentially dangerous operations - Improve code quality: - Remove duplicated ALLOWED_DOMAINS constant - Fix keepAliveInterval variable scoping - Remove deprecated restricted-shell.sh These changes complete several security tasks in docs/TODO.md and significantly improve the security posture of the Docker container.
This commit implements the final set of security hardening features for the Docker container: - Add user namespace remapping support - Create detailed documentation for host setup in docs/User-Namespace-Remapping.md - Update container creation with explicit user settings for proper remapping - Implement AppArmor profile for mandatory access control - Create a comprehensive profile restricting filesystem access and executable paths - Add installation script for easy AppArmor profile setup - Configure terminal-server.ts to dynamically detect and use the profile - Set up enhanced logging and monitoring - Create security-monitor.sh script for comprehensive security event tracking - Implement logging for AppArmor violations, blocked syscalls, and resource usage - Add alerting capability for potential security breaches - Update documentation - Mark all security tasks as completed in TODO.md - Update Security-Hardening.md with implementation details These changes complete the container security hardening roadmap, creating a robust multi-layered security architecture that follows defense-in-depth principles.
Add automated security testing for Docker container hardening: - Created test-security.sh script for verifying container security settings - Implemented GitHub Actions workflow for CI/CD security testing - Added Security-Testing-Auditing.md documentation - Created test-dockerfile for isolated testing environment - Updated TODO.md to mark security testing tasks as completed - Fixed ESLint configuration to ignore examples directory This implementation completes the security testing and auditing requirements for the container hardening initiative, providing both CI/CD validation and local testing capabilities to verify security configurations.
- Implement comprehensive integration tests in `test/integration/docker-container-security.test.ts` to verify Docker container security hardening settings, including: - Readonly root filesystem - Resource limits (PIDs, memory) - Dropped capabilities - Seccomp profile application - Non-root user execution - Filesystem write restrictions - Add basic integration tests in `test/integration/terminal-server.test.ts` to verify the test file structure and dependency availability for the terminal server. - Update `docs/Security-Testing-Auditing.md` to document the new automated security tests. - Update `docs/TODO.md` reflecting completion of test tasks. - Fix `scripts/run-tests.sh` test runner to correctly discover and execute all test suites, explicitly including the new integration tests when running the full suite.
1. Performance improvements to terminal-server.ts: - Optimize seccomp profile handling in container creation - Maintain pre-loaded seccomp profile content in memory - Fix error when seccomp profile isn't correctly formatted 2. Fix lint warnings in web-cli.test.ts: - Properly prefix unused type imports with underscore - Replace problematic test.skip() with environment variable flag - Improve test skipping logic to avoid "nested test" lint errors 3. Add web terminal server tests from the React CLI component
See #9 Process Cleanup (scripts/terminal-server.ts): Added proper process termination waiting using Promise.allSettled Added missing variable declarations for terminalServerProcess and webServerProcess Fixed import statement to use node:child_process Shell Injection Prevention (docker/enhanced-restricted-shell.sh): Replaced unsafe argument parsing with IFS=' ' read -r -a args <<< "$rest_of_line" Removed the old array building loop that could break quoted arguments Session Timeout (scripts/terminal-server.ts): Implemented safer session timeout handling by collecting IDs first Process timeouts after iteration to avoid modifying the Map during iteration Maintained the same timeout logic but in a safer way Seccomp Profile (scripts/terminal-server.ts): Added proper JSON parsing and stringifying to ensure clean JSON format Removed redundant comment Maintained the same error handling but with cleaner code
Preventing premature deletion of containers with AutoRemove: false Giving containers time to start with delays Adding retry logic for container operations Ensuring network configuration is valid Explicitly handling container cleanup
- Add Ubuntu systemd service setup script (`scripts/setup-terminal-server.sh`) with Caddy integration for automatic HTTPS via Let's Encrypt.
- Add server setup documentation (`docs/Server-Setup.md`) including custom branch installation and Caddy details.
- Consolidate server documentation into `Server-Setup.md`, removing `Terminal-Server.md`.
- Rename utility scripts:
- `test-container.sh` -> `diagnostics-container.sh`
- `terminal-test-client.ts` -> `diagnostics-server.ts`
- Enhance `diagnostics-container.sh`:
- Add check for `ably help status` including network access.
- Fix TTY check by overriding entrypoint.
- Pass dummy credentials for container run check.
- Enhance `diagnostics-server.ts`:
- Accept target URL via env/arg.
- Use dummy credentials.
- Improve command checks (`ably` then `ably help status`).
- Make status check more robust (ignore "operational").
- Fix timeouts and linter errors.
- Add E2E tests (`test/e2e/core/diagnostics.test.ts`) for both diagnostic scripts, including self-contained server setup for server diagnostics test.
- Enhance `terminal-server.ts`:
- Add `ensureDockerImage` to build sandbox image if missing.
- Fix container lifecycle race conditions (`attachToContainer`, `cleanupSession`).
- Fix seccomp profile path resolution and application.
- Fix AppArmor check path resolution (run once at start).
- Optimize performance by reading seccomp profile only once.
- Fix `AutoRemove: true` issue causing premature container deletion.
- Add `isAttaching` flag and synchronization logic.
- Enhance `docker/enhanced-restricted-shell.sh`:
- Fix argument parsing for quoted strings (`read -a`).
- Enhance `docker/network-security.sh`:
- Consolidate allowed domains/IPs into a single list.
- Refine iptables rule logic (resolve IPs before setting rules).
- Enhance `examples/web-cli/src/App.tsx`:
- Allow overriding terminal server URL via `VITE_TERMINAL_SERVER_URL` env var or `serverUrl` query param.
- Fix dummy access token structure for local dev.
- Update `README.md` with new Diagnostics section and link to consolidated server setup docs.
- Update `package.json` with new diagnostic scripts, remove redundant ones.
- Apply feedback from CodeRabbit PR #9: - refactor(test): Wait for spawned server process exit in E2E diagnostics test (`test/e2e/core/diagnostics.test.ts`) `after` hook using `Promise.allSettled`. (Addresses Process Cleanup feedback) - fix(shell): Use `read -r -a` for safer argument parsing in `enhanced-restricted-shell.sh`. (Addresses Shell Injection feedback - Note: Verified already applied) - fix(server): Collect session IDs before terminating in `startSessionMonitoring` to avoid map iteration issues (`scripts/terminal-server.ts`). (Addresses Session Timeout Map Iteration feedback - Note: Verified already applied) - fix(server): Ensure seccomp profile JSON is properly stringified in `terminal-server.ts`. (Addresses Seccomp Profile Handling feedback - Note: Verified already applied) - chore: Remove unused variables and commented code from `test/e2e/core/diagnostics.test.ts`.
- At this stage it's evident we have leaky tests as tests individually run, but fail when run as one. We will work on that issue next. - This change also improves test command support for `pnpm test [path]` where path is used for tests and linting
This commit implements a significant portion of the plan outlined in docs/Test-Isolation.md to improve test stability and isolation across the suite.
Key changes include:
- Removed `--exit` flags from test scripts (`scripts/run-tests.sh`, `package.json`) to prevent premature termination and expose potential hangs.
- Isolated `ConfigManager` unit tests (`test/unit/services/config-manager.test.ts`) by using unique temporary configuration directories and refining environment variable handling.
- Enhanced global cleanup logic (`test/setup.ts`) for Ably clients, particularly Realtime connections, adding state checks, timeouts, and more robust listener removal.
- Improved E2E test helpers (`test/helpers/e2e-test-helper.ts`):
- Ensured Ably clients created via helpers are tracked for cleanup.
- Replaced broad `pkill` cleanup with specific PID tracking and killing for background processes (`runLongRunningBackgroundProcess`, `killProcess`).
- Added tracking and cleanup for temporary files (`createTempOutputFile`, `cleanupTrackedResources`).
- Introduced `runBackgroundProcessAndGetOutput` for simpler E2E tests not requiring long-running processes.
- Refactored previously flaky/failing tests (`channel-occupancy-e2e.test.ts`, `channel-subscribe.test.ts` unit tests) to use improved helpers and mocking techniques.
- Fixed various lint warnings identified during the process.
As a result, the test suite is significantly more robust, and individual test groups (`pnpm test:configmanager`, `pnpm test:other`) now pass reliably (excluding pending/skipped E2E).
Known Issue:
A lingering `_ErrorInfo: unable to handle request...` still occurs after the full test suite finishes sequentially (`pnpm test`). This prevents consolidating the main `test` script in `package.json` at this time and requires further, deeper debugging into potential async/cleanup race conditions, likely related to E2E tests or SDK shutdown behavior.
This commit addresses persistent instability in the test suite, primarily manifested as an `_ErrorInfo: unable to handle request; no application id found in request` error occurring non-deterministically after the full test suite (`test/**/*.test.ts`) appeared to complete. The root cause was identified as state leakage between different test groups (unit, integration, e2e) when run together in the same Mocha process. Key contributors were: - Inconsistent Sinon.JS mock/stub cleanup: Some unit tests relied on the global `sinon.restore()` instead of using isolated sandboxes, leading to mocks potentially persisting across tests. - Environment variable leakage: An integration test file (`test/integration/commands/channels.test.ts`) modified `process.env.ABLY_API_KEY` without restoring it, potentially affecting subsequent tests. The following changes were made: - Refactored affected unit tests (`list.test.ts`, `base-command.test.ts`, `auth-info-display.test.ts`, `interactive-helper.test.ts`, hook tests) to consistently use `sinon.createSandbox()` and `sandbox.restore()` for reliable mock isolation. - Corrected environment variable handling in `test/integration/commands/channels.test.ts` to use `beforeEach`/`afterEach` hooks. - Fixed secondary test failures in `test/unit/commands/channels/presence/enter.test.ts` that were revealed by the improved test isolation. This involved correcting mock setups and assertion logic specific to those tests. - Implemented the previously pending test case for JSON output in `enter.test.ts`. - Ensured test cleanup runs reliably via Mocha's root hook mechanism (`test/root-hooks.ts`). - Addressed minor linter warnings related to test structure and unused variables. As a result, the full test suite now passes reliably when executed with `pnpm test` (which runs all tests matching `test/**/*.test.ts` together), resolving the original instability and achieving the desired test isolation.
Lost in rebase when merging in container-hardening
Provide specific versions of dependencies
mattheworiordan
force-pushed
the
initial-test-coverage
branch
from
92d0d5d to
06851fc
Compare
Fixes Playwright issues that existed in CI.
mattheworiordan
force-pushed
the
initial-test-coverage
branch
from
f59932b to
d731bc5
Compare
This PR introduces testing into the CLI (finally).
Testing is approached as unit tests, integration tests and e2e (end to end tests), with the latter exercising the realtime and control APIs.
The goal is to have reasonable coverage of core functionality before we can consider this CLI to be stable.
Summary by CodeRabbit
New Features
versioncommand and improved version flag handling for more flexible CLI version output, including JSON and pretty-printed formats.Bug Fixes
Refactor
Chores
Style
Documentation