Add error messages to the panic in tests and use should_panic more effectively

bauble/src/error.rs

@@ -210,6 +210,13 @@ impl BaubleErrors {
             ctx,
         );
     }
+
+    /// Writes errors and attempts to convert them to a string.
+    pub fn try_to_string(&self, ctx: &BaubleContext) -> Result<String, std::string::FromUtf8Error> {
+        let mut buf = Vec::new();
+        self.write_errors(ctx, &mut buf);
+        String::from_utf8(buf)
+    }
 }
 
 impl<B: BaubleError> From<B> for BaubleErrors {
@@ -228,12 +235,3 @@ impl<B: BaubleError> From<Vec<B>> for BaubleErrors {
         )
     }
 }
-
-/// Will print the errors of `res` in case of an error, otherwise, return the ok value of `res` of type `T`.
-pub fn print_errors<T>(res: Result<T, impl Into<BaubleErrors>>, ctx: &BaubleContext) -> Option<T> {
-    res.map_err(|errors| {
-        let errors: BaubleErrors = errors.into();
-        errors.print_errors(ctx)
-    })
-    .ok()
-}

bauble/src/lib.rs

@@ -16,7 +16,7 @@ pub use bauble_macros::Bauble;
 
 pub use builtin::Ref;
 pub use context::{BaubleContext, BaubleContextBuilder, FileId, PathReference, Source};
-pub use error::{BaubleError, BaubleErrors, CustomError, Level, print_errors};
+pub use error::{BaubleError, BaubleErrors, CustomError, Level};
 pub use spanned::{Span, SpanExt, Spanned};
 pub use traits::{
     Bauble, BaubleAllocator, DefaultAllocator, ToRustError, ToRustErrorKind, VariantKind,
@@ -72,8 +72,8 @@ pub mod private {
         let (objects, errors) = ctx.write().unwrap().load_all();
 
         if !errors.is_empty() {
-            crate::print_errors(Err::<(), _>(errors), &ctx.read().unwrap());
-            panic!("Error converting");
+            let error_msg = errors.try_to_string(&ctx.read().unwrap()).unwrap();
+            panic!("Error converting: \n{error_msg}");
         }
 
         // Test round-trip of objects through source format
@@ -107,11 +107,12 @@ pub mod private {
             .reload_paths(re_path_sources.iter().map(|(p, s)| (p.borrow(), s)));
 
         if !errors.is_empty() {
-            crate::print_errors(Err::<(), _>(errors), &ctx.read().unwrap());
+            let mut error_msg = errors.try_to_string(&ctx.read().unwrap()).unwrap();
             for (path, re_source) in re_path_sources {
-                eprintln!("In file \"{path}\": {re_source}");
+                use std::fmt::Write;
+                writeln!(&mut error_msg, "In file \"{path}\": {re_source}").unwrap();
             }
-            panic!("Error re-converting");
+            panic!("Error re-converting: \n{error_msg}");
         }
 
         assert_eq!(objects, re_objects);
@@ -121,6 +122,30 @@ pub mod private {
         compare_objects(objects, ctx);
         compare_objects(re_objects, ctx);
     }
+
+    /// Converts bauble object to the type of the `expected_value` and then asserts that they are
+    /// the same.
+    ///
+    /// Panics if converting object fails.
+    pub fn expected_from_bauble<T>(
+        expected_value: T,
+        value: crate::Object,
+        ctx: &std::sync::RwLock<crate::BaubleContext>,
+    ) where
+        T: PartialEq + for<'a> crate::Bauble<'a> + std::fmt::Debug,
+    {
+        match <T as crate::Bauble>::from_bauble(value.value, &crate::DefaultAllocator) {
+            Ok(read_value) => {
+                assert_eq!(read_value, expected_value);
+            }
+            Err(error) => {
+                let error_msg = crate::BaubleErrors::from(error)
+                    .try_to_string(&ctx.read().unwrap())
+                    .unwrap();
+                panic!("Error converting object to rust value: \n{error_msg}");
+            }
+        }
+    }
 }
 
 // TODO(@docs)
@@ -147,16 +172,7 @@ macro_rules! bauble_test {
 
                 $(
                     let value = objects.next().expect("Not as many objects as test expr in bauble test?");
-                    // Infer type for `read_value` to be the same as `test_value`.
-                    let [test_value, read_value] = [
-                        $test_value,
-                        $crate::print_errors(
-                            $crate::Bauble::from_bauble(value.value, &$crate::DefaultAllocator),
-                            &ctx.read().unwrap()
-                        ).unwrap(),
-                    ];
-
-                    assert_eq!(read_value, test_value);
+                    $crate::private::expected_from_bauble($test_value, value, &ctx);
                 )*
 
                 if objects.next().is_some() {

bauble/src/types.rs

@@ -616,7 +616,7 @@ impl TypeRegistry {
             let (loaded_objects, errors) = ctx.load_all();
 
             if !errors.is_empty() {
-                crate::print_errors(Err::<(), _>(errors), &ctx);
+                errors.print_errors(&ctx);
 
                 return Err(TypeSystemError::InstantiableErrors);
             }

bauble/tests/integration.rs

@@ -16,8 +16,14 @@ fn expected_value_fn<T: for<'a> Bauble<'a> + PartialEq + std::fmt::Debug>(
 ) -> Box<dyn Fn(Object, &BaubleContext)> {
     Box::new(move |object, ctx| {
         let result = T::from_bauble(object.value, &bauble::DefaultAllocator);
-        let read_value = bauble::print_errors(result, ctx).unwrap();
-        assert_eq!(&read_value, &expected_value);
+        match result {
+            Ok(read_value) => assert_eq!(read_value, expected_value),
+            Err(error) => {
+                let errors = bauble::BaubleErrors::from(error);
+                let error_msg = errors.try_to_string(ctx).unwrap();
+                panic!("Error converting object to rust value: \n{error_msg}");
+            }
+        }
     })
 }
 
@@ -62,6 +68,10 @@ fn make_ctx(with_ctx_builder: &dyn Fn(&mut bauble::BaubleContextBuilder)) -> bau
     ctx
 }
 
+fn panic_errors(ctx: &bauble::BaubleContext, errors: bauble::BaubleErrors) -> ! {
+    panic!("{}", errors.try_to_string(ctx).unwrap());
+}
+
 fn test_load(with_ctx_builder: &dyn Fn(&mut bauble::BaubleContextBuilder), files: &[&TestFile]) {
     let mut ctx = make_ctx(with_ctx_builder);
 
@@ -72,8 +82,7 @@ fn test_load(with_ctx_builder: &dyn Fn(&mut bauble::BaubleContextBuilder), files
 
     let (objects, errors) = ctx.load_all();
     if !errors.is_empty() {
-        bauble::print_errors(Err::<(), _>(errors), &ctx);
-        panic!("Error converting");
+        panic_errors(&ctx, errors);
     }
     compare_objects(objects, files, &ctx);
 }
@@ -92,21 +101,19 @@ fn test_reload(
 
     let (objects, errors) = ctx.load_all();
     if !errors.is_empty() {
-        bauble::print_errors(Err::<(), _>(errors), &ctx);
-        panic!("Error converting");
+        panic_errors(&ctx, errors);
     }
     compare_objects(objects, start, &ctx);
 
     // Test reloading with new content and new files that are nested as submodules.
     let (objects, errors) = ctx.reload_paths(new.iter().map(|f| (f.path.borrow(), &f.content)));
     if !errors.is_empty() {
-        bauble::print_errors(Err::<(), _>(errors), &ctx);
-        panic!("Error converting reload");
+        panic_errors(&ctx, errors);
     }
     compare_objects(objects, new, &ctx);
 }
 
-/// Doesn't fail test when some files have errors as long as they had no expected values.
+/// Doesn't fail test when some files have errors as long as all expected values are loaded.
 ///
 /// Expects at least one error.
 fn test_load_partial(
@@ -124,7 +131,7 @@ fn test_load_partial(
     if errors.is_empty() {
         panic!("At least one error is expected");
     } else {
-        bauble::print_errors(Err::<(), _>(errors), &ctx);
+        errors.print_errors(&ctx);
     }
     compare_objects(objects, files, &ctx);
 }
@@ -170,7 +177,7 @@ fn new_nested_reload_paths() {
 }
 
 #[test]
-#[should_panic]
+#[should_panic = "This identifier was already used"]
 fn duplicate_objects() {
     let a = &test_file!(
         "a",
@@ -190,7 +197,7 @@ fn duplicate_objects() {
 // NOTE: This currently fails because `b::test` isn't allowed by itself but if we add support for
 // that we still want this case to fail.
 #[test]
-#[should_panic]
+#[should_panic = "found ':' expected identifier, or '*'"]
 fn duplicate_objects_across_files() {
     let a = &test_file!(
         "a",
@@ -318,8 +325,8 @@ fn same_file_references() {
 }
 
 #[test]
-#[should_panic] // TODO: see todo in `register_assets` about registering assets in the correct
-// order.
+// TODO: see todo in `register_assets` about registering assets in the correct order.
+#[should_panic = "Expected this path to refer to an asset"]
 fn same_file_references_reverse() {
     let a = &test_file!(
         "a",
@@ -357,7 +364,8 @@ fn same_file_references_reverse_full() {
 }
 
 #[test]
-#[should_panic] // TODO: see todo in `register_assets` where `add_use` is called.
+// TODO: see todo in `register_assets` where `add_use` is called.
+#[should_panic = "Expected this path to refer to a valid reference"]
 fn reference_with_use() {
     let a = &test_file!(
         "a",
@@ -480,7 +488,7 @@ pub fn ref_implicit_type_multiple_files() {
 }
 
 #[test]
-#[should_panic]
+#[should_panic = "Expected this path to refer to a type"]
 pub fn ref_explicit_type_incorrect() {
     #[derive(Bauble, PartialEq, Eq, Debug)]
     struct Incorrect(u32);