From 4bcc6082944af40a820e8962fb0aa8664c7bb803 Mon Sep 17 00:00:00 2001 From: Shane Celis Date: Sun, 14 Jan 2024 05:25:02 -0500 Subject: [PATCH 1/6] Test embedded_path! error behavior A number of cases will panic, which is fine. The issue is there are two or three different errors that all report an unwrap on a None value, so they're hard to differentiate for the user. --- crates/bevy_asset/src/io/embedded/mod.rs | 29 ++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/crates/bevy_asset/src/io/embedded/mod.rs b/crates/bevy_asset/src/io/embedded/mod.rs index 0448a7cdd83a7..7d0e817d2b81f 100644 --- a/crates/bevy_asset/src/io/embedded/mod.rs +++ b/crates/bevy_asset/src/io/embedded/mod.rs @@ -255,3 +255,32 @@ macro_rules! load_internal_binary_asset { ); }}; } + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + #[test] + fn test_embedded_path_no_panics() { + assert_eq!(file!(), "crates/bevy_asset/src/io/embedded/mod.rs"); + assert_eq!(embedded_path!("/src/", "a"), PathBuf::from("bevy_asset/io/embedded/a")); + assert_eq!(embedded_path!("/src", "a"), PathBuf::from("/io/embedded/a")); + assert_eq!(embedded_path!("src", "a"), PathBuf::from("/io/embedded/a")); + } + + #[test] + // Panic message prior to change. + #[should_panic(expected = "called `Option::unwrap()` on a `None` value")] + // #[should_panic(expected = "Expected source path `NOT-IN-PATH` in file path `crates/bevy_asset/src/io/embedded/mod.rs`")] + fn test_embedded_path_panic0() { + assert_eq!(embedded_path!("NOT-IN-PATH", "b"), PathBuf::from("bevy_asset/tes/b")); + } + + + #[test] + #[should_panic(expected = "called `Option::unwrap()` on a `None` value")] + // #[should_panic(expected = "Expected source path `/a/` in file path `crates/bevy_asset/src/io/embedded/mod.rs`")] + fn test_embedded_path_panic1() { + assert_eq!(embedded_path!("/a/", "b"), PathBuf::from("bevy_asset/tes/b")); + } +} From dee5cfab5d8b07716006bcdd63ae29e4aad5e8fc Mon Sep 17 00:00:00 2001 From: Shane Celis Date: Sun, 14 Jan 2024 05:32:10 -0500 Subject: [PATCH 2/6] Better error reporting for embedded_path!() This refactor does not change `embedded_path!`'s behavior. All cases that caused a panic before, will still cause a panic. But this change will provide the user diagnostic information that will hopefully help them right whatever issue is causing the panic. The original behavior was this: ``` #[test] #[should_panic(expected = "called `Option::unwrap()` on a `None` value")] fn test_embedded_path_original_behavior() { embedded_path!("NOT-IN-PATH", "b"), } ``` The changed behavior is this: ``` #[test] #[should_panic( expected = "Expected source path `NOT-IN-PATH` in file path `crates/bevy_asset/src/io/embedded/mod.rs`" )] fn test_embedded_path_original_behavior() { embedded_path!("NOT-IN-PATH", "b") } ``` --- crates/bevy_asset/src/io/embedded/mod.rs | 62 +++++++++++++++--------- 1 file changed, 40 insertions(+), 22 deletions(-) diff --git a/crates/bevy_asset/src/io/embedded/mod.rs b/crates/bevy_asset/src/io/embedded/mod.rs index 7d0e817d2b81f..4a98fc9ec07e4 100644 --- a/crates/bevy_asset/src/io/embedded/mod.rs +++ b/crates/bevy_asset/src/io/embedded/mod.rs @@ -111,13 +111,24 @@ macro_rules! embedded_path { }}; ($source_path: expr, $path_str: expr) => {{ - let crate_name = module_path!().split(':').next().unwrap(); - let after_src = file!().split($source_path).nth(1).unwrap(); - let file_path = std::path::Path::new(after_src) - .parent() - .unwrap() - .join($path_str); - std::path::Path::new(crate_name).join(file_path) + let crate_name = module_path!() + .split(':') + .next() + .expect("Could not find crate name"); + if let Some(after_src) = file!().split($source_path).nth(1) { + let file_path = if let Some(parent) = std::path::Path::new(after_src).parent() { + parent.join($path_str) + } else { + panic!("Expected parent path for `{}` derived from `{}`", after_src, file!()); + }; + std::path::Path::new(crate_name).join(file_path) + } else { + panic!( + "Expected source path `{}` in file path `{}`", + $source_path, + file!() + ); + } }}; } @@ -200,7 +211,11 @@ macro_rules! embedded_asset { .resource_mut::<$crate::io::embedded::EmbeddedAssetRegistry>(); let path = $crate::embedded_path!($source_path, $path); #[cfg(feature = "embedded_watcher")] - let full_path = std::path::Path::new(file!()).parent().unwrap().join($path); + let full_path = if let Some(parent) = std::path::Path::new(file!()).parent() { + parent.join($path) + } else { + panic!("Expected parent path for `{}`", file!()) + }; #[cfg(not(feature = "embedded_watcher"))] let full_path = std::path::PathBuf::new(); embedded.insert_asset(full_path, &path, include_bytes!($path)); @@ -262,25 +277,28 @@ mod tests { #[test] fn test_embedded_path_no_panics() { - assert_eq!(file!(), "crates/bevy_asset/src/io/embedded/mod.rs"); - assert_eq!(embedded_path!("/src/", "a"), PathBuf::from("bevy_asset/io/embedded/a")); - assert_eq!(embedded_path!("/src", "a"), PathBuf::from("/io/embedded/a")); - assert_eq!(embedded_path!("src", "a"), PathBuf::from("/io/embedded/a")); + assert_eq!(file!(), "crates/bevy_asset/src/io/embedded/mod.rs"); + assert_eq!( + embedded_path!("/src/", "a"), + PathBuf::from("bevy_asset/io/embedded/a") + ); + assert_eq!(embedded_path!("/src", "a"), PathBuf::from("/io/embedded/a")); + assert_eq!(embedded_path!("src", "a"), PathBuf::from("/io/embedded/a")); } #[test] - // Panic message prior to change. - #[should_panic(expected = "called `Option::unwrap()` on a `None` value")] - // #[should_panic(expected = "Expected source path `NOT-IN-PATH` in file path `crates/bevy_asset/src/io/embedded/mod.rs`")] - fn test_embedded_path_panic0() { - assert_eq!(embedded_path!("NOT-IN-PATH", "b"), PathBuf::from("bevy_asset/tes/b")); + #[should_panic( + expected = "Expected source path `NOT-IN-PATH` in file path `crates/bevy_asset/src/io/embedded/mod.rs`" + )] + fn test_embedded_path_nonexistent_src_path() { + embedded_path!("NOT-IN-PATH", "b"); } - #[test] - #[should_panic(expected = "called `Option::unwrap()` on a `None` value")] - // #[should_panic(expected = "Expected source path `/a/` in file path `crates/bevy_asset/src/io/embedded/mod.rs`")] - fn test_embedded_path_panic1() { - assert_eq!(embedded_path!("/a/", "b"), PathBuf::from("bevy_asset/tes/b")); + #[should_panic( + expected = "Expected parent path for `` derived from `crates/bevy_asset/src/io/embedded/mod.rs`" + )] + fn test_embedded_parent_panic() { + assert_eq!(embedded_path!("/embedded/mod.rs", "embed.png"), PathBuf::from("bevy_asset/embed.png")); } } From 44129b4a5cfa1507bd3183616bf23a2776577b09 Mon Sep 17 00:00:00 2001 From: Shane Celis Date: Sun, 14 Jan 2024 06:04:09 -0500 Subject: [PATCH 3/6] chore: Run cargo fmt --all --- crates/bevy_asset/src/io/embedded/mod.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/bevy_asset/src/io/embedded/mod.rs b/crates/bevy_asset/src/io/embedded/mod.rs index 4a98fc9ec07e4..3c693ebc9c413 100644 --- a/crates/bevy_asset/src/io/embedded/mod.rs +++ b/crates/bevy_asset/src/io/embedded/mod.rs @@ -119,7 +119,11 @@ macro_rules! embedded_path { let file_path = if let Some(parent) = std::path::Path::new(after_src).parent() { parent.join($path_str) } else { - panic!("Expected parent path for `{}` derived from `{}`", after_src, file!()); + panic!( + "Expected parent path for `{}` derived from `{}`", + after_src, + file!() + ); }; std::path::Path::new(crate_name).join(file_path) } else { @@ -299,6 +303,9 @@ mod tests { expected = "Expected parent path for `` derived from `crates/bevy_asset/src/io/embedded/mod.rs`" )] fn test_embedded_parent_panic() { - assert_eq!(embedded_path!("/embedded/mod.rs", "embed.png"), PathBuf::from("bevy_asset/embed.png")); + assert_eq!( + embedded_path!("/embedded/mod.rs", "embed.png"), + PathBuf::from("bevy_asset/embed.png") + ); } } From 915389835c00671c726fad4747dd81bb3ba1fa10 Mon Sep 17 00:00:00 2001 From: Shane Celis Date: Sun, 14 Jan 2024 06:13:19 -0500 Subject: [PATCH 4/6] test: Fix test to work with Windows paths. --- crates/bevy_asset/src/io/embedded/mod.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/bevy_asset/src/io/embedded/mod.rs b/crates/bevy_asset/src/io/embedded/mod.rs index 3c693ebc9c413..a55ede2f3a5ab 100644 --- a/crates/bevy_asset/src/io/embedded/mod.rs +++ b/crates/bevy_asset/src/io/embedded/mod.rs @@ -292,7 +292,11 @@ mod tests { #[test] #[should_panic( - expected = "Expected source path `NOT-IN-PATH` in file path `crates/bevy_asset/src/io/embedded/mod.rs`" + // Unix + // expected = "Expected source path `NOT-IN-PATH` in file path `crates/bevy_asset/src/io/embedded/mod.rs`" + // Windows + // expected = "Expected source path `NOT-IN-PATH` in file path `crates\\bevy_asset\\src\\io\\embedded\\mod.rs`" + expected = "Expected source path `NOT-IN-PATH` in file path `crates" )] fn test_embedded_path_nonexistent_src_path() { embedded_path!("NOT-IN-PATH", "b"); @@ -300,7 +304,11 @@ mod tests { #[test] #[should_panic( - expected = "Expected parent path for `` derived from `crates/bevy_asset/src/io/embedded/mod.rs`" + // Unix + // expected = "Expected parent path for `` derived from `crates/bevy_asset/src/io/embedded/mod.rs`" + // Windows + // expected = "Expected parent path for `` derived from `crates\\bevy_asset\\src\\io\\embedded\\mod.rs`" + expected = "Expected parent path for `` derived from `crates" )] fn test_embedded_parent_panic() { assert_eq!( From c0d83f358af7ad46dfe65442c6de5eaeae9d3ecf Mon Sep 17 00:00:00 2001 From: Shane Celis Date: Sun, 14 Jan 2024 07:09:33 -0500 Subject: [PATCH 5/6] test: Fix _all_ paths to work on unix and windows. --- crates/bevy_asset/src/io/embedded/mod.rs | 37 +++++++++++++++++------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/crates/bevy_asset/src/io/embedded/mod.rs b/crates/bevy_asset/src/io/embedded/mod.rs index a55ede2f3a5ab..17031dbd81bd1 100644 --- a/crates/bevy_asset/src/io/embedded/mod.rs +++ b/crates/bevy_asset/src/io/embedded/mod.rs @@ -281,13 +281,31 @@ mod tests { #[test] fn test_embedded_path_no_panics() { + #[cfg(not(windows))] assert_eq!(file!(), "crates/bevy_asset/src/io/embedded/mod.rs"); - assert_eq!( - embedded_path!("/src/", "a"), - PathBuf::from("bevy_asset/io/embedded/a") - ); - assert_eq!(embedded_path!("/src", "a"), PathBuf::from("/io/embedded/a")); - assert_eq!(embedded_path!("src", "a"), PathBuf::from("/io/embedded/a")); + #[cfg(windows)] + assert_eq!(file!(), "crates\\bevy_asset\\src\\io\\embedded\\mod.rs"); + + #[cfg(not(windows))] + let path = PathBuf::from("bevy_asset/io/embedded/embed.png"); + #[cfg(windows)] + let path = PathBuf::from("bevy_asset\\io\\embedded\\embed.png"); + + // One argument for embedded_path! uses '/src/' as its source_path. + assert_eq!(&embedded_path!("embed.png"), &path); + assert_eq!(&embedded_path!("/src/", "embed.png"), &path); + } + + /// Using a slightly different source path parameter causes the crate_name to be omitted. + /// Possible bug. + #[test] + fn test_embedded_path_questionable() { + #[cfg(not(windows))] + let path = PathBuf::from("/io/embedded/embed.png"); + #[cfg(windows)] + let path = PathBuf::from("\\io\\embedded\\embed.png"); + assert_eq!(&embedded_path!("/src", "embed.png"), &path); + assert_eq!(&embedded_path!("src", "embed.png"), &path); } #[test] @@ -299,7 +317,7 @@ mod tests { expected = "Expected source path `NOT-IN-PATH` in file path `crates" )] fn test_embedded_path_nonexistent_src_path() { - embedded_path!("NOT-IN-PATH", "b"); + embedded_path!("NOT-IN-PATH", "embed.png"); } #[test] @@ -311,9 +329,6 @@ mod tests { expected = "Expected parent path for `` derived from `crates" )] fn test_embedded_parent_panic() { - assert_eq!( - embedded_path!("/embedded/mod.rs", "embed.png"), - PathBuf::from("bevy_asset/embed.png") - ); + embedded_path!("mod.rs", "embed.png"); } } From d1b93205ed00bbd36597daee8ce77c7ba6522dc4 Mon Sep 17 00:00:00 2001 From: Shane Celis Date: Mon, 15 Jan 2024 01:56:12 -0500 Subject: [PATCH 6/6] doc: Add more information to error report. --- crates/bevy_asset/src/io/embedded/mod.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/bevy_asset/src/io/embedded/mod.rs b/crates/bevy_asset/src/io/embedded/mod.rs index 17031dbd81bd1..de583f7367831 100644 --- a/crates/bevy_asset/src/io/embedded/mod.rs +++ b/crates/bevy_asset/src/io/embedded/mod.rs @@ -111,10 +111,12 @@ macro_rules! embedded_path { }}; ($source_path: expr, $path_str: expr) => {{ - let crate_name = module_path!() - .split(':') - .next() - .expect("Could not find crate name"); + let Some(crate_name) = module_path!().split(':').next() else { + panic!( + "Could not find crate name. Expected ':' in module path `{}`", + module_path!() + ); + }; if let Some(after_src) = file!().split($source_path).nth(1) { let file_path = if let Some(parent) = std::path::Path::new(after_src).parent() { parent.join($path_str)