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

Add better scene tests #6834

Open
3 tasks
MrGVSV opened this issue Dec 3, 2022 · 4 comments
Open
3 tasks

Add better scene tests #6834

MrGVSV opened this issue Dec 3, 2022 · 4 comments
Labels
A-Scenes Serialized ECS data stored on the disk C-Testing A change that impacts how we test Bevy or how users test their apps

Comments

@MrGVSV
Copy link
Member

MrGVSV commented Dec 3, 2022

What problem does this solve or what need does it fill?

When 0.9 was initially released, there were some scene-related bugs that went unnoticed. These included:

Unfortunately, these were not rare edge cases that slipped through. These were just the result of people using the new scene format in actual projects.

The reason we didn't catch these bugs right away is that our current tests don't relate to real-world scenarios— or at least not a decent handful of them.

What solution would you like?

In the tests folder in the root of the project, we should include some tests for scenes. These tests should generally:

  1. Contain moderately complex structures (deep hierarchies, Vec/HashMap usage, etc.)
  2. Use a fair amount of Bevy built-in components
  3. Use differently "shaped" components (tuple structs, unit structs, enums, etc.)

Additionally, they should all ensure that we can:

  1. Serialize to a string/file
  2. Deserialize from a string/file

While we do want to have really complex scenes, we should also try to break things up so we can isolate particular areas of interest. So here are some of the test files I think we should have:

  • scenes/relations.rs - Focus on hierarchies and basic relations (e.g. struct Pet(Entity)).
  • scenes/ui.rs - Focus on (de)serializing UI (if possible).
  • scenes/components.rs - Focus on components of all shapes and sizes— both from Bevy and custom-made.

These are not a requirement and there may be more tests (or fewer) that we want to add. The basic idea is to at least have enough that we can be confident changes to the scene format or other code won't be immediately broken at the next release.

What alternative(s) have you considered?

There are a couple alternatives we could try:

  1. Create new examples
  2. Add to the crate-level tests

The reason why I don't think we should go with the first one is that these really need to be more like tests: slightly more complex, checking edge cases, doing multiple things, etc. I don't think examples are really meant for that and wouldn't really benefit those reading through it.

And the reason I don't think we should go with the second is that we're not really using the same API as users. Not only might there be differences in how the API is used, but we might be missing some types/components that we can't pull in without adding them as a dependency. We could maybe do it, but I think just handling it in the tests directory is going to be much simpler/more accurate.

Additional context

Reference comment: #6580 (comment)

@MrGVSV MrGVSV added C-Testing A change that impacts how we test Bevy or how users test their apps A-Scenes Serialized ECS data stored on the disk labels Dec 3, 2022
@paul-hansen
Copy link
Contributor

Also related, sprites don't currently save to scenes out of the box: #3746 (comment) Probably a good one to have a test for.

@MrGVSV
Copy link
Member Author

MrGVSV commented Dec 3, 2022

Also related, sprites don't currently save to scenes out of the box: #3746 (comment) Probably a good one to have a test for.

If I recall, serializing and deserializing handles currently aren't well supported. I suspect that could cause issues when trying to incorporate sprites 🤔

@paul-hansen
Copy link
Contributor

paul-hansen commented Dec 3, 2022

Ah yeah you're right, I forgot I had that working using a custom AssetHandle component I replace handles with when saving scenes that loads the handle from a relative path when loaded.
Edit: Maybe we just skip comparing handles in the tests for now? Could still check that the rest of Sprite works. Not a big deal though.

@MrGVSV
Copy link
Member Author

MrGVSV commented Dec 3, 2022

Maybe we just skip comparing handles in the tests for now? Could still check that the rest of Sprite works. Not a big deal though.

We can consider it, but it might be difficult to justify testing that something only partially works. On the other hand, though, if we make sure to leave a comment indicating this, it might not be a bad idea to just ensure general serialization/deserialization works on the other parts of the component (especially to test things like Anchor and Option<Rect>).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Testing A change that impacts how we test Bevy or how users test their apps
Projects
Status: Open
Development

No branches or pull requests

2 participants