Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Add tests, include failure conditions. #1

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

shanecelis
Copy link

Hi, Alice invited me to review your PR. I added some tests to check behavior on failure conditions. It all looks good to me.

Copy link
Owner

@bonsairobo bonsairobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution and review on my PR. I like some of these tests and others not so much. I'm curious what you think about my comments.

@@ -299,6 +299,73 @@ mod tests {
assert_eq!(asset_path, Path::new("my_crate/foo/the/asset.png"));
}

// A blank src_path removes the embedded's file path altogether.
#[test]
fn embedded_asset_path_from_local_crate_blank_src_path_questionable() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems like it's testing Path more than _embedded_asset_path per se. So I generally don't like writing these tests when I know that conditions are handled by the infrastructure I'm using. I'd drop this test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one I'd keep partly because it seems surprising to me and someone might think that's desirable behavior. I actually don't understand how a zero-length Path leads to this behavior.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yea I guess it's odd that this is a functional option. We could add a check that src_path isn't empty.

crates/bevy_asset/src/io/embedded/mod.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/io/embedded/mod.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/io/embedded/mod.rs Outdated Show resolved Hide resolved
// Although extraneous slashes are permitted at the end, e.g., "src////", a
// slash at the beginning is not.
#[test]
#[should_panic(expected = "Failed to find src_prefix \"/src\" in")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this pass on Windows? I'm not sure when forward slashes are actually allowed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually very curious about how this will behave on Windows. (I'm on macOS.) I expect everything with a slash will fail actually.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be fine. We will find out in CI.

// Although extraneous slashes are permitted at the end, e.g., "src////", a
// slash at the beginning is not.
#[test]
#[should_panic(expected = "Failed to find src_prefix \"////src\" in")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto RE Windows

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's turn these two similar cases into a comment on one test instead of it being two.

@shanecelis
Copy link
Author

I think I'll drop this one too. That'll be provided by file!(). I don't think we need to second guess it.

    #[should_panic(expected = "Failed to find src_prefix \"src\" in \"\"")]
    #[test]
    fn embedded_asset_path_from_local_crate_blank_file_path() {
        let asset_path = _embedded_asset_path(
            "my_crate",
            "src".as_ref(),
            "".as_ref(),
            "the/asset.png".as_ref(),
        );
        assert_eq!(asset_path, Path::new("my_crate/foo/the/asset.png"));
    }

@shanecelis
Copy link
Author

Thanks for your feedback. Changes incorporated. You're right there was definitely some exploratory programming going on that didn't need to be captured permanently in tests. Thanks for your patience.

@bonsairobo
Copy link
Owner

Thanks for your feedback. Changes incorporated. You're right there was definitely some exploratory programming going on that didn't need to be captured permanently in tests. Thanks for your patience.

Thanks! No worries, I'm glad you had the mindset of trying to break it.

@bonsairobo bonsairobo merged commit e30d685 into bonsairobo:fix_embedded_path Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants