Add tests for asset processing under hot reloading and across asset sources.#21673
Conversation
andriyDev
added
A-Assets
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
Not sure what Pixel Eagle is talking about. The link doesn't point to any images...? Either way, it's not relevant to this PR because it literally changes no behaviour, it only changes the test file for processing. |
| // If the original source changes through an AssetSourceEvent, we'll be racing (on | ||
| // multithreaded) between this and processor thread switching the state to `Processing`. So do a | ||
| // fixed number of iterations so the processor thread is likely to win. | ||
| for _ in 0..5 { | ||
| app.update(); | ||
| } |
There was a problem hiding this comment.
If this is at all flaky it's going to be really annoying for everyone. If it is flaky then we should find some other way of preventing this race, if it's not flaky I think the comment should be reworded to explain why.
There was a problem hiding this comment.
Fixing this properly is very rough. We could fix this by gating the Process (so not a generic solution), then having a specialized run_app_until_processing_then_finished (we'd need to also have this tick the gate, which sucks).
I haven't detected any flakes. This is just me being paranoid, especially since I know Github CI machines are much slower than my machine. So this is me being somewhat cautious haha.
I'm not sure if a "proper" fix is worth the complexity. Wdyt?
There was a problem hiding this comment.
I feel like we'll end up investing in some asset specific testing infrastructure at some point, similarly to how rendering has screenshots and a way to step a certain number of frames. Flaky tests are really annoying for other contributors because they need to retrigger runs and might spend time investigating if the failures are their fault.
But I guess we can wait until we see it fail at least once.
andriyDev
force-pushed
the
processor-sources-tests
branch
from
ecdb663 to
6387c2b
Compare
…ate changes unconditionally.
andriyDev
force-pushed
the
processor-sources-tests
branch
from
6387c2b to
3ef757b
Compare
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
1 similar comment
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
greeble-dev
left a comment
greeble-dev
left a comment
There was a problem hiding this comment.
I'm not familiar with asset processing, but I'm clicking approve as I couldn't spot any issues and the tests passed (Win10). Added just one non-blocking style issue.
I wasn't entirely sure how to run the tests in both single and multi threaded - hopefully this is right:
cargo test -p bevy_asset
cargo test -p bevy_asset --features multi_threaded| let processed_memory_writer = MemoryAssetWriter { | ||
| root: processed_dir.clone(), | ||
| }; | ||
| struct UnfinishedProcessingDirs { |
There was a problem hiding this comment.
Nitpick/style issue: I find this hard to read with UnfinishedProcessingDirs and create_source nested inside create_app_with_asset_processor. Would prefer them to be at file level.
There was a problem hiding this comment.
I intentionally don't want to do this because it already "conflicts" conceptually with ProcessingDirs, and this type is only necessary so we can block on fetching the sender (which is a hack I want to keep "hidden" in the implementation).
greeble-dev
added
S-Ready-For-Final-Review
# Objective - Fixes flaky asset processing tests introduced in #21673. - Previously the hot-reloading processor tests would just check for the finished processor state. However it was possible for processing to not start quickly enough in multi_threaded, resulting in the test still seeing the state as finished. ## Solution - Use a lock to block reading the source directory until after processing has started. This way the asset processor can't even check if there's work to do until the guard is released, so we can't miss the processing state. ## Testing - Ran it locally with singlethreaded and multithreaded mode.
5 participants
Objective
Solution
Testing