Refactor CLI tests: subprocess → integration, fast unit tests#2944
Refactor CLI tests: subprocess → integration, fast unit tests#2944amikofalvy merged 4 commits intomainfrom
Conversation
…it tests Extract `createProgram()` from index.ts into program.ts so CLI command registration can be imported without side effects (env loading, instrumentation). This enables fast unit testing via Commander's programmatic API. Move the subprocess-based cli.test.ts to integration/cli-subprocess.test.ts (real `execSync` spawning of the compiled binary). Replace with new unit tests that use Commander's `exitOverride()` — runs in 14ms vs 33s. Integration tests are chained in the `test` script so they run in CI after unit tests pass. Also available standalone via `pnpm test:integration`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
vitest.integration.config.ts:25Missingretry: 2for subprocess test flakiness
💭 Consider (2) 💭
💭 1) cli.test.ts Add handler invocation tests
Issue: The new unit tests verify command registration and option definitions via static inspection, but do not verify that command handlers are actually invoked when commands are parsed. All command modules are mocked with vi.fn() but never asserted.
Why: The original subprocess tests implicitly verified handler invocation by observing real output/errors. If a command registration accidentally omits .action(handler) or binds the wrong handler, these tests would not catch it. Low risk since integration tests still run.
Fix: Add tests that parse a command and verify the mocked handler was called:
it('should invoke pushCommand handler with parsed options', async () => {
const { pushCommand } = await import('../commands/push');
parseArgs(createProgram(), ['push', '--project', 'my-proj', '--force']);
expect(pushCommand).toHaveBeenCalledWith(expect.objectContaining({ project: 'my-proj', force: true }));
});Inline Comments:
- 💭 Consider:
cli.test.ts:103Add test forpush --helpoutput rendering
💡 APPROVE WITH SUGGESTIONS
Summary: Clean refactoring that delivers a 2,355x speedup for CLI tests. The extraction of createProgram() is well-structured with proper separation of side effects. Two small suggestions: add retry: 2 to integration config for flakiness resilience, and consider adding handler invocation assertions to unit tests for completeness. Nice work! 🚀
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
package.json:23 |
Integration tests chained differently than agents-api pattern | Design choice documented in PR description — intentional deviation with valid justification |
cli.test.ts:145 |
Subcommand registration test does not verify config/profile subcommands | Info-level completeness suggestion, not a bug or correctness issue |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-devops |
2 | 0 | 0 | 0 | 1 | 0 | 1 |
pr-review-tests |
3 | 0 | 1 | 0 | 1 | 0 | 1 |
| Total | 5 | 0 | 1 | 0 | 2 | 0 | 2 |
| }, | ||
| }, | ||
| maxConcurrency: 1, | ||
| }, |
There was a problem hiding this comment.
🟡 Minor: Missing retry option for flaky subprocess tests
Issue: The integration test config does not include the retry: 2 option present in vitest.config.ci.ts.
Why: Subprocess-based tests are inherently more prone to flakiness (process spawn timing, I/O, timeouts). The CI config retries tests twice to handle transient failures. Without this, integration tests may produce false-negative failures in CI.
Fix:
| }, | |
| maxConcurrency: 1, | |
| retry: 2, | |
| }, |
Refs:
- vitest.config.ci.ts:35 — existing retry config
| expect(pushCmd).toBeDefined(); | ||
| expect(pushCmd!.description()).toContain('Push a project configuration'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💭 Consider: Add test for push --help output
Issue: The original integration tests verified that push --help displays correct content including --agents-api-url. The new unit tests inspect options via Commander's internal API but don't test the rendered help output.
Why: If help text generation breaks for subcommands, this would go undetected until integration tests run. A fast unit test provides earlier feedback.
Fix: Add after line 103:
it('should display help for push command', () => {
const { stdout } = parseArgs(createProgram(), ['push', '--help']);
expect(stdout).toContain('Push a project configuration');
expect(stdout).toContain('--agents-api-url');
});Refs:
- cli-subprocess.test.ts:154-160 — original test
|
TL;DR — Splits the Key changes
Summary ｜ 8 files ｜ 4 commits ｜ base: Side-effect-free CLI program factory
The In-process unit tests via Commander's programmatic API
The rewritten suite validates version output, help text, command registration (all 13 commands), option definitions, unknown-command error handling, and CLI naming — all without touching the filesystem or spawning processes. Uses optional chaining ( Two-tier test execution strategy
The integration config enforces sequential execution (
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean refactor — the createProgram() extraction is correct, the unit tests are dramatically faster, and there's no behavior change to the CLI itself. Two things need attention before merge.
Claude Opus ｜ 𝕏
agents-cli/package.json
Outdated
| "test:debug": "vitest --run --reporter=verbose --no-coverage", | ||
| "test:watch": "vitest", | ||
| "test:coverage": "node -e \"process.exit(process.env.CI ? 0 : 1)\" && vitest --run --coverage --config vitest.config.ci.ts || vitest --run --coverage", | ||
| "test:ci": "vitest --run --config vitest.config.ci.ts", |
There was a problem hiding this comment.
test:ci only runs the CI unit config and never chains integration tests. If any CI workflow calls test:ci directly (instead of test), integration tests silently disappear.
Either chain integration here too:
"test:ci": "vitest --run --config vitest.config.ci.ts && vitest --run --config vitest.integration.config.ts",
…or rename it to test:unit:ci so the omission is explicit.
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| // Force garbage collection to clean up any hanging references | ||
| if (global.gc) { |
There was a problem hiding this comment.
The 100ms sleep and global.gc() call were workarounds for subprocess-spawned processes not terminating cleanly. In the integration suite these same subprocess tests still exist so the sleep is arguably still relevant — but global.gc() is a no-op unless Node is started with --expose-gc, which none of the configs pass. Consider removing the GC block to avoid giving the impression it does something.
agents-cli/vitest.config.ci.ts
Outdated
| vmThreads: false, | ||
| }, | ||
| }, | ||
| // Exclude integration tests (run separately via test:integration) |
There was a problem hiding this comment.
Nit: repo convention is no comments unless explicitly requested (AGENTS.md). This comment restates what the config key already says.
| // Exclude integration tests (run separately via test:integration) | |
| exclude: ['node_modules', 'dist', '**/integration/**'], |
- Add retry: 2 to integration config for flaky subprocess tests - Chain integration tests in test:ci script - Remove no-op global.gc() from subprocess test cleanup - Remove unnecessary comment in CI config Skipping push --help unit test: Commander subcommand help doesn't propagate through parent's configureOutput(), so the programmatic option inspection is more reliable for unit tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
All prior review feedback has been addressed in commit 66c77e8f2:
| Prior Feedback | Status |
|---|---|
🟡 Missing retry: 2 in integration config |
✅ Added at line 25 |
🔴 test:ci missing integration test chain |
✅ Fixed in package.json |
💭 Dead global.gc() code |
✅ Removed from cli-subprocess.test.ts |
| 💭 Comment violating repo convention | ✅ Removed from vitest.config.ci.ts |
🕐 Pending Recommendations (0)
None — all prior feedback has been addressed.
✅ APPROVE
Summary: Clean refactoring that delivers a 2,355x speedup for CLI tests (14ms vs 33s). The extraction of createProgram() is well-structured with proper separation of side effects. All prior review feedback has been addressed — retry: 2 added, test:ci now chains integration tests, dead global.gc() removed, and the stray comment eliminated. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No sub-reviewers dispatched for this re-review — delta contained only fix commits addressing prior feedback.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Analysis
The only change since the last review (66c77e8f2) is commit 4d7b5764 which replaces 4 non-null assertions (!) with optional chaining (?.) in cli.test.ts:
- expect(pushCmd!.description()).toContain(...)
+ expect(pushCmd?.description()).toContain(...)This is a pure lint fix — no behavioral changes, no logic modifications. The assertions still verify the same conditions, just with safer syntax that satisfies Biome's style rules.
Prior Feedback Status
All feedback from previous reviews has been addressed:
| Prior Feedback | Status |
|---|---|
🟡 Missing retry: 2 in integration config |
✅ Addressed in 66c77e8f2 |
🔴 test:ci missing integration test chain |
✅ Addressed in 66c77e8f2 |
💭 Dead global.gc() code |
✅ Addressed in 66c77e8f2 |
| 💭 Comment violating repo convention | ✅ Addressed in 66c77e8f2 |
✅ APPROVE
Summary: The delta since last review is a trivial lint fix (non-null assertions → optional chaining). All prior review feedback remains addressed. This PR is ready to merge. 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No sub-reviewers dispatched — delta was a trivial lint fix with no behavioral changes.
1 participant
Summary
createProgram()fromindex.tsintoprogram.tsso CLI command registration is importable without side effects (env loading, instrumentation)cli.test.ts(realexecSyncspawning) tointegration/cli-subprocess.test.tsexitOverride()and programmatic API — 14ms vs 33s (2,355x faster)testscript so they always run in CI after unit tests passpnpm test:integrationTiming impact
Files changed
src/program.tscreateProgram()from index.tssrc/index.tscreateProgram().parse()src/__tests__/cli.test.tssrc/__tests__/integration/cli-subprocess.test.tsvitest.integration.config.tsvitest.config.ts**/integration/**vitest.config.ci.ts**/integration/**package.jsontestscript, addtest:integrationTest plan
pnpm testfrom agents-clipnpm test:integrationfrom agents-clipnpm test🤖 Generated with Claude Code