Skip to content

Commit 4a13fb7

Browse files
authored
Merge pull request #73 from cahaseler/commands-test-refactor
Refactor complete-task for DI and add coverage for command edge cases
2 parents 608a96a + 89b91be commit 4a13fb7

32 files changed

+4685
-2024
lines changed

.claude/backlog.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,5 @@
1818
- extract prompts into dedicated config file sections (or their own files? to allow for users to more easily customize them) (may not be practical depending on how dynamically we're building them)
1919
- [2025-09-15] block attempts to add stupid comments with the preToolUse hook
2020
- [2025-09-15] Enable interactive multi-turn code reviews with persistent sessions - allow Claude to reply to reviewer feedback, push back on criticisms, and have a dialogue about the changes
21-
- [2025-09-15] proper unit testing for commands
21+
- [2025-09-16] Add targeted unit tests for `src/lib/claude-sdk.ts` (retry/timeout/fallback paths) once DI seams are ready
22+
- [2025-09-16] Test quality improvements: Standardize mock patterns across 25+ test files, split complete-task.test.ts (365+ lines) into focused files, add integration tests for critical paths

.claude/tasks/TASK_064.md

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
# TASK_064: Refactor Commands for Testability and Comprehensive Unit Tests
2+
3+
## Purpose
4+
Refactor every CLI command to use explicit dependency injection so their business logic is isolated from Commander wiring, enabling thorough unit tests that cover success and failure scenarios without touching the real filesystem, Git, or external CLIs.
5+
6+
## Status
7+
**in_progress**
8+
9+
## Requirements
10+
- [x] Introduce a shared command dependency context module that exposes default adapters and injectable interfaces for console, process, filesystem, git, GitHub, child process, logging, and time utilities
11+
- [x] Refactor simple commands (`init`, `setup-commands`, `setup-templates`, `backlog`, `parse-logs`) to use the new context and return structured results instead of calling globals
12+
- [x] Refactor mid-complexity commands (`statusline`, `prepare-completion`, `hook`, `git-session`) to follow the same pattern with extracted pure action functions
13+
- [x] Refactor complex commands (`create-task-from-issue`) into smaller injectable units that cover filesystem updates, git workflow, and GitHub integration without direct `process.exit`
14+
- [x] Refactor `complete-task` into injectable components that emulate the legacy behaviour (validation → docs → git → GitHub) while returning structured results
15+
- [x] Update CLI registration to instantiate commands via new factory functions and ensure Commander wiring remains intact
16+
- [x] Add comprehensive unit test suites for each converted command action covering happy paths, validation errors, and dependency failures using stubbed dependencies
17+
- [x] Add light smoke tests for the Commander wrappers to confirm option/argument wiring and result-to-exit-code translation
18+
- [x] Run repository lint, typecheck, and test commands to verify the refactor passes existing quality gates (done incrementally for converted commands; final run pending once `complete-task` is migrated)
19+
20+
## Success Criteria
21+
- [x] All converted command unit tests execute without invoking real child processes, Git commands, or filesystem writes
22+
- [x] Converted commands no longer call `process.exit` or `console` directly from business logic; wrappers handle user IO and exit codes
23+
- [x] `complete-task` matches the DI pattern and its success/error paths are fully covered by tests
24+
- [x] Final `bun test` / lint / typecheck pass after all commands are converted
25+
- [x] CLI behaviour remains consistent for end users (no changes to command names, flags, or outputs beyond improved error handling)
26+
27+
## Technical Approach (Revised)
28+
1. Context module (`src/commands/context.ts`) created with `CommandDeps`, default adapters, `CommandResult`, and helper utilities.
29+
2. Each command now exposes pure helpers (e.g., `runBacklog`, `showRevert`) returning `CommandResult`s; Commander wrappers (`createXCommand`) resolve default deps and handle IO/exit codes.
30+
3. Shared test utilities (mock deps) are included in new command test suites to cover success/failure branches.
31+
4. Remaining scope is to break down `complete-task` into injectable phases (validation, doc updates, git ops, GitHub/PR flow), return structured results, and add unit coverage. **(Completed)**
32+
5. After `complete-task`, revisit CLI registration/`src/cli/index.ts` to ensure all commands use factory wrappers. **(Verified, no changes required)**
33+
6. Run a comprehensive `bun test` plus lint/typecheck once all commands are converted. **(Completed)**
34+
35+
## Recent Progress
36+
- Added `src/commands/context.ts` providing common DI infrastructure and helper utilities (with unit tests in `context.test.ts`).
37+
- Refactored simple commands (`init`, `setup-commands`, `setup-templates`, `backlog`, `parse-logs`) to use the DI pattern; added dedicated tests for each.
38+
- Converted mid-complexity commands (`statusline`, `prepare-completion`, `hook`, `git-session`) to DI style and wrote unit coverage for their core helpers.
39+
- Refactored `create-task-from-issue` into small injectable steps and added focused tests.
40+
- Verified `bun test` passes after each conversion (latest run includes all converted commands).
41+
- Completed `complete-task` refactor to use the DI context, decomposed workflow helpers, and added focused unit tests plus wrapper smoke coverage.
42+
- Updated supporting command modules/tests for new context typing adjustments; ensured lint/typecheck compatibility and reorganized imports across the suite.
43+
- Validated repository with `bun run typecheck`, `bun run lint`, and `bun test` after the full refactor.
44+
45+
### Code Review and Production Quality (2025-09-16)
46+
- Conducted comprehensive code review of PR #73 with detailed analysis of the DI refactor implementation
47+
- Identified production quality improvements needed: shared test utilities, dependency mapping simplification, standardized mock patterns, and test file organization
48+
- Approved PR #73 as high-quality refactoring that significantly improves testability and maintainability
49+
- Task now focused on implementing production quality improvements before final merge
50+
51+
### PR Review Follow-up (2025-01-16)
52+
- Successfully resolved merge conflicts with main branch that introduced CodeRabbit as alternative code review tool
53+
- Fixed P1 regression identified by Codex: Added branch validation before `pushCurrentBranch()` to prevent pushing wrong branch
54+
- Added test case `'skips push when not on task branch'` to prevent regression recurrence
55+
- All 334 tests passing after fixes
56+
57+
## Current Focus
58+
High-priority production quality improvements completed. DI refactor is now production-ready with shared test utilities and simplified dependency mapping.
59+
60+
## Production Quality Improvements (Completed High Priority)
61+
Based on code review, the following improvements are being made for production readiness:
62+
63+
### High Priority
64+
- [x] **Extract shared test utilities** - Created `src/test-utils/command-mocks.ts` with reusable mock factories
65+
- Extracted createMockLogger, createMockConsole, createMockProcess, createMockFileSystem
66+
- Added helper functions for common test assertions
67+
- Updated complete-task and prepare-completion tests to use shared utilities
68+
- [x] **Simplify dependency mapping** - Removed unnecessary `mapPrepareCompletionDeps` function from prepare-completion command
69+
70+
### Deferred to Future Task
71+
The following items have been identified but deferred to a future task as they represent perfectionism rather than production readiness:
72+
- **Standardize mock patterns** - Inconsistent mocking approaches across 25+ test files
73+
- **Split long test files** - `complete-task.test.ts` is 365+ lines and could be split by functionality
74+
- **Add integration tests** - While unit coverage is comprehensive, critical paths need end-to-end validation
75+
- **Expand DI documentation** - Additional guides for future contributors beyond what's in src/CLAUDE.md
76+
77+
## Next Steps
78+
1. ~~Complete high-priority production improvements~~
79+
2. ~~Review and merge PR #73~~ Ready for final review
80+
3. Future task: Test perfectionism improvements (see deferred items)
81+
82+
**Status:** Ready for completion - all critical production improvements done
83+
84+
**Last Updated:** 2025-09-16 21:00 UTC

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# Project: cc-track
22

33
## Active Task
4-
@.claude/no_active_task.md
4+
@.claude/tasks/TASK_064.md
55
<!-- IMPORTANT: Never edit this file to mark a task complete. Use /complete-task command instead. -->
66

77
## Product Vision

biome.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@
7272
},
7373
"overrides": [
7474
{
75-
"includes": ["**/*.test.ts", "**/*.test.js"],
75+
"includes": ["**/*.test.ts", "**/*.test.js", "**/test-utils/**/*.ts"],
7676
"linter": {
7777
"rules": {
7878
"suspicious": {

src/CLAUDE.md

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,27 @@ This codebase uses explicit dependency injection to keep side effects isolated a
3030
- Prefer DI over `mock.module` for Node built‑ins. If `mock.module` is unavoidable, restore it in `afterEach` and use fresh dynamic imports inside the test scope.
3131
- Avoid mutating process‑wide state (e.g., `process.env`) without restoring it within the test.
3232

33+
## Shared Test Utilities
34+
35+
The codebase provides reusable test utilities in `src/test-utils/command-mocks.ts`:
36+
37+
```typescript
38+
import { createMockLogger, createMockConsole, createMockProcess, createMockFileSystem,
39+
createTestDeps, assertFileWritten, assertConsoleContains } from '../test-utils/command-mocks';
40+
41+
// Create all test dependencies at once
42+
const deps = createTestDeps({ cwd: '/project', fixedDate: '2025-01-01' });
43+
44+
// Or create individual mocks
45+
const logger = createMockLogger();
46+
const console = createMockConsole();
47+
const fs = createMockFileSystem({ '/file.txt': 'content' });
48+
49+
// Use assertion helpers
50+
assertFileWritten(deps, '/output.txt', 'expected content');
51+
assertConsoleContains(deps, 'Success message');
52+
```
53+
3354
## Example: Hook With DI
3455

3556
// Implementation (production defaults with overrides)
@@ -45,13 +66,15 @@ export async function exampleHook(input: HookInput, deps: {
4566
return { continue: true };
4667
}
4768

48-
// Test (parallel‑safe)
69+
// Test (parallel‑safe) - using shared utilities
70+
import { createMockLogger } from '../test-utils/command-mocks';
71+
4972
beforeEach(() => { mock.restore(); /* clearConfigCache() if config used */ });
5073

5174
test('does work without touching globals', async () => {
5275
const exec = mock(() => 'ok');
5376
const fileOps = { existsSync: mock(() => true) };
54-
const logger = { debug: mock(() => {}), info: mock(() => {}), warn: mock(() => {}), error: mock(() => {}), exception: mock(() => {}) } as ReturnType<typeof createLogger>;
77+
const logger = createMockLogger(); // Use shared utility
5578

5679
const out = await exampleHook({ hook_event_name: 'PostToolUse' }, { execSync: exec, fileOps, logger });
5780
expect(out.continue).toBe(true);
@@ -86,4 +109,5 @@ test('runs with mocked exec', () => {
86109
- Process execution DI: `GitHelpers(exec?)`, `GitHubHelpers(exec?)`
87110
- Hook deps objects: `capturePlanHook`, `stopReviewHook`, `postCompactHook`, `editValidationHook`
88111
- Config cache reset: `clearConfigCache()` in `src/lib/config.ts`
112+
- Shared test utilities: `src/test-utils/command-mocks.ts` - createMockLogger, createMockConsole, createMockFileSystem, createTestDeps, assertFileWritten, assertConsoleContains
89113

src/commands/backlog.test.ts

Lines changed: 149 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,160 @@
1-
import { describe, expect, test } from 'bun:test';
2-
import { backlogCommand } from './backlog';
1+
import { describe, expect, mock, test } from 'bun:test';
2+
import path from 'node:path';
3+
import { type BacklogDeps, backlogCommand, createBacklogCommand, runBacklog } from './backlog';
4+
import type { ConsoleLike } from './context';
35

4-
describe('backlog command', () => {
5-
test('has correct name and description', () => {
6-
expect(backlogCommand.name()).toBe('backlog');
7-
expect(backlogCommand.description()).toBe('Add items to the project backlog');
6+
interface MockBacklogDeps extends BacklogDeps {
7+
logs: string[];
8+
errors: string[];
9+
warnings: string[];
10+
exitCode?: number;
11+
files: Map<string, string>;
12+
}
13+
14+
function createMockBacklogDeps(initialFiles?: Record<string, string>): MockBacklogDeps {
15+
const files = new Map<string, string>(Object.entries(initialFiles ?? {}));
16+
const logs: string[] = [];
17+
const errors: string[] = [];
18+
const warnings: string[] = [];
19+
let exitCode: number | undefined;
20+
21+
const consoleLike: ConsoleLike = {
22+
log: (...args: unknown[]) => {
23+
logs.push(args.join(' '));
24+
},
25+
error: (...args: unknown[]) => {
26+
errors.push(args.join(' '));
27+
},
28+
warn: (...args: unknown[]) => {
29+
warnings.push(args.join(' '));
30+
},
31+
};
32+
33+
return {
34+
console: consoleLike,
35+
path,
36+
process: {
37+
cwd: () => '/project',
38+
env: {},
39+
getExitCode: () => exitCode,
40+
setExitCode: (code: number) => {
41+
exitCode = code;
42+
},
43+
},
44+
fs: {
45+
existsSync: (targetPath: string) => files.has(targetPath),
46+
readFileSync: (targetPath: string) => {
47+
const value = files.get(targetPath);
48+
if (value === undefined) {
49+
throw new Error(`File not found: ${targetPath}`);
50+
}
51+
return value;
52+
},
53+
writeFileSync: (targetPath: string, content: string | NodeJS.ArrayBufferView) => {
54+
files.set(targetPath, content.toString());
55+
},
56+
appendFileSync: (targetPath: string, content: string | NodeJS.ArrayBufferView) => {
57+
const previous = files.get(targetPath) ?? '';
58+
files.set(targetPath, `${previous}${content.toString()}`);
59+
},
60+
mkdirSync: mock(() => {}),
61+
readdirSync: mock(() => []),
62+
unlinkSync: mock(() => {}),
63+
copyFileSync: mock(() => {}),
64+
},
65+
time: {
66+
now: () => new Date('2025-01-01T00:00:00Z'),
67+
todayISO: () => '2025-01-01',
68+
},
69+
logger: () => ({
70+
info: mock(() => {}),
71+
warn: mock(() => {}),
72+
error: mock(() => {}),
73+
debug: mock(() => {}),
74+
exception: mock(() => {}),
75+
}),
76+
logs,
77+
errors,
78+
warnings,
79+
exitCode,
80+
files,
81+
};
82+
}
83+
84+
describe('runBacklog', () => {
85+
test('fails when no items are provided', () => {
86+
const deps = createMockBacklogDeps();
87+
88+
const result = runBacklog([], {}, deps);
89+
90+
expect(result.success).toBeFalse();
91+
if (result.success) return;
92+
expect(result.error).toContain('No items provided');
93+
expect(result.exitCode).toBe(1);
894
});
995

10-
test('has expected options', () => {
11-
const options = backlogCommand.options;
12-
const optionNames = options.map((opt) => opt.long);
96+
test('creates backlog file and appends items when missing', () => {
97+
const deps = createMockBacklogDeps();
98+
99+
const result = runBacklog(['Improve docs', 'Refactor parser'], {}, deps);
100+
101+
expect(result.success).toBeTrue();
102+
if (!result.success) return;
13103

14-
expect(optionNames).toContain('--list');
15-
expect(optionNames).toContain('--file');
104+
const backlogPath = path.join('/project', '.claude', 'backlog.md');
105+
const fileContent = deps.files.get(backlogPath);
106+
expect(fileContent).toBeDefined();
107+
expect(fileContent).toContain('# Backlog');
108+
expect(fileContent).toContain('[2025-01-01] Improve docs');
109+
expect(fileContent).toContain('[2025-01-01] Refactor parser');
110+
expect(result.messages).toEqual(['✅ Added 2 item(s) to backlog', ' - Improve docs', ' - Refactor parser']);
16111
});
17112

18-
test('command structure is correct', () => {
19-
// Basic validation that command has expected structure
20-
expect(backlogCommand.name()).toBe('backlog');
21-
expect(backlogCommand.options).toBeDefined();
22-
expect(backlogCommand.options.length).toBeGreaterThan(0);
113+
test('lists backlog content when file exists', () => {
114+
const backlogPath = path.join('/project', '.claude', 'backlog.md');
115+
const deps = createMockBacklogDeps({
116+
[backlogPath]: '# Backlog\n\n## Items\n- Existing item\n',
117+
});
118+
119+
const result = runBacklog([], { list: true }, deps);
120+
121+
expect(result.success).toBeTrue();
122+
if (!result.success) return;
123+
124+
expect(result.data?.content).toContain('Existing item');
125+
expect(result.messages?.[0]).toContain('Existing item');
23126
});
24127

25-
test('option descriptions are correct', () => {
26-
const options = backlogCommand.options;
27-
const listOption = options.find((opt) => opt.long === '--list');
28-
const fileOption = options.find((opt) => opt.long === '--file');
128+
test('reports failure when filesystem throws', () => {
129+
const deps = createMockBacklogDeps();
130+
deps.fs.writeFileSync = () => {
131+
throw new Error('disk full');
132+
};
133+
134+
const result = runBacklog(['Improve docs'], {}, deps);
135+
136+
expect(result.success).toBeFalse();
137+
if (result.success) return;
138+
expect(result.error).toContain('Failed to update backlog');
139+
});
140+
});
29141

30-
expect(listOption?.description).toBe('list current backlog items');
31-
expect(fileOption?.description).toBe('backlog file path (default: .claude/backlog.md)');
142+
describe('createBacklogCommand', () => {
143+
test('creates command with expected metadata', () => {
144+
const command = createBacklogCommand();
145+
expect(command.name()).toBe('backlog');
146+
expect(command.description()).toBe('Add items to the project backlog');
147+
148+
const optionFlags = command.options.map((option) => option.long);
149+
expect(optionFlags).toContain('--list');
150+
expect(optionFlags).toContain('--file');
151+
const handler = (command as unknown as { _actionHandler?: unknown })._actionHandler;
152+
expect(typeof handler).toBe('function');
153+
});
154+
});
155+
156+
describe('exported backlogCommand', () => {
157+
test('is instantiated command', () => {
158+
expect(backlogCommand.name()).toBe('backlog');
32159
});
33160
});

0 commit comments

Comments
 (0)