Make itests use expect and expect_err for fallible conversion testing

lilizoey · lilizoey · commit 5bd998477e68 · 2023-11-14T14:23:53.000+01:00
Add some tests to ensure conversion works as expected
Document things
diff --git a/godot-codegen/src/central_generator.rs b/godot-codegen/src/central_generator.rs
@@ -596,17 +596,6 @@ fn make_sys_code(central_items: &CentralItems) -> TokenStream {
             }
         }
 
-        impl std::fmt::Display for VariantType {
-            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-                match self {
-                    Self::Nil => f.write_str("Nil"),
-                    #(
-                        Self::#variant_ty_enumerators_pascal => f.write_str(stringify!(#variant_ty_enumerators_pascal)),
-                    )*
-                }
-            }
-        }
-
         // ----------------------------------------------------------------------------------------------------------------------------------------------
 
         #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)]
diff --git a/godot-core/src/builtin/meta/godot_convert/convert_error.rs b/godot-core/src/builtin/meta/godot_convert/convert_error.rs
@@ -4,15 +4,16 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use std::error::Error as StdError;
+use std::error::Error;
 use std::fmt;
 
 use godot_ffi::VariantType;
 
 use crate::builtin::{array_inner, meta::ClassName};
 
-type Cause = Box<dyn StdError + Send + Sync + 'static>;
+type Cause = Box<dyn Error + Send + Sync>;
 
+/// Represents errors that can occur when converting values from Godot.
 #[derive(Debug)]
 pub struct ConvertError {
     kind: ErrorKind,
@@ -21,7 +22,7 @@ pub struct ConvertError {
 }
 
 impl ConvertError {
-    /// Create a new error for conversion of type `T`.
+    /// Create a new error for a conversion.
     fn new(kind: ErrorKind) -> Self {
         Self {
             kind,
@@ -30,8 +31,9 @@ impl ConvertError {
         }
     }
 
-    pub fn custom<C: Into<Cause>>(cause: C) -> Self {
-        Self::new(ErrorKind::Custom).with_cause(cause)
+    /// Create a new custom error for a conversion.
+    pub fn custom() -> Self {
+        Self::new(ErrorKind::Custom)
     }
 
     /// Add a rust-error as an underlying cause for the conversion error.
@@ -40,12 +42,22 @@ impl ConvertError {
         self
     }
 
+    /// Returns the rust-error that caused this error, if one exists.
+    pub fn cause(&self) -> Option<&(dyn Error + Send + Sync)> {
+        self.cause.as_deref()
+    }
+
     /// Add the value that failed to be converted.
     pub fn with_value<V: fmt::Debug + 'static>(mut self, value: V) -> Self {
         self.value = Some(Box::new(value));
         self
     }
 
+    /// Returns the value that failed to convert, if one exists.
+    pub fn value(&self) -> Option<&(dyn fmt::Debug)> {
+        self.value.as_deref()
+    }
+
     fn description(&self) -> Option<String> {
         self.kind.description()
     }
@@ -68,11 +80,11 @@ impl fmt::Display for ConvertError {
     }
 }
 
-impl StdError for ConvertError {
-    fn source(&self) -> Option<&(dyn StdError + 'static)> {
+impl Error for ConvertError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
         self.cause
             .as_ref()
-            .map(|cause| &**cause as &(dyn StdError + 'static))
+            .map(|cause| &**cause as &(dyn Error + 'static))
     }
 }
 
@@ -115,13 +127,13 @@ impl FromGodotError {
                 if expected.variant_type() != got.variant_type() {
                     if expected.is_typed() {
                         return format!(
-                            "expected array of type {}, got array of type {}",
+                            "expected array of type {:?}, got array of type {:?}",
                             expected.variant_type(),
                             got.variant_type()
                         );
                     } else {
                         return format!(
-                            "expected untyped array, got array of type {}",
+                            "expected untyped array, got array of type {:?}",
                             got.variant_type()
                         );
                     }
@@ -209,7 +221,7 @@ impl FromVariantError {
     fn description(&self) -> String {
         match self {
             Self::BadType { expected, got } => {
-                format!("expected Variant of type `{expected}` but got Variant of type `{got}`")
+                format!("expected Variant of type `{expected:?}` but got Variant of type `{got:?}`")
             }
             Self::WrongClass { expected } => {
                 format!("got variant of wrong class, expected class `{expected}`")
diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs
@@ -12,7 +12,6 @@ use sys::{ffi_methods, interface_fn, GodotFfi};
 
 mod impls;
 
-pub use impls::*;
 pub use sys::{VariantOperator, VariantType};
 
 use super::meta::{impl_godot_as_self, FromGodot, ToGodot};
diff --git a/godot-macros/src/derive/derive_from_variant.rs b/godot-macros/src/derive/derive_from_variant.rs
@@ -25,7 +25,7 @@ pub fn derive_from_godot(decl: Declaration) -> ParseResult<TokenStream> {
 
     let mut body = quote! {
         let root = variant.try_to::<::godot::builtin::Dictionary>()?;
-        let root = root.get(#name_string).ok_or(ConvertError::custom(concat!("missing expected value ", #name_string)).with_value(root.clone()))?;
+        let root = root.get(#name_string).ok_or(ConvertError::custom().with_cause(concat!("missing expected value ", #name_string)).with_value(root.clone()))?;
     };
 
     match decl {
@@ -94,7 +94,7 @@ pub fn derive_from_godot(decl: Declaration) -> ParseResult<TokenStream> {
                 body = quote! {
                     #body
                     #matches
-                    Err(ConvertError::custom("unknown error"))
+                    Err(ConvertError::custom())
                 };
             }
         }
@@ -131,7 +131,7 @@ fn make_named_struct(
         } else {
             (
                 quote! {
-                    let #ident = root.get(#string_ident).ok_or(ConvertError::custom(concat!("missing expected value ", #string_ident)).with_value(root.clone()))?;
+                    let #ident = root.get(#string_ident).ok_or(ConvertError::custom().with_cause(concat!("missing expected value ", #string_ident)).with_value(root.clone()))?;
                 },
                 quote! { #ident: #ident.try_to()? },
             )
@@ -165,7 +165,7 @@ fn make_tuple_struct(
             } else {
                 quote! {
                     let #ident = root.pop_front()
-                                     .ok_or(ConvertError::custom("missing expected value").with_value(root.clone()))?
+                                     .ok_or(ConvertError::custom().with_cause("missing expected value").with_value(root.clone()))?
                                      .try_to::<#field_type>()?;
                 }
             },
@@ -252,7 +252,7 @@ fn make_enum_tuple(
         } else {
             quote! {
                 let #ident = variant.pop_front()
-                                    .ok_or(ConvertError::custom("missing expected value").with_value(variant.clone()))?
+                                    .ok_or(ConvertError::custom().with_cause("missing expected value").with_value(variant.clone()))?
                                     .try_to::<#field_type>()?;
             }
         };
@@ -287,7 +287,7 @@ fn make_enum_named(
         } else {
             quote! {
                 let #field_name = variant.get(#field_name_string)
-                    .ok_or(ConvertError::custom(concat!("missing expected value ", #field_name_string)).with_value(variant.clone()))?
+                    .ok_or(ConvertError::custom().with_cause(concat!("missing expected value ", #field_name_string)).with_value(variant.clone()))?
                     .try_to::<#field_type>()?;
             }
         };
diff --git a/itest/rust/src/builtin_tests/containers/array_test.rs b/itest/rust/src/builtin_tests/containers/array_test.rs
@@ -418,8 +418,11 @@ fn typed_array_return_from_godot_func() {
 fn typed_array_try_from_untyped() {
     let node = Node::new_alloc();
     let array = VariantArray::from(&[node.clone().to_variant()]);
-    let converted = array.to_variant().try_to::<Array<Option<Gd<Node>>>>();
-    assert!(converted.is_err(), "converted = {converted:?}");
+
+    array
+        .to_variant()
+        .try_to::<Array<Option<Gd<Node>>>>()
+        .expect_err("untyped array should not coerce to typed array");
 
     node.free();
 }
@@ -428,8 +431,11 @@ fn typed_array_try_from_untyped() {
 fn untyped_array_try_from_typed() {
     let node = Node::new_alloc();
     let array = Array::<Option<Gd<Node>>>::from(&[Some(node.clone())]);
-    let converted = array.to_variant().try_to::<VariantArray>();
-    assert!(converted.is_err(), "converted = {converted:?}");
+
+    array
+        .to_variant()
+        .try_to::<VariantArray>()
+        .expect_err("typed array should not coerce to untyped array");
 
     node.free();
 }
diff --git a/itest/rust/src/builtin_tests/containers/variant_test.rs b/itest/rust/src/builtin_tests/containers/variant_test.rs
@@ -120,13 +120,11 @@ fn variant_call() {
     let result = variant.call("set_position", &[position.to_variant()]);
     assert!(result.is_nil());
 
-    let result = variant.call("get_position", &[]);
-    assert_eq!(
-        result
-            .try_to::<Vector2>()
-            .expect("`get_position` should return Vector2"),
-        position
-    );
+    let result = variant
+        .call("get_position", &[])
+        .try_to::<Vector2>()
+        .expect("`get_position` should return Vector2");
+    assert_eq!(result, position);
 
     let result = variant.call("to_string", &[]);
     assert_eq!(result.get_type(), VariantType::String);
@@ -300,19 +298,29 @@ fn variant_null_object_is_nil() {
 
 #[itest]
 fn variant_conversion_fails() {
-    assert!("hello".to_variant().try_to::<i64>().is_err(),);
-    assert!(28.to_variant().try_to::<f32>().is_err());
-    assert!(10.to_variant().try_to::<bool>().is_err());
-    assert!(false.to_variant().try_to::<String>().is_err());
-    assert!(VariantArray::default()
-        .to_variant()
-        .try_to::<StringName>()
-        .is_err());
+    fn assert_convert_err<T: ToGodot, U: FromGodot + std::fmt::Debug>(value: T) {
+        use std::any::type_name;
+        value.to_variant().try_to::<U>().expect_err(&format!(
+            "`{}` should not convert to `{}`",
+            type_name::<T>(),
+            type_name::<U>()
+        ));
+    }
+
+    assert_convert_err::<_, i64>("hello");
+    assert_convert_err::<i32, f32>(28);
+    assert_convert_err::<i32, bool>(10);
+    assert_convert_err::<_, String>(false);
+    assert_convert_err::<_, StringName>(VariantArray::default());
+
     //assert_eq!(
     //    Dictionary::default().to_variant().try_to::<Array>(),
     //    Err(VariantConversionError)
     //);
-    assert!(Variant::nil().to_variant().try_to::<Dictionary>().is_err());
+    Variant::nil()
+        .to_variant()
+        .try_to::<Dictionary>()
+        .expect_err("`nil` should not convert to `Dictionary`");
 }
 
 #[itest]
diff --git a/itest/rust/src/builtin_tests/convert_test.rs b/itest/rust/src/builtin_tests/convert_test.rs
diff --git a/itest/rust/src/builtin_tests/mod.rs b/itest/rust/src/builtin_tests/mod.rs
diff --git a/itest/rust/src/object_tests/object_test.rs b/itest/rust/src/object_tests/object_test.rs
