Add TDD skill and fix failing tests#35
Conversation
- Fix test_analyze_logs: use normalizable paths instead of distinct IPs - Fix test_filter_outdated: match real pnpm outdated output format - Fix test_filter_ansi_colors: add missing Tests line for stats parsing - Add .claude/skills/rtk-tdd/ with Red-Green-Refactor workflow - Add testing-patterns.md reference with untested modules backlog - Update CLAUDE.md Testing Strategy with TDD mandate and pre-commit gate - Add 17 tests to diff_cmd.rs (similarity, truncate, compute_diff, condense) - Test suite: 102/105 → 122/122 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes 3 pre-existing test failures and adds comprehensive TDD infrastructure including a generic Rust TDD skill, RTK-specific testing patterns documentation, and 17 new unit tests for diff_cmd.rs.
Changes:
- Fixed test data in 3 modules (
vitest_cmd.rs,pnpm_cmd.rs,log_cmd.rs) to match actual function behavior - Added 17 unit tests to
diff_cmd.rscovering similarity, truncate, compute_diff, and condense_unified_diff functions - Added TDD skill documentation (
.claude/skills/rtk-tdd/) with Red-Green-Refactor workflow and Rust-idiomatic patterns - Added testing patterns reference with RTK-specific patterns and untested modules backlog
- Updated
CLAUDE.mdwith TDD mandate and pre-commit gate requirements
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vitest_cmd.rs | Fixed test data to include "Tests" line required by parse_test_stats |
| src/pnpm_cmd.rs | Updated test data format to match actual pnpm outdated output (box-drawing on separate lines) |
| src/log_cmd.rs | Changed test data from IP addresses to paths to match path normalization regex |
| src/diff_cmd.rs | Added 17 unit tests covering similarity (5), truncate (3), compute_diff (7), and condense_unified_diff (3) |
| CLAUDE.md | Updated Testing Strategy section with TDD workflow, test architecture, and pre-commit gate |
| .claude/skills/rtk-tdd/SKILL.md | Added comprehensive TDD skill with Three Laws, Red-Green-Refactor steps, and Rust patterns |
| .claude/skills/rtk-tdd/references/testing-patterns.md | Added RTK testing patterns reference with untested modules backlog and code examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| | Module | Testable Functions | Notes | | ||
| |--------|-------------------|-------| | ||
| | `diff_cmd.rs` | `compute_diff`, `similarity`, `truncate`, `condense_unified_diff` | 4 pure functions, 0 tests | |
There was a problem hiding this comment.
This line states that diff_cmd.rs has "4 pure functions, 0 tests", but this PR adds 17 tests to diff_cmd.rs. This entry should be removed from the untested modules backlog or updated to reflect that tests have now been added.
The line should either be deleted entirely or moved to a "Recently Added Coverage" section to track progress.
|
|
||
| Run module-specific tests: | ||
| ### Test Architecture | ||
| - **Unit tests**: Embedded `#[cfg(test)] mod tests` in each module (105+ tests, 25+ files) |
There was a problem hiding this comment.
The test count states "105+ tests" but according to the PR description, the test suite now has 122 tests (increased from 105). Consider updating this to "122 tests" or "120+ tests" to accurately reflect the current state of test coverage.
| - **Unit tests**: Embedded `#[cfg(test)] mod tests` in each module (105+ tests, 25+ files) | |
| - **Unit tests**: Embedded `#[cfg(test)] mod tests` in each module (120+ tests, 25+ files) |
| #[test] | ||
| fn test_truncate_short_string() { | ||
| assert_eq!(truncate("hello", 10), "hello"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_truncate_exact_length() { | ||
| assert_eq!(truncate("hello", 5), "hello"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_truncate_long_string() { | ||
| assert_eq!(truncate("hello world!", 8), "hello..."); | ||
| } |
There was a problem hiding this comment.
The truncate function being tested has a potential integer underflow bug when max_len < 3. Consider adding a test case for this edge case to document the expected behavior, such as:
test_truncate_min_lengthwith max_len values of 0, 1, 2, and 3
This would either expose the bug (causing the test to panic) or document that small max_len values are not supported. The function implementation should then be fixed to handle max_len < 3 appropriately, either by returning the string as-is, clamping max_len to a minimum of 3, or documenting the precondition.
Summary
.claude/skills/rtk-tdd/— generic Rust TDD skill (Red-Green-Refactor, Three Laws, idiomatic patterns)references/testing-patterns.md— RTK-specific test patterns catalog + untested modules backlog (12 modules)CLAUDE.mdTesting Strategy section with TDD mandate and pre-commit gatediff_cmd.rsas proof-of-concept (similarity, truncate, compute_diff, condense_unified_diff)Test plan
cargo test— 122/122 passcargo test diff_cmd::tests::— 17 new tests passtest_analyze_logs,test_filter_outdated,test_filter_ansi_colors🤖 Generated with Claude Code