test: introduce mock config manager, use in tests #120
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request refactors the configuration management system from a concrete class to an interface-based design with a factory pattern. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Areas requiring extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AndyTWF
force-pushed
the
config-manager-updates
branch
from
3c433d8 to
55c0272
Compare
AndyTWF
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/unit/commands/channels/history.test.ts (1)
4-9: AddconfigManagerto the type definition.The comment at line 41 indicates that
configManageris expected to exist inglobalThis.__TEST_MOCKS__, but the type definition only declaresablyRestMock. This creates a type safety gap.Apply this diff to include
configManagerin the type definition:declare global { var __TEST_MOCKS__: { ablyRestMock?: unknown; + configManager?: unknown; }; }test/unit/commands/auth/issue-jwt-token.test.ts (1)
184-196: Restore mock configuration state after the test.The test at line 187 clears the app configuration (
setCurrentAppIdForAccount(undefined)) but doesn't restore it afterward. This could affect subsequent tests that rely on the default app configuration fromDEFAULT_TEST_CONFIG.Consider adding an
afterEachhook or wrapping this test to restore the configuration:it("should not produce token output when app configuration is missing", async () => { // Use mock config manager to clear app configuration const mock = getMockConfigManager(); const originalAppId = mock.getCurrentAppId(); mock.setCurrentAppIdForAccount(undefined); const { stdout } = await runCommand( ["auth:issue-jwt-token"], import.meta.url, ); // Restore the original state mock.setCurrentAppIdForAccount(originalAppId); // When no app is configured, command should not produce token output expect(stdout).not.toContain("Generated Ably JWT Token"); });
🧹 Nitpick comments (15)
test/unit/commands/spaces/locations/get-all.test.ts (2)
5-10: beforeEach cleanup might be redundant.The
beforeEachdeletes the same mocks thatafterEachalready cleans up. While this defensive approach ensures a clean state even if a previous test fails, it may be unnecessary given thatafterEachruns after each test. Consider removing this redundant cleanup ifafterEachconsistently runs.
12-17: Add explicit mock restoration and resource cleanup.As per coding guidelines, tests must ensure resources are properly closed and mocks are restored in
afterEach. Consider adding:
vi.restoreAllMocks()to clear all mock/spy state between tests- Although the mock clients are created per-test, establishing the pattern of calling
close()would align with the guideline requirementBased on coding guidelines: "Always ensure resources are properly closed/cleaned up in tests (e.g., call await client.close() and sinon.restore() in afterEach)."
Apply this diff to add explicit cleanup:
afterEach(() => { if (globalThis.__TEST_MOCKS__) { delete globalThis.__TEST_MOCKS__.ablyRealtimeMock; delete globalThis.__TEST_MOCKS__.ablySpacesMock; } + vi.restoreAllMocks(); });test/unit/commands/rooms/reactions/subscribe.test.ts (1)
6-18: Consider consolidating cleanup logic.The mock cleanup is duplicated in both
beforeEachandafterEach. Since each test sets up mocks inline andafterEachalready performs cleanup, thebeforeEachcleanup appears redundant. Consider removing thebeforeEachblock to simplify the test structure.Apply this diff to consolidate cleanup:
- beforeEach(() => { - if (globalThis.__TEST_MOCKS__) { - delete globalThis.__TEST_MOCKS__.ablyRealtimeMock; - delete globalThis.__TEST_MOCKS__.ablyChatMock; - } - }); - afterEach(() => { if (globalThis.__TEST_MOCKS__) { delete globalThis.__TEST_MOCKS__.ablyRealtimeMock; delete globalThis.__TEST_MOCKS__.ablyChatMock; } });test/unit/commands/rooms/messages/reactions/subscribe.test.ts (1)
87-94: Guard against undefined__TEST_MOCKS__when spreading.The spread operator assumes
globalThis.__TEST_MOCKS__is already initialized. If the test setup fails to initialize it, this will throw a runtime error.Consider adding a defensive check:
- globalThis.__TEST_MOCKS__ = { - ...globalThis.__TEST_MOCKS__, + globalThis.__TEST_MOCKS__ = { + ...(globalThis.__TEST_MOCKS__ || {}), ablyChatMock: { rooms: mockRooms, realtime: mockRealtimeClient, } as any, ablyRealtimeMock: mockRealtimeClient as any, };This is optional since the test setup files should initialize
__TEST_MOCKS__, but it adds resilience against setup issues.Also applies to: 183-190
test/unit/commands/spaces/cursors/get-all.test.ts (1)
75-79: Consider defensive spreading for undefined__TEST_MOCKS__.All five mock injection points use the spread operator without checking if
globalThis.__TEST_MOCKS__exists. While test setup should initialize it, adding...(globalThis.__TEST_MOCKS__ || {})would make the tests more resilient.Also applies to: 151-155, 208-212, 259-263, 314-318
test/unit/commands/apps/logs/subscribe.test.ts (1)
51-58: Optional: Add defensive check when spreading__TEST_MOCKS__.Same pattern as other files—consider
...(globalThis.__TEST_MOCKS__ || {})for resilience, though test setup should initialize the global.Also applies to: 95-102
test/unit/commands/rooms/messages/reactions/remove.test.ts (1)
111-115: Optional: Defensive spreading for robustness.Consider using
...(globalThis.__TEST_MOCKS__ || {})to handle cases where test setup might not have initialized__TEST_MOCKS__.test/e2e/auth/basic-auth.test.ts (1)
71-74: Consider moving the dynamic import to the top level.The dynamic import pattern is repeated across multiple test cases. Consider importing
TomlConfigManagerat the module level for cleaner code.Apply this diff to simplify:
+import { TomlConfigManager } from "../../../src/services/config-manager.js"; import { describe, it, ... - const { TomlConfigManager } = await import( - "../../../src/services/config-manager.js" - ); - const configManager = new TomlConfigManager(); + const configManager = new TomlConfigManager();test/unit/commands/auth/issue-ably-token.test.ts (1)
43-48: Optional: Extract mock setup to a helper function.The conditional mock assignment pattern is repeated 9 times. Consider extracting to a helper:
function setAblyRestMock(mockAuth: object) { if (globalThis.__TEST_MOCKS__) { globalThis.__TEST_MOCKS__.ablyRestMock = { auth: mockAuth, close: vi.fn(), }; } }This is a minor suggestion for reducing duplication.
test/unit/commands/queues/list.test.ts (1)
1-20: Missing beforeEach could cause test isolation issues.This file lacks a
beforeEachhook to reset mock state before each test. Most tests assumeDEFAULT_TEST_CONFIGaccounts exist, but the test at line 372-380 callsgetMockConfigManager().clearAccounts()which modifies global state. If tests run in different orders, this could cause failures.Add a
beforeEachto ensure consistent state:+import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { describe, it, expect, afterEach } from "vitest"; import nock from "nock"; import { runCommand } from "@oclif/test"; import { DEFAULT_TEST_CONFIG, getMockConfigManager, + resetMockConfig, } from "../../../helpers/mock-config-manager.js"; describe("queues:list command", () => { const mockAppId = DEFAULT_TEST_CONFIG.appId; const mockAccountId = DEFAULT_TEST_CONFIG.accountId; + beforeEach(() => { + nock.cleanAll(); + resetMockConfig(); // Ensure consistent state + }); + afterEach(() => { nock.cleanAll(); });test/unit/commands/apps/create.test.ts (1)
10-16: Consider resetting mock config in beforeEach for test isolation.The
beforeEachonly cleans nock, but tests modify the mock config (e.g., the "auto-switch" test updates current app). Without resetting the mock between tests, modifications could leak across tests if execution order changes.beforeEach(() => { nock.cleanAll(); + getMockConfigManager().reset(); });test/unit/commands/apps/delete.test.ts (1)
9-15: Consider resetting mock config in beforeEach for test isolation.Similar to create.test.ts, the test at line 262 calls
mock.setCurrentAppIdForAccount(undefined)which modifies shared mock state. Without a reset, this could affect other tests.beforeEach(() => { nock.cleanAll(); + getMockConfigManager().reset(); });src/services/config-manager.ts (1)
468-498: LGTM - Migration logic handles legacy config format.The migration from
current.apptoaccounts[alias].currentAppIdis correct. Usinganyfor accessing legacy properties is appropriate given theeslint-disablecomment.Consider adding a debug log when migration occurs to help users understand config format changes:
this.config.accounts[currentAccountAlias].currentAppId = oldConfig.current.app; delete oldConfig.current.app; // Remove from current section + console.debug('Migrated config from legacy format'); this.saveConfig(); // Save the migrated configtest/helpers/mock-config-manager.ts (2)
255-267: Minor inconsistency:removeAccountdoesn't callsaveConfig().
TomlConfigManager.removeAccount()callssaveConfig()after deletion, butMockConfigManagerdoesn't. While functionally equivalent (mock'ssaveConfigis a no-op), matching the pattern would ensure consistent behavior if tests ever verify save operations.public removeAccount(alias: string): boolean { if (!this.config.accounts[alias]) { return false; } delete this.config.accounts[alias]; if (this.config.current?.account === alias) { delete this.config.current.account; } + // Match TomlConfigManager behavior (no-op for mock but consistent) + this.saveConfig(); return true; }
429-436: Apply the suggested defensive initialization pattern to preserve existing mocks.The test suite consistently uses spread operator merging when updating
__TEST_MOCKS__(see comments inbenchmarking.test.ts,spaces.test.ts, etc. stating "don't overwrite configManager"). To follow this established pattern and guard against accidental re-initialization, conditionally setconfigManageronly if it doesn't already exist:export function initializeMockConfigManager(): void { if (!globalThis.__TEST_MOCKS__) { globalThis.__TEST_MOCKS__ = { ablyRestMock: {}, }; } - globalThis.__TEST_MOCKS__.configManager = new MockConfigManager(); + if (!globalThis.__TEST_MOCKS__.configManager) { + globalThis.__TEST_MOCKS__.configManager = new MockConfigManager(); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (75)
package.json(1 hunks)src/base-command.ts(2 hunks)src/commands/config/show.ts(2 hunks)src/commands/mcp/start-server.ts(2 hunks)src/help.ts(2 hunks)src/services/config-manager.ts(4 hunks)test/e2e/auth/basic-auth.test.ts(6 hunks)test/helpers/mock-config-manager.ts(1 hunks)test/setup.ts(2 hunks)test/unit/base/base-command.test.ts(2 hunks)test/unit/commands/accounts/login.test.ts(6 hunks)test/unit/commands/accounts/logout.test.ts(7 hunks)test/unit/commands/apps/channel-rules/create.test.ts(1 hunks)test/unit/commands/apps/channel-rules/delete.test.ts(1 hunks)test/unit/commands/apps/channel-rules/list.test.ts(1 hunks)test/unit/commands/apps/channel-rules/update.test.ts(1 hunks)test/unit/commands/apps/create.test.ts(13 hunks)test/unit/commands/apps/current.test.ts(2 hunks)test/unit/commands/apps/delete.test.ts(19 hunks)test/unit/commands/apps/logs/history.test.ts(3 hunks)test/unit/commands/apps/logs/subscribe.test.ts(3 hunks)test/unit/commands/apps/set-apns-p12.test.ts(1 hunks)test/unit/commands/apps/update.test.ts(17 hunks)test/unit/commands/auth/issue-ably-token.test.ts(12 hunks)test/unit/commands/auth/issue-jwt-token.test.ts(6 hunks)test/unit/commands/auth/keys/create.test.ts(25 hunks)test/unit/commands/auth/keys/current.test.ts(2 hunks)test/unit/commands/auth/keys/get.test.ts(3 hunks)test/unit/commands/auth/keys/list.test.ts(6 hunks)test/unit/commands/auth/keys/revoke.test.ts(3 hunks)test/unit/commands/auth/keys/update.test.ts(6 hunks)test/unit/commands/auth/revoke-token.test.ts(16 hunks)test/unit/commands/bench/benchmarking.test.ts(2 hunks)test/unit/commands/channel-rule/create.test.ts(1 hunks)test/unit/commands/channel-rule/delete.test.ts(1 hunks)test/unit/commands/channel-rule/list.test.ts(1 hunks)test/unit/commands/channel-rule/update.test.ts(1 hunks)test/unit/commands/channels/batch-publish.test.ts(2 hunks)test/unit/commands/channels/history.test.ts(2 hunks)test/unit/commands/channels/list.test.ts(2 hunks)test/unit/commands/channels/occupancy/subscribe.test.ts(7 hunks)test/unit/commands/channels/presence/enter.test.ts(2 hunks)test/unit/commands/channels/presence/subscribe.test.ts(2 hunks)test/unit/commands/channels/subscribe.test.ts(2 hunks)test/unit/commands/config/path.test.ts(3 hunks)test/unit/commands/config/show.test.ts(2 hunks)test/unit/commands/integrations/create.test.ts(5 hunks)test/unit/commands/integrations/delete.test.ts(6 hunks)test/unit/commands/integrations/get.test.ts(3 hunks)test/unit/commands/integrations/update.test.ts(1 hunks)test/unit/commands/logs/app/subscribe.test.ts(5 hunks)test/unit/commands/logs/channel-lifecycle.test.ts(9 hunks)test/unit/commands/logs/channel-lifecycle/subscribe.test.ts(5 hunks)test/unit/commands/logs/connection-lifecycle/history.test.ts(3 hunks)test/unit/commands/logs/push/history.test.ts(3 hunks)test/unit/commands/queues/create.test.ts(2 hunks)test/unit/commands/queues/delete.test.ts(4 hunks)test/unit/commands/queues/list.test.ts(2 hunks)test/unit/commands/rooms/messages/reactions/remove.test.ts(2 hunks)test/unit/commands/rooms/messages/reactions/send.test.ts(2 hunks)test/unit/commands/rooms/messages/reactions/subscribe.test.ts(3 hunks)test/unit/commands/rooms/presence/subscribe.test.ts(3 hunks)test/unit/commands/rooms/reactions/subscribe.test.ts(3 hunks)test/unit/commands/rooms/typing/subscribe.test.ts(3 hunks)test/unit/commands/spaces/cursors/get-all.test.ts(6 hunks)test/unit/commands/spaces/cursors/subscribe.test.ts(6 hunks)test/unit/commands/spaces/locations/get-all.test.ts(4 hunks)test/unit/commands/spaces/locations/subscribe.test.ts(7 hunks)test/unit/commands/spaces/locks/get-all.test.ts(4 hunks)test/unit/commands/spaces/locks/get.test.ts(4 hunks)test/unit/commands/spaces/locks/subscribe.test.ts(8 hunks)test/unit/commands/spaces/spaces.test.ts(2 hunks)test/unit/services/config-manager.test.ts(7 hunks)test/unit/setup.ts(1 hunks)vitest.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/AI-Assistance.mdc)
**/*.test.ts: When testing, mock the Ably SDK properly by stubbing the constructor (e.g., sinon.stub(Ably, 'Rest').returns(mockClient)).
Always ensure resources are properly closed/cleaned up in tests (e.g., call await client.close() and sinon.restore() in afterEach).
Files:
test/unit/commands/rooms/messages/reactions/send.test.tstest/unit/commands/channels/list.test.tstest/unit/commands/bench/benchmarking.test.tstest/unit/commands/auth/keys/update.test.tstest/unit/commands/apps/channel-rules/list.test.tstest/unit/commands/logs/channel-lifecycle.test.tstest/unit/commands/channels/subscribe.test.tstest/unit/commands/rooms/reactions/subscribe.test.tstest/unit/commands/rooms/messages/reactions/subscribe.test.tstest/unit/commands/auth/keys/get.test.tstest/unit/commands/channels/presence/enter.test.tstest/unit/commands/apps/current.test.tstest/e2e/auth/basic-auth.test.tstest/unit/commands/channels/presence/subscribe.test.tstest/unit/commands/integrations/get.test.tstest/unit/commands/queues/delete.test.tstest/unit/commands/auth/keys/revoke.test.tstest/unit/commands/auth/issue-ably-token.test.tstest/unit/commands/rooms/typing/subscribe.test.tstest/unit/commands/spaces/locations/subscribe.test.tstest/unit/commands/apps/channel-rules/update.test.tstest/unit/commands/channels/history.test.tstest/unit/commands/logs/push/history.test.tstest/unit/commands/rooms/presence/subscribe.test.tstest/unit/commands/auth/issue-jwt-token.test.tstest/unit/commands/channels/occupancy/subscribe.test.tstest/unit/base/base-command.test.tstest/unit/commands/spaces/cursors/get-all.test.tstest/unit/commands/logs/channel-lifecycle/subscribe.test.tstest/unit/commands/config/path.test.tstest/unit/commands/auth/revoke-token.test.tstest/unit/commands/apps/create.test.tstest/unit/commands/integrations/delete.test.tstest/unit/commands/apps/logs/history.test.tstest/unit/commands/auth/keys/current.test.tstest/unit/commands/apps/channel-rules/delete.test.tstest/unit/commands/channel-rule/delete.test.tstest/unit/commands/logs/connection-lifecycle/history.test.tstest/unit/commands/spaces/locks/subscribe.test.tstest/unit/commands/apps/update.test.tstest/unit/commands/config/show.test.tstest/unit/commands/auth/keys/create.test.tstest/unit/services/config-manager.test.tstest/unit/commands/accounts/login.test.tstest/unit/commands/auth/keys/list.test.tstest/unit/commands/apps/logs/subscribe.test.tstest/unit/commands/apps/set-apns-p12.test.tstest/unit/commands/channels/batch-publish.test.tstest/unit/commands/queues/create.test.tstest/unit/commands/integrations/update.test.tstest/unit/commands/integrations/create.test.tstest/unit/commands/spaces/cursors/subscribe.test.tstest/unit/commands/spaces/locks/get-all.test.tstest/unit/commands/spaces/spaces.test.tstest/unit/commands/apps/delete.test.tstest/unit/commands/spaces/locks/get.test.tstest/unit/commands/rooms/messages/reactions/remove.test.tstest/unit/commands/channel-rule/create.test.tstest/unit/commands/channel-rule/update.test.tstest/unit/commands/queues/list.test.tstest/unit/commands/spaces/locations/get-all.test.tstest/unit/commands/apps/channel-rules/create.test.tstest/unit/commands/accounts/logout.test.tstest/unit/commands/logs/app/subscribe.test.tstest/unit/commands/channel-rule/list.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
**/*.{ts,tsx}: Use TypeScript and follow best practice naming conventions
ESLint is used to ensure all code adheres best practices; ensure you check that all code passes the linter before concluding your work.
Files:
test/unit/commands/rooms/messages/reactions/send.test.tstest/setup.tstest/unit/commands/channels/list.test.tstest/unit/commands/bench/benchmarking.test.tssrc/commands/config/show.tstest/unit/commands/auth/keys/update.test.tstest/unit/commands/apps/channel-rules/list.test.tstest/unit/commands/logs/channel-lifecycle.test.tstest/unit/commands/channels/subscribe.test.tstest/unit/commands/rooms/reactions/subscribe.test.tstest/unit/commands/rooms/messages/reactions/subscribe.test.tstest/unit/commands/auth/keys/get.test.tstest/unit/commands/channels/presence/enter.test.tstest/unit/commands/apps/current.test.tstest/e2e/auth/basic-auth.test.tstest/unit/commands/channels/presence/subscribe.test.tstest/unit/commands/integrations/get.test.tstest/unit/commands/queues/delete.test.tstest/unit/commands/auth/keys/revoke.test.tstest/unit/commands/auth/issue-ably-token.test.tssrc/help.tstest/unit/commands/rooms/typing/subscribe.test.tstest/unit/commands/spaces/locations/subscribe.test.tstest/unit/commands/apps/channel-rules/update.test.tstest/unit/commands/channels/history.test.tstest/unit/commands/logs/push/history.test.tstest/unit/commands/rooms/presence/subscribe.test.tssrc/commands/mcp/start-server.tstest/unit/commands/auth/issue-jwt-token.test.tstest/unit/commands/channels/occupancy/subscribe.test.tstest/unit/base/base-command.test.tstest/unit/commands/spaces/cursors/get-all.test.tstest/unit/commands/logs/channel-lifecycle/subscribe.test.tstest/unit/commands/config/path.test.tstest/unit/commands/auth/revoke-token.test.tsvitest.config.tstest/unit/commands/apps/create.test.tstest/unit/commands/integrations/delete.test.tstest/unit/commands/apps/logs/history.test.tstest/unit/commands/auth/keys/current.test.tstest/unit/commands/apps/channel-rules/delete.test.tstest/unit/commands/channel-rule/delete.test.tstest/unit/commands/logs/connection-lifecycle/history.test.tstest/unit/commands/spaces/locks/subscribe.test.tstest/unit/commands/apps/update.test.tstest/unit/commands/config/show.test.tssrc/base-command.tstest/unit/commands/auth/keys/create.test.tstest/unit/services/config-manager.test.tstest/unit/commands/accounts/login.test.tstest/unit/commands/auth/keys/list.test.tstest/unit/commands/apps/logs/subscribe.test.tstest/unit/commands/apps/set-apns-p12.test.tstest/unit/commands/channels/batch-publish.test.tstest/unit/commands/queues/create.test.tstest/unit/commands/integrations/update.test.tstest/unit/commands/integrations/create.test.tstest/unit/commands/spaces/cursors/subscribe.test.tstest/unit/commands/spaces/locks/get-all.test.tstest/unit/commands/spaces/spaces.test.tstest/unit/commands/apps/delete.test.tstest/unit/commands/spaces/locks/get.test.tstest/unit/commands/rooms/messages/reactions/remove.test.tstest/unit/commands/channel-rule/create.test.tstest/unit/commands/channel-rule/update.test.tstest/unit/commands/queues/list.test.tstest/unit/commands/spaces/locations/get-all.test.tstest/unit/commands/apps/channel-rules/create.test.tstest/unit/setup.tssrc/services/config-manager.tstest/unit/commands/accounts/logout.test.tstest/unit/commands/logs/app/subscribe.test.tstest/helpers/mock-config-manager.tstest/unit/commands/channel-rule/list.test.ts
{test/**/*.{ts,js},**/*.{test,spec}.{ts,js}}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
When new features are added or changes made, tests must be updated or added, and it is your responsibility to ensure the tests pass before deeming your work complete.
Files:
test/unit/commands/rooms/messages/reactions/send.test.tstest/setup.tstest/unit/commands/channels/list.test.tstest/unit/commands/bench/benchmarking.test.tstest/unit/commands/auth/keys/update.test.tstest/unit/commands/apps/channel-rules/list.test.tstest/unit/commands/logs/channel-lifecycle.test.tstest/unit/commands/channels/subscribe.test.tstest/unit/commands/rooms/reactions/subscribe.test.tstest/unit/commands/rooms/messages/reactions/subscribe.test.tstest/unit/commands/auth/keys/get.test.tstest/unit/commands/channels/presence/enter.test.tstest/unit/commands/apps/current.test.tstest/e2e/auth/basic-auth.test.tstest/unit/commands/channels/presence/subscribe.test.tstest/unit/commands/integrations/get.test.tstest/unit/commands/queues/delete.test.tstest/unit/commands/auth/keys/revoke.test.tstest/unit/commands/auth/issue-ably-token.test.tstest/unit/commands/rooms/typing/subscribe.test.tstest/unit/commands/spaces/locations/subscribe.test.tstest/unit/commands/apps/channel-rules/update.test.tstest/unit/commands/channels/history.test.tstest/unit/commands/logs/push/history.test.tstest/unit/commands/rooms/presence/subscribe.test.tstest/unit/commands/auth/issue-jwt-token.test.tstest/unit/commands/channels/occupancy/subscribe.test.tstest/unit/base/base-command.test.tstest/unit/commands/spaces/cursors/get-all.test.tstest/unit/commands/logs/channel-lifecycle/subscribe.test.tstest/unit/commands/config/path.test.tstest/unit/commands/auth/revoke-token.test.tstest/unit/commands/apps/create.test.tstest/unit/commands/integrations/delete.test.tstest/unit/commands/apps/logs/history.test.tstest/unit/commands/auth/keys/current.test.tstest/unit/commands/apps/channel-rules/delete.test.tstest/unit/commands/channel-rule/delete.test.tstest/unit/commands/logs/connection-lifecycle/history.test.tstest/unit/commands/spaces/locks/subscribe.test.tstest/unit/commands/apps/update.test.tstest/unit/commands/config/show.test.tstest/unit/commands/auth/keys/create.test.tstest/unit/services/config-manager.test.tstest/unit/commands/accounts/login.test.tstest/unit/commands/auth/keys/list.test.tstest/unit/commands/apps/logs/subscribe.test.tstest/unit/commands/apps/set-apns-p12.test.tstest/unit/commands/channels/batch-publish.test.tstest/unit/commands/queues/create.test.tstest/unit/commands/integrations/update.test.tstest/unit/commands/integrations/create.test.tstest/unit/commands/spaces/cursors/subscribe.test.tstest/unit/commands/spaces/locks/get-all.test.tstest/unit/commands/spaces/spaces.test.tstest/unit/commands/apps/delete.test.tstest/unit/commands/spaces/locks/get.test.tstest/unit/commands/rooms/messages/reactions/remove.test.tstest/unit/commands/channel-rule/create.test.tstest/unit/commands/channel-rule/update.test.tstest/unit/commands/queues/list.test.tstest/unit/commands/spaces/locations/get-all.test.tstest/unit/commands/apps/channel-rules/create.test.tstest/unit/setup.tstest/unit/commands/accounts/logout.test.tstest/unit/commands/logs/app/subscribe.test.tstest/helpers/mock-config-manager.tstest/unit/commands/channel-rule/list.test.ts
src/commands/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/AI-Assistance.mdc)
src/commands/**/*.ts: Commands should follow the oclif structure: export default class MyCommand extends Command { ... }
Command should support auth via the standard auth helper (import { getAuth } from '../../helpers/auth') and use it in the run method.
Use try/catch for error handling in commands, and provide user-friendly error messages (e.g., handle error.code === 40100 for authentication failures).
Do not use direct HTTP requests (e.g., fetch) for data plane APIs; use the Ably SDK instead.
Do not use hardcoded endpoint URLs; use configuration values (e.g., flags.controlHost, config.controlHost) when constructing URLs.
Do not use non-standard command structures (e.g., export default class { async execute(args) { ... } }); always use the oclif Command class.
Files:
src/commands/config/show.tssrc/commands/mcp/start-server.ts
src/commands/**/*.{ts,js}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
Follow oclif framework best practices as described in the oclif documentation.
Files:
src/commands/config/show.tssrc/commands/mcp/start-server.ts
🧬 Code graph analysis (52)
test/setup.ts (1)
test/helpers/mock-config-manager.ts (1)
MockConfigManager(41-401)
test/unit/commands/auth/keys/update.test.ts (3)
test/root-hooks.ts (2)
beforeEach(8-11)afterEach(13-61)test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/apps/channel-rules/list.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/commands/logs/channel-lifecycle.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/rooms/reactions/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/apps/current.test.ts (2)
test/helpers/cli-runner.ts (1)
stdout(125-127)test/helpers/mock-config-manager.ts (2)
DEFAULT_TEST_CONFIG(24-35)getMockConfigManager(407-414)
test/e2e/auth/basic-auth.test.ts (1)
src/services/config-manager.ts (1)
TomlConfigManager(124-499)
test/unit/commands/queues/delete.test.ts (2)
test/helpers/mock-config-manager.ts (2)
DEFAULT_TEST_CONFIG(24-35)getMockConfigManager(407-414)test/helpers/command-helpers.ts (1)
runCommand(67-83)
test/unit/commands/auth/keys/revoke.test.ts (2)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/auth/issue-ably-token.test.ts (3)
test/root-hooks.ts (2)
beforeEach(8-11)afterEach(13-61)test/helpers/mock-config-manager.ts (2)
DEFAULT_TEST_CONFIG(24-35)getMockConfigManager(407-414)test/helpers/command-helpers.ts (1)
runCommand(67-83)
src/help.ts (1)
src/services/config-manager.ts (1)
createConfigManager(114-122)
test/unit/commands/rooms/typing/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/spaces/locations/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/apps/channel-rules/update.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/commands/rooms/presence/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
src/commands/mcp/start-server.ts (1)
src/services/config-manager.ts (1)
createConfigManager(114-122)
test/unit/commands/auth/issue-jwt-token.test.ts (2)
test/helpers/mock-config-manager.ts (2)
DEFAULT_TEST_CONFIG(24-35)getMockConfigManager(407-414)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/channels/occupancy/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/base/base-command.test.ts (1)
src/services/config-manager.ts (1)
TomlConfigManager(124-499)
test/unit/commands/logs/channel-lifecycle/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/config/path.test.ts (2)
test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/apps/create.test.ts (3)
test/root-hooks.ts (2)
beforeEach(8-11)afterEach(13-61)test/helpers/mock-config-manager.ts (1)
getMockConfigManager(407-414)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/integrations/delete.test.ts (3)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)test/helpers/cli-runner.ts (1)
stdout(125-127)test/helpers/command-helpers.ts (1)
runCommand(67-83)
test/unit/commands/auth/keys/current.test.ts (3)
test/helpers/command-helpers.ts (1)
runCommand(67-83)test/helpers/cli-runner.ts (1)
stdout(125-127)test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/commands/apps/channel-rules/delete.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/commands/channel-rule/delete.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/commands/spaces/locks/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/apps/update.test.ts (4)
test/root-hooks.ts (2)
beforeEach(8-11)afterEach(13-61)test/helpers/mock-config-manager.ts (1)
getMockConfigManager(407-414)test/helpers/cli-runner.ts (1)
stdout(125-127)test/helpers/command-helpers.ts (1)
runCommand(67-83)
test/unit/commands/config/show.test.ts (1)
test/root-hooks.ts (1)
beforeEach(8-11)
src/base-command.ts (1)
src/services/config-manager.ts (1)
createConfigManager(114-122)
test/unit/commands/auth/keys/create.test.ts (1)
test/helpers/mock-config-manager.ts (2)
getMockConfigManager(407-414)DEFAULT_TEST_CONFIG(24-35)
test/unit/services/config-manager.test.ts (1)
src/services/config-manager.ts (1)
TomlConfigManager(124-499)
test/unit/commands/accounts/login.test.ts (2)
test/root-hooks.ts (1)
beforeEach(8-11)test/helpers/mock-config-manager.ts (1)
getMockConfigManager(407-414)
test/unit/commands/auth/keys/list.test.ts (4)
test/root-hooks.ts (2)
beforeEach(8-11)afterEach(13-61)test/helpers/mock-config-manager.ts (2)
DEFAULT_TEST_CONFIG(24-35)getMockConfigManager(407-414)test/helpers/cli-runner.ts (1)
stdout(125-127)test/helpers/command-helpers.ts (1)
runCommand(67-83)
test/unit/commands/apps/logs/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/apps/set-apns-p12.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/commands/queues/create.test.ts (1)
test/helpers/mock-config-manager.ts (2)
DEFAULT_TEST_CONFIG(24-35)getMockConfigManager(407-414)
test/unit/commands/integrations/update.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/commands/integrations/create.test.ts (4)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)test/root-hooks.ts (1)
afterEach(13-61)test/helpers/cli-runner.ts (1)
stdout(125-127)test/helpers/command-helpers.ts (1)
runCommand(67-83)
test/unit/commands/spaces/cursors/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/spaces/locks/get-all.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/apps/delete.test.ts (3)
test/root-hooks.ts (2)
beforeEach(8-11)afterEach(13-61)test/helpers/mock-config-manager.ts (1)
getMockConfigManager(407-414)test/helpers/cli-runner.ts (1)
stdout(125-127)
test/unit/commands/channel-rule/create.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/commands/channel-rule/update.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/commands/queues/list.test.ts (2)
test/helpers/mock-config-manager.ts (2)
DEFAULT_TEST_CONFIG(24-35)getMockConfigManager(407-414)test/helpers/command-helpers.ts (1)
runCommand(67-83)
test/unit/commands/spaces/locations/get-all.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/unit/commands/apps/channel-rules/create.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
test/unit/setup.ts (2)
test/helpers/mock-config-manager.ts (2)
initializeMockConfigManager(429-436)resetMockConfig(420-423)test/root-hooks.ts (1)
beforeEach(8-11)
src/services/config-manager.ts (6)
test/unit/commands/rooms/messages.test.ts (3)
parse(19-21)parse(55-57)parse(96-98)test/unit/commands/bench/bench.test.ts (1)
parse(17-19)test/unit/commands/connections/test.test.ts (1)
parse(18-20)test/unit/commands/connections/stats.test.ts (1)
parse(19-21)test/unit/commands/mcp/mcp.test.ts (1)
parse(19-21)test/unit/commands/rooms/features.test.ts (6)
parse(22-24)parse(55-57)parse(89-91)parse(126-128)parse(163-165)parse(200-202)
test/unit/commands/logs/app/subscribe.test.ts (1)
test/root-hooks.ts (1)
afterEach(13-61)
test/helpers/mock-config-manager.ts (1)
src/services/config-manager.ts (4)
ConfigManager(42-99)AblyConfig(27-40)AppConfig(8-13)AccountConfig(15-25)
test/unit/commands/channel-rule/list.test.ts (1)
test/helpers/mock-config-manager.ts (1)
DEFAULT_TEST_CONFIG(24-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: rate-limit-test
AndyTWF
force-pushed
the
config-manager-updates
branch
from
e5d4831 to
3c006ff
Compare
jamiehenson
approved these changes
Dec 16, 2025
Member
jamiehenson
left a comment
jamiehenson
left a comment
There was a problem hiding this comment.
LG. Looks gnarly at first but actually boiled down to a handful of update patterns. Only have small non-blocking nits and questions.
| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1", | ||
| "toml": "^3.0.0", | ||
| "smol-toml": "^1.5.2", |
| // Mock get key details | ||
| nock("https://control.ably.net") | ||
| .get(`/v1/apps/${mockAppId}/keys/${mockKeyId}`) | ||
| .get(`/v1/apps/${DEFAULT_TEST_CONFIG.appId}/keys/${mockKeyId}`) |
Member
There was a problem hiding this comment.
World's smallest nit: could destructure appId off of DEFAULT_TEST_CONFIG first to save all the object access repetition. Same applies to other related files with this pattern
Contributor
Author
There was a problem hiding this comment.
Went a step further - changed tests to use the methods on the config manager, also made the values randomised each test to avoid leakage / accidental passing.
AndyTWF
force-pushed
the
config-manager-updates
branch
from
3c006ff to
b6e05b1
Compare
Replace the unmaintained "toml" package (last published 4 years ago, parse-only) with "smol-toml" which is actively maintained and provides both parse and stringify functions. This change removes ~85 lines of custom TOML formatting code in the formatToToml() method, replacing it with smol-toml's built-in stringify function. The result is simpler, more maintainable code that relies on a well-tested library implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Convert ConfigManager from a concrete class to an interface, with TomlConfigManager as the implementation. This enables easier mocking in tests since consumers can now depend on the interface rather than the concrete implementation. - Add ConfigManager interface with all public method signatures - Rename ConfigManager class to TomlConfigManager implements ConfigManager - Update all instantiation sites to use new TomlConfigManager() - Update test mocks to spy on TomlConfigManager.prototype 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Introduce an in-memory MockConfigManager that implements the ConfigManager interface, allowing unit tests to run without filesystem operations and with fine-grained control over configuration state. - Add MockConfigManager class with DEFAULT_TEST_CONFIG values - Add test helper functions (getMockConfigManager, resetMockConfig, etc.) - Add createConfigManager() factory that returns mock in test mode - Add unit test setup file that initializes mock for all unit tests - Update apps/current.test.ts to use the mock as a reference example This enables faster, more isolated unit tests by removing the dependency on temp directories and file I/O for configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove temp directory config setup from 16 test files - Use DEFAULT_TEST_CONFIG for module-level constants - Use getMockConfigManager().clearAccounts() for "no config" scenarios - Add opt-out mechanism for config/show.test.ts (needs real file I/O) - Fix integrations tests to use correct flag names (--rule-type) - Remove redundant mockConfig.reset() calls (handled by setup file) - Clean up empty afterEach blocks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Now that we don't have tests sharing temporary config files, they don't race anymore.
MockConfigManager is only used for tests, so it belongs in the test folder rather than src. This keeps production code separate from test infrastructure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
AndyTWF
force-pushed
the
config-manager-updates
branch
from
b6e05b1 to
693ef35
Compare
- Made getTestValues() private in MockConfigManager - Added getRegisteredAppId() method for tests that need an appId even when currentAppId is undefined - Updated all test files to use ConfigManager interface methods: - getCurrentAppId() for current app ID - getRegisteredAppId() for tests that modify config state - getApiKey() for API key - getKeyId() for key ID - getKeyName() for key name - getCurrentAccount()!.accountId for account ID - getCurrentAccount()!.accountName for account name - getAppName(appId) for app name This ensures test isolation through randomized values while using the proper ConfigManager interface rather than directly accessing test fixture values. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
denissellu
approved these changes
Dec 16, 2025
Member
denissellu
left a comment
denissellu
left a comment
There was a problem hiding this comment.
⚡ this one wasn't a slug to get through, dare i say i almost smiled :D
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves unit test isolation and maintainability by introducing a
MockConfigManagerthat replaces filesystem-based config setup in tests.FTF-132
Problem
Unit tests were creating temporary directories and writing TOML config files for each test. This caused:
Solution
1. Replace
tomlpackage withsmol-tomltomlpackage was unmaintained (4 years old, parse-only)smol-tomlprovides both parse and stringify, removing ~85 lines of custom TOML formatting code2. Extract ConfigManager interface
ConfigManagerfrom concrete class to interfaceTomlConfigManager3. Add MockConfigManager
ConfigManagerinterfaceDEFAULT_TEST_CONFIGwith standard test valuesgetMockConfigManager(),resetMockConfig(),clearAccounts()4. Migrate all unit tests
config/show.test.ts) can opt-out5. Re-enable parallel test execution
Follow-up
As follow up, I plan to make all the mocks (e.g. realtime, spaces, chat) global in a similar way to the config, so we don't have lots of repetitive setup there.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.