Implementation of Plane integration tests #295
Conversation
|
Also thinking back I could have probably force-push the current state to the old PR that makes the commit log gone but well, it's too late. Also sorry about the probable notifs and the general mess the old PR has generated (I don't know how the Github notification system works but it probably sends them). Regarding the new changes since the last good commit of the old PR, the itest now works with |
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-295 |
godot-core/src/builtin/plane.rs
Outdated
| @@ -217,7 +217,14 @@ impl Plane { | |||
| /// Returns a normalized copy of the plane. | |||
There was a problem hiding this comment.
I know this isn't part of the changeset, but that doc line is really not helpful, as it contains exactly the information of the method and nothing more.
Since you also fixed the implementation, could you maybe describe more precisely what "normalization" in this context means?
itest/rust/src/plane_test.rs
Outdated
| Plane::new(Vector3::UP.normalized(), 0.0), | ||
| Plane::new(Vector3::BACK.normalized(), 0.0), |
There was a problem hiding this comment.
These vectors are unit vectors, no need for normalized().
itest/rust/src/plane_test.rs
Outdated
| #[itest] | ||
| fn plane_equiv() { |
There was a problem hiding this comment.
You have one epic #[itest] of 150 LoC 🙂
Could you extract the local functions to a global level, and maybe split this into multiple #[itest] depending on what you check? That way, tests can succeed/fail independently, and it's easier to detect regressions. Each test also becomes easier to inspect if there is less code.
There was a problem hiding this comment.
Would it be sufficient for just grouped #[itest]s or should every method be given its own #[itest]?
Also, I think issue is also present in rect2i's itest (it's where I got the epic #[itest] from 😄).
There was a problem hiding this comment.
There's no very strict rule. You can group them if things are closely related (e.g. multiple "overloads" or set/get pairs, etc). Otherwise, use multiple ones.
itest/rust/src/plane_test.rs
Outdated
| use godot::{ | ||
| prelude::{inner::InnerPlane, real, Plane, RealConv, ToVariant, Vector3}, | ||
| private::class_macros::assert_eq_approx, | ||
| }; |
There was a problem hiding this comment.
Small nitpick, please use separate lines for use statements without { } nesting.
Also, would be nice to import symbols from their original module, not the prelude.
|
bors try |
tryBuild succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
|
Thanks for reviewing! I'll try sort these out. |
bd20d8f to
67178be
Compare
…in respect to their Godot equivalent
|
For these changes, the itests have been split so each method their own itest. Also, for methods that have two types of results (negative or positive, true or false, etc...) I included two tests and only one for methods that have only one type of result. |
295: Implementation of Plane integration tests r=Bromeon a=T4rmin Adds integration tests to the re-implemented Plane functions (#268, and fixing a function in it) and should also address the Plane section of issue #209. This PR is also a recreation of #286 since it has the great error of force-pushing after rebasing. Co-authored-by: Aaron Lawrence <t4rmin@protonmail.com>
|
Build failed: |
|
bors retry |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Adds integration tests to the re-implemented Plane functions (#268, and fixing a function in it) and should also address the Plane section of issue #209.
This PR is also a recreation of #286 since it has the great error of force-pushing after rebasing.