Skip to content

Add a few basic tests for asset processing. #21409

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Oct 7, 2025
Merged

Add a few basic tests for asset processing. #21409

merged 6 commits into from
Oct 7, 2025
+579 −26

Conversation

andriyDev
Copy link
Contributor

@andriyDev andriyDev commented Oct 5, 2025 •
edited
Loading

Objective

  • bevy_asset needs more tests! This adds three related to asset processing.

Solution

  • Create a new MemoryAssetWriter to pair with MemoryAssetReader.
  • Adds a way to override whether the asset processor is created or not.
  • Make Data::value and Data::path pub so that we can actually see what is written to the processed dir. Note: Dir::get_asset returns Data already, but it isn't usable.
  • Add three tests: one to test that assets are copied to the processed dir for no-processing assets, one to test that using a default processor works, and one to test that an asset meta file works.

Testing

  • This adds testing! :D

@andriyDev andriyDev added A-Assets Load files from disk to use for things like images, models, and sounds C-Testing A change that impacts how we test Bevy or how users test their apps D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 5, 2025
@alice-i-cecile alice-i-cecile added the X-Uncontroversial This work is generally agreed upon label Oct 5, 2025
@andriyDev andriyDev force-pushed the processor_tests branch 4 times, most recently from fb9e912 to fe96cb6 Compare October 5, 2025 22:21
kristoff3r
kristoff3r reviewed Oct 6, 2025
View reviewed changes
crates/bevy_asset/src/lib.rs
Comment on lines +2245 to +2249
#[expect(clippy::allow_attributes, reason = "this is only sometimes unused")]
#[allow(
unused,
reason = "We only use this for asset processor tests, which are only compiled with the `multi_threaded` feature."
)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing all these expect/allow, why not make a submodule for the asset processor tests and mark the entire thing as #[cfg(feature = "multi_threaded")]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I'm not a fan of how indented additional test modules are. We could also split this into its own file, but I don't know if that's a pattern in bevy yet.

I also think we should try to make asset processing work single threaded at least for testing. I think the main reason we don't support single-threaded right now is because listening for changes right now effectively uses a spin loop.

@alice-i-cecile WDYT? We may want these tests in a separate module anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

A separate file would also make sense since were at 2k+ lines. In general I feel like a lot of the files in bevy_asset could use being split up, I always have a hard time remembering where stuff is in these big files.

Copy link
Member

Choose a reason for hiding this comment

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

No strong feelings here TBH 😅

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 6, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 6, 2025
atlv24
atlv24 reviewed Oct 6, 2025
View reviewed changes
atlv24
atlv24 reviewed Oct 6, 2025
View reviewed changes
crates/bevy_asset/src/io/memory.rs
self: Pin<&mut Self>,
_: &mut core::task::Context<'_>,
) -> Poll<std::io::Result<()>> {
// Write the data to our fake disk. This means we will repeatedly reinsert the asset.
Copy link
Contributor

Choose a reason for hiding this comment

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

Im not sure I understand why there's both a DataWriter and MemoryAssetWriter. Is DataWriter also a memory writer? If not, why is this a fake disk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MemoryAssetWriter implements the AssetWriter trait - which is really an interface over the file system to start writes, move directories, etc.

DataWriter implements the AsyncWrite trait that actually writes data.

This just mirrors the existing MemoryAssetReader and DataReader types. I'd like to keep that consistent.

@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Oct 6, 2025
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Oct 6, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 6, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 6, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 6, 2025
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Oct 6, 2025
@alice-i-cecile
Copy link
Member

@andriyDev @mockersf it looks like the example patch is failing now?

@janis-bhm
Copy link
Contributor

Oh hey this is very cool, I have a branch that I haven't even pushed to github because my test requires writing files and I wasn't sure how to do that safely in a test!

@andriyDev
Copy link
Contributor Author

@alice-i-cecile I just recreated the asset_source_website.patch, which should hopefully fix it.

I also realized I was missing a migration guide for the new AssetPlugin field.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 7, 2025
Merged via the queue into bevyengine:main with commit 2146899 Oct 7, 2025
36 checks passed
@andriyDev andriyDev deleted the processor_tests branch October 7, 2025 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+2 more reviewers

@kristoff3r kristoff3r kristoff3r approved these changes

@atlv24 atlv24 atlv24 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Testing A change that impacts how we test Bevy or how users test their apps D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andriyDev @alice-i-cecile @janis-bhm @kristoff3r @atlv24