bevy
bevy copied to clipboard
bevyengine
Avoid unwrap log initialization
What problem does this solve or what need does it fill?
I would like to test heavily my application. Obviously my test suite is composed by multiple test and every test create a new through App::new() and register the plugin I need to the new app instance.
Because logs are important I really would like keep them active during the tests runs.
What solution would you like?
Remove the unwrap from here https://github.com/bevyengine/bevy/blob/5e2cfb2f19e0e2287fd48d55df92f78617acf831/crates/bevy_log/src/lib.rs#L127 substituting it with a different error handling.
What alternative(s) have you considered?
Remove LogPlugin from my tests
Additional context
// test 1
let mut app = App::new();
app.add_plugin(LogPlugin::default());
// test 2
let mut app = App::new();
app.add_plugin(LogPlugin::default()); // <----- this
Error log:
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: SetLoggerError(())', .......\.cargo\registry\src\github.com-1ecc6299db9ec823\bevy_log-0.7.0\src\lib.rs:127:27
when you are using multiple app (or in test), you should setup login yourself, and not add the LogPlugin several time. That way you would have unified login and no panic 🙂
Hi, thanks for your comment.
I wouldn't like to configure by myself the log part because:
- this is not how other plugins work. In fact I'm able to adding them in different tests and all works fine
- tests can not behave like the debug/release build doesp
- copy and paste the code from bevy code to my one just for removing an
unwrapis not so ok - could be a messy managing a own version for future changes
If my solution (adds a flag force) is not the best one, we can leverage on #[config(test)] to avoid the unwrap.
we can leverage on #[config(test)] to avoid the unwrap.
#[cfg(test)] is only enabled for the crate actually being tested, and not for it's dependencies like bevy_log.
unwrap in a library still seems to me an ugly stuff...
Which approach we can take?
Just for knowing if you are interested in this enhancement or no....
Honestly to me it feels like LogPlugin shouldn't be a plugin in the first place as it mutates global state and doesn't affect the App at all. Instead I feel like it should be a bevy::log::setup() call at the start of main. This would have a negative effect on ergonomics in case only a single App is used though.
Fair enough. If you want, I can submit a PR for that
please do. having to change it locally is annoying.
I tried changing LogTracer::init().unwrap(); to if let Ok(_) = LogTracer::init() { here: https://github.com/anchpop/bevy/commit/ec8713b0691b2dfd9441e8a45f7a8d6066b2de51#diff-ce0ef3820ac31e1eaf71e97972a68d832239ca8affc9441f6dd419f434d15781R125
It does work at preventing it from panicking, but the problem is that it seems to be circumventing whatever magic cargo test does to suppress output to stdout when --nocapture isn't passed to the test binary. So I get noise like this in my tests:
2022-09-10T23:10:16.374437Z INFO my_game::file: test log
2022-09-10T23:10:16.375310Z INFO my_game::file: test log // ← noise from logs
test tests::change_tests::test::my_test ... ok // ← test output
2022-09-10T23:10:16.375938Z INFO my_game::file: test log // ← noise from logs
2022-09-10T23:10:16.376509Z INFO my_game::file: test log
2022-09-10T23:10:16.376962Z INFO my_game::file: test log
@bjorn3 do you know of a solution to this? I would be happy to make a PR if so, since would go well with #5931.
Maybe the same thread pool is reused by multiple tests and once the test spawning the thread pool tests initially libtest doesn't know to which test to attribute the output anymore?
