CLI tests for logging flags (#3609)

## Issue Addressed
Adding CLI tests for logging flags: log-color and disable-log-timestamp
Which issue # does this PR address?
#3588 
## Proposed Changes
Add CLI tests for logging flags as described in #3588 
Please list or describe the changes introduced by this PR.
Added logger_config to client::Config as suggested. Implemented Default for LoggerConfig based on what was being done elsewhere in the repo. Created 2 tests for each flag addressed.
## Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

Author: GeemoCandama

Cargo.lock

@@ -1,11 +1,11 @@
 use directory::DEFAULT_ROOT_DIR;
+use environment::LoggerConfig;
 use network::NetworkConfig;
 use sensitive_url::SensitiveUrl;
 use serde_derive::{Deserialize, Serialize};
 use std::fs;
 use std::path::PathBuf;
 use types::{Graffiti, PublicKeyBytes};
-
 /// Default directory name for the freezer database under the top-level data dir.
 const DEFAULT_FREEZER_DB_DIR: &str = "freezer_db";
 
@@ -72,6 +72,7 @@ pub struct Config {
     pub http_metrics: http_metrics::Config,
     pub monitoring_api: Option<monitoring_api::Config>,
     pub slasher: Option<slasher::Config>,
+    pub logger_config: LoggerConfig,
 }
 
 impl Default for Config {
@@ -96,6 +97,7 @@ impl Default for Config {
             slasher: None,
             validator_monitor_auto: false,
             validator_monitor_pubkeys: vec![],
+            logger_config: LoggerConfig::default(),
         }
     }
 }

beacon_node/client/src/config.rs

@@ -781,8 +781,8 @@ fn run<T: EthSpec>(
         .map_err(|e| format!("should start tokio runtime: {:?}", e))?
         .initialize_logger(LoggerConfig {
             path: None,
-            debug_level: "trace",
-            logfile_debug_level: "trace",
+            debug_level: String::from("trace"),
+            logfile_debug_level: String::from("trace"),
             log_format: None,
             log_color: false,
             disable_log_timestamp: false,

lcli/src/main.rs

@@ -18,6 +18,8 @@ slog-async = "2.5.0"
 futures = "0.3.7"
 slog-json = "2.3.0"
 exit-future = "0.2.0"
+serde = "1.0.116"
+serde_derive = "1.0.116"
 
 [target.'cfg(not(target_family = "unix"))'.dependencies]
 ctrlc = { version = "3.1.6", features = ["termination"] }

lighthouse/environment/Cargo.toml

@@ -12,6 +12,7 @@ use eth2_network_config::Eth2NetworkConfig;
 use futures::channel::mpsc::{channel, Receiver, Sender};
 use futures::{future, StreamExt};
 
+use serde_derive::{Deserialize, Serialize};
 use slog::{error, info, o, warn, Drain, Duplicate, Level, Logger};
 use sloggers::{file::FileLoggerBuilder, types::Format, types::Severity, Build};
 use std::fs::create_dir_all;
@@ -43,17 +44,33 @@ const MAXIMUM_SHUTDOWN_TIME: u64 = 15;
 /// - `path` == None,
 /// - `max_log_size` == 0,
 /// - `max_log_number` == 0,
-pub struct LoggerConfig<'a> {
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct LoggerConfig {
     pub path: Option<PathBuf>,
-    pub debug_level: &'a str,
-    pub logfile_debug_level: &'a str,
-    pub log_format: Option<&'a str>,
+    pub debug_level: String,
+    pub logfile_debug_level: String,
+    pub log_format: Option<String>,
     pub log_color: bool,
     pub disable_log_timestamp: bool,
     pub max_log_size: u64,
     pub max_log_number: usize,
     pub compression: bool,
 }
+impl Default for LoggerConfig {
+    fn default() -> Self {
+        LoggerConfig {
+            path: None,
+            debug_level: String::from("info"),
+            logfile_debug_level: String::from("debug"),
+            log_format: None,
+            log_color: false,
+            disable_log_timestamp: false,
+            max_log_size: 200,
+            max_log_number: 5,
+            compression: false,
+        }
+    }
+}
 
 /// Builds an `Environment`.
 pub struct EnvironmentBuilder<E: EthSpec> {
@@ -135,7 +152,7 @@ impl<E: EthSpec> EnvironmentBuilder<E> {
     /// Note that background file logging will spawn a new thread.
     pub fn initialize_logger(mut self, config: LoggerConfig) -> Result<Self, String> {
         // Setting up the initial logger format and build it.
-        let stdout_drain = if let Some(format) = config.log_format {
+        let stdout_drain = if let Some(ref format) = config.log_format {
             match format.to_uppercase().as_str() {
                 "JSON" => {
                     let stdout_drain = slog_json::Json::default(std::io::stdout()).fuse();
@@ -168,7 +185,7 @@ impl<E: EthSpec> EnvironmentBuilder<E> {
                 .build()
         };
 
-        let stdout_drain = match config.debug_level {
+        let stdout_drain = match config.debug_level.as_str() {
             "info" => stdout_drain.filter_level(Level::Info),
             "debug" => stdout_drain.filter_level(Level::Debug),
             "trace" => stdout_drain.filter_level(Level::Trace),
@@ -220,7 +237,7 @@ impl<E: EthSpec> EnvironmentBuilder<E> {
             }
         }
 
-        let logfile_level = match config.logfile_debug_level {
+        let logfile_level = match config.logfile_debug_level.as_str() {
             "info" => Severity::Info,
             "debug" => Severity::Debug,
             "trace" => Severity::Trace,
@@ -233,7 +250,7 @@ impl<E: EthSpec> EnvironmentBuilder<E> {
         let file_logger = FileLoggerBuilder::new(&path)
             .level(logfile_level)
             .channel_size(LOG_CHANNEL_SIZE)
-            .format(match config.log_format {
+            .format(match config.log_format.as_deref() {
                 Some("JSON") => Format::Json,
                 _ => Format::default(),
             })

lighthouse/environment/src/lib.rs

@@ -438,17 +438,17 @@ fn run<E: EthSpec>(
 
     let logger_config = LoggerConfig {
         path: log_path,
-        debug_level,
-        logfile_debug_level,
-        log_format,
+        debug_level: String::from(debug_level),
+        logfile_debug_level: String::from(logfile_debug_level),
+        log_format: log_format.map(String::from),
         log_color,
         disable_log_timestamp,
         max_log_size: logfile_max_size * 1_024 * 1_024,
         max_log_number: logfile_max_number,
         compression: logfile_compress,
     };
 
-    let builder = environment_builder.initialize_logger(logger_config)?;
+    let builder = environment_builder.initialize_logger(logger_config.clone())?;
 
     let mut environment = builder
         .multi_threaded_tokio_runtime()?
@@ -528,7 +528,8 @@ fn run<E: EthSpec>(
             let context = environment.core_context();
             let log = context.log().clone();
             let executor = context.executor.clone();
-            let config = beacon_node::get_config::<E>(matches, &context)?;
+            let mut config = beacon_node::get_config::<E>(matches, &context)?;
+            config.logger_config = logger_config;
             let shutdown_flag = matches.is_present("immediate-shutdown");
             // Dump configs if `dump-config` or `dump-chain-config` flags are set
             clap_utils::check_dump_configs::<_, E>(matches, &config, &context.eth2_config.spec)?;

lighthouse/src/main.rs

@@ -1475,3 +1475,39 @@ fn monitoring_endpoint() {
             assert_eq!(api_conf.update_period_secs, Some(30));
         });
 }
+
+// Tests for Logger flags.
+#[test]
+fn default_log_color_flag() {
+    CommandLineTest::new()
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert!(!config.logger_config.log_color);
+        });
+}
+#[test]
+fn enabled_log_color_flag() {
+    CommandLineTest::new()
+        .flag("log-color", None)
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert!(config.logger_config.log_color);
+        });
+}
+#[test]
+fn default_disable_log_timestamp_flag() {
+    CommandLineTest::new()
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert!(!config.logger_config.disable_log_timestamp);
+        });
+}
+#[test]
+fn enabled_disable_log_timestamp_flag() {
+    CommandLineTest::new()
+        .flag("disable-log-timestamp", None)
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert!(config.logger_config.disable_log_timestamp);
+        });
+}

lighthouse/tests/beacon_node.rs

@@ -56,15 +56,12 @@ pub fn run_eth1_sim(matches: &ArgMatches) -> Result<(), String> {
         })
         .collect::<Vec<_>>();
 
-    let log_level = "debug";
-    let log_format = None;
-
     let mut env = EnvironmentBuilder::minimal()
         .initialize_logger(LoggerConfig {
             path: None,
-            debug_level: log_level,
-            logfile_debug_level: "debug",
-            log_format,
+            debug_level: String::from("debug"),
+            logfile_debug_level: String::from("debug"),
+            log_format: None,
             log_color: false,
             disable_log_timestamp: false,
             max_log_size: 0,

testing/simulator/src/eth1_sim.rs

@@ -41,15 +41,12 @@ pub fn run_no_eth1_sim(matches: &ArgMatches) -> Result<(), String> {
         })
         .collect::<Vec<_>>();
 
-    let log_level = "debug";
-    let log_format = None;
-
     let mut env = EnvironmentBuilder::mainnet()
         .initialize_logger(LoggerConfig {
             path: None,
-            debug_level: log_level,
-            logfile_debug_level: "debug",
-            log_format,
+            debug_level: String::from("debug"),
+            logfile_debug_level: String::from("debug"),
+            log_format: None,
             log_color: false,
             disable_log_timestamp: false,
             max_log_size: 0,

testing/simulator/src/no_eth1_sim.rs

@@ -48,9 +48,9 @@ fn syncing_sim(
     let mut env = EnvironmentBuilder::minimal()
         .initialize_logger(LoggerConfig {
             path: None,
-            debug_level: log_level,
-            logfile_debug_level: "debug",
-            log_format,
+            debug_level: String::from(log_level),
+            logfile_debug_level: String::from("debug"),
+            log_format: log_format.map(String::from),
             log_color: false,
             disable_log_timestamp: false,
             max_log_size: 0,