Add missing tests to #[methods] macro #891

B-head · 2022-05-11T00:38:20Z

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.

Bromeon

This is awesome, thanks a lot for your efforts! 👍

Added some comments.

test/src/test_derive.rs

Bromeon · 2022-05-13T20:47:14Z

test/src/test_derive.rs

+#[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
+}


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.

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.

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.

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.

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.

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

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.

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.

test/src/test_derive.rs

Bromeon

Just minor nitpicks 🙂

test/src/test_derive.rs

B-head · 2022-05-15T00:55:09Z

My one previous fix was careless. Really fixed. 🐤

Bromeon · 2022-05-15T07:09:11Z

test/src/test_derive.rs

+
+#[derive(NativeClass)]
+#[inherit(Reference)]
+struct WithoutBasePrameter(i64);


Bromeon · 2022-05-15T07:10:30Z

test/src/test_derive.rs

+
+    let ok = std::panic::catch_unwind(|| {
+        let thing = Instance::<MinimalDerive, _>::new();
+        let base = thing.into_base();


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.

It meant for the base variable, it should probably be:

let base: Ref<Reference, Unique> = thing.into_base();

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.

Bromeon · 2022-05-15T07:14:21Z

test/src/test_derive.rs

+#[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
+}


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.

Bromeon

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.

Bromeon · 2022-05-18T07:52:09Z

test/src/test_derive.rs

+
+    let ok = std::panic::catch_unwind(|| {
+        let thing = Instance::<MinimalDerive, _>::new();
+        let base = thing.into_base();


It meant for the base variable, it should probably be:

let base: Ref<Reference, Unique> = thing.into_base();

B-head · 2022-05-18T09:03:07Z

It looks like Godot tests currently cause a memory leak:

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 😣)

Bromeon · 2022-05-18T11:15:55Z

CI is always passing on master, so it's specifically this pull request which introduces (or at least triggers) the memory leak 😉
And bors also prevents merging as long as checks don't pass.

I'll run the full suite and see if we get it in all Godot tests.
bors try

What is strange is that every type inherits Reference, so the memory should be cleaned up automatically. Probably we overlooked something...

bors · 2022-05-18T11:27:07Z

try

Build failed:

full-ci

B-head · 2022-05-18T16:01:43Z

CI is always passing on master, so it's specifically this pull request which introduces (or at least triggers) the memory leak 😉

This seems to have occurred at least three months ago in CI.
https://github.com/godot-rust/godot-rust/runs/5280455202?check_suite_focus=true#step:3:1000

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)

And bors also prevents merging as long as checks don't pass.

That's because I wrote code that fails the test as I proclaimed. 🙄
https://github.com/godot-rust/godot-rust/runs/6483628213?check_suite_focus=true#step:3:576

Bromeon · 2022-05-18T22:07:03Z

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

bors · 2022-05-18T22:16:03Z

try

Build succeeded:

full-ci

Bromeon · 2022-05-19T08:06:38Z

The only thing left is #891 (comment), after that I can merge!

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.

B-head · 2022-05-19T15:33:27Z

I would rename GodotAttrOnMacroParameters to RenameAndDerefReturn

That test is checking the working of the high-level code, not the rename or deref-return code.

Bromeon · 2022-05-19T22:19:05Z

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 GodotAttrOnBaseParameter nor GodotAttrOnMacroParameters is really descriptive. What may be obvious to you now, may raise questions in half a year as to what exactly is being tested.

I suggest the following:

Rename GodotAttrOnBaseParameter -> WithBaseParameter, so it's clear this is the counterpart to WithoutBaseParameter.
Rename GodotAttrOnMacroParameters -> MacroAttributeSyntax.
- If I understand you correctly, this only tests whether all macro attribute arguments are syntactically correct.
- Their semantics are verified in separate tests.

In addition to the renames, maybe you could add 1 line of comment above MacroAttributeSyntax:

// Tests that arguments to the #[godot] attribute are syntactically accepted.
struct MacroAttributeSyntax {

Also, would be nice if you could squash your commits into a single commit 🙂
(let me know if you need help with that!)

B-head · 2022-05-20T18:20:00Z

this only tests whether all macro attribute arguments are syntactically correct.

If it were that simple, development would be easy, but it is not.

Bromeon · 2022-05-20T18:56:37Z

this only tests whether all macro attribute arguments are syntactically correct.

If it were that simple, development would be easy, but it is not.

So it's not the syntax we're testing.
Further up, you mentioned:

[Bromeon] So how do we decide which argument combinations we want to test?

[B-head] Typical combinations, Combinations that may have bugs, and Most complex combinations.

But then, I suggested to name the test so it mentions the combined attributes -- to which you replied, those would not be tested:

[Bromeon] I would rename GodotAttrOnMacroParameters to RenameAndDerefReturn

[B-head] That test is checking the working of the high-level code, not the rename or deref-return code.

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. GodotAttrOnMacroParameters does not satisfy that, in my opinion.

I suggested several alternative names. If you're not happy with them, please suggest something else! 🙂

B-head · 2022-05-20T19:24:34Z

"Working of the high-level code" is quite vague, could you elaborate this?

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.

Either way, my main point is descriptive naming -- everyone who looks at the tests (not only you and me) should understand their purpose. GodotAttrOnMacroParameters does not satisfy that, in my opinion.

I suggested several alternative names. If you're not happy with them, please suggest something else! 🙂

My naming convention is "group of a feature" + "context of use". That is, GodotAttr + OnMacroParameters.

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.

Bromeon · 2022-05-20T20:08:49Z

Tests against code that will be added or changed in the future. And we cannot predict what will be added or changed.

Makes sense, thanks!

Currently, the preferred naming convention for this project is only in your mind.

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):

MinimalDerive, WithoutInherit
- Those are good because they refer to NativeClass, which anyway applies to all these tests (and the test names are already prefixed with test_derive_nativeclass)
WithoutBaseParameter -> GodotAttrWithoutBase
- group = "GodotAttr", context = "WithoutBase"
GodotAttrOnBaseParameter -> GodotAttrWithBase
GodotAttrOnMacroParameters -> GodotAttrAllArguments
- rpc, name etc. are technically arguments to the proc-macro attribute #[godot]
RenameMethod, DerefReturn. Two options, I'm fine with both:
- they are good now if you consider them "functionality of NativeClass" and not specifically "#[godot] attribute tests"
- alternatively, you name them GodotAttrName + GodotAttrDerefReturn based on logic above

Does that seem reasonable? 💡

I saw that name and deref_return tests still use #[export], should we change this to #[godot], so the tests remain valid in the future?

B-head · 2022-05-20T20:35:25Z

I think it's good. 👍

Only GodotAttrOnMacroParameters contains the context of use, so I would like to change the name to GodotAttrToUseAllMacroParameters, is this ok?

Bromeon

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!

Bromeon · 2022-05-20T21:14:56Z

test/src/test_derive.rs

+}
+
+fn test_derive_nativeclass_godot_attr_with_base() -> bool {
+    println!(" -- test_derive_nativeclass_godot_attr_on_base_parameter");


These strings in println! and godot_error are outdated again 🙈
Same with the other names you updated.

- Minimal derive - Without inherit - Without base prameter - Deref return - Rename method - `#[godot]` attribute

Bromeon · 2022-05-20T21:28:13Z

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>

bors · 2022-05-20T21:37:27Z

Build failed:

full-ci

Bromeon · 2022-05-20T21:46:37Z

To no one's surprise, CI wasn't happy.
It failed in task test-linux (minimal-deps) -- if this keeps happening, I'll disable UI tests for that one, like I already did for other "non-stable" configurations.

bors r+

bors · 2022-05-20T21:57:30Z

Build succeeded:

full-ci

Bromeon reviewed May 13, 2022

View reviewed changes

Bromeon added c: export Component: export (mod export, derive) quality-of-life No new functionality, but improves ergonomics/internals labels May 13, 2022

Bromeon added this to the v0.10.1 milestone May 13, 2022

B-head force-pushed the derive_tests branch from cb5f84c to 8990125 Compare May 13, 2022 23:38

Bromeon reviewed May 14, 2022

View reviewed changes

B-head force-pushed the derive_tests branch from 8990125 to 0ac88f6 Compare May 15, 2022 00:46

Bromeon reviewed May 15, 2022

View reviewed changes

B-head force-pushed the derive_tests branch from 0ac88f6 to 1069c85 Compare May 18, 2022 05:45

Bromeon reviewed May 18, 2022

View reviewed changes

bors bot added a commit that referenced this pull request May 18, 2022

Try #891:

d91a1fb

B-head force-pushed the derive_tests branch from 1069c85 to 2416a7e Compare May 18, 2022 16:35

B-head mentioned this pull request May 18, 2022

UB at GDNative termination (seen during integration tests) #894

Closed

bors bot added a commit that referenced this pull request May 18, 2022

Try #891:

4ab98ac

B-head force-pushed the derive_tests branch from 2416a7e to 7629394 Compare May 20, 2022 17:59

B-head force-pushed the derive_tests branch from 7629394 to 34ab940 Compare May 20, 2022 21:00

Bromeon reviewed May 20, 2022

View reviewed changes

Add missing tests to #[methods] macro

028e48b

- Minimal derive - Without inherit - Without base prameter - Deref return - Rename method - `#[godot]` attribute

B-head force-pushed the derive_tests branch from 34ab940 to bf53691 Compare May 20, 2022 21:22

Update trybuild tests for Rust 1.61.0

30385bb

B-head force-pushed the derive_tests branch from bf53691 to 30385bb Compare May 20, 2022 21:42

bors bot merged commit c803753 into godot-rust:master May 20, 2022

B-head deleted the derive_tests branch May 20, 2022 21:58

Add missing tests to #[methods] macro #891

Add missing tests to #[methods] macro #891

Conversation

B-head commented May 11, 2022

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

B-head commented May 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Bromeon May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon May 18, 2022 • edited Loading

Choose a reason for hiding this comment

B-head commented May 18, 2022

Bromeon commented May 18, 2022 • edited Loading

bors bot commented May 18, 2022

try

B-head commented May 18, 2022 • edited Loading

Bromeon commented May 18, 2022

bors bot commented May 18, 2022

try

Bromeon commented May 19, 2022

B-head commented May 19, 2022 • edited Loading

Bromeon commented May 19, 2022 • edited Loading

B-head commented May 20, 2022

Bromeon commented May 20, 2022 • edited Loading

B-head commented May 20, 2022

Bromeon commented May 20, 2022

B-head commented May 20, 2022

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented May 20, 2022

bors bot commented May 20, 2022

Bromeon commented May 20, 2022

bors bot commented May 20, 2022

Bromeon May 13, 2022 •

edited

Loading

Bromeon May 18, 2022 •

edited

Loading

Bromeon May 18, 2022 •

edited

Loading

Bromeon commented May 18, 2022 •

edited

Loading

B-head commented May 18, 2022 •

edited

Loading

B-head commented May 19, 2022 •

edited

Loading

Bromeon commented May 19, 2022 •

edited

Loading

Bromeon commented May 20, 2022 •

edited

Loading

Bromeon left a comment •

edited

Loading