From 4e47f9b51a57a16d406be8ce0da6e71ec246d100 Mon Sep 17 00:00:00 2001 From: Clement Tsang <34804052+ClementTsang@users.noreply.github.com> Date: Thu, 12 Sep 2024 05:51:23 -0400 Subject: [PATCH] bug: fix incorrect default config definitions for chart legends (#1594) I had changed how this was parsed in-code but I forgot to update the default configs. This also adds some e2e tests to hopefully catch this all for real in the future, since the schema ones don't catch this stuff and the constants test doesn't actually run the binary for a proper e2e test. --- CHANGELOG.md | 1 + Cargo.lock | 20 +++++ Cargo.toml | 1 + sample_configs/default_config.toml | 34 ++++++++- src/constants.rs | 34 ++++++++- src/options.rs | 8 +- tests/integration/util.rs | 9 ++- tests/integration/valid_config_tests.rs | 98 +++++++++++++++++++++++++ 8 files changed, 194 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce19bdb6..3a33e10e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ That said, these are more guidelines rather than hardset rules, though the proje - [#1552](https://github.com/ClementTsang/bottom/pull/1552): Fix typo in default config. - [#1578](https://github.com/ClementTsang/bottom/pull/1578): Fix missing selected text background colour in `default-light` theme. - [#1593](https://github.com/ClementTsang/bottom/pull/1593): Fix using `"none"` for chart legend position in configs. +- [#1594](https://github.com/ClementTsang/bottom/pull/1594): Fix incorrect default config definitions for chart legends. ### Changes diff --git a/Cargo.lock b/Cargo.lock index f243aab7..55d35847 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -196,6 +196,7 @@ dependencies = [ "strum", "sysctl", "sysinfo", + "tempfile", "time", "toml_edit", "unicode-ellipsis", @@ -568,6 +569,12 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "fastrand" +version = "2.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e8c02a5121d4ea3eb16a80748c74f5549a5665e4c21333c6098f283870fbdea6" + [[package]] name = "fern" version = "0.6.2" @@ -1469,6 +1476,19 @@ dependencies = [ "windows 0.52.0", ] +[[package]] +name = "tempfile" +version = "3.12.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "04cbcdd0c794ebb0d4cf35e88edd2f7d2c4c3e9a5a6dab322839b321c6a87a64" +dependencies = [ + "cfg-if", + "fastrand", + "once_cell", + "rustix", + "windows-sys 0.59.0", +] + [[package]] name = "terminal_size" version = "0.3.0" diff --git a/Cargo.toml b/Cargo.toml index a4e03802..ac832c92 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -143,6 +143,7 @@ filedescriptor = "0.8.2" assert_cmd = "2.0.16" cargo-husky = { version = "1.5.0", default-features = false, features = ["user-hooks"] } predicates = "3.1.2" +tempfile = "3.12.0" [target.'cfg(all(target_arch = "x86_64", target_os = "linux"))'.dev-dependencies] portable-pty = "0.8.1" diff --git a/sample_configs/default_config.toml b/sample_configs/default_config.toml index b0e5d6f7..41998d1f 100644 --- a/sample_configs/default_config.toml +++ b/sample_configs/default_config.toml @@ -77,13 +77,13 @@ # How much data is stored at once in terms of time. #retention = "10m" # Where to place the legend for the memory widget. One of "none", "top-left", "top", "top-right", "left", "right", "bottom-left", "bottom", "bottom-right". -#memory_legend = "TopRight" +#memory_legend = "top-right" # Where to place the legend for the network widget. One of "none", "top-left", "top", "top-right", "left", "right", "bottom-left", "bottom", "bottom-right". -#network_legend = "TopRight" +#network_legend = "top-right" # Processes widget configuration #[processes] -# The columns shown by the process widget. The following columns are supported: +# The columns shown by the process widget. The following columns are supported (the GPU columns are only available if the GPU feature is enabled when built): # PID, Name, CPU%, Mem%, R/s, W/s, T.Read, T.Write, User, State, Time, GMem%, GPU% #columns = ["PID", "Name", "CPU%", "Mem%", "R/s", "W/s", "T.Read", "T.Write", "User", "State", "GMem%", "GPU%"] @@ -94,36 +94,64 @@ # Disk widget configuration #[disk] + +# By default, there are no disk name filters enabled. These can be turned on to filter out specific data entries if you +# don't want to see them. An example use case is provided below. #[disk.name_filter] +# Whether to ignore any matches. Defaults to true. #is_list_ignored = true +# A list of filters to try and match. #list = ["/dev/sda\\d+", "/dev/nvme0n1p2"] +# Whether to use regex. Defaults to false. #regex = true +# Whether to be case-sensitive. Defaults to false. #case_sensitive = false +# Whether to be require matching the whole word. Defaults to false. #whole_word = false +# By default, there are no mount name filters enabled. An example use case is provided below. #[disk.mount_filter] +# Whether to ignore any matches. Defaults to true. #is_list_ignored = true +# A list of filters to try and match. #list = ["/mnt/.*", "/boot"] +# Whether to use regex. Defaults to false. #regex = true +# Whether to be case-sensitive. Defaults to false. #case_sensitive = false +# Whether to be require matching the whole word. Defaults to false. #whole_word = false # Temperature widget configuration #[temperature] + +# By default, there are no temperature sensor filters enabled. An example use case is provided below. #[temperature.sensor_filter] +# Whether to ignore any matches. Defaults to true. #is_list_ignored = true +# A list of filters to try and match. #list = ["cpu", "wifi"] +# Whether to use regex. Defaults to false. #regex = false +# Whether to be case-sensitive. Defaults to false. #case_sensitive = false +# Whether to be require matching the whole word. Defaults to false. #whole_word = false # Network widget configuration #[network] + +# By default, there are no network interface filters enabled. An example use case is provided below. #[network.interface_filter] +# Whether to ignore any matches. Defaults to true. #is_list_ignored = true +# A list of filters to try and match. #list = ["virbr0.*"] +# Whether to use regex. Defaults to false. #regex = true +# Whether to be case-sensitive. Defaults to false. #case_sensitive = false +# Whether to be require matching the whole word. Defaults to false. #whole_word = false # These are all the components that support custom theming. Note that colour support diff --git a/src/constants.rs b/src/constants.rs index bc499bda..3e300744 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -337,13 +337,13 @@ pub(crate) const CONFIG_TEXT: &str = r#"# This is a default config file for bott # How much data is stored at once in terms of time. #retention = "10m" # Where to place the legend for the memory widget. One of "none", "top-left", "top", "top-right", "left", "right", "bottom-left", "bottom", "bottom-right". -#memory_legend = "TopRight" +#memory_legend = "top-right" # Where to place the legend for the network widget. One of "none", "top-left", "top", "top-right", "left", "right", "bottom-left", "bottom", "bottom-right". -#network_legend = "TopRight" +#network_legend = "top-right" # Processes widget configuration #[processes] -# The columns shown by the process widget. The following columns are supported: +# The columns shown by the process widget. The following columns are supported (the GPU columns are only available if the GPU feature is enabled when built): # PID, Name, CPU%, Mem%, R/s, W/s, T.Read, T.Write, User, State, Time, GMem%, GPU% #columns = ["PID", "Name", "CPU%", "Mem%", "R/s", "W/s", "T.Read", "T.Write", "User", "State", "GMem%", "GPU%"] @@ -354,36 +354,64 @@ pub(crate) const CONFIG_TEXT: &str = r#"# This is a default config file for bott # Disk widget configuration #[disk] + +# By default, there are no disk name filters enabled. These can be turned on to filter out specific data entries if you +# don't want to see them. An example use case is provided below. #[disk.name_filter] +# Whether to ignore any matches. Defaults to true. #is_list_ignored = true +# A list of filters to try and match. #list = ["/dev/sda\\d+", "/dev/nvme0n1p2"] +# Whether to use regex. Defaults to false. #regex = true +# Whether to be case-sensitive. Defaults to false. #case_sensitive = false +# Whether to be require matching the whole word. Defaults to false. #whole_word = false +# By default, there are no mount name filters enabled. An example use case is provided below. #[disk.mount_filter] +# Whether to ignore any matches. Defaults to true. #is_list_ignored = true +# A list of filters to try and match. #list = ["/mnt/.*", "/boot"] +# Whether to use regex. Defaults to false. #regex = true +# Whether to be case-sensitive. Defaults to false. #case_sensitive = false +# Whether to be require matching the whole word. Defaults to false. #whole_word = false # Temperature widget configuration #[temperature] + +# By default, there are no temperature sensor filters enabled. An example use case is provided below. #[temperature.sensor_filter] +# Whether to ignore any matches. Defaults to true. #is_list_ignored = true +# A list of filters to try and match. #list = ["cpu", "wifi"] +# Whether to use regex. Defaults to false. #regex = false +# Whether to be case-sensitive. Defaults to false. #case_sensitive = false +# Whether to be require matching the whole word. Defaults to false. #whole_word = false # Network widget configuration #[network] + +# By default, there are no network interface filters enabled. An example use case is provided below. #[network.interface_filter] +# Whether to ignore any matches. Defaults to true. #is_list_ignored = true +# A list of filters to try and match. #list = ["virbr0.*"] +# Whether to use regex. Defaults to false. #regex = true +# Whether to be case-sensitive. Defaults to false. #case_sensitive = false +# Whether to be require matching the whole word. Defaults to false. #whole_word = false # These are all the components that support custom theming. Note that colour support diff --git a/src/options.rs b/src/options.rs index 77f690f9..f051fca4 100644 --- a/src/options.rs +++ b/src/options.rs @@ -957,13 +957,13 @@ fn get_network_legend_position( let result = if let Some(s) = &args.network.network_legend { match s.to_ascii_lowercase().trim() { "none" => None, - position => Some(parse_config_value!(position.parse(), "network_legend")?), + position => Some(parse_arg_value!(position.parse(), "network_legend")?), } } else if let Some(flags) = &config.flags { if let Some(s) = &flags.network_legend { match s.to_ascii_lowercase().trim() { "none" => None, - position => Some(parse_arg_value!(position.parse(), "network_legend")?), + position => Some(parse_config_value!(position.parse(), "network_legend")?), } } else { Some(LegendPosition::default()) @@ -981,13 +981,13 @@ fn get_memory_legend_position( let result = if let Some(s) = &args.memory.memory_legend { match s.to_ascii_lowercase().trim() { "none" => None, - position => Some(parse_config_value!(position.parse(), "memory_legend")?), + position => Some(parse_arg_value!(position.parse(), "memory_legend")?), } } else if let Some(flags) = &config.flags { if let Some(s) = &flags.memory_legend { match s.to_ascii_lowercase().trim() { "none" => None, - position => Some(parse_arg_value!(position.parse(), "memory_legend")?), + position => Some(parse_config_value!(position.parse(), "memory_legend")?), } } else { Some(LegendPosition::default()) diff --git a/tests/integration/util.rs b/tests/integration/util.rs index 350b7da9..e9e33652 100644 --- a/tests/integration/util.rs +++ b/tests/integration/util.rs @@ -5,7 +5,14 @@ use hashbrown::HashMap; use portable_pty::{native_pty_system, Child, CommandBuilder, MasterPty, PtySize}; pub fn abs_path(path: &str) -> OsString { - Path::new(path).canonicalize().unwrap().into_os_string() + let path = Path::new(path); + + if path.exists() { + path.canonicalize().unwrap().into_os_string() + } else { + // We are going to trust that the path given is valid... + path.to_owned().into_os_string() + } } /// Returns a QEMU runner target given an architecture. diff --git a/tests/integration/valid_config_tests.rs b/tests/integration/valid_config_tests.rs index e56f7f56..98450754 100644 --- a/tests/integration/valid_config_tests.rs +++ b/tests/integration/valid_config_tests.rs @@ -2,6 +2,9 @@ use std::{io::Read, thread, time::Duration}; +#[cfg(feature = "default")] +use std::{io::Write, path::Path}; + use crate::util::spawn_btm_in_pty; fn reader_to_string(mut reader: Box) -> String { @@ -53,6 +56,101 @@ fn test_empty() { run_and_kill(&["-C", "./tests/valid_configs/empty_config.toml"]); } +#[cfg(feature = "default")] +fn test_uncommented_default_config(original: &Path, test_name: &str) { + use regex::Regex; + + // Take the default config file and uncomment everything. + let default_config = match std::fs::File::open(original) { + Ok(mut default_config_file) => { + let mut buf = String::new(); + default_config_file + .read_to_string(&mut buf) + .expect("can read file"); + + buf + } + Err(err) => { + println!("Could not open default config, skipping {test_name}. Error: {err:?}"); + return; + } + }; + + let default_config = Regex::new(r"(?m)^#([a-zA-Z\[])") + .unwrap() + .replace_all(&default_config, "$1"); + + let default_config = Regex::new(r"(?m)^#(\s\s+)([a-zA-Z\[])") + .unwrap() + .replace_all(&default_config, "$2"); + + let mut uncommented_config = match tempfile::NamedTempFile::new() { + Ok(tf) => tf, + Err(err) => { + println!("Could not create a temp file, skipping {test_name}. Error: {err:?}"); + return; + } + }; + + if let Err(err) = uncommented_config.write_all(default_config.as_bytes()) { + println!("Could not write to temp file, skipping {test_name}. Error: {err:?}"); + return; + } + + run_and_kill(&["-C", &uncommented_config.path().to_string_lossy()]); + + uncommented_config.close().unwrap(); +} + +#[cfg(feature = "default")] +#[test] +fn test_default() { + test_uncommented_default_config( + Path::new("./sample_configs/default_config.toml"), + "test_default", + ); +} + +#[cfg(feature = "default")] +#[test] +fn test_new_default() { + use tempfile::TempPath; + + let new_temp_default_path = match tempfile::NamedTempFile::new() { + Ok(temp_file) => temp_file.into_temp_path(), + Err(err) => { + println!("Could not create a temp file, skipping test_new_default. Error: {err:?}"); + return; + } + }; + + // This is a hack because we need a temp file that doesn't exist. + let actual_temp_default_path = new_temp_default_path.to_path_buf(); + new_temp_default_path.close().unwrap(); + + if !actual_temp_default_path.exists() { + run_and_kill(&["-C", &(actual_temp_default_path.to_string_lossy())]); + + // Re-take control over the temp path to ensure it gets deleted. + let actual_temp_default_path = TempPath::from_path(actual_temp_default_path); + test_uncommented_default_config(&actual_temp_default_path, "test_new_default"); + + actual_temp_default_path.close().unwrap(); + } else { + println!("temp path we want to check exists, skip test_new_default test."); + } +} + +#[test] +fn test_demo() { + let path: &str = "./sample_configs/demo_config.toml"; + if std::path::Path::new(path).exists() { + run_and_kill(&["-C", path]); + } else { + println!("Could not read demo config."); + } +} + #[test] fn test_many_proc() { run_and_kill(&["-C", "./tests/valid_configs/many_proc.toml"]);