Skip to content

Add tests for Spline tool to support continuing a previously drawn spline #2591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 18, 2025

Conversation

Sidharth-Singh10
Copy link
Contributor

Closes part of #2483

@Keavon Keavon added the Testing Unit and integration tests label Apr 17, 2025
@Keavon Keavon mentioned this pull request Apr 17, 2025
7 tasks
@Keavon
Copy link
Member

Keavon commented Apr 17, 2025

If you were planning to continue adding more tests from #2483, please don't start any new ones until #2368 is merged.

Please also avoid opening too many PRs for small sub-tasks in one of those issues that have multiple bullet points, just add them all to the same PR if you can and they're related. It adds a significant amount of overhead dealing with multiple PRs.

@Sidharth-Singh10
Copy link
Contributor Author

Noted. Shall I close this one or keep it as draft?

@Keavon
Copy link
Member

Keavon commented Apr 17, 2025

Please finish this one, but after it's ready to merge, don't work on new Spline tool things until after that other issue lands.

@Sidharth-Singh10 Sidharth-Singh10 marked this pull request as ready for review April 17, 2025 19:34
Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far.

@Sidharth-Singh10
Copy link
Contributor Author

@0HyperCube I've refactored the redundant code across the three open PRs.
The assert_point_positions function is reusable and can be shared across tests in all tools.

Would it make sense to include this in EditorTestUtils, or do you suggest a more appropriate place for it?

Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; thanks for simplifying the test code.
I think the assert_point_positions is fine in the spline tool as it is useful for that tool only.

@0HyperCube 0HyperCube enabled auto-merge (squash) April 18, 2025 22:16
@0HyperCube 0HyperCube merged commit 73d12bc into GraphiteEditor:master Apr 18, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing Unit and integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants