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.

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 by Bors] - Add unit test with system that panics #7491

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Add unit test with system that panics #7491

wants to merge 1 commit into from

Conversation

bjrnt
Copy link
Contributor

@bjrnt bjrnt commented Feb 3, 2023

Objective

Fixes #7434.

This is my first time contributing to a Rust project, so please let me know if this wasn't the change intended by the linked issue.

Solution

Adds a test with a system that panics to bevy_ecs.

I'm not sure if this is the intended panic message, but this is what the test currently results in:

thread 'system::tests::panic_inside_system' panicked at 'called `Option::unwrap()` on a `None` value', /Users/bjorn/workplace/bevy/crates/bevy_tasks/src/task_pool.rs:354:49

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change A-Animation Make things move and change over time C-Testing A change that impacts how we test Bevy or how users test their apps and removed A-Animation Make things move and change over time C-Code-Quality A section of code that is hard to understand or change labels Feb 3, 2023
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 3, 2023
@alice-i-cecile
Copy link
Member

Thanks, this is great!

bors r+

bors bot pushed a commit that referenced this pull request Feb 3, 2023
@bjrnt
Add unit test with system that panics (#7491)
6506ea4 
# Objective

Fixes #7434.

This is my first time contributing to a Rust project, so please let me know if this wasn't the change intended by the linked issue.

## Solution

Adds a test with a system that panics to `bevy_ecs`.

I'm not sure if this is the intended panic message, but this is what the test currently results in:
```
thread 'system::tests::panic_inside_system' panicked at 'called `Option::unwrap()` on a `None` value', /Users/bjorn/workplace/bevy/crates/bevy_tasks/src/task_pool.rs:354:49
```
@bors
Copy link
Contributor

bors bot commented Feb 3, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add unit test with system that panics [Merged by Bors] - Add unit test with system that panics Feb 3, 2023
@bors bors bot closed this Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james7132 james7132 james7132 approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Testing A change that impacts how we test Bevy or how users test their apps S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add panicking system test to bevy_ecs
3 participants
@bjrnt @alice-i-cecile @james7132