🧪 Add tests for Range contains and overlaps#126
Conversation
…ices/src/types.rs` Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideAdds focused unit tests for Range::contains and Range::overlaps covering a variety of boundary and overlap scenarios, and fixes an unused variable warning in language path extension handling by actually using the filename binding. Class diagram for Range and Position with contains and overlapsclassDiagram
class Position {
+usize line
+usize byte_column
+usize byte_offset
+new(line usize, byte_column usize, byte_offset usize) Position
}
class Range {
+Position start
+Position end
+new(start Position, end Position) Range
+contains(position Position) bool
+overlaps(other Range) bool
}
Range *-- Position
Flow diagram for Range overlaps evaluationflowchart TD
A[Start overlaps] --> B[Given self and other ranges]
B --> C{self.start <= other.end?}
C -- No --> D[Return false]
C -- Yes --> E{other.start <= self.end?}
E -- No --> D[Return false]
E -- Yes --> F[Return true]
D --> G[End]
F --> G[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
test_range_containsandtest_range_overlapsfunctions are quite verbose and repetitive; consider using a table-driven style (e.g., a list of cases with description, input ranges, and expected result) to make the tests more compact and easier to extend. - You already introduced a
poshelper; adding a smallrangehelper (e.g.,fn range(s: (..), e: (..)) -> Range) could further reduce duplication and improve readability in the overlap tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test_range_contains` and `test_range_overlaps` functions are quite verbose and repetitive; consider using a table-driven style (e.g., a list of cases with description, input ranges, and expected result) to make the tests more compact and easier to extend.
- You already introduced a `pos` helper; adding a small `range` helper (e.g., `fn range(s: (..), e: (..)) -> Range`) could further reduce duplication and improve readability in the overlap tests.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Adds missing unit tests for Range::contains and Range::overlaps (including boundary/abut cases) and removes an unused-variable warning in language extension detection.
Changes:
- Added comprehensive
Rangeunit tests forcontainsandoverlapsedge conditions. - Adjusted
from_extensionto use the filename binding (removing an unused variable warning).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/services/src/types.rs | Adds test module covering Range containment/overlap boundary cases. |
| crates/language/src/lib.rs | Renames _file_name to file_name and updates comparisons to use it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 2. Ending exactly at start (overlap) | ||
| let abut_before = Range::new(pos(1, 0, 10), pos(2, 5, 25)); | ||
| assert!(base.overlaps(&abut_before)); | ||
| assert!(abut_before.overlaps(&base)); |
There was a problem hiding this comment.
The comments say abutting at the boundary is an overlap; that documents an inclusive overlap definition. If the public semantics are intended to be half-open ([start, end)) or treat abutting as non-overlapping, these tests will lock in the wrong contract. Please align the test expectations/comments with the intended Range semantics (and consider a short doc comment on Range::overlaps/Range::contains clarifying inclusivity).
| // 6. Starting exactly at end (overlap) | ||
| let abut_after = Range::new(pos(4, 10, 55), pos(5, 0, 65)); | ||
| assert!(base.overlaps(&abut_after)); | ||
| assert!(abut_after.overlaps(&base)); |
There was a problem hiding this comment.
The comments say abutting at the boundary is an overlap; that documents an inclusive overlap definition. If the public semantics are intended to be half-open ([start, end)) or treat abutting as non-overlapping, these tests will lock in the wrong contract. Please align the test expectations/comments with the intended Range semantics (and consider a short doc comment on Range::overlaps/Range::contains clarifying inclusivity).
Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…aScript structs in crates/ast-engine Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…tructs Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…tructs Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
🎯 What: The testing gap for
Range::containsandRange::overlapshas been addressed.📊 Coverage: Tests now cover edge cases like exactly at start, exactly at end, completely inside, completely outside, overlapping starts, overlapping ends, abutting ranges and non-overlapping ranges.
✨ Result: Test coverage for the
typesmodule improved, ensuringRangeboundary conditions and overlaps are securely evaluated going forward. Additionally removed an unused variable warning incrates/language/src/lib.rs.PR created automatically by Jules for task 10248173612806926714 started by @bashandbone
Summary by Sourcery
Add comprehensive tests for Range boundary behavior and overlaps and clean up an unused variable warning in language support detection.
Bug Fixes:
Tests: