Refactor llm-coding-tools-core tests to use rstest parameterization#74
Conversation
…ization - Convert 71 individual tests into 46 parameterized test cases using rstest - Add descriptive case names (#[case::name]) with inline parameter comments - Group related test scenarios to improve readability and maintainability - Document test rationale (e.g., token reduction for LLM outputs) - Remove redundant tests testing serde/thiserror rather than business logic - Add inline explanations aligned in columns for complex test parameters Key improvements: - glob.rs: 6→4 tests (pattern matching + serialization) - grep.rs: 6→5 tests (search scenarios, formatting, edge cases) - allowed.rs: 8→4 tests (path resolution scenarios) - permissions.rs: 22→12 tests (wildcard matching, ruleset evaluation) - error.rs: Removed thiserror display tests (testing framework, not our code) - output.rs: Removed redundant From impl tests Net reduction: ~35% fewer test functions while maintaining equal coverage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
==========================================
- Coverage 78.43% 78.36% -0.08%
==========================================
Files 108 108
Lines 3942 3942
==========================================
- Hits 3092 3089 -3
- Misses 850 853 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🧰 Additional context used📓 Path-based instructions (1)src/**/*.rs📄 CodeRabbit inference engine (src/AGENTS.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-03-17T21:06:53.934ZApplied to files:
🔇 Additional comments (5)
WalkthroughThis PR refactors many test modules in the llm-coding-tools-core crate by consolidating multiple standalone unit tests into parameterized rstest cases. Affected test files include error.rs, internal/hash64.rs, models/provider_type.rs, output.rs, path/allowed.rs, permissions.rs, tools/{glob.rs,grep.rs,todo.rs,webfetch/blocking_impl.rs}. No production code or public API declarations were changed; edits are confined to #[cfg(test)] test modules and test imports. Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/llm-coding-tools-core/src/models/provider_type.rs (1)
72-82: Avoid double-negation in the API key expectation.Line 81 uses
!provider.requires_api_key()againstno_api_key, which makes the cases harder to scan quickly. Prefer assertingrequires_api_key()directly with a positive expected value.♻️ Proposed clarity refactor
- #[case::azure_requires_base_url(ProviderType::Azure, true, false)] - #[case::ollama_no_api_key(ProviderType::Ollama, false, true)] - #[case::openai_requires_api_key(ProviderType::OpenAiCompletions, false, false)] + #[case::azure_requires_base_url(ProviderType::Azure, true, true)] + #[case::ollama_no_api_key(ProviderType::Ollama, false, false)] + #[case::openai_requires_api_key(ProviderType::OpenAiCompletions, false, true)] fn provider_type_flags( #[case] provider: ProviderType, #[case] requires_base_url: bool, - #[case] no_api_key: bool, + #[case] requires_api_key: bool, ) { assert_eq!(provider.requires_base_url(), requires_base_url); - assert_eq!(!provider.requires_api_key(), no_api_key); + assert_eq!(provider.requires_api_key(), requires_api_key); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/models/provider_type.rs` around lines 72 - 82, The test provider_type_flags currently compares !provider.requires_api_key() to no_api_key which uses a double-negation and is confusing; change the test signature so the second boolean represents requires_api_key (or add a new variable expected_requires_api_key) and assert provider.requires_api_key() == expected_requires_api_key, updating the case attributes (e.g., #[case::ollama_no_api_key(..., false)] to reflect the positive requires_api_key value) and replace the assert_eq!(!provider.requires_api_key(), no_api_key) with assert_eq!(provider.requires_api_key(), expected_requires_api_key) to make expectations explicit and readable while keeping the function name provider_type_flags and the call provider.requires_api_key() to locate the change.src/llm-coding-tools-core/src/tools/glob.rs (1)
177-225: Optional: derive expected key presence instead of passing duplicated booleans.
expect_partial_keyandexpect_errors_keyare currently redundant withpartial/errors. Deriving them inside the test avoids accidental mismatch in future cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tools/glob.rs` around lines 177 - 225, The test glob_output_serialization_omits_default_fields currently takes redundant boolean cases expect_partial_key and expect_errors_key; refactor it to derive expected key presence from the inputs instead of passing duplicates: in the test body compute expected_partial_present = partial and expected_errors_present = !errors.is_empty(), remove the #[case] params expect_partial_key and expect_errors_key from the signature and from the #[case::...] annotations, and update assertions to compare json.get("partial").is_some() against expected_partial_present and json.get("errors").is_some() against expected_errors_present; keep references to the GlobOutput struct and existing error_count logic unchanged.src/llm-coding-tools-core/src/tools/todo.rs (1)
152-167: Consider passing the status variant directly as a parameter.The current test structure is inverted—it takes the expected icon and reverse-engineers the input via a match statement, duplicating the mapping logic being tested. This makes the test harder to read and introduces an unreachable panic branch.
♻️ Suggested refactor for clarity
- #[rstest] - #[case::pending("[ ]")] - #[case::in_progress("[>]")] - #[case::completed("[x]")] - #[case::cancelled("[-]")] - fn status_icons(#[case] expected: &str) { - let status = match expected { - "[ ]" => TodoStatus::Pending, - "[>]" => TodoStatus::InProgress, - "[x]" => TodoStatus::Completed, - "[-]" => TodoStatus::Cancelled, - _ => panic!("unknown icon"), - }; - assert_eq!(status.icon(), expected); - } + #[rstest] + #[case::pending(TodoStatus::Pending, "[ ]")] + #[case::in_progress(TodoStatus::InProgress, "[>]")] + #[case::completed(TodoStatus::Completed, "[x]")] + #[case::cancelled(TodoStatus::Cancelled, "[-]")] + fn status_icons(#[case] status: TodoStatus, #[case] expected: &str) { + assert_eq!(status.icon(), expected); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tools/todo.rs` around lines 152 - 167, The test status_icons in the TodoStatus tests is inverted: it takes expected icon strings and reverse-maps them to TodoStatus, duplicating the mapping under test; change the parameterization to pass the TodoStatus variants directly (e.g., TodoStatus::Pending, TodoStatus::InProgress, TodoStatus::Completed, TodoStatus::Cancelled) and the expected icon next to each case, then assert_eq!(status.icon(), expected) without the match; update the #[case] attributes in the status_icons test to use the enum variants and expected strings and remove the unreachable panic branch and reverse-mapping logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/llm-coding-tools-core/src/models/provider_type.rs`:
- Around line 72-82: The test provider_type_flags currently compares
!provider.requires_api_key() to no_api_key which uses a double-negation and is
confusing; change the test signature so the second boolean represents
requires_api_key (or add a new variable expected_requires_api_key) and assert
provider.requires_api_key() == expected_requires_api_key, updating the case
attributes (e.g., #[case::ollama_no_api_key(..., false)] to reflect the positive
requires_api_key value) and replace the assert_eq!(!provider.requires_api_key(),
no_api_key) with assert_eq!(provider.requires_api_key(),
expected_requires_api_key) to make expectations explicit and readable while
keeping the function name provider_type_flags and the call
provider.requires_api_key() to locate the change.
In `@src/llm-coding-tools-core/src/tools/glob.rs`:
- Around line 177-225: The test glob_output_serialization_omits_default_fields
currently takes redundant boolean cases expect_partial_key and
expect_errors_key; refactor it to derive expected key presence from the inputs
instead of passing duplicates: in the test body compute expected_partial_present
= partial and expected_errors_present = !errors.is_empty(), remove the #[case]
params expect_partial_key and expect_errors_key from the signature and from the
#[case::...] annotations, and update assertions to compare
json.get("partial").is_some() against expected_partial_present and
json.get("errors").is_some() against expected_errors_present; keep references to
the GlobOutput struct and existing error_count logic unchanged.
In `@src/llm-coding-tools-core/src/tools/todo.rs`:
- Around line 152-167: The test status_icons in the TodoStatus tests is
inverted: it takes expected icon strings and reverse-maps them to TodoStatus,
duplicating the mapping under test; change the parameterization to pass the
TodoStatus variants directly (e.g., TodoStatus::Pending, TodoStatus::InProgress,
TodoStatus::Completed, TodoStatus::Cancelled) and the expected icon next to each
case, then assert_eq!(status.icon(), expected) without the match; update the
#[case] attributes in the status_icons test to use the enum variants and
expected strings and remove the unreachable panic branch and reverse-mapping
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c110ded-a08f-432c-b734-2e236c144a9d
📒 Files selected for processing (10)
src/llm-coding-tools-core/src/error.rssrc/llm-coding-tools-core/src/internal/hash64.rssrc/llm-coding-tools-core/src/models/provider_type.rssrc/llm-coding-tools-core/src/output.rssrc/llm-coding-tools-core/src/path/allowed.rssrc/llm-coding-tools-core/src/permissions.rssrc/llm-coding-tools-core/src/tools/glob.rssrc/llm-coding-tools-core/src/tools/grep.rssrc/llm-coding-tools-core/src/tools/todo.rssrc/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs
💤 Files with no reviewable changes (1)
- src/llm-coding-tools-core/src/error.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Async Windows
- GitHub Check: Async Linux
- GitHub Check: Async macOS
- GitHub Check: Blocking Linux
- GitHub Check: Blocking macOS
- GitHub Check: Blocking Windows
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/models/provider_type.rssrc/llm-coding-tools-core/src/internal/hash64.rssrc/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rssrc/llm-coding-tools-core/src/path/allowed.rssrc/llm-coding-tools-core/src/tools/glob.rssrc/llm-coding-tools-core/src/tools/todo.rssrc/llm-coding-tools-core/src/output.rssrc/llm-coding-tools-core/src/permissions.rssrc/llm-coding-tools-core/src/tools/grep.rs
🧠 Learnings (3)
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
Applied to files:
src/llm-coding-tools-core/src/path/allowed.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-core/src/tools/glob.rssrc/llm-coding-tools-core/src/tools/todo.rssrc/llm-coding-tools-core/src/permissions.rssrc/llm-coding-tools-core/src/tools/grep.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Use `memchr` for fast byte searching over manual iteration
Applied to files:
src/llm-coding-tools-core/src/tools/grep.rs
🔇 Additional comments (19)
src/llm-coding-tools-core/src/tools/glob.rs (2)
133-175: Good refactor to parameterized glob behavior tests.This consolidation is cleaner, and the case names plus assertion context make failures easier to diagnose.
260-280: Comment updates improve test intent clarity.The revised comments around mtime ordering and forward-slash normalization make these assertions easier to understand.
src/llm-coding-tools-core/src/path/allowed.rs (3)
140-160: LGTM!Good refactoring with clear documentation. The inline comments explaining "exists: created by setup_test_dir()" vs "does NOT exist: tests write path resolution" follow the coding guideline to focus comments on 'why' not 'what'. The descriptive case names make test failures immediately understandable.
162-177: LGTM!The parameterized cases cover the essential traversal patterns. The error message assertion
contains("not within allowed")is appropriate for validating the rejection reason without being overly brittle to exact message wording.
194-205: LGTM!The renamed test clearly states its intent, and the explicit input
subdir/../file.txtmakes the normalization behavior being tested obvious. The assertion is appropriately defensive.src/llm-coding-tools-core/src/tools/todo.rs (1)
132-150: LGTM!The parameterized test correctly validates both empty
idand blankcontentscenarios. The case names provide clear documentation of what each test verifies.src/llm-coding-tools-core/src/permissions.rs (5)
299-428: Well-structured parameterized test coverage.The wildcard matching tests are comprehensive, covering star wildcards, question marks, prefix/suffix patterns, multiple stars, and edge cases with empty inputs. The descriptive case names and inline comments improve test readability and debugging output.
430-449: LGTM!The parameterized test correctly verifies that
Rulepreserves pattern casing without normalization.
470-527: Last-match-wins coverage is maintained.The
last_match_wins_allowandlast_match_wins_denycases properly test both orderings of overlapping wildcard/specific patterns. This covers the critical precedence behavior that production code inextensions.rs,runtime/task.rs, andhandle.rsdepends on per the relevant code snippets.
529-561: LGTM!Good coverage of case sensitivity for both permission keys and subject patterns.
563-600: LGTM!Clear verification that permission keys require exact match while only subject patterns support wildcards. This asymmetry is well-documented in the test case comments.
src/llm-coding-tools-core/src/internal/hash64.rs (1)
44-57: LGTM!The parameterized test cleanly consolidates the original determinism and collision tests. The
should_equalboolean effectively controls the assertion type, and the test cases cover both identical input verification and different-input collision resistance.src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs (1)
126-143: LGTM!The parameterized test effectively consolidates both validation error cases. Using an intentionally unreachable URL is appropriate since validation occurs before any network request, and the doc comment clearly explains this design decision.
src/llm-coding-tools-core/src/output.rs (1)
64-95: LGTM!Both parameterized tests are well-structured. The serialization test at line 94 cleverly validates the
skip_serializing_ifbehavior with a single assertion, and the doc comment provides good rationale for why this behavior matters (reducing unnecessary tokens for LLM consumers).src/llm-coding-tools-core/src/tools/grep.rs (5)
267-338: LGTM!The parameterized test effectively covers the key grep search scenarios. The inline comments documenting each case parameter are helpful for understanding test intent.
One minor note: the glob verification logic at lines 330-337 is tightly coupled to simple
*.extpatterns. This works for the current test cases but would need adjustment if more complex glob patterns are added in the future.
340-374: LGTM!Good coverage of all four boolean combinations for partial/truncated flags. The conditional
errorsvector setup correctly reflects howpartialis derived from!errors.is_empty()in production code.
376-411: LGTM!The conditional file creation pattern at lines 398-400 is a pragmatic approach for testing both missing-file and binary-file edge cases within a single parameterized test.
413-468: LGTM!The truncation test cases comprehensively cover the key boundary conditions: short truncation with/without line numbers, no truncation when content fits, and the exact-boundary case.
470-484: LGTM!Keeping this as a standalone test makes sense—it verifies a specific walker error scenario that doesn't fit the parameterized patterns used elsewhere, and the assertions clearly validate the expected partial-but-not-truncated state.
…mantics - Rename parameter from `no_api_key` to `requires_api_key` - Replace `!provider.requires_api_key()` with direct assertion - Update case values to reflect positive semantics
… to remove redundant parameters - Remove duplicate #[case] params expect_partial_key and expect_errors_key - Compute expected field presence from input values instead: - expected_partial_present = partial - expected_errors_present = !errors.is_empty() - Keeps error_count logic and GlobOutput references unchanged
- Pass TodoStatus enum variants as parameters instead of icon strings - Remove reverse mapping match statement and unreachable panic branch - Simplify test assertion to direct comparison
Refactor llm-coding-tools-core tests to use rstest parameterization
1 participant
Summary
Refactors
llm-coding-tools-coretest suite to userstestparameterized tests with descriptive case names and inline documentation. Reduces test function count by ~35% while maintaining equal coverage.Motivation
The core crate had 71 individual test functions across 10 files, many testing similar scenarios with only input variations. This created:
By using
rstestwith#[case::descriptive_name(...)], we can:Key Changes
Files Modified
tools/glob.rstools/grep.rspath/allowed.rspermissions.rstools/todo.rsoutput.rstools/webfetch/blocking_impl.rsinternal/hash64.rsmodels/provider_type.rserror.rsTest Quality Improvements
1. Descriptive Case Names
2. Inline Parameter Comments (aligned)
3. Documented Rationale
Statistics
llm-coding-tools-coreReview Notes