Merge pull request #126 from ClementTsang/update_tests

Update arg test; add config tests

.travis.yml

@@ -49,6 +49,7 @@ notifications:
     on_success: never
 
 before_deploy:
+  - rustup update
   - cargo install --path . --target $TARGET
   - cargo update
   - |

CHANGELOG.md

@@ -29,6 +29,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Removed an (undocumented) feature in allowing modifying total RX/TX colours. This is mainly due to the legend change.
 
+- Updated error messages to be a bit more consistent/helpful.
+
 - [#117](https://github.com/ClementTsang/bottom/issues/117): Update tui to 0.9:
 
   - Use custom legend-hiding to stop hiding legends for memory and network widgets.
@@ -49,6 +51,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixed bug where a single empty row as a layout would crash without a proper warning.
   The behaviour now errors out with a more helpful message.
 
+### Other
+
+- Updated tests and added config testing.
+
 ## [0.3.0] - 2020-04-07
 
 ### Features

src/app/layout_manager.rs

@@ -933,7 +933,7 @@ impl std::str::FromStr for BottomWidgetType {
             "empty" => Ok(BottomWidgetType::Empty),
             "battery" | "batt" => Ok(BottomWidgetType::Battery),
             _ => Err(BottomError::ConfigError(format!(
-                "Invalid widget type: {}",
+                "invalid widget type: {}",
                 s
             ))),
         }

src/canvas/canvas_colours.rs

@@ -166,7 +166,8 @@ impl CanvasColours {
     pub fn set_battery_colours(&mut self, colours: &[String]) -> error::Result<()> {
         if colours.is_empty() {
             Err(error::BottomError::ConfigError(
-                "Battery colour list must have at least one colour!".to_string(),
+                "invalid colour config: battery colour list must have at least one colour!"
+                    .to_string(),
             ))
         } else {
             let generated_colours: Result<Vec<_>, _> = colours

src/canvas/canvas_colours/colour_utils.rs

@@ -96,17 +96,34 @@ pub fn gen_n_styles(num_to_gen: i32) -> Vec<Style> {
 }
 
 pub fn convert_hex_to_color(hex: &str) -> error::Result<Color> {
+    fn hex_err(hex: &str) -> error::Result<u8> {
+        Err(
+            error::BottomError::ConfigError(format!(
+                "invalid color hex: error when parsing hex value {}.  It must be a valid 7 character hex string of the (ie: \"#112233\")."
+            , hex))
+        )
+    }
+
     fn convert_hex_to_rgb(hex: &str) -> error::Result<(u8, u8, u8)> {
-        if hex.len() == 7 && &hex[0..1] == "#" {
-            let r = u8::from_str_radix(&hex[1..3], 16)?;
-            let g = u8::from_str_radix(&hex[3..5], 16)?;
-            let b = u8::from_str_radix(&hex[5..7], 16)?;
+        let hex_components: Vec<char> = hex.chars().collect();
+
+        if hex_components.len() == 7 {
+            let mut r_string = hex_components[1].to_string();
+            r_string.push(hex_components[2]);
+            let mut g_string = hex_components[3].to_string();
+            g_string.push(hex_components[4]);
+            let mut b_string = hex_components[5].to_string();
+            b_string.push(hex_components[6]);
+
+            let r = u8::from_str_radix(&r_string, 16).or_else(|_err| hex_err(hex))?;
+            let g = u8::from_str_radix(&g_string, 16).or_else(|_err| hex_err(hex))?;
+            let b = u8::from_str_radix(&b_string, 16).or_else(|_err| hex_err(hex))?;
 
             return Ok((r, g, b));
         }
 
-        Err(error::BottomError::GenericError(format!(
-            "Colour hex {} is not of valid length.  It must be a 7 character string of the form \"#112233\".",
+        Err(error::BottomError::ConfigError(format!(
+            "invalid color hex: value {} is not of valid length.  It must be a 7 character string of the form \"#112233\".",
             hex
         )))
     }
@@ -125,8 +142,8 @@ pub fn get_style_from_config(input_val: &str) -> error::Result<Style> {
             get_style_from_color_name(input_val)
         }
     } else {
-        Err(error::BottomError::GenericError(format!(
-            "Colour input {} is not valid.",
+        Err(error::BottomError::ConfigError(format!(
+            "invalid color: value {} is not valid.",
             input_val
         )))
     }
@@ -142,8 +159,8 @@ pub fn get_colour_from_config(input_val: &str) -> error::Result<Color> {
             convert_name_to_color(input_val)
         }
     } else {
-        Err(error::BottomError::GenericError(format!(
-            "Colour input {} is not valid.",
+        Err(error::BottomError::ConfigError(format!(
+            "invalid color: value {} is not valid.",
             input_val
         )))
     }
@@ -154,10 +171,18 @@ pub fn get_style_from_hex(hex: &str) -> error::Result<Style> {
 }
 
 fn convert_rgb_to_color(rgb_str: &str) -> error::Result<Color> {
-    let rgb_list = rgb_str.split(',');
+    let rgb_list = rgb_str.split(',').collect::<Vec<&str>>();
+    if rgb_list.len() != 3 {
+        return Err(error::BottomError::ConfigError(format!(
+            "invalid RGB color: value {} is not of valid length.  It must be a comma separated value with 3 integers from 0 to 255 (ie: \"255, 0, 155\").",
+            rgb_str
+        )));
+    }
+
     let rgb = rgb_list
+        .iter()
         .filter_map(|val| {
-            if let Ok(res) = val.to_string().trim().parse::<u8>() {
+            if let Ok(res) = (*(*val)).to_string().trim().parse::<u8>() {
                 Some(res)
             } else {
                 None
@@ -167,8 +192,8 @@ fn convert_rgb_to_color(rgb_str: &str) -> error::Result<Color> {
     if rgb.len() == 3 {
         Ok(Color::Rgb(rgb[0], rgb[1], rgb[2]))
     } else {
-        Err(error::BottomError::GenericError(format!(
-            "RGB colour {} is not of valid length.  It must be a comma separated value with 3 integers from 0 to 255, like \"255, 0, 155\".",
+        Err(error::BottomError::ConfigError(format!(
+            "invalid RGB color: value {} contained invalid RGB values.  It must be a comma separated value with 3 integers from 0 to 255 (ie: \"255, 0, 155\").",
             rgb_str
         )))
     }
@@ -184,8 +209,8 @@ fn convert_name_to_color(color_name: &str) -> error::Result<Color> {
         return Ok(*color);
     }
 
-    Err(error::BottomError::GenericError(format!(
-        "Color {} is not a supported config colour.  bottom supports the following named colours as strings: \
+    Err(error::BottomError::ConfigError(format!(
+        "invalid named color: value {} is not a supported named colour.  The following are supported strings: \
 		Reset, Black, Red, Green, Yellow, Blue, Magenta, Cyan, Gray, DarkGray, LightRed, LightGreen, \
 		LightYellow, LightBlue, LightMagenta, LightCyan, White",
         color_name

src/main.rs

@@ -108,6 +108,12 @@ fn main() -> error::Result<()> {
     // Create "app" struct, which will control most of the program and store settings/state
     let mut app = build_app(&matches, &config, &widget_layout, default_widget_id)?;
 
+    // Create painter and set colours.
+    let mut painter = canvas::Painter::init(widget_layout, app.app_config_fields.table_gap);
+    generate_config_colours(&config, &mut painter)?;
+    painter.colours.generate_remaining_cpu_colours();
+    painter.complete_painter_init();
+
     // Set up up tui and crossterm
     let mut stdout_val = stdout();
     execute!(stdout_val, EnterAlternateScreen, EnableMouseCapture)?;
@@ -145,14 +151,6 @@ fn main() -> error::Result<()> {
         app.used_widgets.clone(),
     );
 
-    let mut painter = canvas::Painter::init(widget_layout, app.app_config_fields.table_gap);
-    if let Err(config_check) = generate_config_colours(&config, &mut painter) {
-        cleanup_terminal(&mut terminal)?;
-        return Err(config_check);
-    }
-    painter.colours.generate_remaining_cpu_colours();
-    painter.complete_painter_init();
-
     let mut first_run = true;
     loop {
         if let Ok(recv) = rx.recv_timeout(Duration::from_millis(TICK_RATE_IN_MILLISECONDS)) {

src/options.rs

@@ -41,7 +41,7 @@ pub struct ConfigFlags {
     pub default_widget_type: Option<String>,
     pub default_widget_count: Option<u64>,
     pub use_old_network_legend: Option<bool>,
-    pub hide_table_gap : Option<bool>,
+    pub hide_table_gap: Option<bool>,
     //disabled_cpu_cores: Option<Vec<u64>>, // TODO: [FEATURE] Enable disabling cores in config/flags
 }
 
@@ -219,7 +219,11 @@ pub fn build_app(
         hide_time: get_hide_time(matches, config),
         autohide_time,
         use_old_network_legend: get_use_old_network_legend(matches, config),
-        table_gap: if get_hide_table_gap(matches, config){0}else{1},
+        table_gap: if get_hide_table_gap(matches, config) {
+            0
+        } else {
+            1
+        },
     };
 
     let used_widgets = UsedWidgets {
@@ -286,7 +290,7 @@ pub fn get_widget_layout(
             ret_bottom_layout
         } else {
             return Err(error::BottomError::ConfigError(
-                "Invalid layout - please have at least one widget.".to_string(),
+                "invalid layout config: please have at least one widget.".to_string(),
             ));
         }
     } else {
@@ -342,7 +346,7 @@ fn get_temperature(
                 "kelvin" | "k" => Ok(data_harvester::temperature::TemperatureType::Kelvin),
                 "celsius" | "c" => Ok(data_harvester::temperature::TemperatureType::Celsius),
                 _ => Err(BottomError::ConfigError(
-                    "Invalid temperature type.  Please have the value be of the form \
+                    "invalid temperature type: please have the value be of the form \
 						 <kelvin|k|celsius|c|fahrenheit|f>"
                         .to_string(),
                 )),
@@ -633,4 +637,3 @@ pub fn get_hide_table_gap(matches: &clap::ArgMatches<'static>, config: &Config)
     }
     false
 }
-

src/utils/error.rs

@@ -28,24 +28,24 @@ impl std::fmt::Display for BottomError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match *self {
             BottomError::InvalidIO(ref message) => {
-                write!(f, "Encountered an IO exception: {}", message)
+                write!(f, "encountered an IO exception: {}", message)
             }
             BottomError::InvalidArg(ref message) => write!(f, "Invalid argument: {}", message),
             BottomError::InvalidHeim(ref message) => write!(
                 f,
-                "Invalid error during data collection due to Heim: {}",
+                "invalid error during data collection due to heim: {}",
                 message
             ),
             BottomError::CrosstermError(ref message) => {
-                write!(f, "Invalid error due to Crossterm: {}", message)
+                write!(f, "invalid error due to Crossterm: {}", message)
             }
             BottomError::GenericError(ref message) => write!(f, "{}", message),
             BottomError::FernError(ref message) => write!(f, "Invalid fern error: {}", message),
             BottomError::ConfigError(ref message) => {
-                write!(f, "Invalid config file error: {}", message)
+                write!(f, "invalid config file error: {}", message)
             }
             BottomError::ConversionError(ref message) => {
-                write!(f, "Unable to convert: {}", message)
+                write!(f, "unable to convert: {}", message)
             }
         }
     }

tests/arg_tests.rs

@@ -1,29 +1,18 @@
-use std::process::Command;
-
 use assert_cmd::prelude::*;
 use predicates::prelude::*;
+use std::process::Command;
 
 // These tests are mostly here just to ensure that invalid results will be caught when passing arguments...
 
-// TODO: [TEST] Allow for release testing.  Do this with paths.
-
 //======================RATES======================//
 
-fn get_os_binary_loc() -> String {
-    if cfg!(target_os = "linux") {
-        "./target/x86_64-unknown-linux-gnu/debug/btm".to_string()
-    } else if cfg!(target_os = "windows") {
-        "./target/x86_64-pc-windows-msvc/debug/btm".to_string()
-    } else if cfg!(target_os = "macos") {
-        "./target/x86_64-apple-darwin/debug/btm".to_string()
-    } else {
-        "".to_string()
-    }
+fn get_binary_location() -> String {
+    env!("CARGO_BIN_EXE_btm").to_string()
 }
 
 #[test]
 fn test_small_rate() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("-r")
         .arg("249")
         .assert()
@@ -36,7 +25,7 @@ fn test_small_rate() -> Result<(), Box<dyn std::error::Error>> {
 
 #[test]
 fn test_large_default_time() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("-t")
         .arg("18446744073709551616")
         .assert()
@@ -49,7 +38,7 @@ fn test_large_default_time() -> Result<(), Box<dyn std::error::Error>> {
 
 #[test]
 fn test_small_default_time() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("-t")
         .arg("900")
         .assert()
@@ -62,7 +51,7 @@ fn test_small_default_time() -> Result<(), Box<dyn std::error::Error>> {
 
 #[test]
 fn test_large_delta_time() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("-d")
         .arg("18446744073709551616")
         .assert()
@@ -75,7 +64,7 @@ fn test_large_delta_time() -> Result<(), Box<dyn std::error::Error>> {
 
 #[test]
 fn test_small_delta_time() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("-d")
         .arg("900")
         .assert()
@@ -88,7 +77,7 @@ fn test_small_delta_time() -> Result<(), Box<dyn std::error::Error>> {
 
 #[test]
 fn test_large_rate() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("-r")
         .arg("18446744073709551616")
         .assert()
@@ -102,7 +91,7 @@ fn test_large_rate() -> Result<(), Box<dyn std::error::Error>> {
 #[test]
 fn test_negative_rate() -> Result<(), Box<dyn std::error::Error>> {
     // This test should auto fail due to how clap works
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("-r")
         .arg("-1000")
         .assert()
@@ -116,7 +105,7 @@ fn test_negative_rate() -> Result<(), Box<dyn std::error::Error>> {
 
 #[test]
 fn test_invalid_rate() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("-r")
         .arg("100-1000")
         .assert()
@@ -128,7 +117,7 @@ fn test_invalid_rate() -> Result<(), Box<dyn std::error::Error>> {
 
 #[test]
 fn test_conflicting_temps() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("-c")
         .arg("-f")
         .assert()
@@ -142,19 +131,19 @@ fn test_conflicting_temps() -> Result<(), Box<dyn std::error::Error>> {
 
 #[test]
 fn test_invalid_default_widget_1() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("--default_widget_type")
         .arg("fake_widget")
         .assert()
         .failure()
-        .stderr(predicate::str::contains("Invalid widget type"));
+        .stderr(predicate::str::contains("invalid widget type"));
 
     Ok(())
 }
 
 #[test]
 fn test_invalid_default_widget_2() -> Result<(), Box<dyn std::error::Error>> {
-    Command::new(get_os_binary_loc())
+    Command::new(get_binary_location())
         .arg("--default_widget_type")
         .arg("cpu")
         .arg("--default_widget_count")

tests/invalid_config_tests.rs

@@ -1 +1,136 @@
+use assert_cmd::prelude::*;
+use predicates::prelude::*;
+use std::process::Command;
 
+// These tests are for testing some config file-specific options.
+
+fn get_binary_location() -> String {
+    env!("CARGO_BIN_EXE_btm").to_string()
+}
+
+#[test]
+fn test_toml_mismatch_type() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/toml_mismatch_type.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid type"));
+    Ok(())
+}
+
+#[test]
+fn test_empty_layout() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/empty_layout.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid layout config"));
+    Ok(())
+}
+
+#[test]
+fn test_invalid_layout_widget_type() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/invalid_layout_widget_type.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid widget type"));
+    Ok(())
+}
+
+/// This test isn't really needed as this is technically covered by TOML spec.
+/// However, I feel like it's worth checking anyways - not like it takes long.
+#[test]
+fn test_duplicate_temp_type() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/duplicate_temp_type.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("duplicate field"));
+    Ok(())
+}
+
+/// Checks for if a hex is valid
+#[test]
+fn test_invalid_colour_hex() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/invalid_colour_hex.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid color hex"));
+    Ok(())
+}
+
+/// Checks for if a hex is too long
+#[test]
+fn test_invalid_colour_hex_2() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/invalid_colour_hex_2.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid color hex"));
+    Ok(())
+}
+
+/// Checks unicode hex because the way we originally did it could cause char
+/// boundary errors!
+#[test]
+fn test_invalid_colour_hex_3() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/invalid_colour_hex_3.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid color hex"));
+    Ok(())
+}
+
+#[test]
+fn test_invalid_colour_name() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/invalid_colour_name.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid named color"));
+    Ok(())
+}
+
+#[test]
+fn test_invalid_colour_rgb() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/invalid_colour_rgb.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid RGB color"));
+    Ok(())
+}
+
+#[test]
+fn test_invalid_colour_rgb_2() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/invalid_colour_rgb_2.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid RGB color"));
+    Ok(())
+}
+
+#[test]
+fn test_invalid_colour_string() -> Result<(), Box<dyn std::error::Error>> {
+    Command::new(get_binary_location())
+        .arg("-C")
+        .arg("./tests/invalid_configs/invalid_colour_string.toml")
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("invalid named color"));
+    Ok(())
+}

tests/invalid_configs/duplicate_temp_type.toml

@@ -0,0 +1,4 @@
+[flags]
+temperature_type = "k"
+temperature_type = "f"
+temperature_type = "c"

tests/invalid_configs/empty_layout.toml

@@ -0,0 +1 @@
+[[row]]

tests/invalid_configs/invalid_colour_hex.toml

@@ -0,0 +1,2 @@
+[colors]
+table_header_color="#zzzzzz"

tests/invalid_configs/invalid_colour_hex_2.toml

@@ -0,0 +1,2 @@
+[colors]
+table_header_color="#1111111"

tests/invalid_configs/invalid_colour_hex_3.toml

@@ -0,0 +1,2 @@
+[colors]
+table_header_color="#我死"

tests/invalid_configs/invalid_colour_name.toml

@@ -0,0 +1,2 @@
+[colors]
+table_header_color="Light Blue"

tests/invalid_configs/invalid_colour_rgb.toml

@@ -0,0 +1,2 @@
+[colors]
+table_header_color="257, 50, 50"

tests/invalid_configs/invalid_colour_rgb_2.toml

@@ -0,0 +1,2 @@
+[colors]
+table_header_color="50, 50, 50, 50"

tests/invalid_configs/invalid_colour_string.toml

@@ -0,0 +1,2 @@
+[colors]
+table_header_color="this is not a colour"

tests/invalid_configs/invalid_layout_widget_type.toml

@@ -0,0 +1,5 @@
+[[row]]
+    [[row.child]]
+        type="cpu"
+    [[row.child]]
+        type="not_real"

tests/invalid_configs/toml_mismatch_type.toml

@@ -0,0 +1,2 @@
+[flags]
+avg_cpu = "test"