Merge #896

896: Refactor integration tests by reusing godot_test macro r=Bromeon a=Bromeon

Reduces boilerplate of the form
```rs
fn test_with_certain_name() -> bool {
    println!(" -- test_with_certain_name");

    let ok = std::panic::catch_unwind(|| {
        // actual test code here
    })
    .is_ok();

    if !ok {
        godot_error!("   !! Test test_with_certain_name failed");
    }

    ok
}
```
to
```rs
godot_itest! { test_with_certain_name {
    // actual test code here
}}
```
which reduces repetition and focuses on important things. This is done by slightly adjusting the existing `godot_test` macro to `godot_itest`, which is needed so it can also be used in the `tests` crate.

This change leads to a net removal of 300 lines of code.

---

I was also considering a proc-macro attribute
```rs
#[godot_test]
fn test_with_certain_name() {
    // actual test code here
}
```
which might be a tiny bit nicer syntax-wise, and even started implementing it. But I think the declarative macro does the job, is simpler and likely faster to compile (as we don't need `syn` to tokenize the entire code).

---

Tests of this form now all have the `#[must_use]` attribute, yielding a warning if a boolean test result is ignored.

Co-authored-by: Jan Haller <[email protected]>

Author: bors[bot]

gdnative-core/src/macros.rs

@@ -208,12 +208,15 @@ macro_rules! impl_basic_traits_as_sys {
     )
 }
 
-macro_rules! godot_test {
-    ($($test_name:ident $body:block)*) => {
+#[doc(hidden)]
+#[macro_export]
+macro_rules! godot_test_impl {
+    ( $( $test_name:ident $body:block $($attrs:tt)* )* ) => {
         $(
-            #[cfg(feature = "gd-test")]
+            $($attrs)*
             #[doc(hidden)]
             #[inline]
+            #[must_use]
             pub fn $test_name() -> bool {
                 let str_name = stringify!($test_name);
                 println!("   -- {}", str_name);
@@ -231,3 +234,34 @@ macro_rules! godot_test {
         )*
     }
 }
+
+/// Declares a test to be run with the Godot engine (i.e. not a pure Rust unit test).
+///
+/// Creates a wrapper function that catches panics, prints errors and returns true/false.
+/// To be manually invoked in higher-level test routine.
+///
+/// This macro is designed to be used within the current crate only, hence the #[cfg] attribute.
+#[doc(hidden)]
+macro_rules! godot_test {
+    ($($test_name:ident $body:block)*) => {
+        $(
+            godot_test_impl!($test_name $body #[cfg(feature = "gd-test")]);
+        )*
+    }
+}
+
+/// Declares a test to be run with the Godot engine (i.e. not a pure Rust unit test).
+///
+/// Creates a wrapper function that catches panics, prints errors and returns true/false.
+/// To be manually invoked in higher-level test routine.
+///
+/// This macro is designed to be used within the `test` crate, hence the method is always declared (not only in certain features).
+#[doc(hidden)]
+#[macro_export]
+macro_rules! godot_itest {
+    ($($test_name:ident $body:block)*) => {
+        $(
+            $crate::godot_test_impl!($test_name $body);
+        )*
+    }
+}

impl/proc-macros/src/lib.rs

@@ -12,14 +12,14 @@ mod pool_array_element;
 
 #[proc_macro]
 pub fn impl_typed_array_element(input: TokenStream) -> TokenStream {
-    self::pool_array_element::impl_element(input)
+    pool_array_element::impl_element(input)
         .unwrap_or_else(to_compile_errors)
         .into()
 }
 
 #[proc_macro]
 pub fn decl_typed_array_element(input: TokenStream) -> TokenStream {
-    self::pool_array_element::decl_element(input)
+    pool_array_element::decl_element(input)
         .unwrap_or_else(to_compile_errors)
         .into()
 }

test/Cargo.toml

@@ -17,6 +17,7 @@ custom-godot = ["gdnative/custom-godot"]
 
 [dependencies]
 gdnative = { path = "../gdnative", features = ["gd-test", "serde", "async"] }
+gdnative-core = { path = "../gdnative-core" }
 approx = "0.5"
 ron = "0.7"
 serde = "1"

test/src/lib.rs

@@ -2,6 +2,7 @@
 #![allow(deprecated)]
 
 use gdnative::prelude::*;
+use gdnative_core::godot_itest;
 
 mod test_as_arg;
 mod test_async;
@@ -79,25 +80,14 @@ pub extern "C" fn run_tests(
     status &= test_variant_call_args::run_tests();
     status &= test_variant_ops::run_tests();
 
-    gdnative::core_types::Variant::new(status).leak()
+    Variant::new(status).leak()
 }
 
-fn test_underscore_method_binding() -> bool {
-    println!(" -- test_underscore_method_binding");
-
-    let ok = std::panic::catch_unwind(|| {
-        let script = gdnative::api::NativeScript::new();
-        let result = script._new(&[]);
-        assert_eq!(Variant::nil(), result);
-    })
-    .is_ok();
-
-    if !ok {
-        godot_error!("   !! Test test_underscore_method_binding failed");
-    }
-
-    ok
-}
+godot_itest! { test_underscore_method_binding {
+    let script = gdnative::api::NativeScript::new();
+    let result = script._new(&[]);
+    assert_eq!(Variant::nil(), result);
+}}
 
 #[derive(NativeClass)]
 #[inherit(Reference)]
@@ -152,31 +142,19 @@ impl Foo {
     }
 }
 
-fn test_rust_class_construction() -> bool {
-    println!(" -- test_rust_class_construction");
-
-    let ok = std::panic::catch_unwind(|| {
-        let foo = Foo::new_instance();
+godot_itest! { test_rust_class_construction {
+    let foo = Foo::new_instance();
+    assert_eq!(Ok(42), foo.map(|foo, owner| { foo.answer(&*owner) }));
 
-        assert_eq!(Ok(42), foo.map(|foo, owner| { foo.answer(&*owner) }));
+    let base = foo.into_base();
+    assert_eq!(Some(42), unsafe { base.call("answer", &[]).to() });
 
-        let base = foo.into_base();
-        assert_eq!(Some(42), unsafe { base.call("answer", &[]).to() });
+    let foo = Instance::<Foo, _>::try_from_base(base).expect("should be able to downcast");
+    assert_eq!(Ok(42), foo.map(|foo, owner| { foo.answer(&*owner) }));
 
-        let foo = Instance::<Foo, _>::try_from_base(base).expect("should be able to downcast");
-        assert_eq!(Ok(42), foo.map(|foo, owner| { foo.answer(&*owner) }));
-
-        let base = foo.into_base();
-        assert!(Instance::<NotFoo, _>::try_from_base(base).is_err());
-    })
-    .is_ok();
-
-    if !ok {
-        godot_error!("   !! Test test_rust_class_construction failed");
-    }
-
-    ok
-}
+    let base = foo.into_base();
+    assert!(Instance::<NotFoo, _>::try_from_base(base).is_err());
+}}
 
 #[derive(NativeClass)]
 #[inherit(Reference)]
@@ -205,60 +183,49 @@ impl OptionalArgs {
     }
 }
 
-fn test_from_instance_id() -> bool {
-    println!(" -- test_from_instance_id");
+godot_itest! { test_from_instance_id {
+    assert!(unsafe { Node::try_from_instance_id(22).is_none() });
+    assert!(unsafe { Node::try_from_instance_id(42).is_none() });
+    assert!(unsafe { Node::try_from_instance_id(503).is_none() });
 
-    let ok = std::panic::catch_unwind(|| {
-        assert!(unsafe { Node::try_from_instance_id(22).is_none() });
-        assert!(unsafe { Node::try_from_instance_id(42).is_none() });
-        assert!(unsafe { Node::try_from_instance_id(503).is_none() });
+    let instance_id;
 
-        let instance_id;
+    {
+        let foo = unsafe { Node::new().into_shared().assume_safe() };
+        foo.set_name("foo");
 
-        {
-            let foo = unsafe { Node::new().into_shared().assume_safe() };
-            foo.set_name("foo");
+        instance_id = foo.get_instance_id();
 
-            instance_id = foo.get_instance_id();
-
-            assert!(unsafe { Reference::try_from_instance_id(instance_id).is_none() });
-
-            let reconstructed = unsafe { Node::from_instance_id(instance_id) };
-            assert_eq!("foo", reconstructed.name().to_string());
-
-            unsafe { foo.assume_unique().free() };
-        }
+        assert!(unsafe { Reference::try_from_instance_id(instance_id).is_none() });
 
-        assert!(unsafe { Node::try_from_instance_id(instance_id).is_none() });
+        let reconstructed = unsafe { Node::from_instance_id(instance_id) };
+        assert_eq!("foo", reconstructed.name().to_string());
 
-        let instance_id;
+        unsafe { foo.assume_unique().free() };
+    }
 
-        {
-            let foo = Reference::new().into_shared();
-            let foo = unsafe { foo.assume_safe() };
-            foo.set_meta("foo", "bar");
+    assert!(unsafe { Node::try_from_instance_id(instance_id).is_none() });
 
-            instance_id = foo.get_instance_id();
+    let instance_id;
 
-            assert!(unsafe { Node::try_from_instance_id(instance_id).is_none() });
+    {
+        let foo = Reference::new().into_shared();
+        let foo = unsafe { foo.assume_safe() };
+        foo.set_meta("foo", "bar");
 
-            let reconstructed = unsafe { Reference::from_instance_id(instance_id) };
-            assert_eq!(
-                "bar",
-                String::from_variant(&reconstructed.get_meta("foo")).unwrap()
-            );
-        }
+        instance_id = foo.get_instance_id();
 
-        assert!(unsafe { Reference::try_from_instance_id(instance_id).is_none() });
-    })
-    .is_ok();
+        assert!(unsafe { Node::try_from_instance_id(instance_id).is_none() });
 
-    if !ok {
-        godot_error!("   !! Test test_from_instance_id failed");
+        let reconstructed = unsafe { Reference::from_instance_id(instance_id) };
+        assert_eq!(
+            "bar",
+            String::from_variant(&reconstructed.get_meta("foo")).unwrap()
+        );
     }
 
-    ok
-}
+    assert!(unsafe { Reference::try_from_instance_id(instance_id).is_none() });
+}}
 
 fn init(handle: InitHandle) {
     handle.add_class::<Foo>();

test/src/test_as_arg.rs

@@ -8,20 +8,10 @@ pub(crate) fn register(handle: InitHandle) {
 }
 
 pub(crate) fn run_tests() -> bool {
-    println!(" -- test_as_arg");
+    let mut ok = true;
 
-    let ok = std::panic::catch_unwind(|| {
-        println!("   -- test_ref_as_arg");
-        test_ref_as_arg();
-
-        println!("   -- test_instance_as_arg");
-        test_instance_as_arg();
-    })
-    .is_ok();
-
-    if !ok {
-        godot_error!("   !! Test test_as_arg failed");
-    }
+    ok &= test_as_arg_ref();
+    ok &= test_as_arg_instance();
 
     ok
 }
@@ -40,7 +30,7 @@ impl MyNode {}
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
-fn test_ref_as_arg() {
+crate::godot_itest! { test_as_arg_ref {
     // Ref<T, Unique>
     add_node_with(|n: Ref<Node2D, Unique>| n);
 
@@ -56,9 +46,9 @@ fn test_ref_as_arg() {
 
     // TRef<T, Shared>
     add_node_with(|n: Ref<Node2D, Unique>| unsafe { n.into_shared().assume_safe() });
-}
+}}
 
-fn test_instance_as_arg() {
+crate::godot_itest! { test_as_arg_instance {
     // Instance<T, Unique>
     add_instance_with(|n: Instance<MyNode, Unique>| n);
 
@@ -74,7 +64,7 @@ fn test_instance_as_arg() {
 
     // TInstance<T, Shared>
     add_instance_with(|n: Instance<MyNode, Unique>| unsafe { n.into_shared().assume_safe() });
-}
+}}
 
 fn add_node_with<F, T>(to_arg: F)
 where

test/src/test_constructor.rs

@@ -27,43 +27,32 @@ fn test_constructor() -> bool {
     true
 }
 
-fn test_from_class_name() -> bool {
-    println!(" -- test_from_class_name");
-
-    let ok = std::panic::catch_unwind(|| {
-        // Since this method is restricted to Godot types, there is no way we can detect
-        // here whether any invalid objects are leaked. Instead, the CI script is modified
-        // to look at stdout for any reported leaks.
-
-        let node = Ref::<Node, _>::by_class_name("Node2D").unwrap();
-        assert_eq!("Node2D", node.get_class().to_string());
-        let node = node.cast::<Node2D>().unwrap();
-        assert_eq!("Node2D", node.get_class().to_string());
-        let _ = node.position();
-        node.free();
-
-        let shader = Ref::<Reference, _>::by_class_name("Shader").unwrap();
-        assert_eq!("Shader", &shader.get_class().to_string());
-        let shader = shader.cast::<Shader>().unwrap();
-        assert_eq!("Shader", &shader.get_class().to_string());
-
-        let none = Ref::<Object, _>::by_class_name("Shader");
-        assert!(none.is_none());
-
-        let none = Ref::<Node2D, _>::by_class_name("Spatial");
-        assert!(none.is_none());
-
-        let none = Ref::<Shader, _>::by_class_name("AudioEffectReverb");
-        assert!(none.is_none());
-
-        let none = Ref::<Object, _>::by_class_name("ClassThatDoesNotExistProbably");
-        assert!(none.is_none());
-    })
-    .is_ok();
-
-    if !ok {
-        godot_error!("   !! Test test_from_class_name failed");
-    }
-
-    ok
-}
+crate::godot_itest! { test_from_class_name {
+    // Since this method is restricted to Godot types, there is no way we can detect
+    // here whether any invalid objects are leaked. Instead, the CI script is modified
+    // to look at stdout for any reported leaks.
+
+    let node = Ref::<Node, _>::by_class_name("Node2D").unwrap();
+    assert_eq!("Node2D", node.get_class().to_string());
+    let node = node.cast::<Node2D>().unwrap();
+    assert_eq!("Node2D", node.get_class().to_string());
+    let _ = node.position();
+    node.free();
+
+    let shader = Ref::<Reference, _>::by_class_name("Shader").unwrap();
+    assert_eq!("Shader", &shader.get_class().to_string());
+    let shader = shader.cast::<Shader>().unwrap();
+    assert_eq!("Shader", &shader.get_class().to_string());
+
+    let none = Ref::<Object, _>::by_class_name("Shader");
+    assert!(none.is_none());
+
+    let none = Ref::<Node2D, _>::by_class_name("Spatial");
+    assert!(none.is_none());
+
+    let none = Ref::<Shader, _>::by_class_name("AudioEffectReverb");
+    assert!(none.is_none());
+
+    let none = Ref::<Object, _>::by_class_name("ClassThatDoesNotExistProbably");
+    assert!(none.is_none());
+}}