From 953f8c31e9c467d30ea4c2dd7d79ba5f74c37220 Mon Sep 17 00:00:00 2001 From: Imbris Date: Tue, 2 Dec 2025 11:47:39 -0500 Subject: [PATCH 1/2] Add error messages to the panic in tests and use should_panic more effectively --- bauble/src/lib.rs | 13 +++++++++---- bauble/tests/integration.rs | 33 +++++++++++++++++++-------------- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/bauble/src/lib.rs b/bauble/src/lib.rs index 9fe9b15..6e9a178 100644 --- a/bauble/src/lib.rs +++ b/bauble/src/lib.rs @@ -72,8 +72,10 @@ 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 mut buf = Vec::new(); + errors.write_errors(&ctx.read().unwrap(), &mut buf); + let error_msg = String::from_utf8(buf).unwrap(); + panic!("Error converting: \n{error_msg}"); } // Test round-trip of objects through source format @@ -107,11 +109,14 @@ 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()); + errors.print_errors(&ctx.read().unwrap()); for (path, re_source) in re_path_sources { eprintln!("In file \"{path}\": {re_source}"); } - panic!("Error re-converting"); + let mut buf = Vec::new(); + errors.write_errors(&ctx.read().unwrap(), &mut buf); + let error_msg = String::from_utf8(buf).unwrap(); + panic!("Error re-converting: \n{error_msg}"); } assert_eq!(objects, re_objects); diff --git a/bauble/tests/integration.rs b/bauble/tests/integration.rs index cd65ef5..ca97b26 100644 --- a/bauble/tests/integration.rs +++ b/bauble/tests/integration.rs @@ -62,6 +62,13 @@ fn make_ctx(with_ctx_builder: &dyn Fn(&mut bauble::BaubleContextBuilder)) -> bau ctx } +fn panic_errors(ctx: &bauble::BaubleContext, errors: bauble::BaubleErrors) -> ! { + let mut buf = Vec::new(); + errors.write_errors(ctx, &mut buf); + let error_msg = String::from_utf8(buf).unwrap(); + panic!("{error_msg}"); +} + fn test_load(with_ctx_builder: &dyn Fn(&mut bauble::BaubleContextBuilder), files: &[&TestFile]) { let mut ctx = make_ctx(with_ctx_builder); @@ -72,8 +79,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 +98,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 +128,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 +174,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 +194,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 +322,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 +361,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 +485,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); From 5809124aed9543f7fdc6166893446a7b1887dd42 Mon Sep 17 00:00:00 2001 From: Imbris Date: Wed, 3 Dec 2025 11:42:07 -0500 Subject: [PATCH 2/2] Remove remaining uses of bauble::print_errors, add error to panic message when converting from bauble value to rust fails, and add helper method for writing errors to a string --- bauble/src/error.rs | 16 ++++++------ bauble/src/lib.rs | 49 +++++++++++++++++++++++-------------- bauble/src/types.rs | 2 +- bauble/tests/integration.rs | 15 +++++++----- 4 files changed, 47 insertions(+), 35 deletions(-) diff --git a/bauble/src/error.rs b/bauble/src/error.rs index 038ce8f..f623e4b 100644 --- a/bauble/src/error.rs +++ b/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 { + let mut buf = Vec::new(); + self.write_errors(ctx, &mut buf); + String::from_utf8(buf) + } } impl From for BaubleErrors { @@ -228,12 +235,3 @@ impl From> 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(res: Result>, ctx: &BaubleContext) -> Option { - res.map_err(|errors| { - let errors: BaubleErrors = errors.into(); - errors.print_errors(ctx) - }) - .ok() -} diff --git a/bauble/src/lib.rs b/bauble/src/lib.rs index 6e9a178..dde2603 100644 --- a/bauble/src/lib.rs +++ b/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,9 +72,7 @@ pub mod private { let (objects, errors) = ctx.write().unwrap().load_all(); if !errors.is_empty() { - let mut buf = Vec::new(); - errors.write_errors(&ctx.read().unwrap(), &mut buf); - let error_msg = String::from_utf8(buf).unwrap(); + let error_msg = errors.try_to_string(&ctx.read().unwrap()).unwrap(); panic!("Error converting: \n{error_msg}"); } @@ -109,13 +107,11 @@ pub mod private { .reload_paths(re_path_sources.iter().map(|(p, s)| (p.borrow(), s))); if !errors.is_empty() { - errors.print_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(); } - let mut buf = Vec::new(); - errors.write_errors(&ctx.read().unwrap(), &mut buf); - let error_msg = String::from_utf8(buf).unwrap(); panic!("Error re-converting: \n{error_msg}"); } @@ -126,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( + expected_value: T, + value: crate::Object, + ctx: &std::sync::RwLock, + ) where + T: PartialEq + for<'a> crate::Bauble<'a> + std::fmt::Debug, + { + match ::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) @@ -152,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() { diff --git a/bauble/src/types.rs b/bauble/src/types.rs index 32bdbe6..931e3f5 100644 --- a/bauble/src/types.rs +++ b/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); } diff --git a/bauble/tests/integration.rs b/bauble/tests/integration.rs index ca97b26..8df7794 100644 --- a/bauble/tests/integration.rs +++ b/bauble/tests/integration.rs @@ -16,8 +16,14 @@ fn expected_value_fn Bauble<'a> + PartialEq + std::fmt::Debug>( ) -> Box { 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}"); + } + } }) } @@ -63,10 +69,7 @@ fn make_ctx(with_ctx_builder: &dyn Fn(&mut bauble::BaubleContextBuilder)) -> bau } fn panic_errors(ctx: &bauble::BaubleContext, errors: bauble::BaubleErrors) -> ! { - let mut buf = Vec::new(); - errors.write_errors(ctx, &mut buf); - let error_msg = String::from_utf8(buf).unwrap(); - panic!("{error_msg}"); + panic!("{}", errors.try_to_string(ctx).unwrap()); } fn test_load(with_ctx_builder: &dyn Fn(&mut bauble::BaubleContextBuilder), files: &[&TestFile]) {