From 2d65da093c32bad033a8c95c6680643070315d8a Mon Sep 17 00:00:00 2001 From: James Brink Date: Sat, 8 Feb 2025 20:27:43 -0700 Subject: [PATCH] feat: add 80% minimum code coverage requirement and improve test reliability - Added 80% minimum code coverage requirement to CI pipeline - Added EnvGuard for reliable environment variable cleanup in tests - Added tests for Anthropic provider and provider factory - Fixed environment variable handling in config tests - Updated cargo-tarpaulin installation check --- .github/workflows/test.yml | 2 +- scripts/check.sh | 8 +++--- src/providers/anthropic.rs | 38 +++++++++++++++++++++++++++ src/providers/mod.rs | 45 +++++++++++++++++++++++++++++++- src/test_utils.rs | 2 +- tests/config_test.rs | 53 +++++++++++++++++++++++++++++--------- 6 files changed, 129 insertions(+), 19 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f739ecf..a0e8c42 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -75,7 +75,7 @@ jobs: - name: Generate coverage report run: | - cargo tarpaulin --features testing --out Xml --output-dir coverage + cargo tarpaulin --features testing --out Xml --output-dir coverage --fail-under 80 - name: Upload coverage to Codecov uses: codecov/codecov-action@v5 diff --git a/scripts/check.sh b/scripts/check.sh index 85d218a..ca2fd04 100755 --- a/scripts/check.sh +++ b/scripts/check.sh @@ -9,10 +9,10 @@ NC='\033[0m' # No Color echo -e "${BLUE}Running quality checks for Strainer...${NC}\n" -# Check if cargo-tarpaulin is installed -if ! command -v cargo-tarpaulin &> /dev/null; then - echo -e "${RED}cargo-tarpaulin is not installed. Installing...${NC}" - cargo install cargo-tarpaulin +# Check if cargo-tarpaulin is installed and working +if ! cargo tarpaulin --version &> /dev/null; then + echo -e "${BLUE}Installing or updating cargo-tarpaulin...${NC}" + cargo install --force cargo-tarpaulin fi # Function to run a command and check its status diff --git a/src/providers/anthropic.rs b/src/providers/anthropic.rs index 22ae898..2a18868 100644 --- a/src/providers/anthropic.rs +++ b/src/providers/anthropic.rs @@ -4,6 +4,7 @@ use anyhow::Result; /// Provider implementation for Anthropic's API #[allow(dead_code)] +#[derive(Debug)] pub struct AnthropicProvider { api_key: String, base_url: String, @@ -43,3 +44,40 @@ impl Provider for AnthropicProvider { }) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_anthropic_provider_new() { + let config = ApiConfig { + api_key: Some("test_key".to_string()), + ..Default::default() + }; + let provider = AnthropicProvider::new(&config); + assert!(provider.is_ok()); + } + + #[test] + fn test_anthropic_provider_missing_key() { + let config = ApiConfig::default(); + let provider = AnthropicProvider::new(&config); + assert!(provider.is_err()); + } + + #[test] + fn test_anthropic_provider_rate_limits() { + let config = ApiConfig { + api_key: Some("test_key".to_string()), + ..Default::default() + }; + let provider = AnthropicProvider::new(&config).unwrap(); + let limits = provider.get_rate_limits(); + assert!(limits.is_ok()); + let limits = limits.unwrap(); + assert_eq!(limits.requests_used, 0); + assert_eq!(limits.tokens_used, 0); + assert_eq!(limits.input_tokens_used, 0); + } +} diff --git a/src/providers/mod.rs b/src/providers/mod.rs index 4d59185..3dc2d38 100644 --- a/src/providers/mod.rs +++ b/src/providers/mod.rs @@ -2,7 +2,7 @@ use crate::config::ApiConfig; use anyhow::Result; /// Trait defining the interface for rate limit providers -pub trait Provider { +pub trait Provider: std::fmt::Debug { /// Get the current rate limits from the provider /// Get the current rate limits for the API provider /// @@ -41,3 +41,46 @@ pub fn create_provider(config: &ApiConfig) -> Result> { pub mod anthropic; pub mod rate_limiter; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_create_anthropic_provider() { + let config = ApiConfig { + provider: String::from("anthropic"), + api_key: Some("test_key".to_string()), + ..Default::default() + }; + let provider = create_provider(&config); + assert!(provider.is_ok()); + } + + #[test] + fn test_create_unsupported_provider() { + let config = ApiConfig { + provider: String::from("unsupported"), + ..Default::default() + }; + let provider = create_provider(&config); + assert!(provider.is_err()); + assert_eq!( + provider.unwrap_err().to_string(), + "Unsupported provider: unsupported" + ); + } + + #[test] + fn test_rate_limit_info_debug() { + let info = RateLimitInfo { + requests_used: 10, + tokens_used: 100, + input_tokens_used: 50, + }; + let debug_str = format!("{info:?}"); + assert!(debug_str.contains("requests_used: 10")); + assert!(debug_str.contains("tokens_used: 100")); + assert!(debug_str.contains("input_tokens_used: 50")); + } +} diff --git a/src/test_utils.rs b/src/test_utils.rs index c7ae806..e847fae 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -6,7 +6,7 @@ use anyhow::Result; use std::collections::HashMap; use std::sync::{Arc, Mutex}; -#[derive(Default)] +#[derive(Default, Debug)] pub struct MockProvider { pub calls: Arc>>, pub responses: Arc>>, diff --git a/tests/config_test.rs b/tests/config_test.rs index dd64cbc..3de9ee8 100644 --- a/tests/config_test.rs +++ b/tests/config_test.rs @@ -57,8 +57,40 @@ fn test_config_from_file() -> Result<()> { Ok(()) } +struct EnvGuard { + vars: Vec<&'static str>, +} + +impl EnvGuard { + fn new(vars: Vec<&'static str>) -> Self { + // Clean up any existing values + for var in &vars { + env::remove_var(var); + } + Self { vars } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + // Clean up on scope exit + for var in &self.vars { + env::remove_var(var); + } + } +} + #[test] fn test_config_from_env() -> Result<()> { + let _guard = EnvGuard::new(vec![ + "STRAINER_API_KEY", + "STRAINER_PROVIDER", + "STRAINER_BASE_URL", + "STRAINER_REQUESTS_PER_MINUTE", + "STRAINER_TOKENS_PER_MINUTE", + ]); + + // Set test environment variables env::set_var("STRAINER_API_KEY", "env-test-key"); env::set_var("STRAINER_PROVIDER", "anthropic"); env::set_var("STRAINER_BASE_URL", "https://env.api.com"); @@ -72,13 +104,6 @@ fn test_config_from_env() -> Result<()> { assert_eq!(config.limits.requests_per_minute, Some(30)); assert_eq!(config.limits.tokens_per_minute, Some(50_000)); - // Cleanup - env::remove_var("STRAINER_API_KEY"); - env::remove_var("STRAINER_PROVIDER"); - env::remove_var("STRAINER_BASE_URL"); - env::remove_var("STRAINER_REQUESTS_PER_MINUTE"); - env::remove_var("STRAINER_TOKENS_PER_MINUTE"); - Ok(()) } @@ -140,6 +165,13 @@ fn test_config_validation() { #[test] fn test_load_with_env_override() -> Result<()> { + let _guard = EnvGuard::new(vec![ + "STRAINER_API_KEY", + "STRAINER_TOKENS_PER_MINUTE", + "STRAINER_BASE_URL", + "STRAINER_REQUESTS_PER_MINUTE", + ]); + let dir = tempdir()?; let config_path = dir.path().join("strainer.toml"); @@ -150,7 +182,7 @@ fn test_load_with_env_override() -> Result<()> { base_url = "https://file.api.com" [limits] - requests_per_minute = 60 + requests_per_minute = 30 "#; fs::write(&config_path, config_content)?; @@ -158,6 +190,7 @@ fn test_load_with_env_override() -> Result<()> { // Set environment variables env::set_var("STRAINER_API_KEY", "env-key"); env::set_var("STRAINER_TOKENS_PER_MINUTE", "50000"); + env::set_var("STRAINER_REQUESTS_PER_MINUTE", "60"); // Change to the temporary directory let original_dir = env::current_dir()?; @@ -168,10 +201,6 @@ fn test_load_with_env_override() -> Result<()> { // Restore original directory env::set_current_dir(original_dir)?; - // Clean up environment - env::remove_var("STRAINER_API_KEY"); - env::remove_var("STRAINER_TOKENS_PER_MINUTE"); - assert_eq!(config.api.api_key, Some("env-key".to_string())); assert_eq!( config.api.base_url,