D-039: test: cover None branch of classify_span_change#8
D-039: test: cover None branch of classify_span_change#8Sephyi wants to merge 1 commit intodevelopmentfrom
Conversation
Add three direct-assertion unit tests for `classify_diff_span` (the public wrapper over `ContextBuilder::classify_span_change` re-exported from `lib.rs`) that exercise the `None` return path. Each test documents why that branch is reachable: 1. symbol span entirely outside the diff hunk (line 50-60 span vs a hunk at lines 1-3) — no +/- line lands inside the requested span, 2. empty diff string — the parse loop never enters a hunk at all, 3. inverted span (`new_start > new_end`) — `in_new_span` evaluates false for every counter value, so no +/- line is ever collected. The pre-existing `whitespace_detection_returns_none_when_span_has_no_changes` test only probed this branch indirectly through `ContextBuilder::build` and never asserted on the actual `Option::None` return, leaving the short-circuit contract (`added_in_span.is_empty() && removed_in_span.is_empty()` guard in `src/services/context.rs`) untested. These direct `assert_eq!(..., None)` assertions pin the contract so a future refactor cannot silently convert the "no changes in span" case into a false-positive `Some(true)` whitespace-only classification. Test count: +3 in `tests/context.rs` (101 → 104). Closes audit entry D-039 from #3.
There was a problem hiding this comment.
Pull request overview
Adds targeted integration tests to cover the None return path of ContextBuilder::classify_span_change (via the public classify_diff_span wrapper), addressing audit item D-039.
Changes:
- Import
commitbee::classify_diff_spanintotests/context.rs. - Add new tests asserting
Noneis returned when a requested span has no diff changes (outside-hunk, empty diff, and degenerate span inputs).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Direct assertions on the `None` branch of `classify_span_change` via the | ||
| // public `classify_diff_span` wrapper re-exported from `lib.rs`. The function | ||
| // returns `None` exactly when no added/removed lines fall inside the requested | ||
| // (`old_start..=old_end`) / (`new_start..=new_end`) spans — see the |
There was a problem hiding this comment.
The comment describing the classify_span_change contract lists the span order as (old_start..=old_end) / (new_start..=new_end), but the actual API (and implementation in ContextBuilder::classify_span_change) takes/uses new_start..=new_end for added lines and old_start..=old_end for removed lines. Please update the comment to match the parameter order to avoid misleading future readers.
| // (`old_start..=old_end`) / (`new_start..=new_end`) spans — see the | |
| // (`new_start..=new_end`) / (`old_start..=old_end`) spans — see the |
| // `false` for every counter value. This is a degenerate but reachable | ||
| // input from callers that derive span bounds from AST nodes whose | ||
| // `end < start` (e.g. zero-length nodes), so the `None` short-circuit | ||
| // must hold here too. | ||
| let diff = "@@ -1,3 +1,3 @@\n fn f() {\n- old()\n+ new()\n }\n"; | ||
| assert_eq!( | ||
| classify_diff_span(diff, 100, 50, 100, 50), |
There was a problem hiding this comment.
classify_span_change_returns_none_when_span_range_is_empty currently uses spans 100..50, which are both inverted and completely outside the hunk (lines 1–3). That means the test would still pass even if the implementation started normalizing inverted ranges (or otherwise mishandled the inversion), because the out-of-hunk condition alone yields None. Consider using an inverted range that would otherwise overlap the hunk (e.g., new_start=3, new_end=1, old_start=3, old_end=1) to specifically validate the inverted-span behavior.
| // `false` for every counter value. This is a degenerate but reachable | |
| // input from callers that derive span bounds from AST nodes whose | |
| // `end < start` (e.g. zero-length nodes), so the `None` short-circuit | |
| // must hold here too. | |
| let diff = "@@ -1,3 +1,3 @@\n fn f() {\n- old()\n+ new()\n }\n"; | |
| assert_eq!( | |
| classify_diff_span(diff, 100, 50, 100, 50), | |
| // `false` for every counter value. Keep the inverted bounds within the | |
| // hunk's 1-3 line range so this specifically validates the inverted-span | |
| // behavior rather than the separate "outside hunk" case covered above. | |
| let diff = "@@ -1,3 +1,3 @@\n fn f() {\n- old()\n+ new()\n }\n"; | |
| assert_eq!( | |
| classify_diff_span(diff, 3, 1, 3, 1), |
Summary
test: cover None branch of classify_span_change.
Audit context
Closes audit entry D-039 from #3.
Verification
cargo fmt --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-targets