|
| 1 | +# Centralized Config Implementation - Completed |
| 2 | + |
| 3 | +## Summary |
| 4 | + |
| 5 | +Successfully centralized environment variable loading in `src/config.rs` and fixed Docker test behavior to fail properly when Docker is unavailable but `ZERV_TEST_DOCKER=true`. |
| 6 | + |
| 7 | +## Changes Made |
| 8 | + |
| 9 | +### 1. Centralized Configuration (`src/config.rs`) |
| 10 | + |
| 11 | +**Replaced config crate with direct environment variable parsing:** |
| 12 | + |
| 13 | +```rust |
| 14 | +#[derive(Debug, Clone, Default)] |
| 15 | +pub struct ZervConfig { |
| 16 | + pub test_native_git: bool, // renamed from ci |
| 17 | + pub test_docker: bool, // new field |
| 18 | +} |
| 19 | + |
| 20 | +impl ZervConfig { |
| 21 | + pub fn load() -> Result<Self, Box<dyn std::error::Error>> { |
| 22 | + let test_native_git = Self::parse_bool_env("ZERV_TEST_NATIVE_GIT")?; |
| 23 | + let test_docker = Self::parse_bool_env("ZERV_TEST_DOCKER")?; |
| 24 | + |
| 25 | + Ok(ZervConfig { test_native_git, test_docker }) |
| 26 | + } |
| 27 | + |
| 28 | + fn parse_bool_env(var_name: &str) -> Result<bool, Box<dyn std::error::Error>> { |
| 29 | + match env::var(var_name) { |
| 30 | + Ok(val) => Ok(val == "true" || val == "1"), |
| 31 | + Err(_) => Ok(false), |
| 32 | + } |
| 33 | + } |
| 34 | +} |
| 35 | +``` |
| 36 | + |
| 37 | +**Added comprehensive tests:** |
| 38 | + |
| 39 | +- `test_default_config()` - verifies defaults when no env vars set |
| 40 | +- `test_native_git_env_var()` - tests `ZERV_TEST_NATIVE_GIT=true` |
| 41 | +- `test_docker_tests_env_var()` - tests `ZERV_TEST_DOCKER=true` |
| 42 | +- `test_both_env_vars()` - tests both variables together |
| 43 | +- `test_false_values()` - tests explicit `false` values |
| 44 | + |
| 45 | +### 2. Updated Helper Functions (`src/test_utils/mod.rs`) |
| 46 | + |
| 47 | +**Replaced scattered environment variable access:** |
| 48 | + |
| 49 | +```rust |
| 50 | +pub fn should_run_docker_tests() -> bool { |
| 51 | + ZervConfig::load() |
| 52 | + .map(|config| config.should_run_docker_tests()) |
| 53 | + .unwrap_or(false) |
| 54 | +} |
| 55 | +``` |
| 56 | + |
| 57 | +### 3. Fixed Docker Test Logic (`src/test_utils/git/docker.rs`) |
| 58 | + |
| 59 | +**Fixed tests with `_without_docker` suffix to skip when Docker tests are enabled:** |
| 60 | + |
| 61 | +```rust |
| 62 | +#[test] |
| 63 | +fn test_docker_git_commands_without_docker(#[case] args: &[&str]) { |
| 64 | + if should_run_docker_tests() { |
| 65 | + return; // Skip when Docker tests are enabled |
| 66 | + } |
| 67 | + // test code |
| 68 | +} |
| 69 | +``` |
| 70 | + |
| 71 | +### 4. Removed Dependencies |
| 72 | + |
| 73 | +**Removed problematic `config` crate from `Cargo.toml`:** |
| 74 | + |
| 75 | +- The config crate had boolean parsing issues |
| 76 | +- Direct environment variable parsing is more reliable |
| 77 | + |
| 78 | +## Environment Variable Matrix |
| 79 | + |
| 80 | +| Scenario | `ZERV_TEST_NATIVE_GIT` | `ZERV_TEST_DOCKER` | Git Implementation | Docker Tests | Result | |
| 81 | +| ---------------- | ---------------------- | ------------------ | ------------------ | ------------ | ------------------ | |
| 82 | +| Local Easy | `false` | `false` | Docker Git | Skipped | Coverage with gaps | |
| 83 | +| Local Full | `false` | `true` | Docker Git | Run | Full coverage | |
| 84 | +| CI Linux | `true` | `true` | Native Git | Run | Platform coverage | |
| 85 | +| CI macOS/Windows | `true` | `false` | Native Git | Skipped | Platform coverage | |
| 86 | + |
| 87 | +## Key Behavior Changes |
| 88 | + |
| 89 | +### Before |
| 90 | + |
| 91 | +- Docker tests silently passed when Docker was unavailable |
| 92 | +- Environment variables scattered across codebase |
| 93 | +- Config crate had boolean parsing issues |
| 94 | + |
| 95 | +### After |
| 96 | + |
| 97 | +- Docker tests **fail** with clear error messages when `ZERV_TEST_DOCKER=true` but Docker unavailable |
| 98 | +- All environment variable loading centralized in `src/config.rs` |
| 99 | +- Comprehensive test coverage for config loading |
| 100 | +- Proper test separation between Docker-dependent and Docker-independent tests |
| 101 | + |
| 102 | +## Verification |
| 103 | + |
| 104 | +**Test Results:** |
| 105 | + |
| 106 | +- `make test_easy` (Docker closed) → ✅ Passes (Docker tests skipped) |
| 107 | +- `make test` (Docker closed) → ❌ Fails with "Cannot connect to the Docker daemon" (as expected) |
| 108 | +- Config tests → ✅ All 5 tests pass with proper environment variable handling |
| 109 | + |
| 110 | +## Files Modified |
| 111 | + |
| 112 | +1. `src/config.rs` - Centralized config with tests |
| 113 | +2. `src/test_utils/mod.rs` - Updated helper functions |
| 114 | +3. `src/test_utils/git/docker.rs` - Fixed Docker test logic |
| 115 | +4. `Cargo.toml` - Removed config crate dependency |
| 116 | + |
| 117 | +## Benefits Achieved |
| 118 | + |
| 119 | +- **Centralized Control**: All environment variable loading in one place |
| 120 | +- **Proper Failure Behavior**: Docker tests fail when expected, don't silently pass |
| 121 | +- **Comprehensive Testing**: Full test coverage for config functionality |
| 122 | +- **Simplified Dependencies**: Removed problematic config crate |
| 123 | +- **Clear Separation**: Docker tests vs non-Docker tests properly separated |
0 commit comments