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

Check for conflicting accesses in assert_is_system #8154

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Mar 22, 2023

Objective

The function assert_is_system is used in documentation tests to ensure that example code actually produces valid systems. Currently, assert_is_system just checks that each function parameter implements SystemParam. To further check the validity of the system, we should initialize the passed system so that it will be checked for conflicting accesses. Not only does this enforce the validity of our examples, but it provides a convenient way to demonstrate conflicting accesses via a should_panic example, which is nicely rendered by rustdoc:

should_panic example

Solution

Initialize the system with an empty world to trigger its internal access conflict checks.


Changelog

The function bevy::ecs::system::assert_is_system now panics when passed a system with conflicting world accesses, as does assert_is_read_only_system.

Migration Guide

The functions assert_is_system and assert_is_read_only_system (in bevy_ecs::system) now panic if the passed system has invalid world accesses. Any tests that called this function on a system with invalid accesses will now fail. Either fix the system's conflicting accesses, or specify that the test is meant to fail:

  1. For regular tests (that is, functions annotated with #[test]), add the #[should_panic] attribute to the function.
  2. For documentation tests, add should_panic to the start of the code block: ```should_panic

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Testing A change that impacts how we test Bevy or how users test their apps M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 22, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Good docs, and good functionality.

Copy link
Contributor

@Pietrek14 Pietrek14 left a comment

Choose a reason for hiding this comment

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

LGTM

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 22, 2023

Thank you for the reviews. I have filled in the migration guide.

@james7132 james7132 added this to the 0.11 milestone Mar 22, 2023
@james7132 james7132 added this pull request to the merge queue Mar 22, 2023
@james7132 james7132 merged commit daa1b02 into bevyengine:main Mar 22, 2023
@JoJoJet JoJoJet deleted the assert_check_access branch March 22, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Testing A change that impacts how we test Bevy or how users test their apps M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants