From b9ed127c797b75cfc903fb6dfd8122fbc03d2cb2 Mon Sep 17 00:00:00 2001 From: Mauro Ezequiel Moltrasio Date: Mon, 1 Jun 2026 15:59:30 +0200 Subject: [PATCH 1/2] test: add unit tests for environment variable bindings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds coverage for all FACT_* env-var bindings in FactCli: - env_vars: each env var populates the correct FactConfig field - env_vars_override_yaml: env vars take precedence over YAML config - env_vars_invalid_values: invalid values produce ValueValidation errors Introduces EnvVar and EnvVarGuard helper types that together acquire ENV_MUTEX, set the variable, and guarantee removal on drop — even if the test panics. std::env::set_var is not safe in multi-threaded programs; the mutex serialises all env-var mutations within the suite as the least invasive available mitigation. Closes #730 Assisted-by: Claude Sonnet 4.6 --- fact/src/config/tests.rs | 377 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 377 insertions(+) diff --git a/fact/src/config/tests.rs b/fact/src/config/tests.rs index 0079d672..15a32164 100644 --- a/fact/src/config/tests.rs +++ b/fact/src/config/tests.rs @@ -1,3 +1,9 @@ +use std::{ + error::Error, + fmt::Display, + sync::{Mutex, MutexGuard}, +}; + use super::*; #[test] @@ -1100,3 +1106,374 @@ fn defaults() { assert_eq!(config.bpf.inodes_max(), 65536); assert!(config.hotreload()); } + +static ENV_MUTEX: Mutex<()> = Mutex::new(()); + +/// RAII guard that holds the `ENV_MUTEX` lock and removes the named environment +/// variable when dropped, ensuring both are released even if the test panics +/// after calling [`EnvVar::set`]. +/// +/// The mutex is released after the variable is removed, so no other test can +/// observe the env var in a partially-cleaned-up state. +struct EnvVarGuard { + name: &'static str, + _guard: MutexGuard<'static, ()>, +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + unsafe { std::env::remove_var(self.name) }; + } +} + +/// An environment variable key-value pair used in tests to exercise env-var +/// bindings in [`FactCli`]. +/// +/// `std::env::set_var` is not safe to call from multi-threaded programs, as +/// other threads may be reading the environment concurrently. There is no +/// better alternative at the moment: the test binary is multi-threaded by +/// default, and the only truly sound options would be forcing +/// `RUST_TEST_THREADS=1` or switching to a single-threaded runtime, both of +/// which are more invasive than the mutex approach used here. [`EnvVar::set`] +/// acquires `ENV_MUTEX` to at least serialise all env-var mutations within our +/// own test suite. +#[derive(Clone, Copy)] +struct EnvVar { + name: &'static str, + value: &'static str, +} + +impl EnvVar { + /// Acquires `ENV_MUTEX`, sets the environment variable, and returns an + /// [`EnvVarGuard`] that holds the lock and removes the variable on drop. + fn set(self) -> EnvVarGuard { + let _guard = ENV_MUTEX.lock().unwrap(); + unsafe { std::env::set_var(self.name, self.value) }; + EnvVarGuard { + name: self.name, + _guard, + } + } +} + +impl Display for EnvVar { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}={}", self.name, self.value) + } +} + +fn with_env_var(env: EnvVar) -> Result { + let _guard = env.set(); + FactCli::try_parse_from(["fact"]).map(|cli| cli.to_config()) +} + +#[test] +fn env_vars() { + let tests = [ + ( + EnvVar { + name: "FACT_INODES_MAX", + value: "1024", + }, + FactConfig { + bpf: BpfConfig { + inodes_max: Some(1024), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_RINGBUF_SIZE", + value: "128", + }, + FactConfig { + bpf: BpfConfig { + ringbuf_size: Some(128), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_PATHS", + value: "/etc:/var/log", + }, + FactConfig { + paths: Some(vec![PathBuf::from("/etc"), PathBuf::from("/var/log")]), + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_JSON", + value: "true", + }, + FactConfig { + json: Some(true), + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_SCAN_INTERVAL", + value: "45.5", + }, + FactConfig { + scan_interval: Some(Duration::from_secs_f64(45.5)), + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_RATE_LIMIT", + value: "500", + }, + FactConfig { + rate_limit: Some(500), + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_URL", + value: "https://svc.sensor.stackrox:9090", + }, + FactConfig { + grpc: GrpcConfig { + url: Some(String::from("https://svc.sensor.stackrox:9090")), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_CERTS", + value: "/etc/stackrox/certs", + }, + FactConfig { + grpc: GrpcConfig { + certs: Some(PathBuf::from("/etc/stackrox/certs")), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_ENDPOINT_ADDRESS", + value: "0.0.0.0:8080", + }, + FactConfig { + endpoint: EndpointConfig { + address: Some(SocketAddr::from(([0, 0, 0, 0], 8080))), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_ENDPOINT_EXPOSE_METRICS", + value: "true", + }, + FactConfig { + endpoint: EndpointConfig { + expose_metrics: Some(true), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_ENDPOINT_HEALTH_CHECK", + value: "true", + }, + FactConfig { + endpoint: EndpointConfig { + health_check: Some(true), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_SKIP_PRE_FLIGHT", + value: "true", + }, + FactConfig { + skip_pre_flight: Some(true), + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_HOTRELOAD", + value: "true", + }, + FactConfig { + hotreload: Some(true), + ..Default::default() + }, + ), + ]; + for (env, expected) in tests { + let config = with_env_var(env).expect("env-var CLI parse failed"); + assert_eq!(config, expected, "env var {env}"); + } +} + +#[test] +fn env_vars_override_yaml() { + let tests = [ + ( + EnvVar { + name: "FACT_INODES_MAX", + value: "2048", + }, + "bpf:\n inodes_max: 1024", + FactConfig { + bpf: BpfConfig { + inodes_max: Some(2048), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_RINGBUF_SIZE", + value: "256", + }, + "bpf:\n ringbuf_size: 128", + FactConfig { + bpf: BpfConfig { + ringbuf_size: Some(256), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_URL", + value: "https://override:9090", + }, + "grpc:\n url: 'https://original:9090'", + FactConfig { + grpc: GrpcConfig { + url: Some(String::from("https://override:9090")), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_PATHS", + value: "/var/log", + }, + "paths:\n- /etc", + FactConfig { + paths: Some(vec![PathBuf::from("/var/log")]), + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_JSON", + value: "true", + }, + "json: false", + FactConfig { + json: Some(true), + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_SCAN_INTERVAL", + value: "60", + }, + "scan_interval: 30", + FactConfig { + scan_interval: Some(Duration::from_secs(60)), + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_RATE_LIMIT", + value: "1000", + }, + "rate_limit: 500", + FactConfig { + rate_limit: Some(1000), + ..Default::default() + }, + ), + ]; + for (env, yaml, expected) in tests { + let mut config = match FactConfig::try_from(yaml) { + Ok(c) => c, + Err(e) => panic!("Failed to parse YAML\n\tError: {e}\n\tyaml: {yaml}"), + }; + config.update(&with_env_var(env).expect("env-var CLI parse failed")); + assert_eq!(config, expected, "env var {env} should override yaml",); + } +} + +#[test] +fn env_vars_invalid_values() { + let tests = [ + ( + EnvVar { + name: "FACT_INODES_MAX", + value: "not_a_number", + }, + "invalid digit found in string", + ), + ( + EnvVar { + name: "FACT_RINGBUF_SIZE", + value: "not_a_number", + }, + "invalid digit found in string", + ), + ( + EnvVar { + name: "FACT_ENDPOINT_ADDRESS", + value: "not_an_address", + }, + "invalid socket address syntax", + ), + ( + EnvVar { + name: "FACT_SCAN_INTERVAL", + value: "not_a_float", + }, + "invalid float literal", + ), + ( + EnvVar { + name: "FACT_RATE_LIMIT", + value: "not_a_number", + }, + "invalid digit found in string", + ), + ]; + for (env, expected) in tests { + let Err(err) = with_env_var(env) else { + panic!("Expected Error was not caught - expected: {expected}"); + }; + assert_eq!( + err.source().map(|e| e.to_string()).unwrap_or_default(), + expected, + ); + } +} From cedeabe3f9f3ed3b075ab4864664cc80bd039df8 Mon Sep 17 00:00:00 2001 From: Mauro Ezequiel Moltrasio Date: Tue, 2 Jun 2026 12:56:59 +0200 Subject: [PATCH 2/2] test: add comprehensive env var override and validation tests Extends env_vars_override_yaml test to cover all configuration variables that can be set via environment variables: - FACT_CERTS - FACT_ENDPOINT_ADDRESS - FACT_ENDPOINT_EXPOSE_METRICS - FACT_ENDPOINT_HEALTH_CHECK - FACT_SKIP_PRE_FLIGHT - FACT_HOTRELOAD Extends env_vars_invalid_values test to cover boolean flags and improves error message assertions by checking the full first line of the error output instead of just the error source. All 13 environment variables from FactCli are now comprehensively tested across env_vars, env_vars_override_yaml, and env_vars_invalid_values test functions. Assisted-by: Claude Code (claude-sonnet-4-5@20250929) --- fact/src/config/tests.rs | 139 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 129 insertions(+), 10 deletions(-) diff --git a/fact/src/config/tests.rs b/fact/src/config/tests.rs index 15a32164..f25d86c5 100644 --- a/fact/src/config/tests.rs +++ b/fact/src/config/tests.rs @@ -1,5 +1,4 @@ use std::{ - error::Error, fmt::Display, sync::{Mutex, MutexGuard}, }; @@ -1417,6 +1416,84 @@ fn env_vars_override_yaml() { ..Default::default() }, ), + ( + EnvVar { + name: "FACT_CERTS", + value: "/etc/override/certs", + }, + "grpc:\n certs: /etc/original/certs", + FactConfig { + grpc: GrpcConfig { + certs: Some(PathBuf::from("/etc/override/certs")), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_ENDPOINT_ADDRESS", + value: "127.0.0.1:9090", + }, + "endpoint:\n address: 0.0.0.0:8080", + FactConfig { + endpoint: EndpointConfig { + address: Some(SocketAddr::from(([127, 0, 0, 1], 9090))), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_ENDPOINT_EXPOSE_METRICS", + value: "true", + }, + "endpoint:\n expose_metrics: false", + FactConfig { + endpoint: EndpointConfig { + expose_metrics: Some(true), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_ENDPOINT_HEALTH_CHECK", + value: "true", + }, + "endpoint:\n health_check: false", + FactConfig { + endpoint: EndpointConfig { + health_check: Some(true), + ..Default::default() + }, + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_SKIP_PRE_FLIGHT", + value: "true", + }, + "skip_pre_flight: false", + FactConfig { + skip_pre_flight: Some(true), + ..Default::default() + }, + ), + ( + EnvVar { + name: "FACT_HOTRELOAD", + value: "true", + }, + "hotreload: false", + FactConfig { + hotreload: Some(true), + ..Default::default() + }, + ), ]; for (env, yaml, expected) in tests { let mut config = match FactConfig::try_from(yaml) { @@ -1430,50 +1507,92 @@ fn env_vars_override_yaml() { #[test] fn env_vars_invalid_values() { + fn first_line(err: clap::Error) -> String { + let err = err.to_string(); + let Some((line, _)) = err.split_once('\n') else { + panic!("Error did not have a newline: {err}"); + }; + + line.to_string() + } + let tests = [ ( EnvVar { name: "FACT_INODES_MAX", value: "not_a_number", }, - "invalid digit found in string", + "error: invalid value 'not_a_number' for '--inodes-max ': invalid digit found in string", ), ( EnvVar { name: "FACT_RINGBUF_SIZE", value: "not_a_number", }, - "invalid digit found in string", + "error: invalid value 'not_a_number' for '--ringbuf-size ': invalid digit found in string", ), ( EnvVar { name: "FACT_ENDPOINT_ADDRESS", value: "not_an_address", }, - "invalid socket address syntax", + "error: invalid value 'not_an_address' for '--address
': invalid socket address syntax", + ), + ( + EnvVar { + name: "FACT_ENDPOINT_EXPOSE_METRICS", + value: "not_a_boolean", + }, + "error: invalid value 'not_a_boolean' for '--expose-metrics'", + ), + ( + EnvVar { + name: "FACT_ENDPOINT_HEALTH_CHECK", + value: "not_a_boolean", + }, + "error: invalid value 'not_a_boolean' for '--health-check'", ), ( EnvVar { name: "FACT_SCAN_INTERVAL", value: "not_a_float", }, - "invalid float literal", + "error: invalid value 'not_a_float' for '--scan-interval ': invalid float literal", ), ( EnvVar { name: "FACT_RATE_LIMIT", value: "not_a_number", }, - "invalid digit found in string", + "error: invalid value 'not_a_number' for '--rate-limit ': invalid digit found in string", + ), + ( + EnvVar { + name: "FACT_JSON", + value: "not_a_boolean", + }, + "error: invalid value 'not_a_boolean' for '--json'", + ), + ( + EnvVar { + name: "FACT_SKIP_PRE_FLIGHT", + value: "not_a_boolean", + }, + "error: invalid value 'not_a_boolean' for '--skip-pre-flight'", + ), + ( + EnvVar { + name: "FACT_HOTRELOAD", + value: "not_a_boolean", + }, + "error: invalid value 'not_a_boolean' for '--hotreload'", ), ]; for (env, expected) in tests { let Err(err) = with_env_var(env) else { panic!("Expected Error was not caught - expected: {expected}"); }; - assert_eq!( - err.source().map(|e| e.to_string()).unwrap_or_default(), - expected, - ); + let err = first_line(err); + assert_eq!(err, expected); } }