Conversation
…ests The test suite had regressed from ~1s to ~9s. The main culprit was completion.test.ts — 24 tests each spawning a `./bun index.ts` subprocess at ~260ms startup cost each (~6.2s total). Changes: - Extract `createProgram()` factory into `src/createProgram.ts` so tests can build the commander tree without side effects - Add `initializeTab()`, `resolveCompletions()`, `generateCompletionScript()` in-process helpers to `completion.ts` that capture console.log output instead of spawning subprocesses - Rewrite Tier 1 completion tests (19 of 24) to run in-process (<1ms each instead of ~260ms each) - Rename `src/commands/branch.ts` to `src/commands/root.ts` and move the root command action handler from `src/index.ts` into it, so `createProgram()` is fully self-contained - Simplify `src/index.ts` to just program creation, analytics, process handlers, and parsing Test suite time: ~9s → ~3.5s Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR restructures the CLI bootstrap so tests can construct and exercise the commander + completion tree in-process (avoiding subprocess overhead), significantly reducing overall test runtime.
Changes:
- Introduces
createProgram()to build the commander program without analytics/process-handler side effects. - Moves the default/root command action into
src/commands/root.tsand simplifiessrc/index.tsto bootstrap concerns only. - Adds in-process completion helpers and rewrites most completion tests to run without spawning subprocesses.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Switches to createProgram() and keeps only bootstrap concerns (analytics, handlers, parsing, completion pre-parse hook). |
| src/createProgram.ts | New commander program factory that registers root action + all subcommands. |
| src/commands/root.ts | Adds rootAction() (default/root behavior) and adjusts imports accordingly. |
| src/commands/completion.ts | Exposes shell types/constants and adds in-process tab/completion helpers used by tests. |
| src/commands/completion.test.ts | Migrates Tier-1 completion tests to in-process execution and keeps bash e2e tests. |
| AGENTS.md | Adds guidance on where to store implementation plans/specs. |
Comments suppressed due to low confidence (1)
src/commands/root.ts:335
rootActiondynamically imports../services/git.tsto calltryGetRepoInfo, but this module already importstryGetRepoInfoat the top (import { ... tryGetRepoInfo } from '../services/git'). This makes the dynamic import redundant and also mixes two different specifiers for the same module (../services/gitvs../services/git.ts), which can be fragile. Prefer using the existingtryGetRepoInfoimport (or consistently switch the whole file to one specifier).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Extract shared captureLog helper into src/utils/captureLog.ts - Fix createProgram docstring to note subcommands are shared singletons - Add missing 'session' to EXPECTED_SUBCOMMANDS in tests - Remove redundant dynamic import of tryGetRepoInfo in rootAction Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/commands/completion.test.ts:30
EXPECTED_SUBCOMMANDSis labeled as “All subcommands registered via createProgram()”, but it doesn’t include commands thatcreateProgram()registers (e.g.codex,feedback,sandbox). Either expand the list to actually cover all registered subcommands, or rename/comment it as a representative subset so the assertion doesn’t silently miss regressions for the omitted commands.
// All subcommands registered via createProgram()
const EXPECTED_SUBCOMMANDS = [
'auth',
'claude',
'colors',
'completions',
'config',
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| console.log = origLog; | ||
| } | ||
| return lines.join('\n'); | ||
| } |
There was a problem hiding this comment.
captureLog can be passed an async function (TypeScript allows () => Promise<void> where () => void is expected), but the implementation doesn’t await it. That means logs emitted after an await won’t be captured and console.log may be restored too early, leading to confusing/flaky output. Consider adding an async-aware overload (or a separate captureLogAsync) that awaits the returned promise before restoring console.log.
| } | |
| } | |
| /** | |
| * Async variant of captureLog for functions that return a Promise. | |
| * Ensures that console.log is only restored after the Promise settles, | |
| * so that logs emitted after awaits are also captured. | |
| */ | |
| export async function captureLogAsync(fn: () => Promise<void>): Promise<string> { | |
| const lines: string[] = []; | |
| const origLog = console.log; | |
| console.log = (...a: unknown[]) => lines.push(a.join(' ')); | |
| try { | |
| await fn(); | |
| } finally { | |
| console.log = origLog; | |
| } | |
| return lines.join('\n'); | |
| } |
Summary
createProgram()factory intosrc/createProgram.tsso tests can build the commander tree in-process without side effectsinitializeTab,resolveCompletions,generateCompletionScript) that capture console.log output instead of spawning subprocessessrc/commands/branch.ts→src/commands/root.ts, move root command action fromsrc/index.tsinto it socreateProgram()is fully self-containedsrc/index.tsto just program creation, analytics, process handlers, and parsingBefore: 644 tests in ~9s (completion tests alone: ~6.2s)
After: 642 tests in ~3.5s (completion tests: ~1.3s)
2 low-value tests removed (tested
process.exit(1)error paths which can't run in-process).Test plan
./bun run checkpasses (typecheck + lint + all 642 tests)./bun index.ts completions,./bun index.ts complete bash,./bun index.ts --help🤖 Generated with Claude Code