test(cli): add comprehensive v2 CLI integration test suite - #304
Conversation
57 tests across 15 classes covering the full CLI surface area: init, status, summary, PRD, tasks, work, batch, blocker, checkpoint, patch, schedule, templates, review, and golden-path E2E. Uses CliRunner against real SQLite databases (no mocks).
WalkthroughAdds a new end-to-end v2 CLI integration test module exercising Typer-based commands, AI-driven flows, and workspace fixtures; tests run against a real SQLite-backed workspace and include mock LLM provider scenarios and golden-path E2E coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review: v2 CLI Integration Test SuiteThis is an excellent addition to the test suite! The comprehensive coverage across 57 integration tests provides strong validation of the CLI surface area. Here are my findings: ✅ Strengths
🐛 Minor Issues to Fix1. Unchecked CLI Invocations (Silent Failures)Several tests invoke CLI commands without verifying the result, which could mask failures:
Recommendation: Add assertion checks or use 2. Flaky ID Extraction PatternThe regex pattern Example (line 197): ids = re.findall(r"[0-9a-f]{8,}", show_result.output)
if ids:
# Uses first match, might not be actual PRD IDRecommendation: Use a more specific pattern or extract IDs via the core API (as done in 3. Overly Permissive Exit Code AssertionsSome tests accept multiple exit codes without verifying expected behavior:
Recommendation: Check output content to understand why the command exited with a specific code. 💡 Suggestions for Enhancement1. Add Parameterization for Similar TestsTests like @pytest.mark.parametrize("create_checkpoint,expected_in_output", [(False, ""), (True, "my-cp")])
def test_checkpoint_list(create_checkpoint, expected_in_output, workspace_path):
if create_checkpoint:
runner.invoke(app, ["checkpoint", "create", "my-cp", "-w", str(workspace_path)])
result = runner.invoke(app, ["checkpoint", "list", "-w", str(workspace_path)])
assert result.exit_code == 0
if expected_in_output:
assert expected_in_output in result.output2. Strengthen Test AssertionsIn # After delete, verify no tasks remain
result = runner.invoke(app, ["tasks", "list", "-w", str(workspace_with_tasks)])
assert "0 tasks" in result.output or "no tasks" in result.output.lower()3. Add Negative Test CasesConsider adding tests for edge cases:
4. Improve Test DocumentationAdd docstrings to test methods explaining what they validate, especially for complex flows like the E2E test. ⚡ Performance Considerations
Recommendation: For large test suites, add: # In pyproject.toml
[tool.pytest.ini_options]
addopts = "-nauto" # Auto-detect CPU cores for parallel execution🔒 Security Notes
📊 Coverage AssessmentWell-Covered (✅):
Could Be Strengthened (
SummaryThis is a solid foundation for CLI integration testing. The tests are well-structured and comprehensive. Addressing the unchecked CLI invocations and improving ID extraction reliability would make the suite more robust. The suggestions above are optional enhancements—this PR is ready to merge as-is after fixing the unchecked invocations. Great work! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/cli/test_v2_cli_integration.py`:
- Around line 10-11: Remove the unused import Path from the top-level import
list (the line importing Path) in tests/cli/test_v2_cli_integration.py; the
tmp_path pytest fixture already yields pathlib.Path objects so delete the "Path"
import to resolve the F401 linter error and run the tests to confirm no other
references to Path remain.
|
Code review posted: #304 (comment) |
6 new tests exercise the LLM code paths through the CLI: - Task generation via LLM (with valid JSON, overwrite, and fallback) - Agent execution dry-run (planner + executor) - Agent execution creating a file (verifies file on disk) - Full AI golden path E2E (generate → plan → execute) Uses MockProvider injected via monkeypatch on get_provider, with canned plan JSON and file content responses.
Add v2 CLI integration test suite to validate init, tasks, work, PRD, and AI agent flows using pytest and Typer CliRunner in tests/cli/test_v2_cli_integration.pyIntroduce a pytest-based v2 CLI integration suite with fixtures for temp repos and workspaces, coverage for core commands and end-to-end flows, and AI-path tests driven by a mocked LLM provider in tests/cli/test_v2_cli_integration.py. 📍Where to StartStart with the top-level test flow definitions in tests/cli/test_v2_cli_integration.py, then review the Macroscope summarized 0350031. |
Code Review: v2 CLI Integration Test SuiteExcellent addition to the test coverage! The comprehensive test suite provides strong validation of the CLI surface area. Here are my findings: ✅ Strengths
🐛 Issues to Address1. Remove Unused ImportLine 10 imports which is not used: Fix: Remove this line (already flagged by CodeRabbit). 2. Unchecked CLI InvocationsSeveral tests invoke CLI commands without checking exit codes, which could mask failures:
Recommendation: Add assertions or verify results: 3. Fragile ID Extraction PatternThe regex (lines 197, 432, 445, 459) could match unintended hex strings: Recommendation: Use core API for reliable ID extraction (as done in ): 4. Overly Permissive Exit Code AssertionsSome tests accept multiple exit codes without verifying expected behavior:
Recommendation: Check output to understand why specific exit codes occur: 💡 Enhancement Suggestions1. Add Parameterization for Similar TestsTests like and could use : 2. Strengthen Test AssertionsIn (line 305), verify tasks are actually deleted: 3. Add Negative Test CasesConsider edge cases:
4. Add Test Method DocstringsDocument what each test validates, especially for complex flows like E2E tests. ⚡ Performance Notes
🔒 Security Assessment✅ No concerns - tests use fixtures and mock provider with fake API key. 📊 Coverage SummaryWell-Covered (✅):
Could Be Strengthened (
SummaryThis is a solid foundation for CLI integration testing. The tests are well-structured and comprehensive. Addressing the unchecked CLI invocations and fixing the unused import would make the suite more robust. The enhancement suggestions are optional—this PR is close to merge-ready after addressing the critical issues. Great work on the comprehensive coverage! 🎉 |
|
I've reviewed PR #304 and posted a comprehensive code review comment. The review covers: Key findings:
Recommendations include:
Optional enhancements:
The test suite is well-structured and provides solid coverage. After addressing the critical issues (especially the unused import and unchecked invocations), this PR will be ready to merge. Review comment posted: #304 (comment) |
Code Review: v2 CLI Integration Test SuiteThis PR adds excellent comprehensive CLI integration coverage! The test suite is well-structured and aligns well with the v2 architecture. However, there's one critical issue that must be addressed before merging. 🔴 Critical Issue (Must Fix)Unused Import Fails CILine 10: The Fix: Remove line 10 entirely. ✅ Strengths
🐛 Minor Issues (Recommended to Address)1. Unchecked CLI Invocations (Could Mask Failures)Several tests invoke setup CLI commands without verifying results:
Impact: If a setup command fails silently, the actual test assertion might pass for the wrong reasons. Recommendation: Add exit code checks or use the assertion pattern: setup_result = runner.invoke(app, ["work", "start", tid, "-w", str(workspace_with_ready_tasks)])
assert setup_result.exit_code == 0, f"Setup failed: {setup_result.output}"2. Fragile ID Extraction PatternLines 197, 432, 445, 459: Using Example (line 197): ids = re.findall(r"[0-9a-f]{8,}", show_result.output)
if ids:
# Uses first match - might not be the PRD ID!Recommendation: Use core API for reliable ID extraction (as done correctly in ws = create_or_load_workspace(workspace_with_prd)
prd_record = prd.get_latest(ws)
assert prd_record is not None
# Use prd_record.id directly3. Overly Permissive Exit Code AssertionsSome tests accept multiple exit codes without verifying expected behavior:
Recommendation: Check output content to understand why specific exit codes occur: assert result.exit_code == 0 or "confirm" in result.output.lower()💡 Enhancement Suggestions (Optional)1. Add Parameterization to Reduce DuplicationTests like @pytest.mark.parametrize("create_checkpoint,expected_in_output", [(False, ""), (True, "my-cp")])
def test_checkpoint_list(create_checkpoint, expected_in_output, workspace_path):
if create_checkpoint:
runner.invoke(app, ["checkpoint", "create", "my-cp", "-w", str(workspace_path)])
result = runner.invoke(app, ["checkpoint", "list", "-w", str(workspace_path)])
assert result.exit_code == 0
if expected_in_output:
assert expected_in_output in result.output2. Strengthen Test AssertionsLine 305 ( result = runner.invoke(app, ["tasks", "list", "-w", str(workspace_with_tasks)])
assert "0 tasks" in result.output or "no tasks" in result.output.lower()3. Add Negative Test CasesConsider edge cases for robustness:
4. Consider Test ParallelizationCurrent runtime (~24s for 57 tests) is reasonable, but could be faster with [tool.pytest.ini_options]
addopts = "-nauto" # Auto-detect CPU cores📊 Coverage SummaryWell-Covered (✅):
Could Be Strengthened (
🔒 Security Assessment✅ No security concerns - tests use SummaryThis is a solid foundation for CLI integration testing. The tests are comprehensive, well-structured, and align with the v2 architecture. Must fix before merge: Remove the unused Recommended after that: Address the unchecked CLI invocations to prevent silent test failures. The enhancement suggestions are optional - this PR provides excellent coverage as-is. Great work! 🎉 |
|
I've posted a comprehensive code review to PR #304. The review covers: Key findings:
Recommendations:
Review posted: #304 (comment) |
1 participant
Summary
CliRunner(no mocks)Test plan
uv run pytest tests/cli/test_v2_cli_integration.py -v— 57 passed in ~24sSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.