Improved and Added unit tests for untested tool and editor modules#3948
Improved and Added unit tests for untested tool and editor modules#3948Chetansahney wants to merge 9 commits intoGraphiteEditor:masterfrom
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…nsahney/Graphite into fix-shape-handle-mirroring
… rectangle, and polygon
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the test coverage for the editor's core functionalities, focusing on robust snapping behavior and reliable shape drawing. It also enhances the usability of generated vector shapes by linking their control handles, making them more intuitive to edit. These changes aim to prevent regressions and ensure a stable user experience for fundamental drawing operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly improves test coverage by adding unit tests for snapping logic and integration tests for shape tools. The changes are generally good, but I've found a few areas for improvement.
Most notably, there's a copy-paste error where polygon tests were added to the rectangle shape file. I've also identified some code duplication in generator_nodes.rs where similar logic for linking manipulator handles could be extracted into helper functions to improve maintainability.
The new tests for snapping logic are comprehensive and well-written.
| // Created the collinear_manipulators so that all handles are linked, making it easier to edit the circle as a circle instead of a 4 point shape. | ||
| let ids = circle.segment_domain.ids(); | ||
| let len = ids.len(); | ||
| for i in 0..len { | ||
| circle.colinear_manipulators.push([ | ||
| HandleId::end(ids[i]), | ||
| HandleId::primary(ids[(i + 1) % len]), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
This logic for linking handles on a closed path is also present in the ellipse function (lines 163-167). To improve maintainability and avoid code duplication, consider extracting this logic into a private helper function within this module.
For example:
fn link_closed_path_handles(vector: &mut Vector) {
let ids = vector.segment_domain.ids();
let len = ids.len();
for i in 0..len {
vector.colinear_manipulators.push([
HandleId::end(ids[i]),
HandleId::primary(ids[(i + 1) % len]),
]);
}
}You could then call this helper here and in the ellipse function.
There was a problem hiding this comment.
3 issues found across 5 files
Confidence score: 4/5
- This PR is likely safe to merge, but there is mild regression-risk because multiple tests are not asserting the documented defaults, so behavior changes could slip through undetected.
- Most severe issue: in
editor/src/messages/tool/common_functionality/shapes/rectangle_shape.rs, new tests validate regular polygon cases rather than rectangle-specific behavior, leaving rectangle logic effectively untested. - In
editor/src/messages/tool/common_functionality/shapes/polygon_shape.rs,polygon_draw_simpleonly checks>= 3sides, which would not catch a wrong default side count. - Pay close attention to
editor/src/messages/tool/common_functionality/shapes/rectangle_shape.rsandeditor/src/messages/tool/common_functionality/shapes/polygon_shape.rs- tighten assertions to exact documented defaults and rectangle-specific expectations.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/tool/common_functionality/shapes/polygon_shape.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/shapes/polygon_shape.rs:232">
P2: `polygon_draw_simple` does not verify the actual default polygon side count; it only checks `>= 3`, so default-side regressions can pass undetected.</violation>
</file>
<file name="editor/src/messages/tool/common_functionality/shapes/rectangle_shape.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/shapes/rectangle_shape.rs:74">
P2: Added tests in `rectangle_shape.rs` validate regular polygons, not rectangle behavior, leaving rectangle logic in this module untested.</violation>
<violation number="2" location="editor/src/messages/tool/common_functionality/shapes/rectangle_shape.rs:94">
P2: Regression test does not actually verify the documented default polygon vertex count.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fixes #3936
I noticed our test coverage in editor/src/ was sitting around 11%, so I’ve added 22 new tests to shore up some of the more regression-prone areas - specifically the snapping logic and basic shape drawing.
What I added:
Snapping Logic (snap_results.rs): Wrote unit tests to verify the other_snap_better logic. This ensures we correctly prioritize constrained snaps over alignment guides and handle infinite distances without crashing.
Shape Tools (rectangle_shape.rs & polygon_shape.rs): Added integration tests using EditorTestUtils. These verify that drawing still works as expected when using modifiers like SHIFT (lock ratio), ALT (draw from center), and RMB (cancel).
Utility Types: Added coverage for MessageData and DebugMessageTree to ensure our internal message metadata stays consistent