-
Notifications
You must be signed in to change notification settings - Fork 210
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
Add missing tests to #[methods] macro #891
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks a lot for your efforts! 👍
Added some comments.
test/src/test_derive.rs
Outdated
#[derive(NativeClass)] | ||
#[inherit(Reference)] | ||
struct MethodOnMacroParameters(i64); | ||
|
||
#[methods] | ||
impl MethodOnMacroParameters { | ||
fn new(_owner: &Reference) -> Self { | ||
Self(54) | ||
} | ||
|
||
#[godot(name = "ask", deref_return)] | ||
fn answer(&self, #[base] _base: &Reference) -> &i64 { | ||
&self.0 | ||
} | ||
} | ||
|
||
fn test_derive_nativeclass_method_on_macro_parameters() -> bool { | ||
println!(" -- test_derive_nativeclass_method_on_macro_parameters"); | ||
|
||
let ok = std::panic::catch_unwind(|| { | ||
let thing = Instance::<MethodOnMacroParameters, _>::new(); | ||
let base = thing.into_base(); | ||
assert_eq!(Some(54), unsafe { base.call("ask", &[]).to::<i64>() }); | ||
}) | ||
.is_ok(); | ||
|
||
if !ok { | ||
godot_error!(" !! Test test_derive_nativeclass_method_on_macro_parameters failed"); | ||
} | ||
|
||
ok | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is being tested here?
There seem to be separate tests for name
and deref_return
, and I wouldn't try to test attribute combinations, as that will explode exponentially.
If it's only about a method having attributes, this is implicitly covered by other tests, so I would remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other test is for #[export]
, so we are testing that #[godot]
also accepts parameters.
But originally, it might be better to test with trybuild.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. #[export]
is deprecated though, so there's no need to add more tests. The #[export(deref_return)]
is not even a documented feature, it just happens to work because we didn't exclude it in our implementation.
I would test #[godot(name)]
and #[godot(deref_return)]
, but separately. No combined tests or more #[export]
tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an integration test, we need combination tests that assumes the actual code.
Also, it helps to identify buggy code by seeing which tests are failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what is our strategy here? This will quickly lead to combinatorial explosion. So how do we decide which argument combinations we want to test?
Also, it helps to identify buggy code by seeing which tests are failing.
I would argue testing each feature individually makes it clearer which parts are broken. If the combined test fails, it can be either the of the two parts who have a bug, or only the combination of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how do we decide which argument combinations we want to test?
Typical combinations, Combinations that may have bugs, and Most complex combinations.
For example, suppose there are individual tests test_a
and test_b
and a combination test_a_and_b
.
If the test has this result, there is a bug in code tested with test_a
:
test_a -- fail
test_b -- ok
test_a_and_b -- fail
If the test has this result, there is a bug in Higher-level code:
test_a -- ok
test_b -- ok
test_a_and_b -- fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's keep the number of "combined attribute tests" small however (now is good). We can always add more tests if we see that a certain combination causes problems.
In that case, I would rename GodotAttrOnMacroParameters
to RenameAndDerefReturn
, and move it after both RenameMethod
and DerefReturn
tests. It makes sense that the single attribute tests are run prior to the combinations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll change to only fail godot_attr_on_macro_parameters()
, so if you see that and want to remove the test, do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor nitpicks 🙂
My one previous fix was careless. Really fixed. 🐤 |
test/src/test_derive.rs
Outdated
|
||
#[derive(NativeClass)] | ||
#[inherit(Reference)] | ||
struct WithoutBasePrameter(i64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
test/src/test_derive.rs
Outdated
|
||
let ok = std::panic::catch_unwind(|| { | ||
let thing = Instance::<MinimalDerive, _>::new(); | ||
let base = thing.into_base(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should explictly specify the type of variable base
here?
That would additionally test that Reference
is really chosen as the base, and not e.g. Object
.
Same for other tests.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It meant for the base
variable, it should probably be:
let base: Ref<Reference, Unique> = thing.into_base();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry. I did not understand.
I'll do that for now, but, I think it is trybuild
oriented test case because the test is to see if the macro causes compile errors.
test/src/test_derive.rs
Outdated
#[derive(NativeClass)] | ||
#[inherit(Reference)] | ||
struct MethodOnMacroParameters(i64); | ||
|
||
#[methods] | ||
impl MethodOnMacroParameters { | ||
fn new(_owner: &Reference) -> Self { | ||
Self(54) | ||
} | ||
|
||
#[godot(name = "ask", deref_return)] | ||
fn answer(&self, #[base] _base: &Reference) -> &i64 { | ||
&self.0 | ||
} | ||
} | ||
|
||
fn test_derive_nativeclass_method_on_macro_parameters() -> bool { | ||
println!(" -- test_derive_nativeclass_method_on_macro_parameters"); | ||
|
||
let ok = std::panic::catch_unwind(|| { | ||
let thing = Instance::<MethodOnMacroParameters, _>::new(); | ||
let base = thing.into_base(); | ||
assert_eq!(Some(54), unsafe { base.call("ask", &[]).to::<i64>() }); | ||
}) | ||
.is_ok(); | ||
|
||
if !ok { | ||
godot_error!(" !! Test test_derive_nativeclass_method_on_macro_parameters failed"); | ||
} | ||
|
||
ok | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's keep the number of "combined attribute tests" small however (now is good). We can always add more tests if we see that a certain combination causes problems.
In that case, I would rename GodotAttrOnMacroParameters
to RenameAndDerefReturn
, and move it after both RenameMethod
and DerefReturn
tests. It makes sense that the single attribute tests are run prior to the combinations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like Godot tests currently cause a memory leak:
WARNING: ObjectDB instances leaked at exit (run with --verbose for details).
at: cleanup (core/object.cpp:2064)
ERROR: Resources still in use at exit (run with --verbose for details).
at: clear (core/resource.cpp:417)
Error: Process completed with exit code 1.
test/src/test_derive.rs
Outdated
|
||
let ok = std::panic::catch_unwind(|| { | ||
let thing = Instance::<MinimalDerive, _>::new(); | ||
let base = thing.into_base(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It meant for the base
variable, it should probably be:
let base: Ref<Reference, Unique> = thing.into_base();
If it happens in testing, it will happen in the production environment, so I think we should open the issue with it. (Bug fixes are out of scope for this PR 😣) |
CI is always passing on I'll run the full suite and see if we get it in all Godot tests. What is strange is that every type inherits |
tryBuild failed: |
This seems to have occurred at least three months ago in CI. And I could not reproduce the bug in my environment. Instead, the external terminal causes the segmentation fault. 😵 ... Undefined behavior! 🎲 (I was going to open the issue after I narrowed down the code that caused UB)
That's because I wrote code that fails the test as I proclaimed. 🙄 |
You're completely right, I misinterpreted the CI output. I thought we parse output for memory leaks and cause failure, but seems not. Thanks for the clarification! So I agree that the leak is out of scope for this and we can look at it separately. bors try |
tryBuild succeeded: |
The only thing left is #891 (comment), after that I can merge!
|
That test is checking the working of the high-level code, not the rename or deref-return code. |
Okay, I think I know what you mean. The thing is, neither I suggest the following:
Also, would be nice if you could squash your commits into a single commit 🙂 |
If it were that simple, development would be easy, but it is not. |
So it's not the syntax we're testing.
But then, I suggested to name the test so it mentions the combined attributes -- to which you replied, those would not be tested:
So... I honestly don't know what the purpose is. "Working of the high-level code" is quite vague, could you elaborate this? Either way, my main point is descriptive naming -- everyone who looks at the tests (not only you and me) should understand their purpose. I suggested several alternative names. If you're not happy with them, please suggest something else! 🙂 |
That is really vague. Tests against code that will be added or changed in the future. And we cannot predict what will be added or changed.
My naming convention is "group of a feature" + "context of use". That is, Currently, the preferred naming convention for this project is only in your mind. I do not want to repeat these questions and answers, so please define the preferred naming convention. |
Makes sense, thanks!
There's some truth in that, these things are nowhere formally specified. I'm not sure though how far we want to drive bureaucracy and write everything down... My experience is that most people wouldn't read it anyway 😉 so I try to go the automation way where possible (e.g. clippy), but of course there are code-style issues which are more subjective. Most important is that the relation between tests in the same file are clear. I had a look at 5 tests and highlighted how they differ from the previous (maybe open the image in a separate tab to see side-by-side): So I'll forget everything we previously discussed and try to apply your "group of a feature" + "context of use" convention (which I consider good idea):
Does that seem reasonable? 💡 I saw that |
I think it's good. 👍 Only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically not macro parameters, but macro attribute parameters (or arguments at call site) -- but it's definitely understandable enough now and I don't want to add even more cosmetic work. So feel free to leave it!
test/src/test_derive.rs
Outdated
} | ||
|
||
fn test_derive_nativeclass_godot_attr_with_base() -> bool { | ||
println!(" -- test_derive_nativeclass_godot_attr_on_base_parameter"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These strings in println!
and godot_error
are outdated again 🙈
Same with the other names you updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍌 🚶
- Minimal derive - Without inherit - Without base prameter - Deref return - Rename method - `#[godot]` attribute
Looks good now! Thanks a lot for all the patience, and also updating trybuild for Rust 1.61 (I hope CI is happy with that 🙂). bors r+ |
891: Add missing tests to #[methods] macro r=Bromeon a=B-head Added tests for the following features. - Minimal derive - Omitted inherit - Omitted base prameter - Method attribute - Deref return - Rename method I could not add a test for Rpc mode. We need a way to add a node to the scene tree during a test. Co-authored-by: B_head <b_head@outlook.jp>
Build failed: |
To no one's surprise, CI wasn't happy. bors r+ |
Build succeeded: |
Added tests for the following features.
I could not add a test for Rpc mode. We need a way to add a node to the scene tree during a test.