Add unit tests for grid and polygon shapes, and enhance star shape tests#3955
Add unit tests for grid and polygon shapes, and enhance star shape tests#3955tarone-saloni wants to merge 1 commit intoGraphiteEditor:masterfrom
Conversation
Signed-off-by: Saloni Tarone <taronesaloni@gmail.com>
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 robustness of the editor's shape tools by adding extensive unit test coverage for grid, polygon, and star shapes. These tests validate core drawing logic, modifier key interactions, and cancellation behavior, ensuring greater stability and preventing regressions in future development. 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
|
|
/gemini summary |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new test cases for grid_shape.rs, polygon_shape.rs, and star_shape.rs to validate their drawing and parameter calculation logic, including aspects like ratio locking, centering, negative drags, and isometric modes. The review comments suggest improvements such as using glam's abs_diff_eq for more concise floating-point vector comparisons, reducing the tolerance for floating-point assertions in tests to improve precision, and correcting a misleading test name for polygon default sides.
| assert!((translation.x - 10.).abs() < 1e-10, "Expected translation.x=10, got {}", translation.x); | ||
| assert!((translation.y - 10.).abs() < 1e-10, "Expected translation.y=10, got {}", translation.y); |
There was a problem hiding this comment.
For comparing floating-point vectors, it's cleaner to use abs_diff_eq from the glam crate. This allows you to assert the near-equality of the entire DVec2 in a single line, making the test more concise and readable.
| assert!((translation.x - 10.).abs() < 1e-10, "Expected translation.x=10, got {}", translation.x); | |
| assert!((translation.y - 10.).abs() < 1e-10, "Expected translation.y=10, got {}", translation.y); | |
| assert!(translation.abs_diff_eq(DVec2::splat(10.), 1e-9), "Expected translation to be close to (10, 10), got {}", translation); |
| assert_eq!(editor.active_document().metadata().all_layers().count(), 1); | ||
| let (sides, radius) = get_polygon_inputs(&editor).expect("Polygon node should exist after draw"); | ||
| assert!(sides >= 3, "Polygon should have at least 3 sides, got {sides}"); | ||
| assert!((radius - 50.).abs() < 1., "Expected radius ≈ 50 for 100×100 drag, got {radius}"); |
There was a problem hiding this comment.
The tolerance of 1.0 for this floating-point comparison seems quite large. While some tolerance is expected in integration tests due to floating-point arithmetic, a value this high might mask potential inaccuracies. Consider using a smaller tolerance, like 1e-9, to ensure the calculation is as precise as expected. This also applies to other similar assertions in this test module.
| assert!((radius - 50.).abs() < 1., "Expected radius ≈ 50 for 100×100 drag, got {radius}"); | |
| assert!((radius - 50.).abs() < 1e-9, "Expected radius ≈ 50 for 100×100 drag, got {radius}"); |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn polygon_default_six_sides() { |
There was a problem hiding this comment.
The test name polygon_default_six_sides is misleading as the test asserts that the default number of sides is 5. To improve clarity and prevent confusion, the test name should be updated to reflect the actual assertion.
| async fn polygon_default_six_sides() { | |
| async fn polygon_default_five_sides() { |
| draw_star(&mut editor, 0., 0., 100., 100., ModifierKeys::empty()).await; | ||
|
|
||
| let (_, outer_radius, _) = get_star_inputs(&editor).expect("Star node should exist"); | ||
| assert!((outer_radius - 50.).abs() < 1., "Expected outer radius ~50 for 100x100 drag, got {outer_radius}"); |
There was a problem hiding this comment.
The tolerance of 1.0 for this floating-point comparison seems quite large. While some tolerance is expected in integration tests, a value this high might mask potential inaccuracies. Consider using a smaller tolerance, like 1e-9, to ensure the calculation is as precise as expected.
| assert!((outer_radius - 50.).abs() < 1., "Expected outer radius ~50 for 100x100 drag, got {outer_radius}"); | |
| assert!((outer_radius - 50.).abs() < 1e-9, "Expected outer radius ~50 for 100x100 drag, got {outer_radius}"); |
Summary of ChangesThis pull request significantly enhances the stability and reliability of the editor's shape tools by introducing extensive unit test coverage for grid, polygon, and star shapes. The new tests validate core drawing logic, interactions with modifier keys, radius calculations, and cancellation behaviors, ensuring that these fundamental functionalities work as expected and preventing future regressions. Highlights
Changelog
Activity
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 3 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
1 participant
#[cfg(test)]coverage to three previously untested files in the tool system.Tests cover polygon/star draw, modifier keys (Shift/Alt), RMB cancel, radius correctness,
and pure unit tests for grid spacing math in
calculate_grid_params.Fixed #3954