Skip to content

Add tests for asset processing to show the problem with #21269. #21433

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 2 commits into from
Oct 13, 2025
Merged

Add tests for asset processing to show the problem with #21269. #21433

merged 2 commits into from
Oct 13, 2025

Conversation

andriyDev
Copy link
Contributor

@andriyDev andriyDev commented Oct 7, 2025 •
edited
Loading

Objective

Solution

  • Add a test to show that loaders during processing can read processed assets.
  • Add an ignored test to show that loaders during processing cannot currently read source assets.
  • This also tests that nested asset loads during processing actually works.

Testing

  • Oops! All tests!

@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 X-Uncontroversial This work is generally agreed upon 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 7, 2025
@andriyDev andriyDev force-pushed the src-proc-ambiguity-tests branch 4 times, most recently from 1d1dc96 to f97df7e Compare October 7, 2025 06:03
@andriyDev
Copy link
Contributor Author

Hmmm looks like it's deadlocking for some reason? That's very strange... I can't reproduce it on my machine (and I'm also on Windows). I'll need to investigate.

@andriyDev andriyDev added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 7, 2025
@andriyDev
Copy link
Contributor Author

andriyDev commented Oct 8, 2025 •
edited
Loading

Ooof ok I understand the problem. I was able to reproduce this locally - it's very flaky. Asset processing currently requires writing the transaction log to an actual file (which we don't control). This means that if a previous test was using the transaction log (since we never kill the asset processing thread), this can leave the log in an opened state, resulting in Windows locking down the file and preventing writes from other tests.

Clearly we need to be able to mock out the asset processor's transaction log. We might need to make a couple traits to mock this out.

Edit: Just to clarify, this is already a problem with the existing tests. Hopefully with so few asset processing tests the likelihood is low of errors. I will try to fix this soon.

@andriyDev andriyDev added S-Blocked This cannot move forward until something else changes and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 8, 2025
@andriyDev andriyDev mentioned this pull request Oct 9, 2025
Merged
@andriyDev
Copy link
Contributor Author

Blocked on #21476

github-merge-queue bot pushed a commit that referenced this pull request Oct 10, 2025
@andriyDev
Convert ProcessorTransactionLog into a trait. (#21476)
ea8f22e 
# Objective

- There was a TODO to make this type actually a trait.
- #21409 introduced a test flake on WIndows (since the transaction log
file may not have been fully released by the time the next test ran),
and made tests create the `crates/bevy_asset/imported_assets/log` file.

## Solution

- Create two traits: `ProcessorTransactionLogFactory` and
`ProcessorTransactionLog`. The former creates the latter.
- Rewrite the test helper for asset processing to use a fake transaction
log that doesn't do anything.
- Note: pretty much the entire implementation was just reformatted into
the trait.
- Note: these transaction log methods are returning `BevyError` instead
of something like `std::io::Error`.

## Testing

- I ran the asset tests on repeat for a while (including with #21433
where the flake was first seen and I was able to reproduce fairly
quickly). No issues!

Note: we could make a release notes for the fact that users can make
their own transaction logs, but this feature is primarily for unit
testing. Maybe a user could make a more robust transaction log, but it's
really not clear that it would be important for anyone.
@andriyDev andriyDev force-pushed the src-proc-ambiguity-tests branch from f97df7e to 807168a Compare October 10, 2025 23:12
@andriyDev andriyDev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Oct 10, 2025
@andriyDev
Copy link
Contributor Author

Unblocked now that #21476 is merged!

janis-bhm
janis-bhm reviewed Oct 11, 2025
View reviewed changes
crates/bevy_asset/src/lib.rs
// This processor loads a gltf file, converts it to BSN and then saves out the BSN.
type GltfProcessor = LoadTransformAndSave<FakeGltfLoader, GltfToBsn, FakeBsnSaver>;
// This processor loads a gltfx file (including its gltf files) and converts it to BSN.
type GltfxProcessor = LoadTransformAndSave<FakeGltfxLoader, GltfxToBsn, FakeBsnSaver>;
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading #21269 correctly, then this will have to be changed, maybe with a marker type, so that the load_source_asset call can be told to load source assets here (or some other solution to the issue)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup that's correct. I still think it's worth it to have most of a test ready to test against. It might just be as simple as adding a bool to load_source_asset for like processed_dependencies or something.

@andriyDev andriyDev requested a review from janis-bhm October 11, 2025 22:23
ChristopherBiscardi
ChristopherBiscardi reviewed Oct 12, 2025
View reviewed changes
@andriyDev andriyDev force-pushed the src-proc-ambiguity-tests branch from 807168a to daa638d Compare October 12, 2025 05:53
ChristopherBiscardi
ChristopherBiscardi approved these changes Oct 12, 2025
View reviewed changes
Copy link
Contributor

@ChristopherBiscardi ChristopherBiscardi left a comment

Choose a reason for hiding this comment

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

awesome, thanks for the test change!

@andriyDev andriyDev 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 12, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 13, 2025
Merged via the queue into bevyengine:main with commit d5e450c Oct 13, 2025
38 checks passed
mate-h pushed a commit to mate-h/bevy that referenced this pull request Oct 22, 2025
@andriyDev @mate-h
Convert ProcessorTransactionLog into a trait. (bevyengine#21476)
ae47099 
# Objective

- There was a TODO to make this type actually a trait.
- bevyengine#21409 introduced a test flake on WIndows (since the transaction log
file may not have been fully released by the time the next test ran),
and made tests create the `crates/bevy_asset/imported_assets/log` file.

## Solution

- Create two traits: `ProcessorTransactionLogFactory` and
`ProcessorTransactionLog`. The former creates the latter.
- Rewrite the test helper for asset processing to use a fake transaction
log that doesn't do anything.
- Note: pretty much the entire implementation was just reformatted into
the trait.
- Note: these transaction log methods are returning `BevyError` instead
of something like `std::io::Error`.

## Testing

- I ran the asset tests on repeat for a while (including with bevyengine#21433
where the flake was first seen and I was able to reproduce fairly
quickly). No issues!

Note: we could make a release notes for the fact that users can make
their own transaction logs, but this feature is primarily for unit
testing. Maybe a user could make a more robust transaction log, but it's
really not clear that it would be important for anyone.
mate-h pushed a commit to mate-h/bevy that referenced this pull request Oct 22, 2025
@andriyDev @mate-h
Add tests for asset processing to show the problem with bevyengine#21269
787f7ad 
. (bevyengine#21433)

# Objective

- Make the problem with bevyengine#21269 more concrete.

## Solution

- Add a test to show that loaders during processing can read processed
assets.
- Add an ignored test to show that loaders during processing cannot
currently read source assets.
- This also tests that nested asset loads during processing actually
works.

## Testing

- Oops! All tests!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@ChristopherBiscardi ChristopherBiscardi ChristopherBiscardi approved these changes

@janis-bhm janis-bhm janis-bhm 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.

4 participants

@andriyDev @ChristopherBiscardi @janis-bhm @alice-i-cecile