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

Utility functions for integration testing #3839

Closed

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Feb 1, 2022

Objective

Solution

  • Demonstrate how to integration test apps effectively
  • Implement some related helpers on World

Limitations

Input mocking has not been included. Fundamentally; we need an abstraction over the input methods to handle this nicely.
See https://github.com/Leafwing-Studios/leafwing-input-manager/blob/main/src/input_mocking.rs for an external implementation of this.

Working with queries in integration tests still fundamentally sucks, because of the need to store query state somewhere. As a result, I've included some uncomfortably special-cased helpers.

TODO

  • FIx CI
  • Add set_component
  • Add run_system
  • Hide methods behind a trait that's not in the prelude
    fn set_component<C, F>(&mut self, value: &C)
    where
        C: Component + PartialEq + Debug + Clone,
        F: WorldQuery,
        <F as WorldQuery>::Fetch: FilterFetch,
    {
        let mut query_state = self.world.query_filtered::<&mut C, F>();
        for mut component in query_state.iter_mut(&mut self.world) {
            if *component != *value {
                *component = value.clone();
            }
        }
    }

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 1, 2022
@alice-i-cecile alice-i-cecile added A-App Bevy apps and plugins A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Feb 1, 2022
@shanesveller
Copy link

Very interested in this topic. In the past I've used a minimal App, a setup startup system and an examine system in CoreStage::Last, along with an enum resource to define common initial scenarios. Then I add the systems under test to the other stages as appropriate, set it to RunOnce and let 'er rip. It seems relatively effective, but that definitely winds up being quite a lot to put in one test method, both cognitively and probably for runtime cost as well.

@alice-i-cecile alice-i-cecile marked this pull request as ready for review February 1, 2022 22:32
@alice-i-cecile
Copy link
Member Author

@shanesveller This PR is now ready for review; I'd love to hear what you think of the end result.

@james7132
Copy link
Member

Overall, I think this is very useful, but this seems like something that should be behind a #[cfg(test)] conditional, or separated out entirely an optional dev-dependency. It's definitely not optimal to be compiling testing code into production binaries, even if it gets optimized out.

@alice-i-cecile
Copy link
Member Author

Overall, I think this is very useful, but this seems like something that should be behind a #[cfg(test)] conditional, or separated out entirely an optional dev-dependency. It's definitely not optimal to be compiling testing code into production binaries, even if it gets optimized out.

That's an interesting idea. I think that I agree with you on the assertions, but the other helpers have important non-testing uses (even if they don't enable new functionality). I'll shuffle this around.

@mockersf
Copy link
Member

mockersf commented Feb 5, 2022

I would prefer those methods to not be available during standard development, but they should be documented on docs.rs. I think the best way to have both would be to have everything in a bevy_test crate and use it only behind a #[cfg(test)]. But that seems not very nice to do... So my next preference would be to have disclaimers in the docs that those methods should be used for test purpose only.

query_len

aka the one I dislike the less... Ok I guess it's nice to hide the query building in an helper.

send_event and events_len

in both those cases, I find the Events<E> struct to be easy enough already to work with. I would prefer an helper method on World to get the events struct, and its friend on App, and then showing it's easy to call send or len on it. As it's limited in scope to tests, exposing a mut getter on the events resource should be fine.

assert_component_eq

I find the name very confusing, and the functionality very limited.

  • naming: it doesn't have any plural in it
  • functionality: limited to eq on the whole component, what if I wanted to pass a closure that would have a custom check

I think what I would like is a method returning an iterator on the result of the query on that component with the filter that are passed as type param, so something like fn get_components<C, F>(&mut self) -> impl Iterator<Item = C>. Then I can use all the tools on iter to do my checks.

assert_system

This one is hard to understand from the name. I don't have a good suggestion though. It also can't be used with any system as it requires one that output a Result, which is not somethings that's nice to use in Bevy by default. I don't really see the point of testing a system that would be built only for tests. Unless we start to push for systems returning a Result much more widely in Bevy, I don't see this being really useful...

After reading the example integration test

I have even a strong feeling that what would be really useful is hiding the complexity of getting an iterator from a query which is something you often do:

So merging query_len and assert_component_eq into an easy helper to get the iterator from a query.

About the example itself used in the test, I don't think it's a good idea to show something that is using Velocity or Gravity without checking time at any point. If someone was to copy this code as a basis, it would work in unexpected way.

@mockersf
Copy link
Member

mockersf commented Feb 5, 2022

an examine system in CoreStage::Last

That could be an interesting pattern to show for tests: put all your invariants check in a system, and run that systems after an update

@alice-i-cecile
Copy link
Member Author

I think I agree with basically all of your proposals. I'll make those changes and then re-request review :)

@alice-i-cecile alice-i-cecile marked this pull request as draft February 7, 2022 04:58
@alice-i-cecile alice-i-cecile marked this pull request as ready for review February 7, 2022 20:19
@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Feb 7, 2022

Alright, so I think this is the best we can do for now. Fundamentally, I want to be able to query the world in a single line, but, because query is assumed to have a cached state stored somewhere, this is impossible.

The only decent work-around is to include special-cased helper methods, to reduce the intense boilerplate of checking "is this value what I expect".

In the long run, I very much want a better solution, but that will involve much more serious design work and engine refactoring.

@alice-i-cecile
Copy link
Member Author

Closing this out in favor of something more scoped.

@alice-i-cecile alice-i-cecile added the A-Diagnostics Logging, crash handling, error reporting and performance analysis label Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins A-Diagnostics Logging, crash handling, error reporting and performance analysis A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants