improve test quality and co-locate test files#161
Conversation
|
Warning Rate limit exceeded@saddlepaddle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
WalkthroughFlattened import paths across CLI tests, added adapter.clear() test setup, introduced a default "default" environment in storage/orchestrator tests, expanded WorkspaceOrchestrator.create to accept an optional branch option, refocused desktop workspace tests on canDelete, updated test setup mocks, and removed some desktop component tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
saddlepaddle
force-pushed
the
magenta-peak-43
branch
6 times, most recently
from
5bc5d5f to
ae0415e
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
saddlepaddle
force-pushed
the
magenta-peak-43
branch
from
ae0415e to
103f4dc
Compare
saddlepaddle
force-pushed
the
magenta-peak-43
branch
from
103f4dc to
cc1c436
Compare
- Co-locate CLI tests next to source files (move from __tests__/ dirs) - Delete low-value tests that only verified mock behavior: - GroupTabView.test.tsx (redundant coverage) - FilePathLinkProvider.test.ts (90% mock verification) - Simplify workspaces.test.ts: keep valuable git parsing tests, remove create/delete tests that just verified mock calls - Clean up test-setup.ts with better documentation - Fix test isolation issues (adapter.clear() in beforeEach) - Standardize test scripts: use `bun test` everywhere - CLI: remove prettier/xo/ava, use bun test - Desktop: add test script to package.json - Delete stale test.js from CLI (referenced non-existent files) Test count: 238 → 256 (CLI 59 + Desktop 196 + Scripts 1) All tests run via `turbo test` from root. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
saddlepaddle
force-pushed
the
magenta-peak-43
branch
from
cc1c436 to
e235dd3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/cli/src/lib/orchestrators/change-orchestrator.test.ts (1)
18-25: Addadapter.clear()for test isolation consistency.According to the PR summary,
adapter.clear()was added inbeforeEachto ensure test isolation. However, this file is missing that addition whileenvironment-orchestrator.test.tsandworkspace-orchestrator.test.tsboth include it. This inconsistency could lead to test pollution if tests are run in different orders.Apply this diff to add the cleanup:
beforeEach(async () => { tempDir = await mkdtemp(join(tmpdir(), "change-test-")); const dbPath = join(tempDir, "test-db.json"); adapter = new LowdbAdapter(dbPath); + // Ensure clean state for each test + await adapter.clear(); orchestrator = new ChangeOrchestrator(adapter); workspaceOrchestrator = new WorkspaceOrchestrator(adapter); environmentOrchestrator = new EnvironmentOrchestrator(adapter); });apps/cli/src/lib/orchestrators/process-orchestrator.test.ts (1)
20-27: Addadapter.clear()for test isolation consistency.This file is missing the
adapter.clear()call inbeforeEachthat was added toenvironment-orchestrator.test.tsandworkspace-orchestrator.test.tsfor test isolation. Per the PR summary, this was added "to ensure isolation." The absence of this cleanup could lead to test pollution.Apply this diff to add the cleanup:
beforeEach(async () => { tempDir = await mkdtemp(join(tmpdir(), "process-test-")); const dbPath = join(tempDir, "test-db.json"); adapter = new LowdbAdapter(dbPath); + // Ensure clean state for each test + await adapter.clear(); orchestrator = new ProcessOrchestrator(adapter); workspaceOrchestrator = new WorkspaceOrchestrator(adapter); environmentOrchestrator = new EnvironmentOrchestrator(adapter); });
🧹 Nitpick comments (3)
apps/cli/src/lib/storage/lowdb-adapter.test.ts (1)
44-49: Remove redundantadapter.clear()call.Line 45 calls
adapter.clear(), butbeforeEachalready clears the adapter on line 21. This redundant call can be removed since each test already starts with a clean state.Apply this diff to remove the redundant call:
test("creates and persists data", async () => { - await adapter.clear(); const data = await adapter.read(); expect(data).toBeDefined(); expect(data.environments).toBeDefined(); });apps/desktop/src/renderer/stores/tabs/store.test.ts (1)
1-1: Good test isolation pattern with a minor robustness consideration.The afterEach cleanup correctly ensures test independence and repeatability. Using
setState(initialState, true)properly replaces the entire store state between tests.One optional enhancement: consider using
structuredClone(useTabsStore.getState())when capturinginitialStateto avoid potential issues if future tests directly mutate nested state objects (though current tests safely usesetState).Optional improvement:
// Store initial state for cleanup -const initialState = useTabsStore.getState(); +const initialState = structuredClone(useTabsStore.getState());Based on coding guidelines emphasizing tests should be independent and repeatable.
Also applies to: 11-17
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
71-90: Align with “one assert per test” guideline and consider a couple more branchesThe
mockSimpleGitWithWorktreeList/mockSimpleGitWithErrorhelpers plus the targetedcanDeletetests give good coverage of git edge cases (exact path, prefixes, whitespace, error path,--porcelainflag) with minimal duplication. One stylistic tweak to better match the desktop testing guidelines would be to collapse the multipleexpectcalls in each test into a single assertion that checks the result shape.For example, the first test could be written as:
it("returns true when worktree exists in git", async () => { mockSimpleGitWithWorktreeList( "worktree /path/to/worktree\nHEAD abc123\nbranch refs/heads/test-branch", ); const router = createWorkspacesRouter(); const caller = router.createCaller({}); const result = await caller.canDelete({ id: "workspace-1" }); - expect(result.canDelete).toBe(true); - expect(result.reason).toBeNull(); - expect(result.warning).toBeNull(); + expect(result).toMatchObject({ + canDelete: true, + reason: null, + warning: null, + }); });Similar patterns (e.g., asserting an object with
canDeletepluswarning/reason) would let the other tests keep the same intent with a singleexpecteach.Optionally, you might also add small tests for:
canDeletewhen the workspace id is unknown (canDelete: false,reason: "Workspace not found").canDeletewhen there is no associated worktree (canDelete: truewith"No associated worktree found"warning).That would round out coverage of all the branches in
canDeletewhile keeping this suite focused. As per coding guidelines, tests underapps/desktopshould have one assert per test, and these changes would bring this file fully in line with that rule.Also applies to: 105-209
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ci.yml(2 hunks)apps/cli/package.json(2 hunks)apps/cli/src/lib/orchestrators/change-orchestrator.test.ts(1 hunks)apps/cli/src/lib/orchestrators/environment-orchestrator.test.ts(3 hunks)apps/cli/src/lib/orchestrators/process-orchestrator.test.ts(1 hunks)apps/cli/src/lib/orchestrators/workspace-orchestrator.test.ts(4 hunks)apps/cli/src/lib/storage/lowdb-adapter.test.ts(6 hunks)apps/cli/test.js(0 hunks)apps/desktop/package.json(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts(4 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabView.test.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/store.test.ts(2 hunks)apps/desktop/test-setup.ts(3 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (3)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/FilePathLinkProvider.test.ts
- apps/cli/test.js
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabView.test.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply Biome formatting, linting, and import organization at the root level for all code files
Files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/cli/package.jsonapps/cli/src/lib/orchestrators/environment-orchestrator.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/cli/src/lib/storage/lowdb-adapter.test.tsapps/cli/src/lib/orchestrators/change-orchestrator.test.tsapps/desktop/package.jsonpackage.jsonapps/cli/src/lib/orchestrators/workspace-orchestrator.test.tsapps/cli/src/lib/orchestrators/process-orchestrator.test.tsapps/desktop/test-setup.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype; use type-safe approaches instead, unless necessary
Ensure TypeScript type checking passes across all packages using bun run typecheck
Files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/cli/src/lib/orchestrators/environment-orchestrator.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/cli/src/lib/storage/lowdb-adapter.test.tsapps/cli/src/lib/orchestrators/change-orchestrator.test.tsapps/cli/src/lib/orchestrators/workspace-orchestrator.test.tsapps/cli/src/lib/orchestrators/process-orchestrator.test.tsapps/desktop/test-setup.ts
apps/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
apps/**/*.{tsx,ts}: Structure React applications with one folder per component following the pattern: ComponentName/ComponentName.tsx with index.ts barrel export
Co-locate component dependencies (hooks, utils, constants, tests, stories) next to the file using them
Files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/cli/src/lib/orchestrators/environment-orchestrator.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/cli/src/lib/storage/lowdb-adapter.test.tsapps/cli/src/lib/orchestrators/change-orchestrator.test.tsapps/cli/src/lib/orchestrators/workspace-orchestrator.test.tsapps/cli/src/lib/orchestrators/process-orchestrator.test.tsapps/desktop/test-setup.ts
apps/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with their corresponding components (e.g., Component.test.tsx next to Component.tsx)
Files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/cli/src/lib/orchestrators/environment-orchestrator.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/cli/src/lib/storage/lowdb-adapter.test.tsapps/cli/src/lib/orchestrators/change-orchestrator.test.tsapps/cli/src/lib/orchestrators/workspace-orchestrator.test.tsapps/cli/src/lib/orchestrators/process-orchestrator.test.ts
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/desktop/test-setup.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/desktop/test-setup.ts
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts
🧠 Learnings (17)
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be independent
Applied to files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/cli/src/lib/orchestrators/environment-orchestrator.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/cli/src/lib/orchestrators/change-orchestrator.test.tsapps/desktop/package.jsonapps/cli/src/lib/orchestrators/workspace-orchestrator.test.tsapps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/cli/src/lib/orchestrators/environment-orchestrator.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/cli/src/lib/orchestrators/change-orchestrator.test.tsapps/desktop/package.jsonapps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/**/*.test.{ts,tsx} : Co-locate tests with their corresponding components (e.g., Component.test.tsx next to Component.tsx)
Applied to files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be readable
Applied to files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/desktop/package.jsonapps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be fast
Applied to files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Use Bun as the package manager exclusively; do not use npm, yarn, or pnpm
Applied to files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/cli/package.jsonapps/desktop/package.jsonapps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should have one assert per test
Applied to files:
apps/desktop/src/renderer/stores/tabs/store.test.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to **/*.{ts,tsx} : Ensure TypeScript type checking passes across all packages using bun run typecheck
Applied to files:
apps/desktop/src/renderer/stores/tabs/store.test.tsapps/cli/package.jsonapps/desktop/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to **/*.{ts,tsx,js,jsx,json} : Apply Biome formatting, linting, and import organization at the root level for all code files
Applied to files:
apps/cli/package.jsonapps/desktop/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/**/*.{tsx,ts} : Co-locate component dependencies (hooks, utils, constants, tests, stories) next to the file using them
Applied to files:
apps/cli/package.json
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env file in apps/desktop/src/main/index.ts before any imports with override: true
Applied to files:
apps/cli/src/lib/orchestrators/environment-orchestrator.test.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/main/lib/*-ipcs.ts : Implement Electron IPC handlers in dedicated files under apps/desktop/src/main/lib/ (e.g., workspace-ipcs.ts, terminal-ipcs.ts)
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/cli/src/lib/orchestrators/process-orchestrator.test.tsapps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Use Turborepo as the build system for managing monorepo tasks and builds
Applied to files:
package.json
📚 Learning: 2025-11-24T21:33:13.244Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.244Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to @(apps/desktop/src/renderer|apps/desktop/src/shared)/**/*.{ts,tsx} : Never import Node.js modules in Electron renderer process or shared code; use IPC for communication between main and renderer processes
Applied to files:
apps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules in apps/desktop/src/lib/electron-router-dom.ts as it is shared between main and renderer processes
Applied to files:
apps/desktop/test-setup.ts
📚 Learning: 2025-11-24T21:32:46.559Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T21:32:46.559Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define all Electron IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/test-setup.ts
🧬 Code graph analysis (3)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
createWorkspacesRouter(17-433)
apps/cli/src/lib/storage/lowdb-adapter.test.ts (1)
apps/cli/src/types/environment.ts (1)
Environment(1-3)
apps/cli/src/lib/orchestrators/workspace-orchestrator.test.ts (2)
apps/cli/src/types/workspace.ts (1)
LocalWorkspace(18-21)apps/cli/src/lib/orchestrators/__tests__/workspace-orchestrator.test.ts (1)
test(111-123)
🔇 Additional comments (26)
package.json (1)
25-25: Build filter aligns with PR objectives.Filtering out
@superset/websitefrom the Turbo build is a legitimate optimization that complements the test quality improvements outlined in the PR. The syntax is correct, and the change reduces build scope appropriately..github/workflows/ci.yml (2)
10-10: Cosmetic job name change is appropriate.Renaming from "Lint & Format" to "Lint" is a harmless simplification that does not affect the workflow execution.
82-82: Cosmetic job name change is appropriate.Renaming from "Unit Test" to "Test" is a harmless simplification that does not affect the workflow execution. The underlying
bun run testcommand remains unchanged.apps/desktop/package.json (1)
29-29: Test script addition is consistent with monorepo tooling consolidation.Adding
"test": "bun test"aligns with the Bun-based testing approach across the monorepo and the PR's objective to improve test quality. The change mirrors the CLI package.json update and supports the unified testing strategy.apps/cli/package.json (2)
17-17: Test script consolidation to Bun is aligned with monorepo strategy.Switching from the previous multi-tool setup (ava, prettier, xo) to
"test": "bun test"is consistent with the monorepo's shift to Bun-based testing and centralized Biome linting (at the root level). The change supports the PR's objective of improving test quality through a streamlined tooling approach.
42-50: DevDependency consolidation removes redundant testing and linting tools.Removing ava, prettier, xo, and related ESLint plugins is justified because:
- Bun now handles testing (line 17).
- Biome (provided at the root level) handles linting and formatting (lines 18–21).
- TypeScript is appropriately retained for type checking (line 28).
This consolidation aligns with the coding guidelines requiring Biome for formatting/linting and the learnings mandating Bun as the exclusive package manager. Based on learnings, root-level Biome configuration should provide all linting/formatting needs for CLI.
apps/cli/src/lib/orchestrators/change-orchestrator.test.ts (1)
5-9: LGTM: Import paths refactored correctly.The import paths have been successfully updated to use shorter, sibling-relative paths, aligning with the PR objective of co-locating tests with source files.
apps/cli/src/lib/orchestrators/environment-orchestrator.test.ts (3)
5-10: LGTM: Import paths refactored correctly.The import paths have been successfully updated to use shorter, sibling-relative paths, consistent with the PR's co-location objective.
23-24: LGTM: Test isolation improved withadapter.clear().The addition of
adapter.clear()inbeforeEachensures each test starts with a clean state, preventing test pollution. The explanatory comment is helpful.
66-84: LGTM: Test expectations updated for default environment.The test expectations have been appropriately updated to account for the default environment that
createEmptyDatabase()now includes. The test names have been clarified, and the assertions correctly expect the "default" environment ID.apps/cli/src/lib/orchestrators/process-orchestrator.test.ts (1)
5-11: LGTM: Import paths refactored correctly.The import paths have been successfully updated to use shorter, sibling-relative paths, aligning with the PR objective of co-locating tests with source files.
apps/cli/src/lib/orchestrators/workspace-orchestrator.test.ts (4)
5-10: LGTM: Import paths refactored correctly.The import paths have been successfully updated to use shorter, sibling-relative paths, consistent with the PR's co-location objective.
23-24: LGTM: Test isolation improved withadapter.clear().The addition of
adapter.clear()inbeforeEachensures each test starts with a clean state, preventing test pollution.
47-56: LGTM: Test updated for new branch option.The test correctly exercises the new optional
branchparameter in thecreatemethod for cloud workspaces, expanding test coverage for the API enhancement.
67-72: LGTM: Assertion strategy improved for reliability.The change from
toEqualto field-by-field comparison is appropriate. The comment clearly explains thatlastUsedAtis set by theuse()call increate(), which could cause timing-based test flakiness with direct equality comparison.apps/cli/src/lib/storage/lowdb-adapter.test.ts (5)
6-8: LGTM! Import paths updated for co-location.The relative import paths are appropriate now that the test file is co-located with the source file, consistent with the PR's objective of moving tests out of
__tests__/directories.
20-21: LGTM! Clear call ensures test isolation.Adding
adapter.clear()inbeforeEachensures each test starts with a clean, consistent state (with the default environment), preventing interference between tests.
82-92: LGTM! Clear behavior correctly tested.The test correctly verifies that
clear()resets to the default database state (which includes the default environment). The inline comments helpfully document this behavior.
104-108: LGTM! Collection test correctly accounts for default environment.The test expectations are properly updated to reflect that the collection now includes the default environment in addition to the explicitly added ones.
194-195: LGTM! Concurrent operations test correctly updated.The test properly accounts for the default environment when verifying the result of concurrent writes.
apps/desktop/test-setup.ts (4)
3-10: Excellent documentation clarifying test setup boundaries.The added comments clearly communicate that this file is strictly for external dependency mocking, which helps maintain good test practices and prevents the anti-pattern of mocking internal code globally.
Based on PR objectives to clarify test setup.
20-44: Well-structured browser globals mock for renderer tests.The document mock provides essential DOM APIs (style properties and classList) using appropriate data structures (Map for styles, Set for classes). This minimal implementation is sufficient for testing renderer code without a full browser environment.
64-68: Appropriate mock for trpc-electron renderer.The
_callbackunderscore prefix correctly indicates the parameter is intentionally unused in this global mock. Individual tests can override this mock if they need to simulate message handling.
70-72: Good organizational improvements.The section comment enhances readability, and the webContents formatting is more concise while maintaining functionality.
Also applies to: 90-90
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (2)
3-12: Docstring clearly explains narrowed test scopeThe commentary around focusing this suite on
canDeleteand dropping low‑value create/delete tests is clear and matches the PR intent of testing real behavior instead of mock interactions. This should help future maintainers understand why the coverage is intentionally narrow here.
14-70: DB mock andbeforeEachsetup keep tests isolatedUsing a minimal
mockDbwired throughmock.module("main/lib/db"), plus resettingmockDb.data.worktreesinbeforeEach, gives each test a clean, predictable workspace/project/worktree state while avoiding over‑mocking. This supports independence and repeatability without adding complexity. If you later start mutating other collections (e.g.,workspaces,projects) in tests, consider snapshotting/cloning the initialdataobject inbeforeEach, but there’s no need to change anything now. Based on learnings, this pattern keeps tests independent and repeatable.Also applies to: 92-103
1 participant
Summary
__tests__/directories per repo standards)Changes
Deleted Tests (gave false confidence)
GroupTabView.test.tsx- Redundant coverage of utilities already tested indrag-logic.test.tsFilePathLinkProvider.test.ts- 90% mock verification, didn't test real behaviorSimplified Tests
workspaces.test.ts(427 → 211 lines): Removed create/delete tests that just verified mock calls, kept valuable git output parsing teststest-setup.ts: Added documentation clarifying it only mocks external dependenciesCo-located Tests
Moved CLI tests from
__tests__/directories to sit next to their source files:lowdb-adapter.test.tsenvironment-orchestrator.test.tsworkspace-orchestrator.test.tsprocess-orchestrator.test.tschange-orchestrator.test.tsFixed
adapter.clear()in beforeEach)simple-gitmocks in delete testscreateEmptyDatabase()includes default environmentTest Results
Before: 238 tests (many testing mock behavior)
After: 196 tests (all testing real behavior)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.