|
| 1 | +<!-- 43a57cce-dba6-4d82-adb9-0e50aa327a01 4cbd79df-8e32-47bc-9d02-844f2b4e79a7 --> |
| 2 | +# Unit Test Implementation Plan: ConfigurationService |
| 3 | + |
| 4 | +## Current State |
| 5 | + |
| 6 | +- **File**: `10xGitHubPolicies.Tests/Services/Configuration/ConfigurationServiceTests.cs` |
| 7 | +- **Tests Implemented**: 1 (happy path) |
| 8 | +- **Tests Needed**: 11 additional tests |
| 9 | +- **Coverage Target**: 85-90% |
| 10 | + |
| 11 | +## Test Cases to Implement |
| 12 | + |
| 13 | +### 1. Cache Hit Scenario |
| 14 | + |
| 15 | +**Test**: `GetConfigAsync_WhenConfigCached_ReturnsCachedConfig` |
| 16 | + |
| 17 | +**Priority**: HIGH (Core caching behavior) |
| 18 | + |
| 19 | +Mock `IMemoryCache.TryGetValue()` to return a cached `AppConfig`. Verify: |
| 20 | + |
| 21 | +- GitHub service is NOT called |
| 22 | +- Same cached instance is returned |
| 23 | +- Logger logs cache hit message |
| 24 | + |
| 25 | +### 2. Missing Configuration File |
| 26 | + |
| 27 | +**Test**: `GetConfigAsync_WhenConfigFileMissing_ThrowsConfigurationNotFoundException` |
| 28 | + |
| 29 | +**Priority**: HIGH (Error handling - TC-CONFIG-006) |
| 30 | + |
| 31 | +Mock `IGitHubService.GetFileContentAsync()` to return `null` or empty string. Verify: |
| 32 | + |
| 33 | +- Throws `ConfigurationNotFoundException` |
| 34 | +- Exception message contains `.github/config.yaml` |
| 35 | +- Logger logs warning message |
| 36 | + |
| 37 | +### 3. Invalid YAML Syntax |
| 38 | + |
| 39 | +**Test**: `GetConfigAsync_WhenYamlInvalid_ThrowsInvalidConfigurationException` |
| 40 | + |
| 41 | +**Priority**: HIGH (Error handling - TC-CONFIG-006) |
| 42 | + |
| 43 | +Use malformed YAML: |
| 44 | + |
| 45 | +```yaml |
| 46 | +invalid: yaml: syntax: |
| 47 | + - missing quotes |
| 48 | + unclosed: [bracket |
| 49 | +``` |
| 50 | +
|
| 51 | +Verify: |
| 52 | +
|
| 53 | +- Throws `InvalidConfigurationException` |
| 54 | +- Inner exception is `YamlException` |
| 55 | +- Logger logs error |
| 56 | + |
| 57 | +### 4. Missing Authorized Team Field |
| 58 | + |
| 59 | +**Test**: `GetConfigAsync_WhenAuthorizedTeamMissing_ThrowsInvalidConfigurationException` |
| 60 | + |
| 61 | +**Priority**: HIGH (Validation - TC-CONFIG-006) |
| 62 | + |
| 63 | +Use YAML without `access_control.authorized_team`: |
| 64 | + |
| 65 | +```yaml |
| 66 | +policies: |
| 67 | + - type: "has_agents_md" |
| 68 | +``` |
| 69 | + |
| 70 | +Verify exception message contains "authorized_team must be set" |
| 71 | + |
| 72 | +### 5. Empty Authorized Team |
| 73 | + |
| 74 | +**Test**: `GetConfigAsync_WhenAuthorizedTeamEmpty_ThrowsInvalidConfigurationException` |
| 75 | + |
| 76 | +**Priority**: HIGH (Validation - TC-CONFIG-006) |
| 77 | + |
| 78 | +Use YAML with empty `authorized_team`: |
| 79 | + |
| 80 | +```yaml |
| 81 | +access_control: |
| 82 | + authorized_team: "" |
| 83 | +``` |
| 84 | + |
| 85 | +Verify validation fails with clear message |
| 86 | + |
| 87 | +### 6. Null Access Control Object |
| 88 | + |
| 89 | +**Test**: `GetConfigAsync_WhenAccessControlNull_ThrowsInvalidConfigurationException` |
| 90 | + |
| 91 | +**Priority**: MEDIUM |
| 92 | + |
| 93 | +Use YAML without `access_control` section. Verify validation catches null check. |
| 94 | + |
| 95 | +### 7. Force Refresh Bypasses Cache |
| 96 | + |
| 97 | +**Test**: `GetConfigAsync_WhenForceRefreshTrue_BypassesCache` |
| 98 | + |
| 99 | +**Priority**: HIGH (Core feature - TC-CONFIG-007) |
| 100 | + |
| 101 | +Steps: |
| 102 | + |
| 103 | +1. First call populates cache |
| 104 | +2. Update mock to return different config |
| 105 | +3. Call with `forceRefresh: true` |
| 106 | +4. Verify GitHub service called again |
| 107 | +5. Verify new config returned (not cached) |
| 108 | + |
| 109 | +### 8. Concurrent Requests (Semaphore Test) |
| 110 | + |
| 111 | +**Test**: `GetConfigAsync_WhenCalledConcurrently_FetchesOnlyOnce` |
| 112 | + |
| 113 | +**Priority**: CRITICAL (Thread-safety - TC-CONFIG-004) |
| 114 | + |
| 115 | +Implementation: |
| 116 | + |
| 117 | +```csharp |
| 118 | +var callCount = 0; |
| 119 | +_githubService.GetFileContentAsync(...) |
| 120 | + .Returns(_ => { |
| 121 | + Interlocked.Increment(ref callCount); |
| 122 | + return Task.Delay(100).ContinueWith(_ => base64Content); |
| 123 | + }); |
| 124 | +
|
| 125 | +// Act - 10 concurrent calls |
| 126 | +var tasks = Enumerable.Range(0, 10) |
| 127 | + .Select(_ => _sut.GetConfigAsync()) |
| 128 | + .ToList(); |
| 129 | +var results = await Task.WhenAll(tasks); |
| 130 | +
|
| 131 | +// Assert |
| 132 | +callCount.Should().Be(1, "semaphore should prevent duplicate fetches"); |
| 133 | +``` |
| 134 | + |
| 135 | +### 9. Double-Check Locking |
| 136 | + |
| 137 | +**Test**: `GetConfigAsync_WhenMultipleThreadsWait_SecondCheckPreventsRefetch` |
| 138 | + |
| 139 | +**Priority**: MEDIUM (Thread-safety edge case) |
| 140 | + |
| 141 | +Verify that after acquiring semaphore, cache is checked again before fetching. |
| 142 | + |
| 143 | +### 10. Cache Entry Options Validation |
| 144 | + |
| 145 | +**Test**: `GetConfigAsync_WhenCaching_SetsSlidingExpiration15Minutes` |
| 146 | + |
| 147 | +**Priority**: MEDIUM (TC-CONFIG-005) |
| 148 | + |
| 149 | +Verify `IMemoryCache.Set()` is called with correct expiration: |
| 150 | + |
| 151 | +```csharp |
| 152 | +_cache.Received(1).Set( |
| 153 | + Arg.Is<string>(k => k == "AppConfigCacheKey"), |
| 154 | + Arg.Any<AppConfig>(), |
| 155 | + Arg.Is<MemoryCacheEntryOptions>(opts => |
| 156 | + // Verify sliding expiration = 15 minutes |
| 157 | + ) |
| 158 | +); |
| 159 | +``` |
| 160 | + |
| 161 | +### 11. Multiple Policy Parsing |
| 162 | + |
| 163 | +**Test**: `GetConfigAsync_WhenMultiplePolicies_ParsesAllCorrectly` |
| 164 | + |
| 165 | +**Priority**: MEDIUM (Data parsing validation) |
| 166 | + |
| 167 | +Use YAML with 3 different policy types. Verify all are parsed with correct properties. |
| 168 | + |
| 169 | +### 12. GitHub Service Error Propagation |
| 170 | + |
| 171 | +**Test**: `GetConfigAsync_WhenGitHubServiceFails_PropagatesException` |
| 172 | + |
| 173 | +**Priority**: MEDIUM (Error handling) |
| 174 | + |
| 175 | +Mock GitHub service to throw exception. Verify: |
| 176 | + |
| 177 | +- Exception propagates to caller |
| 178 | +- Logger logs error |
| 179 | +- Cache is not updated with invalid data |
| 180 | + |
| 181 | +## Implementation Notes |
| 182 | + |
| 183 | +### Mock Cache Correctly |
| 184 | + |
| 185 | +The existing test shows the pattern for mocking `IMemoryCache.TryGetValue()`: |
| 186 | + |
| 187 | +```csharp |
| 188 | +_cache.TryGetValue(Arg.Any<object>(), out Arg.Any<AppConfig?>()) |
| 189 | + .Returns(x => { |
| 190 | + x[1] = cachedValue; // or null for cache miss |
| 191 | + return cachedValue != null; |
| 192 | + }); |
| 193 | +``` |
| 194 | + |
| 195 | +### Base64 Encoding Helper |
| 196 | + |
| 197 | +Consider extracting to helper method: |
| 198 | + |
| 199 | +```csharp |
| 200 | +private string ToBase64(string yaml) |
| 201 | + => Convert.ToBase64String(Encoding.UTF8.GetBytes(yaml)); |
| 202 | +``` |
| 203 | + |
| 204 | +### Test Organization |
| 205 | + |
| 206 | +Group tests using nested classes if file gets too large: |
| 207 | + |
| 208 | +```csharp |
| 209 | +public class ConfigurationServiceTests |
| 210 | +{ |
| 211 | + public class GetConfigAsyncTests : BaseTests { } |
| 212 | + public class ValidationTests : BaseTests { } |
| 213 | + public class CachingTests : BaseTests { } |
| 214 | +} |
| 215 | +``` |
| 216 | + |
| 217 | +### Traits for Filtering |
| 218 | + |
| 219 | +All tests should have: |
| 220 | + |
| 221 | +```csharp |
| 222 | +[Trait("Category", "Unit")] |
| 223 | +[Trait("Feature", "Configuration")] |
| 224 | +``` |
| 225 | + |
| 226 | +## Expected Outcomes |
| 227 | + |
| 228 | +- **Total Tests**: 13 (1 existing + 12 new) |
| 229 | +- **Coverage**: 85-90% of ConfigurationService |
| 230 | +- **Test Execution Time**: < 2 seconds total |
| 231 | +- **All tests follow naming convention**: `MethodName_WhenCondition_ExpectedBehavior` |
| 232 | + |
| 233 | +## Test Coverage Mapping |
| 234 | + |
| 235 | +| ConfigurationService Feature | Test Case | Priority | |
| 236 | + |
| 237 | +|------------------------------|-----------|----------| |
| 238 | + |
| 239 | +| Happy path | ✅ Implemented | HIGH | |
| 240 | + |
| 241 | +| Cache hit | #1 | HIGH | |
| 242 | + |
| 243 | +| Cache miss → fetch | ✅ Implemented | HIGH | |
| 244 | + |
| 245 | +| Force refresh | #7 | HIGH | |
| 246 | + |
| 247 | +| Missing file | #2 | HIGH | |
| 248 | + |
| 249 | +| Invalid YAML | #3 | HIGH | |
| 250 | + |
| 251 | +| Validation (authorized_team) | #4, #5, #6 | HIGH | |
| 252 | + |
| 253 | +| Thread-safety (semaphore) | #8, #9 | CRITICAL | |
| 254 | + |
| 255 | +| Cache expiration config | #10 | MEDIUM | |
| 256 | + |
| 257 | +| Multiple policies | #11 | MEDIUM | |
| 258 | + |
| 259 | +| Error propagation | #12 | MEDIUM | |
| 260 | + |
| 261 | +### To-dos |
| 262 | + |
| 263 | +- [ ] Implement GetConfigAsync_WhenConfigCached_ReturnsCachedConfig test |
| 264 | +- [ ] Implement error handling tests: missing file, invalid YAML, validation failures (tests #2-#6) |
| 265 | +- [ ] Implement GetConfigAsync_WhenForceRefreshTrue_BypassesCache test |
| 266 | +- [ ] Implement concurrent requests and double-check locking tests (#8-#9) |
| 267 | +- [ ] Implement cache expiration, multiple policies, and error propagation tests (#10-#12) |
0 commit comments