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

fix max_level_hint for empty cases for Option/Vec Subscribe impls #2195

Merged
merged 10 commits into from
Jul 1, 2022
9 changes: 7 additions & 2 deletions tracing-subscriber/src/subscribe/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1485,7 +1485,11 @@ where
fn max_level_hint(&self) -> Option<LevelFilter> {
match self {
Some(ref inner) => inner.max_level_hint(),
None => None,
None => {
// There is no inner subscriber, so this subscriber will
// never enable anything.
Some(LevelFilter::OFF)
}
}
}

Expand Down Expand Up @@ -1690,7 +1694,8 @@ feature! {
}

fn max_level_hint(&self) -> Option<LevelFilter> {
let mut max_level = LevelFilter::ERROR;
// Default to `OFF` if there are no underlying subscribers
let mut max_level = LevelFilter::OFF;
for s in self {
// NOTE(eliza): this is slightly subtle: if *any* subscriber
// returns `None`, we have to return `None`, assuming there is
Expand Down
41 changes: 41 additions & 0 deletions tracing-subscriber/tests/option.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#![cfg(feature = "registry")]
use tracing::level_filters::LevelFilter;
hawkw marked this conversation as resolved.
Show resolved Hide resolved
use tracing::Collect;
use tracing_subscriber::prelude::*;

// This test is just used to compare to the tests below
#[test]
fn just_subscriber() {
let collector = tracing_subscriber::registry().with(LevelFilter::INFO);
assert_eq!(collector.max_level_hint(), Some(LevelFilter::INFO));
}

#[test]
fn subscriber_and_option_some_subscriber() {
let collector = tracing_subscriber::registry()
.with(LevelFilter::INFO)
.with(Some(LevelFilter::DEBUG));
assert_eq!(collector.max_level_hint(), Some(LevelFilter::DEBUG));
}

#[test]
fn subscriber_and_option_none_subscriber() {
// None means the other layer takes control
let collector = tracing_subscriber::registry()
.with(LevelFilter::ERROR)
.with(None::<LevelFilter>);
assert_eq!(collector.max_level_hint(), Some(LevelFilter::ERROR));
}

#[test]
fn just_option_some_subscriber() {
// Just a None means everything is off
let collector = tracing_subscriber::registry().with(None::<LevelFilter>);
assert_eq!(collector.max_level_hint(), Some(LevelFilter::OFF));
}

#[test]
fn just_option_none_subscriber() {
let collector = tracing_subscriber::registry().with(Some(LevelFilter::ERROR));
assert_eq!(collector.max_level_hint(), Some(LevelFilter::ERROR));
}
7 changes: 7 additions & 0 deletions tracing-subscriber/tests/subscriber_filters/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,10 @@ fn all_filtered_max_level_hint() {

assert_eq!(collector.max_level_hint(), Some(LevelFilter::DEBUG));
}

#[test]
fn empty_vec() {
// Just a None means everything is off
let collector = tracing_subscriber::registry().with(Vec::<ExpectSubscriber>::new());
assert_eq!(collector.max_level_hint(), Some(LevelFilter::OFF));
}
guswynn marked this conversation as resolved.
Show resolved Hide resolved
19 changes: 19 additions & 0 deletions tracing-subscriber/tests/vec.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#![cfg(feature = "registry")]
use tracing::level_filters::LevelFilter;
hawkw marked this conversation as resolved.
Show resolved Hide resolved
use tracing::Collect;
use tracing_subscriber::prelude::*;

#[test]
fn just_empty_vec() {
// Just a None means everything is off
let collector = tracing_subscriber::registry().with(Vec::<LevelFilter>::new());
assert_eq!(collector.max_level_hint(), Some(LevelFilter::OFF));
}

#[test]
fn subscriber_and_empty_vec() {
let collector = tracing_subscriber::registry()
.with(LevelFilter::INFO)
.with(Vec::<LevelFilter>::new());
assert_eq!(collector.max_level_hint(), Some(LevelFilter::INFO));
}