From 4c96f55ac1697f4616eb9224065996f22e38d6dc Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Wed, 25 Sep 2024 23:46:41 -0700 Subject: [PATCH 1/5] test: Verbosity calculatio boundary conditions Adds tests for the verbosity calculation behavior, including characterization tests that show the current behavior is incorrectly calculated when u8::MAX is passed to either verbose or quiet. --- src/lib.rs | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 9959aae..363cac3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -235,4 +235,100 @@ mod test { use clap::CommandFactory; Cli::command().debug_assert(); } + + #[test] + fn verbosity_error_level() { + let tests = [ + // verbose, quiet, expected_level, expected_filter + (0, 0, Some(Level::Error), LevelFilter::Error), + (1, 0, Some(Level::Warn), LevelFilter::Warn), + (2, 0, Some(Level::Info), LevelFilter::Info), + (3, 0, Some(Level::Debug), LevelFilter::Debug), + (4, 0, Some(Level::Trace), LevelFilter::Trace), + (5, 0, Some(Level::Trace), LevelFilter::Trace), + (255, 0, None, LevelFilter::Off), + (0, 1, None, LevelFilter::Off), + (0, 2, None, LevelFilter::Off), + (0, 255, Some(Level::Warn), LevelFilter::Warn), + (255, 255, Some(Level::Error), LevelFilter::Error), + ]; + + for (verbose, quiet, expected_level, expected_filter) in tests.iter() { + let v = Verbosity::::new(*verbose, *quiet); + assert_eq!( + v.log_level(), + *expected_level, + "verbose = {verbose}, quiet = {quiet}" + ); + assert_eq!( + v.log_level_filter(), + *expected_filter, + "verbose = {verbose}, quiet = {quiet}" + ); + } + } + + #[test] + fn verbosity_warn_level() { + let tests = [ + // verbose, quiet, expected_level, expected_filter + (0, 0, Some(Level::Warn), LevelFilter::Warn), + (1, 0, Some(Level::Info), LevelFilter::Info), + (2, 0, Some(Level::Debug), LevelFilter::Debug), + (3, 0, Some(Level::Trace), LevelFilter::Trace), + (4, 0, Some(Level::Trace), LevelFilter::Trace), + (255, 0, Some(Level::Error), LevelFilter::Error), + (0, 1, Some(Level::Error), LevelFilter::Error), + (0, 2, None, LevelFilter::Off), + (0, 3, None, LevelFilter::Off), + (0, 255, Some(Level::Info), LevelFilter::Info), + (255, 255, Some(Level::Warn), LevelFilter::Warn), + ]; + + for (verbose, quiet, expected_level, expected_filter) in tests.iter() { + let v = Verbosity::::new(*verbose, *quiet); + assert_eq!( + v.log_level(), + *expected_level, + "verbose = {verbose}, quiet = {quiet}" + ); + assert_eq!( + v.log_level_filter(), + *expected_filter, + "verbose = {verbose}, quiet = {quiet}" + ); + } + } + + #[test] + fn verbosity_info_level() { + let tests = [ + // verbose, quiet, expected_level, expected_filter + (0, 0, Some(Level::Info), LevelFilter::Info), + (1, 0, Some(Level::Debug), LevelFilter::Debug), + (2, 0, Some(Level::Trace), LevelFilter::Trace), + (3, 0, Some(Level::Trace), LevelFilter::Trace), + (255, 0, Some(Level::Warn), LevelFilter::Warn), + (0, 1, Some(Level::Warn), LevelFilter::Warn), + (0, 2, Some(Level::Error), LevelFilter::Error), + (0, 3, None, LevelFilter::Off), + (0, 4, None, LevelFilter::Off), + (0, 255, Some(Level::Debug), LevelFilter::Debug), + (255, 255, Some(Level::Info), LevelFilter::Info), + ]; + + for (verbose, quiet, expected_level, expected_filter) in tests.iter() { + let v = Verbosity::::new(*verbose, *quiet); + assert_eq!( + v.log_level(), + *expected_level, + "verbose = {verbose}, quiet = {quiet}" + ); + assert_eq!( + v.log_level_filter(), + *expected_filter, + "verbose = {verbose}, quiet = {quiet}" + ); + } + } } From 02ddbeff5eaa98ac95aef19b519e6dfec0e08a97 Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Wed, 25 Sep 2024 23:54:32 -0700 Subject: [PATCH 2/5] refactor: Shift numeric level to be 0 based --- src/lib.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 363cac3..48a9dc1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -133,23 +133,23 @@ impl Verbosity { fn level_value(level: Option) -> i8 { match level { - None => -1, - Some(Level::Error) => 0, - Some(Level::Warn) => 1, - Some(Level::Info) => 2, - Some(Level::Debug) => 3, - Some(Level::Trace) => 4, + None => 0, + Some(Level::Error) => 1, + Some(Level::Warn) => 2, + Some(Level::Info) => 3, + Some(Level::Debug) => 4, + Some(Level::Trace) => 5, } } fn level_enum(verbosity: i8) -> Option { match verbosity { - i8::MIN..=-1 => None, - 0 => Some(Level::Error), - 1 => Some(Level::Warn), - 2 => Some(Level::Info), - 3 => Some(Level::Debug), - 4..=i8::MAX => Some(Level::Trace), + i8::MIN..=0 => None, + 1 => Some(Level::Error), + 2 => Some(Level::Warn), + 3 => Some(Level::Info), + 4 => Some(Level::Debug), + 5..=i8::MAX => Some(Level::Trace), } } From 4e3f4e59ac5f6881f2acda69263281e3777fe1df Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Wed, 25 Sep 2024 23:54:32 -0700 Subject: [PATCH 3/5] refactor: Break out parts of verbosity calculation --- src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 48a9dc1..afb98ce 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -127,7 +127,9 @@ impl Verbosity { } fn verbosity(&self) -> i8 { - level_value(L::default()) - (self.quiet as i8) + (self.verbose as i8) + let default_verbosity = level_value(L::default()); + let verbosity = default_verbosity - (self.quiet as i8) + (self.verbose as i8); + verbosity } } From c684b6f0f453d29fedbbde204e19cdd6b140531e Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Wed, 25 Sep 2024 23:54:32 -0700 Subject: [PATCH 4/5] refactor: Track numeric levels as a u8 --- src/lib.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index afb98ce..79c741a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -126,14 +126,14 @@ impl Verbosity { self.log_level().is_none() } - fn verbosity(&self) -> i8 { + fn verbosity(&self) -> u8 { let default_verbosity = level_value(L::default()); - let verbosity = default_verbosity - (self.quiet as i8) + (self.verbose as i8); - verbosity + let verbosity = default_verbosity as i8 - (self.quiet as i8) + (self.verbose as i8); + verbosity as u8 } } -fn level_value(level: Option) -> i8 { +fn level_value(level: Option) -> u8 { match level { None => 0, Some(Level::Error) => 1, @@ -144,14 +144,14 @@ fn level_value(level: Option) -> i8 { } } -fn level_enum(verbosity: i8) -> Option { +fn level_enum(verbosity: u8) -> Option { match verbosity { - i8::MIN..=0 => None, + 0 => None, 1 => Some(Level::Error), 2 => Some(Level::Warn), 3 => Some(Level::Info), 4 => Some(Level::Debug), - 5..=i8::MAX => Some(Level::Trace), + 5..=u8::MAX => Some(Level::Trace), } } @@ -250,7 +250,7 @@ mod test { (5, 0, Some(Level::Trace), LevelFilter::Trace), (255, 0, None, LevelFilter::Off), (0, 1, None, LevelFilter::Off), - (0, 2, None, LevelFilter::Off), + (0, 2, Some(Level::Trace), LevelFilter::Trace), (0, 255, Some(Level::Warn), LevelFilter::Warn), (255, 255, Some(Level::Error), LevelFilter::Error), ]; @@ -282,7 +282,7 @@ mod test { (255, 0, Some(Level::Error), LevelFilter::Error), (0, 1, Some(Level::Error), LevelFilter::Error), (0, 2, None, LevelFilter::Off), - (0, 3, None, LevelFilter::Off), + (0, 3, Some(Level::Trace), LevelFilter::Trace), (0, 255, Some(Level::Info), LevelFilter::Info), (255, 255, Some(Level::Warn), LevelFilter::Warn), ]; @@ -314,7 +314,7 @@ mod test { (0, 1, Some(Level::Warn), LevelFilter::Warn), (0, 2, Some(Level::Error), LevelFilter::Error), (0, 3, None, LevelFilter::Off), - (0, 4, None, LevelFilter::Off), + (0, 4, Some(Level::Trace), LevelFilter::Trace), (0, 255, Some(Level::Debug), LevelFilter::Debug), (255, 255, Some(Level::Info), LevelFilter::Info), ]; From 993695846bac406038659b1f8cc94cd0943bb950 Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Wed, 25 Sep 2024 23:54:32 -0700 Subject: [PATCH 5/5] fix: Don't overflow on boundary conditions Prior to this change, constructing a `Verbosity` with either parameter equal to `u8::MAX` would result in incorrect verbosity level calculation. --- src/lib.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 79c741a..72ab300 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -128,8 +128,8 @@ impl Verbosity { fn verbosity(&self) -> u8 { let default_verbosity = level_value(L::default()); - let verbosity = default_verbosity as i8 - (self.quiet as i8) + (self.verbose as i8); - verbosity as u8 + let verbosity = default_verbosity as i16 - self.quiet as i16 + self.verbose as i16; + verbosity.clamp(0, u8::MAX as i16) as u8 } } @@ -248,10 +248,10 @@ mod test { (3, 0, Some(Level::Debug), LevelFilter::Debug), (4, 0, Some(Level::Trace), LevelFilter::Trace), (5, 0, Some(Level::Trace), LevelFilter::Trace), - (255, 0, None, LevelFilter::Off), + (255, 0, Some(Level::Trace), LevelFilter::Trace), (0, 1, None, LevelFilter::Off), - (0, 2, Some(Level::Trace), LevelFilter::Trace), - (0, 255, Some(Level::Warn), LevelFilter::Warn), + (0, 2, None, LevelFilter::Off), + (0, 255, None, LevelFilter::Off), (255, 255, Some(Level::Error), LevelFilter::Error), ]; @@ -279,11 +279,11 @@ mod test { (2, 0, Some(Level::Debug), LevelFilter::Debug), (3, 0, Some(Level::Trace), LevelFilter::Trace), (4, 0, Some(Level::Trace), LevelFilter::Trace), - (255, 0, Some(Level::Error), LevelFilter::Error), + (255, 0, Some(Level::Trace), LevelFilter::Trace), (0, 1, Some(Level::Error), LevelFilter::Error), (0, 2, None, LevelFilter::Off), - (0, 3, Some(Level::Trace), LevelFilter::Trace), - (0, 255, Some(Level::Info), LevelFilter::Info), + (0, 3, None, LevelFilter::Off), + (0, 255, None, LevelFilter::Off), (255, 255, Some(Level::Warn), LevelFilter::Warn), ]; @@ -310,12 +310,12 @@ mod test { (1, 0, Some(Level::Debug), LevelFilter::Debug), (2, 0, Some(Level::Trace), LevelFilter::Trace), (3, 0, Some(Level::Trace), LevelFilter::Trace), - (255, 0, Some(Level::Warn), LevelFilter::Warn), + (255, 0, Some(Level::Trace), LevelFilter::Trace), (0, 1, Some(Level::Warn), LevelFilter::Warn), (0, 2, Some(Level::Error), LevelFilter::Error), (0, 3, None, LevelFilter::Off), - (0, 4, Some(Level::Trace), LevelFilter::Trace), - (0, 255, Some(Level::Debug), LevelFilter::Debug), + (0, 4, None, LevelFilter::Off), + (0, 255, None, LevelFilter::Off), (255, 255, Some(Level::Info), LevelFilter::Info), ];