fix: Cache Functionality Fixes and Comprehensive Test Suite#3
fix: Cache Functionality Fixes and Comprehensive Test Suite#3
Conversation
… is in fact testing the correct behaviour
…ain in the Example UI.
…asn't properly checked for null
WalkthroughProject and CI metadata and Xcode projects migrated to newer formats; Example app AppDelegate/SwiftUI updated; URLSession/URLCache handling and synthetic caching added; SSE completion calls guarded; extensive cache-focused unit/integration tests, TestConfig and testing docs added; podspec/SPM test resources updated; local Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Example UI
participant FS as Flagsmith.shared
participant API as APIManager
participant Sess as URLSession
participant Cache as URLCache
UI->>FS: getFeatureFlags(identity?)
FS->>API: request(router)
API->>API: ensure session uses configured URLCache
alt skipAPI == true
API->>Cache: cachedResponse(for: request)
alt cached
Cache-->>API: CachedURLResponse
API->>API: decode cached data
API-->>FS: return flags (from cache)
else no cache
API-->>FS: return error (skipAPI forbids network)
end
else skipAPI == false
API->>Sess: dataTask(request)
Sess-->>API: data + response
API->>API: decode
alt decode success and useCache == true
API->>Cache: store synthetic CachedURLResponse with TTL
API-->>FS: return flags (from network)
else
API-->>FS: return error
end
end
sequenceDiagram
autonumber
participant SSE as SSEManager
participant URLS as URLSession
SSE->>URLS: start stream (start(completion:))
URLS-->>SSE: task didCompleteWithError(error)
alt error == nil
opt completionHandler exists
SSE->>SSE: call completionHandler()
end
SSE->>SSE: schedule reconnect/backoff
opt self & completionHandler exist
SSE->>URLS: start(completion:)
end
else error != nil
SSE->>SSE: handle error/backoff
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Comment |
gazreese
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
FlagsmithClient/Classes/Internal/SSEManager.swift (1)
120-139: Bug: timeout path falls through and also schedules a delayed reconnect (double reconnect).On
.timedOutyou immediately callstart(...)but don’treturn, so the “Otherwise reconnect with increasing delay” block also runs, causing an unnecessary second reconnect attempt.Apply this minimal fix:
- if let error = error { - if let error = error as? URLError, error.code == .timedOut { - if let completionHandler = completionHandler { - start(completion: completionHandler) - } - } + if let error = error { + if let urlError = error as? URLError, urlError.code == .timedOut { + if let completionHandler = completionHandler { + start(completion: completionHandler) + } + return + }
🧹 Nitpick comments (32)
FlagsmithClient/Classes/Internal/SSEManager.swift (3)
151-154: Build URLs with URL APIs, not string concatenation.Avoids missing/duplicate “/” when
baseURLchanges and handles encoding safely.Apply:
- guard let completeEventSourceUrl = URL(string: "\(baseURL.absoluteString)sse/environments/\(apiKey)/stream") else { + let completeEventSourceUrl = baseURL + .appendingPathComponent("sse") + .appendingPathComponent("environments") + .appendingPathComponent(apiKey) + .appendingPathComponent("stream") + guard URL(string: completeEventSourceUrl.absoluteString) != nil else { completion(.failure(FlagsmithError.apiURL("Invalid event source URL"))) return }Alternatively:
- guard let completeEventSourceUrl = URL(string: "\(baseURL.absoluteString)sse/environments/\(apiKey)/stream") else { + guard let completeEventSourceUrl = URL(string: "sse/environments/\(apiKey)/stream", relativeTo: baseURL) else { completion(.failure(FlagsmithError.apiURL("Invalid event source URL"))) return }
82-101: Reduce allocations: reuse a singleJSONDecoder.Hot path; creating a decoder per line is unnecessary.
Apply:
- let flagEvent = try JSONDecoder().decode(FlagEvent.self, from: jsonData) + let flagEvent = try jsonDecoder.decode(FlagEvent.self, from: jsonData)Add property near other fields:
private let jsonDecoder = JSONDecoder()
166-169: Consider invalidating the session on stop/deinit to break strong delegate cycle.If
SSEManagerisn’t a true singleton,URLSession(strong) → delegate (self) andself→ session can leak.Option:
func stop() { dataTask?.cancel() completionHandler = nil + session.invalidateAndCancel() }Or add:
deinit { session.invalidateAndCancel() }Example/FlagsmithClient/AppDelegate.swift (1)
28-28: Placeholder text tweak LGTM.Optional: read from an env var if set to simplify local runs without editing code.
-Flagsmith.shared.apiKey = "<add your API key from the Flagsmith dashboard here>" +Flagsmith.shared.apiKey = ProcessInfo.processInfo.environment["FLAGSMITH_TEST_API_KEY"] + ?? "<add your API key from the Flagsmith dashboard here>"Example/Pods/Pods.xcodeproj/project.pbxproj (1)
25-25: PrivacyInfo.xcprivacy is added to Sources and Resources; keep it only in Resources..xcprivacy should be a resource, not a compile source, to avoid duplicate processing.
Action: remove “PrivacyInfo.xcprivacy in Sources” from the FlagsmithClient target; keep the bundle/resource entries.
Also applies to: 389-390, 407-427
FlagsmithClient/Tests/TestConfig.swift (1)
48-48: Add trailing newline to file.SwiftLint correctly identifies a missing trailing newline at the end of the file.
Apply this diff to fix the formatting:
@objc class TestConfigObjC: NSObject { @objc static let apiKey = TestConfig.apiKey @objc static let hasRealApiKey = TestConfig.hasRealApiKey -} +} +FlagsmithClient/Classes/Internal/APIManager.swift (2)
215-253: Manual caching fallback is well-implemented but consider edge cases.The
ensureResponseIsCachedmethod provides a good fallback mechanism for ensuring responses are cached. The implementation correctly:
- Checks for existing cached responses before storing
- Creates appropriate cache headers
- Handles errors gracefully
However, there are a few considerations:
- The forced unwrap on Line 230 (
request.url!) could theoretically crash if the router somehow produces a request without a URL- The print statement on Line 251 might be better as a debug-level log if you have a logging framework
Consider safer URL handling:
- let httpResponse = HTTPURLResponse( - url: request.url!, + guard let url = request.url else { + print("Flagsmith: Cannot cache response without URL") + return + } + + let httpResponse = HTTPURLResponse( + url: url,
145-150: Make session recreation atomicThe session getter/setter are synchronized, but the read-then-write at lines 145–150 is not atomic; wrap the compare-and-recreate in the propertiesSerialAccessQueue and use the backing _session to avoid races.
- // Update session cache configuration if it has changed - if session.configuration.urlCache !== Flagsmith.shared.cacheConfig.cache { - let configuration = URLSessionConfiguration.default - configuration.urlCache = Flagsmith.shared.cacheConfig.cache - session = URLSession(configuration: configuration, delegate: self, delegateQueue: OperationQueue.main) - } + // Update session cache configuration if it has changed + propertiesSerialAccessQueue.sync(flags: .barrier) { + if _session.configuration.urlCache !== Flagsmith.shared.cacheConfig.cache { + let configuration = URLSessionConfiguration.default + configuration.urlCache = Flagsmith.shared.cacheConfig.cache + _session = URLSession(configuration: configuration, delegate: self, delegateQueue: OperationQueue.main) + } + }FlagsmithClient/Tests/CachedURLResponseTests.swift (1)
166-166: Use more descriptive variable name in loop.SwiftLint correctly identifies that the single-character variable name
idoesn't follow naming conventions.Apply this diff to improve readability:
- for i in 0..<10 { + for threadIndex in 0..<10 { DispatchQueue.global().async { - let ttl = 60 + i // Different TTL for each thread + let ttl = 60 + threadIndex // Different TTL for each thread let modifiedResponse = cachedResponse.response(withExpirationDuration: ttl) // Verify the modification worked if let httpResp = modifiedResponse.response as? HTTPURLResponse { let cacheControl = httpResp.allHeaderFields["Cache-Control"] as? String - XCTAssertEqual(cacheControl, "max-age=\(ttl)", "TTL should be set correctly in thread \(i)") + XCTAssertEqual(cacheControl, "max-age=\(ttl)", "TTL should be set correctly in thread \(threadIndex)") }FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift (7)
42-47: Prefer skipping over failing when no real API key is present.Failing the suite on missing secrets hurts local/CI runs. Use XCTSkip to mark as skipped.
- guard TestConfig.hasRealApiKey else { - XCTFail("This integration test requires FLAGSMITH_TEST_API_KEY environment variable to be set") - return - } + guard TestConfig.hasRealApiKey else { + throw XCTSkip("Requires FLAGSMITH_TEST_API_KEY; skipping.") + }
113-116: Remove duplicate comment.Line 115 repeats line 113.
- // Test different TTL values using public API
171-206: Assert skipAPI behavior explicitly or gate behind a real key.This test only logs; it never asserts intended semantics and can pass regardless of regressions. Either (a) assert the exact contract or (b) skip without a real key.
+ guard TestConfig.hasRealApiKey else { throw XCTSkip("Network-dependent: requires real API key.") }
332-357: Gate network-dependent feature-method tests behind a real key or pre-populate cache.As written, these can flap on CI. Either pre-seed URLCache (as you do elsewhere) or skip without a key.
+ guard TestConfig.hasRealApiKey else { throw XCTSkip("Requires real API key or pre-populated cache.") }
364-393: Gate analytics test or mock network.Same concern: it exercises network and timers without assertions; prefer skip or cache seeding plus concrete asserts.
+ guard TestConfig.hasRealApiKey else { throw XCTSkip("Requires real API key.") }
424-427: Build identity URL with URLComponents to avoid encoding bugs.Direct string interpolation risks invalid URLs for identities with special characters.
- let mockURL = URL(string: "https://edge.api.flagsmith.com/api/v1/identities/?identifier=\(testIdentity)")! + var comps = URLComponents(string: "https://edge.api.flagsmith.com/api/v1/identities/")! + comps.queryItems = [URLQueryItem(name: "identifier", value: testIdentity)] + let mockURL = comps.url!
291-324: Realtime test has no assertions and does network toggling.Either assert a measurable effect (e.g., cache unchanged across toggle) or mark as skipped without a key to avoid dead time in CI.
FlagsmithClient/Tests/CacheTests.swift (6)
99-162: Ensure request equality with cached key; set explicit cachePolicy in getFeatureFlags path or pre-match URLRequest properties.You store with .returnCacheDataElseLoad on mockRequest but Flagsmith’s internal request may use a different policy; flakiness risk. Consider pre-populating using the same policy as APIManager or asserting via URLProtocol.
165-202: Test name vs. expectation mismatch for skipAPI.Name says “should attempt network” but skipAPI=true commonly implies “prefer cache.” Clarify the contract and assert it; currently this passes regardless.
205-253: TTL test doesn’t validate expiration.After waiting 2s, no assertion is made. Either try fetching via URLSession with useProtocolCachePolicy to observe eviction or at least verify headers/absence by cachePolicy .returnCacheDataDontLoad.
+ // After delay, ensure URLCache returns nil when using default policy + let postDelay = testCache.cachedResponse(for: mockRequest) + XCTAssertNotNil(postDelay, "URLCache stores entries; TTL is enforced on fetch. Consider asserting via a URLSession request instead.")
461-503: Unreliable “isFromCache” check and long line.The heuristic tries to parse flag.value to detect cache use; remove it. Also hits swiftlint line_length.
- // If it succeeded, verify it's NOT the cached data - let isFromCache = flags.isEmpty ? false : (String(data: flags.first?.value.stringValue?.data(using: .utf8) ?? Data(), encoding: .utf8) == "cached data") - XCTAssertFalse(isFromCache, "Should not get cached data when useCache=false") + // If it succeeded, success was via network since useCache=false. No cache-origin check needed. + XCTAssertTrue(true)
700-736: Negative TTL handling should be defined.Current extension will emit max-age=-1; recommend clamping to 0 (or skipping cache) and asserting that behavior here. Consider updating CachedURLResponse.response(withExpirationDuration:) accordingly.
1-1: swiftlint: consider per-file disables for test-only rules or split the suite.Current suite trips type_body_length, file_length, function_body_length, and line_length. Either split into smaller test classes or disable rules at top of this test file.
+// swiftlint:disable file_length type_body_length function_body_length line_lengthFlagsmithClient/Tests/APIManagerTests.swift (2)
88-149: This “correct behavior” test doesn’t assert; add assertions or skip without a key.It logs only. Either assert the expected skipAPI contract or pre-seed cache and assert cache hits; otherwise skip without a real key.
+ guard TestConfig.hasRealApiKey else { throw XCTSkip("Network-dependent; skipping without real key.") } + // Optionally: assert that secondResult succeeds when cache present.
187-284: Solid cache pre-seed and verification; add one assertion on URLCache key.LGTM overall; consider verifying cachedResponse(for:) is non-nil before calling getFeatureFlags to fail fast if pre-seed breaks.
Example/FlagsmithClient/SwiftUIView.swift (4)
156-170: Avoid List inside a ScrollView; use LazyVStack to prevent nested scrolling issues.Also prefer a stable id (e.g., flag.id) if available; feature.name may collide.
- List(flags, id: \.feature.name) { flag in - HStack { - Text("\(flag.feature.name): \(flag.value)") - .font(.system(size: 12)) - Spacer() - Text("\(flag.enabled ? "✅" : "❌")") - } - } - .frame(height: 200) + LazyVStack(alignment: .leading, spacing: 8) { + ForEach(flags, id: \.id) { flag in + HStack { + Text("\(flag.feature.name): \(flag.value)") + .font(.system(size: 12)) + Spacer() + Text(flag.enabled ? "✅" : "❌") + } + } + } + .frame(maxHeight: 200)
294-305: Fix force-refresh race: restore useCache after the async call completes.Current code restores immediately; use defer inside Task to guarantee restoration post-request.
- func testForceRefresh() { + func testForceRefresh() { addLog("Testing force refresh (temporarily disabling cache)...") - - let originalUseCache = flagsmith.cacheConfig.useCache - flagsmith.cacheConfig.useCache = false - - testGetFeatureFlagsForIdentity() - - // Restore cache setting - flagsmith.cacheConfig.useCache = originalUseCache - addLog("Restored cache setting: useCache=\(originalUseCache)") + Task { + let originalUseCache = flagsmith.cacheConfig.useCache + flagsmith.cacheConfig.useCache = false + defer { + flagsmith.cacheConfig.useCache = originalUseCache + addLog("Restored cache setting: useCache=\(originalUseCache)") + } + do { + _ = try await flagsmith.getFeatureFlags(forIdentity: identity) + addLog("✅ Force refresh completed") + } catch { + addLog("❌ Force refresh error: \(error)") + } + checkCacheStatus() + } }
307-319: Reuse a static DateFormatter to avoid repeated allocations.Minor perf nit.
- let timestamp = DateFormatter().apply { - $0.dateFormat = "HH:mm:ss.SSS" - }.string(from: Date()) + struct DF { static let shared: DateFormatter = { + let df = DateFormatter(); df.dateFormat = "HH:mm:ss.SSS"; return df + }() } + let timestamp = DF.shared.string(from: Date())
209-214: Consider formatting cache usage human-readably (MB) and updating on main thread.This is UI state; ensure main-thread updates and friendlier units.
FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift (4)
15-20: Skip rather than fail when missing API key.Keep CI green for contributors without secrets.
- guard TestConfig.hasRealApiKey else { - XCTFail("This customer use case test requires FLAGSMITH_TEST_API_KEY environment variable to be set") - return - } + guard TestConfig.hasRealApiKey else { + throw XCTSkip("Requires FLAGSMITH_TEST_API_KEY; skipping.") + }
127-131: Use URLComponents for identity URLs.Safer construction.
- let mockURL = URL(string: "https://edge.api.flagsmith.com/api/v1/identities/?identifier=\(testIdentity)")! + var comps = URLComponents(string: "https://edge.api.flagsmith.com/api/v1/identities/")! + comps.queryItems = [URLQueryItem(name: "identifier", value: testIdentity)] + let mockURL = comps.url!
308-397: Remove unused RequestCounter or assert with it.It’s incremented but never asserted; either use it or delete to reduce noise.
- class RequestCounter { - static var count = 0 - static func increment() { count += 1 } - static func reset() { count = 0 } - } + // Optional: track and assert call count if you wire in a URLProtocol mock.
73-94: Explicitly assert the intended behavior after first success.If skipAPI=true and cache exists, second call should not require network. Add an assertion to lock the contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Example/Pods/Manifest.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.gitignore(1 hunks)Example/FlagsmithClient.xcodeproj/project.pbxproj(6 hunks)Example/FlagsmithClient/AppDelegate.swift(1 hunks)Example/FlagsmithClient/SwiftUIView.swift(1 hunks)Example/Pods/Local Podspecs/FlagsmithClient.podspec.json(2 hunks)Example/Pods/Pods.xcodeproj/project.pbxproj(21 hunks)FlagsmithClient/Classes/Internal/APIManager.swift(4 hunks)FlagsmithClient/Classes/Internal/SSEManager.swift(1 hunks)FlagsmithClient/Tests/APIManagerTests.swift(1 hunks)FlagsmithClient/Tests/CacheTests.swift(1 hunks)FlagsmithClient/Tests/CachedURLResponseTests.swift(1 hunks)FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift(1 hunks)FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift(1 hunks)FlagsmithClient/Tests/README_Testing.md(1 hunks)FlagsmithClient/Tests/TestConfig.swift(1 hunks)Package.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift (1)
FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)
FlagsmithClient/Tests/CacheTests.swift (3)
FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift (2)
setUp(14-29)tearDown(31-38)FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)FlagsmithClient/Tests/APIManagerTests.swift (1)
testSkipAPIWithCacheAvailable(152-185)
FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift (2)
FlagsmithClient/Tests/CacheTests.swift (2)
setUp(15-27)tearDown(29-34)FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)
Example/FlagsmithClient/SwiftUIView.swift (2)
FlagsmithClient/Classes/Flagsmith.swift (2)
getFeatureFlags(126-171)hasFeatureFlag(187-205)FlagsmithClient/Classes/Flagsmith+Concurrency.swift (2)
getFeatureFlags(32-43)hasFeatureFlag(51-62)
FlagsmithClient/Classes/Internal/SSEManager.swift (1)
FlagsmithClient/Classes/Internal/ReconnectionDelay.swift (1)
nextDelay(23-27)
FlagsmithClient/Tests/CachedURLResponseTests.swift (1)
FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)
FlagsmithClient/Classes/Internal/APIManager.swift (2)
FlagsmithClient/Classes/Internal/Router.swift (1)
request(81-107)FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)
FlagsmithClient/Tests/APIManagerTests.swift (3)
FlagsmithClient/Classes/Flagsmith.swift (1)
getFeatureFlags(126-171)FlagsmithClient/Tests/CacheTests.swift (1)
testSkipAPIWithCacheAvailable(100-162)FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)
🪛 GitHub Check: swift-lint
FlagsmithClient/Tests/CacheTests.swift
[failure] 11-11:
Type Body Length Violation: Class body should span 350 lines or less excluding comments and whitespace: currently spans 508 lines (type_body_length)
[failure] 493-493:
Line Length Violation: Line should be 160 characters or less; currently it has 168 characters (line_length)
[warning] 100-100:
Function Body Length Violation: Function body should span 50 lines or less excluding comments and whitespace: currently spans 51 lines (function_body_length)
[warning] 737-737:
File Length Violation: File should contain 400 lines or less: currently contains 737 (file_length)
[warning] 495-495:
Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)
[warning] 176-176:
Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)
FlagsmithClient/Tests/CachedURLResponseTests.swift
[failure] 166-166:
Identifier Name Violation: Variable name 'i' should be between 3 and 40 characters long (identifier_name)
FlagsmithClient/Tests/TestConfig.swift
[warning] 48-48:
Trailing Newline Violation: Files should have a single trailing newline (trailing_newline)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ubuntu-build
- GitHub Check: macos-build-13
🔇 Additional comments (20)
FlagsmithClient/Classes/Internal/SSEManager.swift (2)
128-130: Good: removed force‑unwrap, now guardingcompletionHandlerbefore reconnect.Safer and prevents accidental reconnects after
stop().
136-137: Good:[weak self]and optional bind prevent retain cycle and stray reconnects.This ensures we don’t restart after deallocation or after
stop()..gitignore (1)
56-57: Good call: ignore real-keys test config.Prevents accidental commits of secrets used by tests.
Example/FlagsmithClient.xcodeproj/project.pbxproj (1)
369-373: Deployment target raised to 15.6 for the Example app.Intentional? Library still targets iOS 12 via CocoaPods; the Example won’t install on older devices. If intentional, ignore.
Also applies to: 388-392
Example/Pods/Pods.xcodeproj/project.pbxproj (3)
10-36: Sources wiring (APIManager, SSEManager, cache files) looks correct.New/updated internals are included in the FlagsmithClient target as expected.
Also applies to: 405-427
39-52: Target dependency proxies rewired.Looks consistent after the project upgrade; no action.
Also applies to: 440-451
485-487: Build hardening flags disabled — confirm intent.
ENABLE_MODULE_VERIFIER = NO and ENABLE_USER_SCRIPT_SANDBOXING = NO are present in Example/Pods/Pods.xcodeproj/project.pbxproj at lines 485–486, 670–671, 710–711, 748–749; confirm this is intentional (commonly set by CocoaPods/Xcode) and enable these flags where compatible.Example/Pods/Local Podspecs/FlagsmithClient.podspec.json (1)
3-3: Confirmed: remote tag v3.8.3 exists — no action required.
Example/Pods/Local Podspecs/FlagsmithClient.podspec.json contains "version": "3.8.3" and "tag": "v3.8.3"; the Flagsmith/flagsmith-ios-client remote has refs/tags/v3.8.3.Package.swift (1)
29-30: LGTM! Proper test documentation exclusion.The exclusion of
README_Testing.mdfrom the test target is appropriate since it's documentation rather than test code. This prevents any potential build issues while keeping the documentation accessible in the repository.FlagsmithClient/Tests/README_Testing.md (1)
1-46: Well-structured testing documentation!The documentation clearly explains the dual testing approach (mocked vs. real API keys) and provides practical examples for running different test suites. The security consideration of not committing real API keys is properly addressed.
FlagsmithClient/Tests/TestConfig.swift (1)
13-30: Secure and flexible API key resolution logic.The cascading fallback mechanism (environment → file → mock) is well-designed, allowing for flexible test configurations while maintaining security by defaulting to mock keys.
FlagsmithClient/Classes/Internal/APIManager.swift (1)
74-76: Good practice: Early cache initialization.Setting the URLCache during initialization ensures consistent behavior from the start and avoids potential race conditions.
FlagsmithClient/Tests/CachedURLResponseTests.swift (2)
14-64: Comprehensive TTL extension testing!The test thoroughly validates the cache header manipulation logic with various TTL durations, properly asserting:
- Correct Cache-Control header values
- Removal of Expires and s-maxage headers
- Preservation of other headers
- Data integrity
129-136: Define expected behavior for negative expiration durations and assert it in tests.Current implementation (FlagsmithClient/Classes/Internal/CachedURLResponse.swift lines 20–21) sets:
headers["Cache-Control"] = "max-age=(duration == 0 ? 31_536_000 : duration)" — negative durations will produce a negative max-age string. The test (FlagsmithClient/Tests/CachedURLResponseTests.swift lines 129–136) only prints the result.
- Either normalize negatives in code (recommended: clamp to a sensible non‑negative value and document whether 0 means “1 year” or “immediate expiry”).
- Or keep current behavior but update the test to assert the exact expected header (e.g., XCTAssertEqual(cacheControl, "max-age=-1") or the normalized value you choose).
FlagsmithClient/Tests/CacheTests.swift (6)
36-97: Good, self-contained cache storage test.Well-scoped and deterministic.
256-290: Nice coverage for TTL=0 → 1 year mapping.Matches CachedURLResponse extension behavior.
334-413: Good end-to-end TTL application check; assert path is deterministic by pre-seeding only.Looks solid; uses configured TTL in header.
574-631: Great parameterized TTL scenarios; clean up between iterations is correct.LGTM.
506-533: decoder is defined in FlagsmithClientTestCase — no action requiredFlagsmithClient/Tests/FlagsmithClientTestCase.swift declares
let decoder: JSONDecoder = .init(), so the test's use ofdecoderis valid.
15-27: Avoid mutating Flagsmith.shared in tests — isolate or restore singleton state.setUp mutates Flagsmith.shared.cacheConfig (FlagsmithClient/Tests/CacheTests.swift:15–27) and can race when tests run in parallel. Fix options:
- Inject a per-test Flagsmith instance (preferred) or add a test-only initializer that accepts cacheConfig.
- Save original Flagsmith.shared state in setUp and fully restore it in tearDown.
- Disable parallel test execution for this target or guard mutations with @mainactor.
| inputFileListPaths = ( | ||
| "${PODS_ROOT}/Target Support Files/Pods-FlagsmithClient_Example/Pods-FlagsmithClient_Example-frameworks-${CONFIGURATION}-input-files.xcfilelist", | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Switch to filelists for Embed Pods frameworks is good; ensure filelists exist.
Using xcfilelists reduces merge noise. Verify the Debug/Release filelist files are present under Target Support Files.
Also applies to: 210-212
🏁 Script executed:
#!/bin/bash
fd -H --strip-cwd-prefix 'Pods-FlagsmithClient_Example-frameworks-*-(input|output)-files.xcfilelist' \
'Example/Pods/Target Support Files/Pods-FlagsmithClient_Example'Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for xcfilelist files for Pods-FlagsmithClient_Example..."
paths=(
"Example/Pods/Target Support Files/Pods-FlagsmithClient_Example"
"Pods/Target Support Files/Pods-FlagsmithClient_Example"
)
found=0
for p in "${paths[@]}"; do
if [ -d "$p" ]; then
echo "Directory exists: $p"
echo "Listing .xcfilelist files (maxdepth 1):"
find "$p" -maxdepth 1 -type f -iname "*.xcfilelist" -print -exec ls -l {} \; || true
echo "Listing files matching frameworks input/output patterns:"
find "$p" -maxdepth 1 -type f \( -iname "Pods-FlagsmithClient_Example-frameworks-*-input-files.xcfilelist" -o -iname "Pods-FlagsmithClient_Example-frameworks-*-output-files.xcfilelist" \) -print -exec ls -l {} \; || true
found=1
else
echo "Directory not found: $p"
fi
done
echo "Also searching the whole repo for any .xcfilelist files (will print matches):"
find . -type f -iname "*.xcfilelist" -print -exec ls -l {} \; || true
if [ "$found" -eq 0 ]; then
echo "No Target Support Files dirs found for Pods-FlagsmithClient_Example. Showing Example/ and Pods/ top-level directories for context:"
echo "---- Example/ ----"
ls -la Example || true
echo "---- Pods/ ----"
ls -la Pods || true
fiLength of output: 397
Missing .xcfilelist files referenced by project
Example/FlagsmithClient.xcodeproj/project.pbxproj references ${PODS_ROOT}/Target Support Files/Pods-FlagsmithClient_Example/Pods-FlagsmithClient_Example-frameworks-${CONFIGURATION}-input-files.xcfilelist (lines 204–206; also applies to 210–212), but Example/Pods/Target Support Files/Pods-FlagsmithClient_Example contains no .xcfilelist files.
- Action: either add the generated Debug/Release input/output .xcfilelist files (e.g., Pods-FlagsmithClient_Example-frameworks-Debug-input-files.xcfilelist and corresponding output files — typically created by running pod install) and commit them, or remove/replace the inputFileListPaths entries in the Xcode project so the build does not reference missing files.
🤖 Prompt for AI Agents
In Example/FlagsmithClient.xcodeproj/project.pbxproj around lines 204–206 (and
similarly 210–212) the project references missing .xcfilelist files under
${PODS_ROOT}/Target Support Files/Pods-FlagsmithClient_Example; fix by either
(A) regenerating and committing the Pod-generated input/output .xcfilelist files
(run pod install in Example to produce
Pods-FlagsmithClient_Example-frameworks-{Debug,Release}-{input,output}-files.xcfilelist
and add them to the repo), or (B) remove or replace the inputFileListPaths
entries in the pbxproj so the build phases do not reference non-existent files
(update the project file to point to valid files or remove the file list paths),
then verify the Xcode build succeeds.
| 27D373CC0D0864ABD84A3A70B28A7E2F /* APIManager.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = APIManager.swift; sourceTree = "<group>"; }; | ||
| 2F655DCB3D9C96A4B79EFCD4A6DE5EC1 /* TypedValue.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; name = TypedValue.swift; path = FlagsmithClient/Classes/TypedValue.swift; sourceTree = "<group>"; }; | ||
| 368DFAE2AAAB80524EFAFD71A2C92F84 /* Pods-FlagsmithClient_Example-acknowledgements.markdown */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text; path = "Pods-FlagsmithClient_Example-acknowledgements.markdown"; sourceTree = "<group>"; }; | ||
| 384DDA2CB25005BD6479B5987C619DD4 /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS18.0.sdk/System/Library/Frameworks/Foundation.framework; sourceTree = DEVELOPER_DIR; }; |
There was a problem hiding this comment.
Hard‑coded Foundation.framework to iPhoneOS18.0.sdk — fragile across Xcode/SDK versions.
Prefer SDKROOT-relative path or rely on implicit linkage to avoid breakage on older/newer Xcode.
-384DDA2CB25005BD6479B5987C619DD4 /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS18.0.sdk/System/Library/Frameworks/Foundation.framework; sourceTree = DEVELOPER_DIR; };
+384DDA2CB25005BD6479B5987C619DD4 /* Foundation.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Foundation.framework; path = System/Library/Frameworks/Foundation.framework; sourceTree = SDKROOT; };Alternatively, remove explicit Foundation linkage and let Swift implicitly link it.
Also applies to: 109-116, 117-124
🤖 Prompt for AI Agents
In Example/Pods/Pods.xcodeproj/project.pbxproj around line 68 (also applies to
blocks at lines 109-116 and 117-124), the Foundation.framework file reference is
hard-coded to Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS18.0.sdk/...
which is fragile across Xcode/SDK versions; change the fileReference path to use
SDKROOT-relative syntax (e.g., replace the absolute
Platforms/.../iPhoneOS18.0.sdk path with
SDKROOT/System/Library/Frameworks/Foundation.framework or set sourceTree to
SDKROOT and use System/Library/Frameworks/Foundation.framework) or remove the
explicit Foundation.framework PBXFileReference/BuildPhase entry so Swift/Clang
implicit linkage can handle it; update all the identical occurrences at the
noted line ranges to the SDKROOT-relative form or remove the explicit linkage
consistently.
| /// Test that verifies network requests are properly avoided when cache is available and skipAPI is true | ||
| func testSkipAPIWithCacheAvailable() throws { | ||
| let testCache = URLCache(memoryCapacity: 8 * 1024 * 1024, diskCapacity: 64 * 1024 * 1024, directory: nil) | ||
|
|
||
| // Configure Flagsmith like the customer | ||
| Flagsmith.shared.cacheConfig.useCache = true | ||
| Flagsmith.shared.cacheConfig.cache = testCache | ||
| Flagsmith.shared.cacheConfig.cacheTTL = 180 | ||
| Flagsmith.shared.cacheConfig.skipAPI = true | ||
|
|
||
| // Clear cache | ||
| testCache.removeAllCachedResponses() | ||
|
|
||
| let expectation = expectation(description: "Request with skipAPI and no cache should fail or return default") | ||
|
|
||
| // This should demonstrate the issue - when there's no cache and skipAPI=true, | ||
| // the behavior should be to NOT make a network request | ||
| Flagsmith.shared.getFeatureFlags { result in | ||
| switch result { | ||
| case .success(_): | ||
| // If it succeeds, it means a network request was made, which shouldn't happen with skipAPI=true | ||
| print("DEBUG: Request succeeded - this might indicate a network request was made when it shouldn't have been") | ||
| case .failure(_): | ||
| // This is expected behavior when no cache is available and skipAPI=true | ||
| print("DEBUG: Request failed as expected when no cache available and skipAPI=true") | ||
| } | ||
| expectation.fulfill() | ||
| } | ||
|
|
||
| wait(for: [expectation], timeout: 10.0) | ||
|
|
||
| // Cleanup | ||
| Flagsmith.shared.cacheConfig.skipAPI = false | ||
| Flagsmith.shared.cacheConfig.useCache = false | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Conflicting semantics with previous test.
This test expects no network when skipAPI=true and no cache, whereas testSkipAPICorrectBehavior allows network. Align tests to a single contract and assert it.
🤖 Prompt for AI Agents
In FlagsmithClient/Tests/APIManagerTests.swift around lines 151 to 186, the new
test has semantics conflicting with testSkipAPICorrectBehavior — it logs
ambiguous DEBUG statements instead of asserting a single contract for skipAPI.
Decide on one contract (e.g., skipAPI=true must NOT hit network and must return
failure when cache empty), then update both tests to assert that behavior
explicitly: replace the print/debug branches with XCTAssertFailure/
XCTAssertSuccess as appropriate so this test asserts failure when no cache and
skipAPI=true, and modify the other test to expect the same contract (or change
both to the opposite contract if you prefer network-allowed); remove ambiguous
prints, keep cache clearing, and ensure config flags are reset in a tearDown or
finally block to avoid test flakiness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
FlagsmithClient/Tests/CacheTests.swift (4)
11-11: Fix SwiftLint blanket disable warning.Use a more specific disable directive instead of a blanket disable for better code hygiene.
-// swiftlint:disable type_body_length +// swiftlint:disable:next type_body_length final class CacheTests: FlagsmithClientTestCase {
177-177: Fix SwiftLint empty enum arguments warnings.The linter correctly identifies that success case arguments are not being used.
case .success(_): // Success means network request was attempted and worked (unexpected with test key, but possible) print("DEBUG: Request succeeded - network request was made (expected behavior with no cache)") expectation.fulfill() - case .failure(let error): + case .failure(let error): // Failure means network request was attempted but failed (expected with invalid test key) // This is still the correct behavior - the important thing is that a network request was attempted print("DEBUG: Request failed as expected: \(error)")And similarly for line 497:
- case .success(_): + case .success: print("DEBUG: Unexpected success when cache disabled") // If it succeeded, verify it's NOT the cached data - let isFromCache = flags.isEmpty ? false : - (String(data: flags.first?.value.stringValue?.data(using: .utf8) ?? Data(), encoding: .utf8) == "cached data") - XCTAssertFalse(isFromCache, "Should not get cached data when useCache=false") + // Since we can't access flags here, just validate that network was attempted + breakAlso applies to: 497-497
461-505: Logic issue in cache disabled test.The test attempts to verify that cached data is not returned when
useCache=false, but the logic on lines 494-496 is problematic - it tries to accessflags.first?.value.stringValuebutflagsis not in scope in the success case.Simplify the test logic:
case .success(_): print("DEBUG: Unexpected success when cache disabled") - // If it succeeded, verify it's NOT the cached data - let isFromCache = flags.isEmpty ? false : - (String(data: flags.first?.value.stringValue?.data(using: .utf8) ?? Data(), encoding: .utf8) == "cached data") - XCTAssertFalse(isFromCache, "Should not get cached data when useCache=false") + // Success indicates network request was attempted, which is correct behavior case .failure(_): print("DEBUG: Expected failure when cache disabled and invalid API key") // This is fine - proves network request was attempted instead of using cache
739-739: File length exceeds SwiftLint recommendations.The file contains 739 lines, exceeding the 400-line limit. Consider splitting into smaller, focused test classes.
Consider splitting this into separate test classes:
CacheBasicTests.swift- Basic cache operations (lines 37-163)CacheTTLTests.swift- TTL-related tests (lines 205-738)CacheIntegrationTests.swift- Integration scenarios (lines 335-414, 576-632)This would improve maintainability and make tests easier to navigate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
FlagsmithClient/Tests/CacheTests.swift(1 hunks)FlagsmithClient/Tests/CachedURLResponseTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- FlagsmithClient/Tests/CachedURLResponseTests.swift
🧰 Additional context used
🧬 Code graph analysis (1)
FlagsmithClient/Tests/CacheTests.swift (3)
FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift (2)
setUp(14-29)tearDown(31-38)FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)FlagsmithClient/Tests/APIManagerTests.swift (1)
testSkipAPIWithCacheAvailable(152-185)
🪛 GitHub Check: swift-lint
FlagsmithClient/Tests/CacheTests.swift
[warning] 739-739:
File Length Violation: File should contain 400 lines or less: currently contains 739 (file_length)
[warning] 497-497:
Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)
[warning] 177-177:
Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)
[warning] 11-11:
Blanket Disable Command Violation: Use 'next', 'this' or 'previous' instead to disable the 'type_body_length' rule once, or re-enable it as soon as possible` (blanket_disable_command)
🪛 GitHub Actions: Pull Request Build and Test
FlagsmithClient/Tests/CacheTests.swift
[error] 13-13: URLCache is unavailable: This type has moved to the FoundationNetworking module. Import FoundationNetworking to use it.
[error] 68-68: Cannot find 'URLRequest' in scope.
[error] 71-73: HTTPURLResponse cannot be constructed: No accessible initializers.
[error] 81-85: CachedURLResponse cannot be constructed: No accessible initializers.
[error] 89-89: URLCache has no member 'storeCachedResponse'.
[error] 92-92: URLCache has no member 'cachedResponse'.
🔇 Additional comments (14)
FlagsmithClient/Tests/CacheTests.swift (14)
12-35: LGTM! Well-structured test setup with proper isolation.The test setup correctly creates isolated cache instances and properly resets Flagsmith configuration between tests, ensuring test independence.
37-98: LGTM! Comprehensive cache storage validation.The test effectively validates the basic cache storage and retrieval functionality with proper mock data structure. The JSON structure matches the expected Flag model format.
100-163: LGTM! Correctly tests skipAPI cache-hit behavior.This test properly validates that when
skipAPI=trueand cache data is available, the system returns cached data. The test setup and assertions are appropriate.
165-203: Well-designed test for skipAPI cache-miss behavior.The test correctly validates the expected behavior when
skipAPI=truebut no cache is available - the system should still attempt a network request. The debug logging and error analysis provide good insight into the behavior.
205-254: LGTM! Good TTL validation with proper cache header verification.The test correctly validates short TTL behavior using the cache extension and verifies that Cache-Control headers are set appropriately.
256-291: LGTM! Proper infinite TTL testing.Correctly validates that TTL=0 results in the expected 1-year max-age value (31,536,000 seconds) as per the implementation.
293-333: LGTM! Comprehensive custom TTL validation.Excellent test coverage for various TTL values with proper loop-based testing and cache cleanup between iterations.
335-414: LGTM! Effective TTL integration testing.Good integration test that validates TTL behavior in realistic request scenarios, properly simulating the
ensureResponseIsCachedfunctionality.
416-459: LGTM! Important server header override validation.Critical test that validates the client-side TTL configuration correctly overrides server-provided cache headers, which is essential for proper cache control.
507-534: LGTM! Good validation of the caching mechanism.The test effectively validates the manual cache fallback by testing the decode path that would be used by the caching system.
536-574: LGTM! Important cache isolation test.Critical test that validates cache entries are properly isolated by URL/endpoint, ensuring no cache collisions between different API calls.
576-632: LGTM! Comprehensive realistic scenario testing.Excellent comprehensive test covering multiple TTL scenarios with proper cleanup and validation of cache behavior across different configurations.
634-700: LGTM! Important dynamic TTL configuration test.Critical test that validates the system correctly handles TTL configuration changes between requests, ensuring cache entries use the updated TTL values.
702-738: LGTM! Good edge case coverage for invalid TTL values.Properly tests that the system handles invalid (negative) TTL values gracefully without crashing, with appropriate debug logging for validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/pull-request.yml (2)
19-22: Split the Swift version check into its own step and pass the API key for integration tests.Separating the version probe improves logs and failure signals. Also, tests in this PR mention requiring FLAGSMITH_TEST_API_KEY—surface it from secrets so CI can run the integration suite.
steps: - uses: actions/checkout@v4 - - name: Build (macOS) - run: swift --version && swift build -v + - name: Swift toolchain + run: swift --version + - name: Build (macOS) + run: swift build -v - - name: Run tests - run: swift test -v + - name: Run tests + env: + FLAGSMITH_TEST_API_KEY: ${{ secrets.FLAGSMITH_TEST_API_KEY }} + run: swift test -vVerification ask:
- Confirm that tests gracefully skip integration paths when the secret is unset (e.g., on forks). If not, we should gate the step with an if condition or split unit vs. integration jobs.
41-44: Install a pinned Swift toolchain on Ubuntu and export the API key to tests.ubuntu-latest doesn’t guarantee Swift availability. Use setup-swift to pin the toolchain, keep the version print separate, and pass FLAGSMITH_TEST_API_KEY.
steps: - uses: actions/checkout@v4 + - name: Setup Swift + uses: swiftlang/setup-swift@v1 + with: + swift-version: '5.10' # align with project; adjust if needed - - name: Build (Ubuntu) - run: swift --version && swift build -v + - name: Swift toolchain + run: swift --version + - name: Build (Ubuntu) + run: swift build -v - - name: Run tests - run: swift test -v + - name: Run tests + env: + FLAGSMITH_TEST_API_KEY: ${{ secrets.FLAGSMITH_TEST_API_KEY }} + run: swift test -vVerification ask:
- Please confirm which Swift version the repo targets (e.g., Package.swift tools version). We can convert this to a matrix or a repo-wide variable to keep macOS and Linux in lock‑step.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(2 hunks)FlagsmithClient/Tests/CachedURLResponseTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- FlagsmithClient/Tests/CachedURLResponseTests.swift
…e the API key available
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
FlagsmithClient/Tests/CacheTests.swift (8)
16-28: Initialize shared apiKey; remove unused apiManagerTests that go through Flagsmith.shared (e.g., getFeatureFlags) should set Flagsmith.shared.apiKey so cache keys/requests match. apiManager is unused here—either use it or drop it.
Apply:
- var apiManager: APIManager! + // No per-test APIManager needed; use Flagsmith.shared @@ - apiManager = APIManager() - apiManager.apiKey = "test-cache-api-key" + Flagsmith.shared.apiKey = "test-cache-api-key" @@ Flagsmith.shared.cacheConfig.skipAPI = false super.tearDown() } + override func tearDown() { + Flagsmith.shared.apiKey = nil + super.tearDown() + }Also applies to: 30-35
536-563: “Manual cache fallback” test never exercises ensureResponseIsCached()This only decodes JSON; it doesn’t store to URLCache nor validate the fallback path.
Example fix (adjust to actual signature/visibility):
- // Test the ensureResponseIsCached method indirectly - // by simulating a successful decode operation + // Exercise ensureResponseIsCached directly and then read back from testCache + let url = URL(string: "https://edge.api.flagsmith.com/api/v1/flags/")! + var req = URLRequest(url: url) + req.setValue("test-key", forHTTPHeaderField: "X-Environment-Key") + let resp = HTTPURLResponse(url: url, statusCode: 200, httpVersion: "HTTP/1.1", + headerFields: ["Content-Type": "application/json"])! + + // Assuming internal visibility under @testable import: + // APIManager().ensureResponseIsCached(request: req, data: mockData, response: resp) + // If the method is private, expose a test-only hook or move the logic behind a protocol you can mock. + + // Assert it landed in URLCache + let cached = testCache.cachedResponse(for: req) + XCTAssertNotNil(cached) + XCTAssertEqual(cached?.data, mockData)Confirm the method signature and visibility and I’ll tailor the exact patch.
11-12: Don’t blanket-disable SwiftLint rule; scope it or split testsSwitch to a scoped disable or split the suite to keep the class small.
-// swiftlint:disable type_body_length +// swiftlint:disable:next type_body_length final class CacheTests: FlagsmithClientTestCase {Also address file_length (768 lines > 400) by splitting into focused files: CacheBasicsTests, CacheTTLTests, SkipAPITests, IntegrationTTLTests.
176-181: Clean up empty enum argument matches (SwiftLint warning)Replace unused associated value matches with bare cases.
- case .success(_): + case .success: @@ - case .failure(_): + case .failure:Also applies to: 526-529
205-277: Short-TTL test still relies on timing + URLCache internalsEven with the session fix, timing-based expiry is flaky. Prefer asserting headers via response(withExpirationDuration:) and verifying SDK fetch logic against a stubbed session rather than sleeping.
I can provide a URLProtocol-based stub that serves cached/expired responses deterministically. Want me to drop in a helper and refactor this test set?
731-767: Negative TTL: decide and assert a policy (clamp or reject)response(withExpirationDuration:) will emit “max-age=-1”. Define behavior (e.g., clamp to 0→31536000 or to 0 meaning “default”), then assert it.
If clamping is desired, update CachedURLResponse.response(withExpirationDuration:) to:
- duration <= 0 → 31_536_000
- else → duration
and adjust this test accordingly.
507-531: Avoid print-heavy tests; use assertions or XCTAttachmentsConsole noise hinders CI logs. Replace prints with XCTFail/expectations or attach context as needed.
Also applies to: 175-200, 429-431
1-768: General: prefer stubbing over live network in unit testsSeveral tests allow or encourage real requests. Use an injectable URLSession/URLProtocol to keep unit tests fast, deterministic, and offline.
I can add a small TestSessionFactory and URLProtocol stub with counters to assert “no network” and serve canned JSON across the suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
FlagsmithClient/Tests/CacheTests.swift(1 hunks)FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift
🧰 Additional context used
🧬 Code graph analysis (1)
FlagsmithClient/Tests/CacheTests.swift (3)
FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift (2)
setUp(14-29)tearDown(31-38)FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)FlagsmithClient/Tests/APIManagerTests.swift (1)
testSkipAPIWithCacheAvailable(152-185)
🪛 GitHub Check: swift-lint
FlagsmithClient/Tests/CacheTests.swift
[warning] 768-768:
File Length Violation: File should contain 400 lines or less: currently contains 768 (file_length)
[warning] 526-526:
Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)
[warning] 177-177:
Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)
[warning] 11-11:
Blanket Disable Command Violation: Use 'next', 'this' or 'previous' instead to disable the 'type_body_length' rule once, or re-enable it as soon as possible` (blanket_disable_command)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ubuntu-build
- GitHub Check: macos-build-13
🔇 Additional comments (1)
FlagsmithClient/Tests/CacheTests.swift (1)
165-203: skipAPI semantics are correct — do not flip this testskipAPI only skips calling the API when a valid cache exists; with an empty cache the SDK should still attempt the network, so the current test behaviour is correct. (docs.flagsmith.com)
Add a URLProtocol/CountingURLProtocol spy if you want a stronger assertion of whether the network was hit; do not rename/flip the expectation.
Likely an incorrect or invalid review comment.
that expired cache entries are not returned when using .returnCacheDataDontLoad policy
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
.github/workflows/pull-request.yml (4)
22-25: Inject FLAGSMITH_TEST_API_KEY for integration tests or explicitly skip them.Per the PR, some tests rely on this env var. Without it, tests may fail or silently skip.
Option A: provide the secret in CI (recommended):
build: name: Swift ${{ matrix.swift }} on ${{ matrix.os }} strategy: matrix: os: [ubuntu-latest, macos-latest] swift: ["5.10", "6.0", "6.1"] runs-on: ${{ matrix.os }} + env: + FLAGSMITH_TEST_API_KEY: ${{ secrets.FLAGSMITH_TEST_API_KEY }} steps:Option B: add an explicit skip condition (if tests can be gated), to make CI intent clear.
12-16: Consider fail-fast: false to see all matrix failures.Default is true; setting false helps surface all breaks across Swift/OS combinations in one run.
strategy: - matrix: + fail-fast: false + matrix: os: [ubuntu-latest, macos-latest] swift: ["5.10", "6.0", "6.1"]
17-25: Speed up builds with SPM caching.Cache .build and SPM artifacts to reduce CI time/cost.
steps: - - uses: swift-actions/setup-swift@v2 + - uses: swift-actions/setup-swift@v2 with: swift-version: ${{ matrix.swift }} + - uses: actions/cache@v4 + with: + path: | + .build + ~/.swiftpm + key: ${{ runner.os }}-swift-${{ matrix.swift }}-${{ hashFiles('**/Package.resolved') }} + restore-keys: | + ${{ runner.os }}-swift-${{ matrix.swift }}- + ${{ runner.os }}-swift- - uses: actions/checkout@v5
1-9: Harden workflow security: minimal permissions and cancel stale runs.Reduce GITHUB_TOKEN scope and avoid wasting minutes on superseded runs.
name: Pull Request Build and Test on: @@ jobs: +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/pull-request.yml
18-18: specifying action "swift-actions/setup-swift" in invalid format because ref is missing. available formats are "{owner}/{repo}@{ref}" or "{owner}/{repo}/{path}@{ref}"
(action)
🔇 Additional comments (2)
.github/workflows/pull-request.yml (2)
21-21: Standardize actions/checkout to v5 — verify runner compatibility.actions/checkout@v5 is released and supported but requires GitHub Actions Runner v2.327.1 or newer (GHES may need sync). Update .github/workflows/pull-request.yml (lines 31, 42) from actions/checkout@v4 → @v5 only if your runners meet that minimum; otherwise keep @v4 or upgrade runners.
37-46: Pin third‑party actions to commit SHAs and address SwiftLint compatibilityFile: .github/workflows/pull-request.yml (lines 37–46)
- Finding: norio-nomura/action-swiftlint shows little recent maintenance (latest release >4 years ago) and ubuntu-latest moved to Ubuntu 24.04 on Dec 5, 2024 — compatibility is not guaranteed.
- Action: Pin actions to commit SHAs (actions/checkout and norio-nomura/action-swiftlint). Then either run this job on macos-latest, pin the runner to a known Ubuntu image (e.g., ubuntu-22.04), or replace the action with a maintained alternative or a custom step that installs a known Swift toolchain + SwiftLint. Test the workflow on the chosen runner.
Example pinning (replace with the real commit SHA):
- uses: norio-nomura/action-swiftlint@3.2.1 + uses: norio-nomura/action-swiftlint@<commit-sha>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/pull-request.yml (1)
18-21: Workflow won’t parse: missing @ref on action.GitHub Actions requires a ref for
uses:. Add a version (and ideally pin to a commit SHA) or the job won’t start.Apply:
- - uses: swift-actions/setup-swift + - uses: swift-actions/setup-swift@v2 with: swift-version: ${{ matrix.swift }}Supply‑chain hardening (recommended):
- - uses: swift-actions/setup-swift@v2 + - uses: swift-actions/setup-swift@<trusted-commit-sha>
🧹 Nitpick comments (3)
.github/workflows/pull-request.yml (3)
21-25: Speed up CI with SPM cache.Caching
.buildkeyed byPackage.resolvedcuts minutes off builds.- uses: actions/checkout@v5 + - name: Cache SPM + uses: actions/cache@v4 + with: + path: | + .build + ~/.swiftpm + key: ${{ runner.os }}-swift-${{ matrix.swift }}-${{ hashFiles('**/Package.resolved') }} + restore-keys: | + ${{ runner.os }}-swift-${{ matrix.swift }}- - name: Build run: swift build
30-33: Align checkout to v5 and pin actions for supply‑chain hygiene.Use the latest major and pin to SHAs where possible.
- - uses: actions/checkout@v4 + - uses: actions/checkout@v5Also consider pinning:
actions/checkout@v5→actions/checkout@<commit-sha>norio-nomura/action-swiftlint@3.2.1→norio-nomura/action-swiftlint@<commit-sha>
27-39: Optional: add run cancellation to avoid redundant CI on rapid pushes.Prevents wasting minutes on superseded builds.
jobs: build: + concurrency: + group: pr-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/pull-request.yml
18-18: specifying action "swift-actions/setup-swift" in invalid format because ref is missing. available formats are "{owner}/{repo}@{ref}" or "{owner}/{repo}/{path}@{ref}"
(action)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
12-25: Pass FLAGSMITH_TEST_API_KEY into the CI workflow so integration tests run.Your new tests require this env var; inject the secret at job or step scope in .github/workflows/pull-request.yml.
build: name: Swift ${{ matrix.swift }} on ${{ matrix.os }} + env: + FLAGSMITH_TEST_API_KEY: ${{ secrets.FLAGSMITH_TEST_API_KEY }} strategy: matrix: os: [ubuntu-latest, macos-latest] swift: ["5.10", "6.0"] runs-on: ${{ matrix.os }} steps: - uses: swift-actions/setup-swift@v2 with: swift-version: ${{ matrix.swift }}If secrets aren’t available for PRs from forks, guard integration tests or add a conditional job: if: secrets.FLAGSMITH_TEST_API_KEY != ''.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/pull-request.yml (1)
45-47: Avoid floating action ref “@next”; pin to a stable tag or commit SHA.Floating refs are a supply‑chain risk and can break builds unpredictably.
- - uses: swift-actions/setup-swift@next + - uses: swift-actions/setup-swift@v2 + # For stronger supply-chain guarantees, pin to a trusted commit SHA: + # uses: swift-actions/setup-swift@<trusted-commit-sha>
🧹 Nitpick comments (2)
.github/workflows/pull-request.yml (2)
10-21: Deduplicate macOS jobs or clarify intent.You now have three macOS paths (this job, xcrun job, and matrix macos-latest). Consolidate to one to cut CI time and flakiness, or gate extras behind a label.
Example options:
- Keep only build-matrix.
- Keep this as a “smoke” job and remove macos-latest-xcrun.
- If keeping xcrun for Xcode toolchain validation, skip tests here and run tests only in the matrix.
49-52: Gate or split integration tests that require secrets on PRs from forks.Pull request workflows don’t expose repository secrets to forks; tests needing FLAGSMITH_TEST_API_KEY should be skipped or moved to a protected workflow to avoid false failures.
Options:
- Detect missing key in tests and
XCTSkipintegration cases.- Split unit/integration: run unit on PRs; run integration on
pushorpull_request_targetwith strict permissions.- Conditionally run integration when
github.event.pull_request.head.repo.fork == false.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: macos-latest-xcrun
- GitHub Check: macos-latest
.github/workflows/pull-request.yml
Outdated
| macos-latest-xcrun: | ||
| # macOS-latest images are not the most recent | ||
| # The vast majority of macOS developers would be using the latest version of macOS | ||
| # Current list here: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#choosing-github-hosted-runners | ||
| runs-on: macOS-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Build (macOS) | ||
| run: swift build -v | ||
| - name: Run tests | ||
| run: swift test -v | ||
|
|
||
| ubuntu-build: | ||
| runs-on: ubuntu-latest | ||
| - name: Get Swift version | ||
| run: /usr/bin/xcrun --toolchain -f swift --version | ||
| - name: Run build | ||
| run: /usr/bin/xcrun --toolchain -f swift build -v | ||
|
|
||
| - name: Run Tests | ||
| run: /usr/bin/xcrun --toolchain -f swift test -v | ||
|
|
There was a problem hiding this comment.
xcrun invocations are invalid; steps won’t execute Swift.
You’re mixing --toolchain and -f/--find (which only prints a path). These commands will fail or no-op. Also fix the runner label casing.
- runs-on: macOS-latest
+ runs-on: macos-latest
@@
- - name: Get Swift version
- run: /usr/bin/xcrun --toolchain -f swift --version
+ - name: Get Swift version
+ run: /usr/bin/xcrun swift --version
@@
- - name: Run build
- run: /usr/bin/xcrun --toolchain -f swift build -v
+ - name: Run build
+ run: /usr/bin/xcrun swift build -v
@@
- - name: Run Tests
- run: /usr/bin/xcrun --toolchain -f swift test -v
+ - name: Run tests
+ run: /usr/bin/xcrun swift test -v📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| macos-latest-xcrun: | |
| # macOS-latest images are not the most recent | |
| # The vast majority of macOS developers would be using the latest version of macOS | |
| # Current list here: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#choosing-github-hosted-runners | |
| runs-on: macOS-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - name: Build (macOS) | |
| run: swift build -v | |
| - name: Run tests | |
| run: swift test -v | |
| ubuntu-build: | |
| runs-on: ubuntu-latest | |
| - name: Get Swift version | |
| run: /usr/bin/xcrun --toolchain -f swift --version | |
| - name: Run build | |
| run: /usr/bin/xcrun --toolchain -f swift build -v | |
| - name: Run Tests | |
| run: /usr/bin/xcrun --toolchain -f swift test -v | |
| macos-latest-xcrun: | |
| # macOS-latest images are not the most recent | |
| # The vast majority of macOS developers would be using the latest version of macOS | |
| # Current list here: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#choosing-github-hosted-runners | |
| runs-on: macos-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - name: Get Swift version | |
| run: /usr/bin/xcrun swift --version | |
| - name: Run build | |
| run: /usr/bin/xcrun swift build -v | |
| - name: Run tests | |
| run: /usr/bin/xcrun swift test -v |
🤖 Prompt for AI Agents
In .github/workflows/pull-request.yml around lines 22 to 36, the workflow uses
an incorrect runner casing and invalid xcrun flag order: change runs-on to the
correct lowercase "macos-latest", and fix the xcrun invocations so they either
run the swift binary or locate it correctly — e.g., remove the stray -f when you
intend to execute swift and use "--toolchain <toolchain> swift <args>" to run
with a specific toolchain, or keep "-f --toolchain <toolchain> swift" if you
only want the path; update the three step run lines (Get Swift version, Run
build, Run Tests) to use the proper flag ordering and a real toolchain name or
simply call "swift" directly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift (7)
15-19: Skip live‑API tests instead of failing when FLAGSMITH_TEST_API_KEY is absentFailing on missing env makes CI noisy; prefer skipping. Also, the ternary for
testIdentitybecomes unreachable once you skip early.Apply:
- guard TestConfig.hasRealApiKey else { - XCTFail("This customer use case test requires FLAGSMITH_TEST_API_KEY environment variable to be set") - return - } + try XCTSkipIf(!TestConfig.hasRealApiKey, "Requires FLAGSMITH_TEST_API_KEY to run against live API") ... - let testIdentity = TestConfig.hasRealApiKey ? TestConfig.testIdentity : "customer-test-identity" + let testIdentity = TestConfig.testIdentityAlso applies to: 44-44
54-56: Avoid arbitrary sleeps; wait deterministically for cacheThe 100ms delay is flaky. Either (a) invoke the second call immediately (URLSession writes cache before completion), or (b) poll URLCache until the entry exists, then proceed.
Example approach (conceptual):
// Immediately call without delay: Flagsmith.shared.getFeatureFlags(forIdentity: testIdentity) { secondResult in ... } // Or poll deterministically (build the same request the SDK uses, then): let deadline = Date().addingTimeInterval(2) while Flagsmith.shared.cacheConfig.cache.cachedResponse(for: request) == nil, Date() < deadline { RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01)) }Also applies to: 71-71
137-141: Build the mock request via URLComponents to avoid URL/encoding mismatchesHand‑built strings risk cache key mismatches with the SDK’s actual URL. Compose via URLComponents so it matches exactly (including encoding and trailing slash).
Apply:
- let mockURL = URL(string: "https://edge.api.flagsmith.com/api/v1/identities/?identifier=\(testIdentity)")! - var mockRequest = URLRequest(url: mockURL) - mockRequest.setValue(testApiKey, forHTTPHeaderField: "X-Environment-Key") - mockRequest.setValue("application/json", forHTTPHeaderField: "Content-Type") - mockRequest.cachePolicy = .returnCacheDataElseLoad + var comps = URLComponents(string: "https://edge.api.flagsmith.com/api/v1/identities/")! + comps.queryItems = [URLQueryItem(name: "identifier", value: testIdentity)] + let mockURL = comps.url! + var mockRequest = URLRequest(url: mockURL) + mockRequest.httpMethod = "GET" + mockRequest.setValue(testApiKey, forHTTPHeaderField: "X-Environment-Key") + mockRequest.setValue("application/json", forHTTPHeaderField: "Content-Type") + mockRequest.cachePolicy = .returnCacheDataElseLoad
221-277: Add assertions; this “session‑long caching” test currently passes regardless of behaviorNo assertions verify session behavior. Either pre‑populate cache (like the previous test) and assert all N calls succeed, or assert consistent outcomes across the session.
Example:
- let totalRequests = 5 // Simulate multiple requests during session + let totalRequests = 5 + var outcomes: [Bool] = [] ... - switch result { - case .success(_): - // Request succeeded - break - case .failure(_): - // Request failed as expected - // If we have a real API key and caching is enabled, failures after first success indicate a problem - if TestConfig.hasRealApiKey && requestCount > 1 { - // Warning: Session request failed - cache might not be working - // Note: Not failing here as session behavior may vary, but logging concern - } - break - } + outcomes.append({ + if case .success = result { return true } else { return false } + }()) ... - } else { - // Session simulation complete - expectation.fulfill() - } + } else { + XCTAssert(outcomes.allSatisfy({ $0 }) || outcomes.allSatisfy({ !$0 }), + "Expect consistent session behavior; got mixed outcomes: \(outcomes)") + expectation.fulfill() + }Alternatively, pre‑populate cache first and assert all succeed.
345-349: Replace RequestCounter with a URLProtocol to count real HTTP hitsIncrementing on completion conflates cache and network. If you need this signal, register a custom URLProtocol to count actual HTTP requests.
Sketch:
final class CountingURLProtocol: URLProtocol { static var count = 0 override class func canInit(with request: URLRequest) -> Bool { true } override class func canonicalRequest(for request: URLRequest) -> URLRequest { request } override func startLoading() { Self.count += 1; client?.urlProtocol(self, didFailWithError: URLError(.cancelled)) } override func stopLoading() {} } // In test setup: inject into the session configuration used by the SDK.Also applies to: 351-352, 372-379
384-384: Fix swift-lint line length (163 > 160)Wrap the long debug line.
Apply:
- print("DEBUG: Real API key provided but all requests failed - this may indicate API key/environment issues rather than cache problems") + print("DEBUG: Real API key provided but all requests failed — " + + "this may indicate API key/environment issues rather than cache problems")
169-181: Consider setting a Public cache directive and using the TTL helper for parity with productionTo mirror production behavior, add
publicto Cache-Control and/or use yourresponse(withExpirationDuration:)helper to normalize headers consistently across tests.Example:
- let cachedResponse = CachedURLResponse(response: httpResponse, data: mockIdentityResponse) + let cachedResponse = CachedURLResponse(response: httpResponse, data: mockIdentityResponse) + .response(withExpirationDuration: 180)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
FlagsmithClient/Tests/APIManagerTests.swift(1 hunks)FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift(1 hunks)FlagsmithClient/Tests/TestConfig.swift(1 hunks)Package.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- FlagsmithClient/Tests/APIManagerTests.swift
- FlagsmithClient/Tests/TestConfig.swift
- Package.swift
🧰 Additional context used
🧬 Code graph analysis (1)
FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift (1)
FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)
🪛 GitHub Check: swift-lint
FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift
[failure] 384-384:
Line Length Violation: Line should be 160 characters or less; currently it has 163 characters (line_length)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: macos-latest
| // Configure exactly as the customer reported | ||
| let env = (baseURL: URL(string: "https://edge.api.flagsmith.com/api/v1/")!, apiKey: TestConfig.apiKey) | ||
|
|
||
| Flagsmith.shared.apiKey = env.apiKey | ||
| Flagsmith.shared.baseURL = env.baseURL | ||
| Flagsmith.shared.enableRealtimeUpdates = false | ||
| Flagsmith.shared.cacheConfig.useCache = true | ||
| Flagsmith.shared.cacheConfig.cache = URLCache( | ||
| memoryCapacity: 8 * 1024 * 1024, // 8 MB | ||
| diskCapacity: 64 * 1024 * 1024, // 64 MB | ||
| directory: nil | ||
| ) | ||
| Flagsmith.shared.cacheConfig.cacheTTL = 180 | ||
| Flagsmith.shared.cacheConfig.skipAPI = true | ||
|
|
||
| // Configuration matches customer's exact settings | ||
|
|
There was a problem hiding this comment.
Restore global Flagsmith.shared state and clear URLCache to prevent cross‑test bleed
Tests mutate global config and cache but only partially reset. This will cause order‑dependent flaky tests under parallel runs.
Apply this pattern at the start of each test (or in setUp/tearDown):
+ // Snapshot and auto‑restore global client state
+ let prior = (
+ apiKey: Flagsmith.shared.apiKey,
+ baseURL: Flagsmith.shared.baseURL,
+ enableRealtimeUpdates: Flagsmith.shared.enableRealtimeUpdates,
+ cacheConfig: Flagsmith.shared.cacheConfig
+ )
+ defer {
+ Flagsmith.shared.apiKey = prior.apiKey
+ Flagsmith.shared.baseURL = prior.baseURL
+ Flagsmith.shared.enableRealtimeUpdates = prior.enableRealtimeUpdates
+ Flagsmith.shared.cacheConfig = prior.cacheConfig
+ Flagsmith.shared.cacheConfig.cache.removeAllCachedResponses()
+ }Then remove the ad‑hoc cleanups:
- Flagsmith.shared.cacheConfig.skipAPI = false
- Flagsmith.shared.cacheConfig.useCache = false
- Flagsmith.shared.apiKey = nilAlso applies to: 106-110, 214-218, 226-231, 273-277, 316-320, 333-341, 412-416
🤖 Prompt for AI Agents
In FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift around lines 25 to 41,
the test mutates global Flagsmith.shared and URLCache without fully restoring
them which causes cross-test bleeding; save the current Flagsmith.shared
configuration and URLCache.shared state at test start (or in setUp), perform the
test-specific mutations, and then fully restore the original Flagsmith.shared
properties and clear/restore URLCache.shared (or call
URLCache.shared.removeAllCachedResponses and reset to the previous cache) in
tearDown so each test leaves global state unchanged; apply the same pattern to
the other affected ranges (106-110, 214-218, 226-231, 273-277, 316-320, 333-341,
412-416).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
FlagsmithClient/Tests/APIManagerTests.swift (2)
89-130: Test logic conflicts with skipAPI semantics from other tests.This test assumes skipAPI=true allows network requests when no cache is available, but
testSkipAPIWithCacheAvailable(lines 133-160) suggests the opposite - that skipAPI=true should NOT make network requests. The tests have conflicting expectations for the same configuration.Looking at the APIManager implementation,
skipAPI=truesets cache policy to.returnCacheDataElseLoadwhich will attempt network if no cache exists. Clarify the intended skipAPI behavior and align both tests to assert the same contract consistently.- // First request - no cache available, network request expected + // First request - no cache available, should either succeed (network allowed) + // or fail (network prohibited) - document expected behavior Flagsmith.shared.getFeatureFlags { result in + // TODO: Assert expected behavior based on clarified skipAPI contract expectation1.fulfill()
133-160: Test expectations are unclear and conflict with other test assumptions.This test comments suggest skipAPI=true should prevent network requests, but doesn't assert this behavior. It also conflicts with
testSkipAPICorrectBehaviorwhich expects network requests with skipAPI=true.The test needs clear assertions about what constitutes correct behavior:
Flagsmith.shared.getFeatureFlags { result in - // If it succeeds, it means a network request was made, which shouldn't happen with skipAPI=true - // If it fails, this is expected behavior when no cache is available and skipAPI=true + switch result { + case .success: + XCTFail("skipAPI=true with no cache should not make network requests") + case .failure: + // Expected behavior - no network request made + break + } expectation.fulfill() }
🧹 Nitpick comments (1)
FlagsmithClient/Tests/APIManagerTests.swift (1)
62-87: Good test resilience improvements, but consider asserting test behavior.The test improvements are solid: gating on real API key availability, reducing iterations, and adding timeout protection. However, the test now only logs results without any assertions, making it unclear what constitutes a pass/fail.
Consider adding at least a basic assertion to verify the concurrent requests don't crash:
concurrentQueue.async { self.apiManager.request(.getFlags) { (result: Result<Void, any Error>) in - // For concurrent testing, we just need to ensure the request completes - // without crashing - the result doesn't matter for this test - print("Iteration \(concurrentIteration) finished with \(result)") + // For concurrent testing, we just need to ensure the request completes + // The result doesn't matter - both success and failure are acceptable expectation.fulfill() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
FlagsmithClient/Tests/APIManagerTests.swift(2 hunks)FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift
🧰 Additional context used
🧬 Code graph analysis (1)
FlagsmithClient/Tests/APIManagerTests.swift (5)
FlagsmithClient/Classes/Internal/APIManager.swift (3)
request(131-167)request(174-183)request(191-213)FlagsmithClient/Classes/Internal/Router.swift (1)
request(81-107)FlagsmithClient/Classes/Flagsmith.swift (1)
getFeatureFlags(126-171)FlagsmithClient/Tests/CacheTests.swift (1)
testSkipAPIWithCacheAvailable(101-163)FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: macos-latest
🔇 Additional comments (2)
FlagsmithClient/Tests/APIManagerTests.swift (2)
39-39: LGTM! Good test isolation with mock API key.Using a mock API key instead of a real one properly isolates the test to verify the invalid URL behavior without depending on external resources.
163-251: Good validation of caching fixes implementation.This test effectively validates the manual caching fallback by pre-populating the cache and verifying skipAPI behavior works correctly. The test structure is solid and the assertions properly verify the cached data is used.
The test correctly simulates the
ensureResponseIsCachedfunctionality and validates that cached responses are properly utilized when skipAPI=true.
The concurrent request test was failing and this solves this issue
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
FlagsmithClient/Classes/Internal/APIManager.swift (1)
149-151: Enforce skipAPI: don’t fall back to network — use returnCacheDataDontLoadskipAPI=true must never trigger network requests; .returnCacheDataElseLoad will hit network if cache is missing. Change to .returnCacheDataDontLoad.
File: FlagsmithClient/Classes/Internal/APIManager.swift (around lines 149–151)
- request.cachePolicy = .returnCacheDataElseLoad + request.cachePolicy = .returnCacheDataDontLoad
🧹 Nitpick comments (1)
FlagsmithClient/Classes/Internal/APIManager.swift (1)
201-206: Manual cache fallback is helpful — but ensure it only applies to cache‑eligible responses.This hook runs after decode; make sure the fallback only stores idempotent GET responses (see next comment for a concrete guard).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
FlagsmithClient/Classes/Internal/APIManager.swift(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
FlagsmithClient/Classes/Internal/APIManager.swift (2)
FlagsmithClient/Classes/Internal/Router.swift (1)
request(81-107)FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)
🔇 Additional comments (5)
FlagsmithClient/Classes/Internal/APIManager.swift (5)
34-37: Good: property write barrier for baseURL.Using a barrier on propertiesSerialAccessQueue makes the setter thread-safe.
47-50: Good: property write barrier for apiKey.Thread-safe and consistent with baseURL.
59-62: Good: property write barrier for lastUpdatedAt.Consistent synchronization model.
74-76: Good: bind initial session to shared URLCache.Sets a sensible baseline before dynamic reconfiguration.
105-107: Correct: bypass caching when disabled.completionHandler(nil) is the right behavior.
| // Update session cache configuration if it has changed (must be done inside the serial queue) | ||
| if session.configuration.urlCache !== Flagsmith.shared.cacheConfig.cache { | ||
| let configuration = URLSessionConfiguration.default | ||
| configuration.urlCache = Flagsmith.shared.cacheConfig.cache | ||
| session = URLSession(configuration: configuration, delegate: self, delegateQueue: OperationQueue.main) | ||
| } |
There was a problem hiding this comment.
Don’t rebuild URLSession from URLSessionConfiguration.default — preserves must be kept.
Recreating from .default can silently reset critical settings (timeouts, waitsForConnectivity, headers, proxy/TLS, multipath, etc.). Copy the existing configuration, change only urlCache, and rebuild the session.
Apply this diff:
- let configuration = URLSessionConfiguration.default
- configuration.urlCache = Flagsmith.shared.cacheConfig.cache
- session = URLSession(configuration: configuration, delegate: self, delegateQueue: OperationQueue.main)
+ var configuration = session.configuration // returns a copy of the current config
+ configuration.urlCache = Flagsmith.shared.cacheConfig.cache
+ session = URLSession(configuration: configuration,
+ delegate: self,
+ delegateQueue: OperationQueue.main)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Update session cache configuration if it has changed (must be done inside the serial queue) | |
| if session.configuration.urlCache !== Flagsmith.shared.cacheConfig.cache { | |
| let configuration = URLSessionConfiguration.default | |
| configuration.urlCache = Flagsmith.shared.cacheConfig.cache | |
| session = URLSession(configuration: configuration, delegate: self, delegateQueue: OperationQueue.main) | |
| } | |
| // Update session cache configuration if it has changed (must be done inside the serial queue) | |
| if session.configuration.urlCache !== Flagsmith.shared.cacheConfig.cache { | |
| var configuration = session.configuration // returns a copy of the current config | |
| configuration.urlCache = Flagsmith.shared.cacheConfig.cache | |
| session = URLSession(configuration: configuration, | |
| delegate: self, | |
| delegateQueue: OperationQueue.main) | |
| } |
🤖 Prompt for AI Agents
In FlagsmithClient/Classes/Internal/APIManager.swift around lines 156 to 161,
the code recreates the URLSession using URLSessionConfiguration.default which
can reset important settings; instead obtain the current session.configuration,
make a mutable copy (e.g. let configuration = session.configuration), update
only configuration.urlCache = Flagsmith.shared.cacheConfig.cache, then recreate
the session with that modified configuration (keeping delegate and
delegateQueue) inside the serial queue so all other existing settings are
preserved.
|
|
||
| /// Ensures that a successful response is properly cached | ||
| /// This is a fallback mechanism in case URLSession's automatic caching fails | ||
| private func ensureResponseIsCached(router: Router, data: Data) { | ||
| guard let apiKey = apiKey, !apiKey.isEmpty else { return } | ||
|
|
||
| do { | ||
| let request = try router.request(baseUrl: baseURL, apiKey: apiKey) | ||
|
|
||
| // Check if response is already cached | ||
| if Flagsmith.shared.cacheConfig.cache.cachedResponse(for: request) != nil { | ||
| return // Already cached | ||
| } | ||
|
|
||
| // Create a cacheable response | ||
| let httpResponse = HTTPURLResponse( | ||
| url: request.url!, | ||
| statusCode: 200, | ||
| httpVersion: "HTTP/1.1", | ||
| headerFields: [ | ||
| "Content-Type": "application/json", | ||
| "Cache-Control": "max-age=\(Int(Flagsmith.shared.cacheConfig.cacheTTL))" | ||
| ] | ||
| )! | ||
|
|
||
| let cachedResponse = CachedURLResponse( | ||
| response: httpResponse, | ||
| data: data, | ||
| userInfo: nil, | ||
| storagePolicy: .allowed | ||
| ) | ||
|
|
||
| // Store the response in cache | ||
| Flagsmith.shared.cacheConfig.cache.storeCachedResponse(cachedResponse, for: request) | ||
|
|
||
| } catch { | ||
| // If we can't create the request, just skip caching | ||
| print("Flagsmith: Failed to manually cache response: \(error)") | ||
| } | ||
| } |
There was a problem hiding this comment.
Harden ensureResponseIsCached: restrict to GET, avoid force‑unwraps, and align TTL semantics.
- Only cache GET requests to avoid incorrect reuse of POST/PUT responses.
- Avoid force‑unwraps when creating HTTPURLResponse.
- Reuse CachedURLResponse.response(withExpirationDuration:) so TTL=0 behavior matches the delegate path (maps to 1 year).
- Prefer consistent logging (avoid print in SDK code).
Apply this diff:
private func ensureResponseIsCached(router: Router, data: Data) {
guard let apiKey = apiKey, !apiKey.isEmpty else { return }
do {
- let request = try router.request(baseUrl: baseURL, apiKey: apiKey)
+ let request = try router.request(baseUrl: baseURL, apiKey: apiKey)
+
+ // Only cache idempotent GET requests
+ if request.httpMethod?.uppercased() != "GET" { return }
// Check if response is already cached
if Flagsmith.shared.cacheConfig.cache.cachedResponse(for: request) != nil {
return // Already cached
}
- // Create a cacheable response
- let httpResponse = HTTPURLResponse(
- url: request.url!,
- statusCode: 200,
- httpVersion: "HTTP/1.1",
- headerFields: [
- "Content-Type": "application/json",
- "Cache-Control": "max-age=\(Int(Flagsmith.shared.cacheConfig.cacheTTL))"
- ]
- )!
-
- let cachedResponse = CachedURLResponse(
- response: httpResponse,
- data: data,
- userInfo: nil,
- storagePolicy: .allowed
- )
+ guard let url = request.url else { return }
+ // Create a minimal 200 OK response; headers will be normalized by response(withExpirationDuration:)
+ guard let httpResponse = HTTPURLResponse(url: url,
+ statusCode: 200,
+ httpVersion: "HTTP/1.1",
+ headerFields: ["Content-Type": "application/json"])
+ else { return }
+
+ var cachedResponse = CachedURLResponse(response: httpResponse,
+ data: data,
+ userInfo: nil,
+ storagePolicy: .allowed)
+ // Normalize Cache-Control (incl. TTL==0 -> 1 year) to match delegate path
+ cachedResponse = cachedResponse.response(withExpirationDuration: Int(Flagsmith.shared.cacheConfig.cacheTTL))
// Store the response in cache
Flagsmith.shared.cacheConfig.cache.storeCachedResponse(cachedResponse, for: request)
} catch {
- // If we can't create the request, just skip caching
- print("Flagsmith: Failed to manually cache response: \(error)")
+ // If we can't create the request, just skip caching
+ #if DEBUG
+ debugPrint("Flagsmith: Failed to manually cache response: \(error)")
+ #endif
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In FlagsmithClient/Classes/Internal/APIManager.swift around lines 214-253,
restrict manual caching to only HTTP GET requests, avoid force-unwrapping
optionals (e.g. request.url and HTTPURLResponse initialiser), use the existing
CachedURLResponse.response(withExpirationDuration:) helper to construct the
cacheable response so TTL=0 maps to the one-year semantics, and replace the
print call with the SDK logger (e.g. Flagsmith.shared.logger?.error). Ensure you
bail early for non-GET methods, safely unwrap and guard needed values, build the
HTTPURLResponse safely before creating the CachedURLResponse via the shared
helper, then store it in the cache; log failures through the shared logger
rather than using print.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
AppDelegate.swift (6)
108-115: Mark async UI-affecting methods as @mainactor.Prevents accidental UI work off-main and quiets concurrency warnings.
#if swift(>=5.5.2) @@ - @available(iOS 13.0, *) - func determineAppConfiguration() async { + @available(iOS 13.0, *) + @MainActor + func determineAppConfiguration() async { @@ - func setTheme(_ theme: TypedValue) {} - func processFlags(_ flags: [Flag]) {} + @MainActor func setTheme(_ theme: TypedValue) {} + @MainActor func processFlags(_ flags: [Flag]) {} #endifAlso applies to: 136-138
19-21: Prefer @main over @UIApplicationMain.Modern attribute; reduces legacy wrinkles.
-@UIApplicationMain +@main class AppDelegate: UIResponder, UIApplicationDelegate {
15-17: Refactor isSuccess into a Result extension (or constrain Failure).Cleaner call-sites and proper
Failure: Errorconstraint. Also appears unused here.-func isSuccess<T, F>(_ result: Result<T, F>) -> Bool { - if case .success = result { return true } else { return false } -} +extension Result { + var isSuccess: Bool { if case .success = self { true } else { false } } +}
28-28: Avoid hardcoding API keys; read from Info.plist.Prevents accidental commits of secrets and eases configuration.
- Flagsmith.shared.apiKey = "<add your API key from the Flagsmith dashboard here>" + if let key = Bundle.main.object(forInfoDictionaryKey: "FLAGSMITH_API_KEY") as? String, !key.isEmpty { + Flagsmith.shared.apiKey = key + } else { + assertionFailure("Missing FLAGSMITH_API_KEY in Info.plist") + }
56-62: Gate noisy prints to DEBUG (avoid leaking values in prod logs).- Flagsmith.shared.getFeatureFlags { (result) in - print(result) - } - Flagsmith.shared.hasFeatureFlag(withID: "freeze_delinquent_accounts") { (result) in - print(result) - } + #if DEBUG + Flagsmith.shared.getFeatureFlags { result in print(result) } + Flagsmith.shared.hasFeatureFlag(withID: "freeze_delinquent_accounts") { result in print(result) } + #endif
63-71: Gate the 20 concurrent startup calls behind DEBUG (can hammer APIs/analytics).- for _ in 1...20 { - concurrentQueue.async { - Flagsmith.shared.getFeatureFlags { (_) in - } - } - } + #if DEBUG + for _ in 1...20 { + concurrentQueue.async { Flagsmith.shared.getFeatureFlags { _ in } } + } + #endifPlease confirm this burst is intended for demos only and won’t run in release builds.
FlagsmithClient/Tests/CacheTests.swift (9)
261-270: Use an ephemeral session for isolationPrefer
URLSessionConfiguration.ephemeralto avoid interacting with shared cookies/cache across tests.- let config = URLSessionConfiguration.default + let config = URLSessionConfiguration.ephemeral config.urlCache = testCache config.requestCachePolicy = .returnCacheDataDontLoad let session = URLSession(configuration: config)
271-279: Flaky assumption: .returnCacheDataDontLoad does not enforce TTL
.returnCacheDataDontLoadreturns cached data even if stale; expecting an error after TTL expiry is unreliable across platforms. Assert TTL application via headers (already done) or switch to.useProtocolCachePolicywith a stubbed protocol to observe a revalidation attempt.
186-186: SwiftLint: remove empty enum argumentsReplace
case .success(_)andcase .failure(_)with argument-less matches.- case .success(_): + case .success: ... - case .failure(_): + case .failure:Also applies to: 541-541
11-11: Avoid blanket SwiftLint disable; scope or split insteadUse
// swiftlint:disable:this type_body_lengthnear the class or break the suite into smaller files. The file also exceeds the 400-line guideline; consider splitting by concern (skipAPI, TTL, keying).
748-781: Negative TTLs produce invalid max-age; clamp in implementation and assert hereYour extension currently yields
Cache-Control: max-age=-1for negatives. Recommend clamping to non-negative (treat ≤0 as 1 year per your 0 rule) and update this test to assert the clamped value.Suggested change (in CachedURLResponse.swift, for maintainers of that file):
- headers["Cache-Control"] = "max-age=\(duration == 0 ? 31_536_000 : duration)" + let safe = duration <= 0 ? 31_536_000 : duration + headers["Cache-Control"] = "max-age=\(safe)"Then assert here:
XCTAssertEqual(cacheControl, "max-age=31536000")
47-104: Synchronous test; drop the expectationThis test is fully synchronous. Removing the expectation reduces noise and test time.
525-544: Unreliable cache-bypass verificationThe success-path check compares a string payload to a typed flag value and will never trigger. Prefer asserting that a network attempt occurred (spy protocol) or that the decoded flags do not match the cached bytes by struct equality if you must.
328-367: Reduce duplication with helpersA lot of repeated boilerplate for building requests/responses. Extract small helpers to cut noise and trim function lengths.
Example:
func makeRequest(_ url: URL, key: String) -> URLRequest { var r = URLRequest(url: url); r.setValue(key, forHTTPHeaderField: "X-Environment-Key"); return r } func makeHTTP200(_ url: URL, headers: [String:String] = ["Content-Type":"application/json"]) -> HTTPURLResponse { HTTPURLResponse(url: url, statusCode: 200, httpVersion: "HTTP/1.1", headerFields: headers)! }Also applies to: 370-452, 678-744
255-286: Non-deterministic timingRelying on 2s delays for TTL expiry can be flaky under CI load. Prefer deterministic checks (header assertions) or a stubbed loader to simulate expiry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
AppDelegate.swift(1 hunks)FlagsmithClient/Tests/CacheTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
FlagsmithClient/Tests/CacheTests.swift (3)
FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift (2)
setUp(14-29)tearDown(31-38)FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)FlagsmithClient/Tests/APIManagerTests.swift (1)
testSkipAPIWithCacheAvailable(133-160)
🪛 GitHub Check: swift-lint
FlagsmithClient/Tests/CacheTests.swift
[warning] 107-107:
Function Body Length Violation: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines (function_body_length)
[warning] 783-783:
File Length Violation: File should contain 400 lines or less: currently contains 783 (file_length)
[warning] 541-541:
Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)
[warning] 186-186:
Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)
[warning] 11-11:
Blanket Disable Command Violation: Use 'next', 'this' or 'previous' instead to disable the 'type_body_length' rule once, or re-enable it as soon as possible` (blanket_disable_command)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: macos-latest
🔇 Additional comments (2)
AppDelegate.swift (2)
30-55: SDK init and cache/analytics/realtime toggles look consistent with the PR’s fixes.If the example app is used in CI runs, confirm defaults (useCache=true, TTL=90, skipAPI=false) align with test expectations to avoid flaky integration tests.
75-81: Scene/UIScene check — no UIApplicationSceneManifest found; AppDelegate-managed window is OK.
Example/FlagsmithClient/Info.plist contains no UIApplicationSceneManifest/UIScene entries and AppDelegate.swift still declares var window, so no SceneDelegate changes are required.
| // Launch SwiftUIView | ||
| if #available(iOS 13.0, *) { | ||
| let swiftUIView = SwiftUIView() | ||
| window = UIWindow(frame: UIScreen.main.bounds) | ||
| window?.rootViewController = UIHostingController(rootView: swiftUIView) | ||
| window?.makeKeyAndVisible() | ||
| } |
There was a problem hiding this comment.
Guard SwiftUI usage with canImport to prevent build failures on toolchains without SwiftUI.
UIHostingController is referenced outside the #if canImport(SwiftUI) guard. Wrap usage too.
- // Launch SwiftUIView
- if #available(iOS 13.0, *) {
- let swiftUIView = SwiftUIView()
- window = UIWindow(frame: UIScreen.main.bounds)
- window?.rootViewController = UIHostingController(rootView: swiftUIView)
- window?.makeKeyAndVisible()
- }
+ // Launch SwiftUIView
+ #if canImport(SwiftUI)
+ if #available(iOS 13.0, *) {
+ let swiftUIView = SwiftUIView()
+ window = UIWindow(frame: UIScreen.main.bounds)
+ window?.rootViewController = UIHostingController(rootView: swiftUIView)
+ window?.makeKeyAndVisible()
+ }
+ #endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Launch SwiftUIView | |
| if #available(iOS 13.0, *) { | |
| let swiftUIView = SwiftUIView() | |
| window = UIWindow(frame: UIScreen.main.bounds) | |
| window?.rootViewController = UIHostingController(rootView: swiftUIView) | |
| window?.makeKeyAndVisible() | |
| } | |
| // Launch SwiftUIView | |
| #if canImport(SwiftUI) | |
| if #available(iOS 13.0, *) { | |
| let swiftUIView = SwiftUIView() | |
| window = UIWindow(frame: UIScreen.main.bounds) | |
| window?.rootViewController = UIHostingController(rootView: swiftUIView) | |
| window?.makeKeyAndVisible() | |
| } | |
| #endif |
🤖 Prompt for AI Agents
In AppDelegate.swift around lines 75–81, the SwiftUI usage references
UIHostingController outside of a canImport(SwiftUI) guard which can break builds
on toolchains without SwiftUI; wrap the SwiftUIView, UIHostingController and
related UIWindow code in a conditional compilation block using #if
canImport(SwiftUI) (and import SwiftUI inside it), keeping the existing
@available(iOS 13.0, *) runtime check nested as needed, and provide an
alternative (or leave blank) for toolchains that cannot import SwiftUI so the
file compiles everywhere.
| /// Test skipAPI behavior when no cache is available - should attempt network | ||
| func testSkipAPIWithNoCacheAvailable() throws { | ||
| Flagsmith.shared.cacheConfig.useCache = true | ||
| Flagsmith.shared.cacheConfig.skipAPI = true | ||
|
|
||
| // Clear cache to ensure no data is available | ||
| testCache.removeAllCachedResponses() | ||
|
|
||
| let expectation = expectation(description: "Should attempt network request") | ||
|
|
||
| Flagsmith.shared.getFeatureFlags { result in | ||
| switch result { | ||
| case .success(_): | ||
| // Success means network request was attempted and worked (unexpected with test key, but possible) | ||
| print("DEBUG: Request succeeded - network request was made (expected behavior with no cache)") | ||
| expectation.fulfill() | ||
| case .failure(let error): | ||
| // Failure means network request was attempted but failed (expected with invalid test key) | ||
| // This is still the correct behavior - the important thing is that a network request was attempted | ||
| print("DEBUG: Request failed as expected: \(error)") | ||
|
|
||
| // Verify that the error indicates a network attempt was made, not a cache-only failure | ||
| let errorDescription = error.localizedDescription | ||
| if errorDescription.contains("JSON") || errorDescription.contains("decoding") { | ||
| // This suggests a network request was made but returned invalid data | ||
| print("DEBUG: ✅ Network request was attempted (got response data)") | ||
| } else if errorDescription.contains("apiKey") { | ||
| // API key error means network request logic was executed | ||
| print("DEBUG: ✅ Network request was attempted (API key validation)") | ||
| } else { | ||
| print("DEBUG: Error type: \(errorDescription)") | ||
| } | ||
|
|
||
| expectation.fulfill() | ||
| } | ||
| } | ||
|
|
||
| wait(for: [expectation], timeout: 10.0) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Misaligned semantics: skipAPI=true must NOT hit network on cache miss
This test asserts the opposite of the intended behavior (PR objective: do not make a network request when skipAPI=true and no cache). It also performs a live network call, which is flaky in CI.
- Update the test to assert no network requests occur and that the SDK returns default flags or a cache-miss error without attempting I/O.
- Use a URLProtocol spy to prove zero requests rather than inferring from the result.
Example helper to add (top-level in test target):
final class NetworkSpyProtocol: URLProtocol {
static var requests = AtomicInt() // or DispatchQueue-protected Int
override class func canInit(with request: URLRequest) -> Bool { true }
override class func canonicalRequest(for request: URLRequest) -> URLRequest { request }
override func startLoading() { NetworkSpyProtocol.requests.increment(); client?.urlProtocol(self, didFailWithError: URLError(.cancelled)) }
override func stopLoading() {}
}Then in this test:
URLProtocol.registerClass(NetworkSpyProtocol.self)
defer { URLProtocol.unregisterClass(NetworkSpyProtocol.self) }
Flagsmith.shared.cacheConfig.useCache = true
Flagsmith.shared.cacheConfig.skipAPI = true
testCache.removeAllCachedResponses()
let exp = expectation(description: "skipAPI should not attempt network")
Flagsmith.shared.getFeatureFlags { _ in exp.fulfill() }
wait(for: [exp], timeout: 2)
XCTAssertEqual(NetworkSpyProtocol.requests.value, 0, "No network requests expected when skipAPI=true and no cache")🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo root: $(pwd)"
echo "--- Finding CacheTests.swift ---"
rg -n --hidden --glob '!**/node_modules/**' 'CacheTests.swift' || true
echo "--- Displaying FlagsmithClient/Tests/CacheTests.swift (if present) ---"
if [ -f "FlagsmithClient/Tests/CacheTests.swift" ]; then
sed -n '1,400p' FlagsmithClient/Tests/CacheTests.swift || true
else
file=$(rg -n --hidden --glob '!**/node_modules/**' 'CacheTests.swift' --files | head -n 1 || true)
if [ -n "$file" ]; then
echo "Found file: $file"
sed -n '1,400p' "$file" || true
else
echo "CacheTests.swift not found in repository."
fi
fi
echo "--- Searching for related symbols ---"
rg -n --hidden --glob '!**/node_modules/**' 'skipAPI|cacheConfig|getFeatureFlags|testSkipAPIWithNoCacheAvailable' -S || true
echo "--- Searching for URLProtocol/URLSession usage in repo ---"
rg -n --hidden --glob '!**/node_modules/**' 'URLProtocol|URLSession' -S || trueLength of output: 17762
Fix test: skipAPI=true must NOT make network requests — assert zero requests and avoid live I/O
The test currently performs a live network call and asserts the opposite of the PR intent. Replace it to prove no network I/O occurs and to assert the SDK's documented cache-only behavior.
- Location: FlagsmithClient/Tests/CacheTests.swift — testSkipAPIWithNoCacheAvailable
- Replace the test body to:
- register a URLProtocol spy that counts requests,
- clear the cache,
- call Flagsmith.shared.getFeatureFlags,
- wait short (≈2s) and assert the spy saw 0 requests,
- assert the callback returns the SDK's expected cache-only result (explicitly check the SDK's cache-miss error or default/empty flags), not by inspecting localizedDescription.
Example helper (add top-level in test target):
final class NetworkSpyProtocol: URLProtocol {
private static let lock = DispatchQueue(label: "NetworkSpyProtocol.lock")
private static var _requests = 0
static var requests: Int { lock.sync { _requests } }
override class func canInit(with request: URLRequest) -> Bool { true }
override class func canonicalRequest(for request: URLRequest) -> URLRequest { request }
override func startLoading() {
Self.lock.sync { Self._requests += 1 }
client?.urlProtocol(self, didFailWithError: URLError(.cancelled))
}
override func stopLoading() {}
}Example test pattern:
URLProtocol.registerClass(NetworkSpyProtocol.self)
defer { URLProtocol.unregisterClass(NetworkSpyProtocol.self) }
Flagsmith.shared.cacheConfig.useCache = true
Flagsmith.shared.cacheConfig.skipAPI = true
testCache.removeAllCachedResponses()
let exp = expectation(description: "skipAPI should not attempt network")
Flagsmith.shared.getFeatureFlags { result in
// Assert the SDK's documented cache-only behavior (e.g., specific cache-miss error or default flags)
exp.fulfill()
}
wait(for: [exp], timeout: 2.0)
XCTAssertEqual(NetworkSpyProtocol.requests, 0, "No network requests expected when skipAPI=true and no cache")Do not rely on inspecting error.localizedDescription to infer whether a network request occurred.
🧰 Tools
🪛 GitHub Check: swift-lint
[warning] 186-186:
Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)
🤖 Prompt for AI Agents
In FlagsmithClient/Tests/CacheTests.swift around lines 174–212 the test
testSkipAPIWithNoCacheAvailable currently performs live network I/O and inspects
error.localizedDescription; change it to assert zero network requests and verify
the SDK's documented cache-only response: register a URLProtocol spy that counts
requests (register before the call and unregister in defer), set
Flagsmith.shared.cacheConfig.useCache = true and skipAPI = true, clear the test
cache, call getFeatureFlags and wait ~2s for the callback, then assert
NetworkSpyProtocol.requests == 0 and that the callback returns the explicit
cache-miss result or default/empty flags the SDK documents (do not inspect
localizedDescription); ensure the spy increments on startLoading and returns a
cancelled error so any accidental network attempt is visible.
| func testManualCacheFallback() throws { | ||
| let apiManager = APIManager() | ||
| apiManager.apiKey = "test-key" | ||
|
|
||
| Flagsmith.shared.cacheConfig.useCache = true | ||
|
|
||
| let mockData = """ | ||
| [{"id": 1, "feature": {"name": "test"}, "enabled": true, "feature_state_value": null}] | ||
| """.data(using: .utf8)! | ||
|
|
||
| // Test the ensureResponseIsCached method indirectly | ||
| // by simulating a successful decode operation | ||
|
|
||
| let expectation = expectation(description: "Manual caching should work") | ||
|
|
||
| // This tests our new caching logic in the success path | ||
| do { | ||
| let flags = try decoder.decode([Flag].self, from: mockData) | ||
| XCTAssertEqual(flags.count, 1, "Should decode one flag") | ||
| expectation.fulfill() | ||
| } catch { | ||
| XCTFail("Should be able to decode mock data: \(error)") | ||
| expectation.fulfill() | ||
| } | ||
|
|
||
| wait(for: [expectation], timeout: 1.0) | ||
| } |
There was a problem hiding this comment.
This test doesn’t validate the manual caching fallback
It decodes JSON but never exercises the new ensureResponseIsCached path. Either rename the test or actually trigger the fallback by invoking the API path that calls it (or call the method directly via @testable if accessible).
🤖 Prompt for AI Agents
In FlagsmithClient/Tests/CacheTests.swift around lines 552–578, the test decodes
JSON but never calls the new ensureResponseIsCached path so it doesn't validate
the manual caching fallback; either rename the test to reflect it only verifies
decoding, or update it to actually exercise the caching code by invoking the API
path that triggers ensureResponseIsCached or calling ensureResponseIsCached
directly (use @testable to access internal methods), pass the mockData and
expected cache key, then assert the cache contains the saved response and adjust
the expectation to wait for that cache verification.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift (4)
25-39: Restore global Flagsmith.shared and clear cache to avoid cross-test bleed (repeat)Tests mutate the singleton and URLCache without full restore; this causes order‑dependent flakiness.
// Configure exactly as the customer reported + // Snapshot and auto‑restore global client state to avoid cross‑test bleed + let prior = ( + apiKey: Flagsmith.shared.apiKey, + baseURL: Flagsmith.shared.baseURL, + enableRealtimeUpdates: Flagsmith.shared.enableRealtimeUpdates, + useCache: Flagsmith.shared.cacheConfig.useCache, + cacheTTL: Flagsmith.shared.cacheConfig.cacheTTL, + skipAPI: Flagsmith.shared.cacheConfig.skipAPI, + cache: Flagsmith.shared.cacheConfig.cache + ) + defer { + Flagsmith.shared.apiKey = prior.apiKey + Flagsmith.shared.baseURL = prior.baseURL + Flagsmith.shared.enableRealtimeUpdates = prior.enableRealtimeUpdates + Flagsmith.shared.cacheConfig.useCache = prior.useCache + Flagsmith.shared.cacheConfig.cacheTTL = prior.cacheTTL + Flagsmith.shared.cacheConfig.skipAPI = prior.skipAPI + Flagsmith.shared.cacheConfig.cache = prior.cache + Flagsmith.shared.cacheConfig.cache.removeAllCachedResponses() + } @@ - // Cleanup - Flagsmith.shared.cacheConfig.skipAPI = false - Flagsmith.shared.cacheConfig.useCache = false - Flagsmith.shared.apiKey = nilAlso applies to: 104-108
115-131: Apply the same snapshot/auto-restore pattern herePrevent leakage of apiKey/baseURL/cache settings into other tests.
// Test customer configuration with simulated successful cache + // Snapshot and auto‑restore + let prior = ( + apiKey: Flagsmith.shared.apiKey, + baseURL: Flagsmith.shared.baseURL, + enableRealtimeUpdates: Flagsmith.shared.enableRealtimeUpdates, + useCache: Flagsmith.shared.cacheConfig.useCache, + cacheTTL: Flagsmith.shared.cacheConfig.cacheTTL, + skipAPI: Flagsmith.shared.cacheConfig.skipAPI, + cache: Flagsmith.shared.cacheConfig.cache + ) + defer { + Flagsmith.shared.apiKey = prior.apiKey + Flagsmith.shared.baseURL = prior.baseURL + Flagsmith.shared.enableRealtimeUpdates = prior.enableRealtimeUpdates + Flagsmith.shared.cacheConfig.useCache = prior.useCache + Flagsmith.shared.cacheConfig.cacheTTL = prior.cacheTTL + Flagsmith.shared.cacheConfig.skipAPI = prior.skipAPI + Flagsmith.shared.cacheConfig.cache = prior.cache + Flagsmith.shared.cacheConfig.cache.removeAllCachedResponses() + } @@ - // Cleanup - Flagsmith.shared.cacheConfig.skipAPI = false - Flagsmith.shared.cacheConfig.useCache = false - Flagsmith.shared.apiKey = nilAlso applies to: 212-216
287-317: Edge-cases test lacks assertionsAdd concrete expectations (e.g., per-identity cache isolation: prepopulate for user_a, assert user_b/user_c fail with skipAPI=true, and no-identity path remains independent). Also apply the snapshot/restore pattern here.
Also applies to: 321-325
335-347: Apply snapshot/restore in the final test as wellSame cross‑test bleed risk. Remove ad‑hoc cleanup.
// Exact customer setup - ensure API key is set for proper cache behavior + // Snapshot and auto‑restore + let prior = ( + apiKey: Flagsmith.shared.apiKey, + baseURL: Flagsmith.shared.baseURL, + enableRealtimeUpdates: Flagsmith.shared.enableRealtimeUpdates, + useCache: Flagsmith.shared.cacheConfig.useCache, + cacheTTL: Flagsmith.shared.cacheConfig.cacheTTL, + skipAPI: Flagsmith.shared.cacheConfig.skipAPI, + cache: Flagsmith.shared.cacheConfig.cache + ) + defer { + Flagsmith.shared.apiKey = prior.apiKey + Flagsmith.shared.baseURL = prior.baseURL + Flagsmith.shared.enableRealtimeUpdates = prior.enableRealtimeUpdates + Flagsmith.shared.cacheConfig.useCache = prior.useCache + Flagsmith.shared.cacheConfig.cacheTTL = prior.cacheTTL + Flagsmith.shared.cacheConfig.skipAPI = prior.skipAPI + Flagsmith.shared.cacheConfig.cache = prior.cache + Flagsmith.shared.cacheConfig.cache.removeAllCachedResponses() + } @@ - // Cleanup - Flagsmith.shared.cacheConfig.skipAPI = false - Flagsmith.shared.cacheConfig.useCache = false - Flagsmith.shared.apiKey = nilAlso applies to: 412-416
🧹 Nitpick comments (8)
FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift (8)
15-19: Skip, don’t fail, when FLAGSMITH_TEST_API_KEY is missingGate live-key tests with XCTSkipUnless to avoid red CI when the env var isn’t set.
- guard TestConfig.hasRealApiKey else { - XCTFail("This customer use case test requires FLAGSMITH_TEST_API_KEY environment variable to be set") - return - } + try XCTSkipUnless(TestConfig.hasRealApiKey, "Requires FLAGSMITH_TEST_API_KEY for live API verification")
44-45: Drop fallback identity after skip gatingOnce the test is skipped without a real key, this branch is unnecessary.
- let testIdentity = TestConfig.hasRealApiKey ? TestConfig.testIdentity : "customer-test-identity" + let testIdentity = TestConfig.testIdentity
54-56: Avoid arbitrary sleep; call the second request immediatelyThe caching fix writes deterministically; the delay just adds flakiness.
- // Wait a moment to ensure cache is stored properly - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { - // Second call - customer expects this to use cache, not HTTP - Flagsmith.shared.getFeatureFlags(forIdentity: testIdentity) { secondResult in + // Second call - should use cache, not HTTP + Flagsmith.shared.getFeatureFlags(forIdentity: testIdentity) { secondResult in @@ - expectation.fulfill() - } - } + expectation.fulfill() + }Also applies to: 70-72
51-53: Strengthen assertions to avoid false positives on empty arraysIf both calls return zero flags, equality passes while not proving cache use.
case .success(let flags): + XCTAssertFalse(flags.isEmpty, "First call returned no flags; cannot validate caching") @@ - case .success(let cachedFlags): + case .success(let cachedFlags): + XCTAssertFalse(cachedFlags.isEmpty, "Cached response is empty; expected data in cache")Also applies to: 59-65
177-179: Use CachedURLResponse.response(withExpirationDuration:) for consistent TTL semanticsAligns with production code that rewrites caching headers and userInfo.
- let cachedResponse = CachedURLResponse(response: httpResponse, data: mockIdentityResponse) + let cachedResponse = CachedURLResponse(response: httpResponse, data: mockIdentityResponse) + .response(withExpirationDuration: 180)
135-139: Build mock URL via URLComponents to safely encode identityPrevents breakage if identity contains reserved characters.
Swift snippet:
var comps = URLComponents(string: "https://edge.api.flagsmith.com/api/v1/identities/")! comps.queryItems = [URLQueryItem(name: "identifier", value: testIdentity)] let mockURL = comps.url!
191-193: Remove duplicate comment lineSmall cleanup.
- // Test subsequent calls also work - // Test subsequent calls also work + // Test subsequent calls also work
350-357: Remove RequestCounter or wire it to a URLProtocol stubAs-is it’s dead code and not used in assertions. Prefer a custom URLProtocol to count real HTTP requests, or delete.
- class RequestCounter { - static var count = 0 - static func increment() { count += 1 } - static func reset() { count = 0 } - } - - RequestCounter.reset() @@ - RequestCounter.increment() @@ - RequestCounter.increment() @@ - RequestCounter.increment()Also applies to: 362-362, 368-368, 374-374
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift(1 hunks)FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift(1 hunks)FlagsmithClient/Tests/TestConfig.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- FlagsmithClient/Tests/FlagsmithCacheIntegrationTests.swift
- FlagsmithClient/Tests/TestConfig.swift
🧰 Additional context used
🧬 Code graph analysis (1)
FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift (1)
FlagsmithClient/Classes/Internal/CachedURLResponse.swift (1)
response(14-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: macos-latest
| // Customer configuration | ||
| Flagsmith.shared.baseURL = URL(string: "https://edge.api.flagsmith.com/api/v1/")! | ||
| Flagsmith.shared.cacheConfig.useCache = true | ||
| Flagsmith.shared.cacheConfig.cacheTTL = 180 // Customer's 3-minute TTL | ||
| Flagsmith.shared.cacheConfig.skipAPI = true | ||
|
|
There was a problem hiding this comment.
Add assertions or prepopulate cache; current test validates nothing
This “session-long” test never asserts outcomes. Either prepopulate cache and assert all N calls succeed, or skip when not measurable.
Option A (skip without live key):
+ try XCTSkipUnless(TestConfig.hasRealApiKey, "Requires real key or prepopulated cache to be meaningful")Option B (preferred): prepopulate cache (as in the previous test) for sessionIdentity, then assert each call returns success.
Also applies to: 270-279
🤖 Prompt for AI Agents
In FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift around lines 224-229
(and also address similar gap at 270-279), the test enables cache but performs
no assertions; update the test to either skip when a live key isn't available or
(preferred) prepopulate the cache for the sessionIdentity exactly as the
previous test does, then perform the N cached calls and assert each call returns
the expected success/result; ensure setup populates the same key/identity and
TTL, and add assertions after each call (or a final assertion verifying all
calls succeeded).
|
|
||
| // All three will likely fail with test credentials, demonstrating the issue: | ||
| // Cache is never populated because requests fail, so skipAPI falls back to HTTP | ||
| switch (firstResult, secondResult, thirdResult) { | ||
| case (.failure(_), .failure(_), .failure(_)): | ||
| // Issue reproduced: all requests failed, proving HTTP calls were made | ||
| // Root cause: skipAPI=true with no cache falls back to HTTP | ||
|
|
||
| // With real API keys, all failures might indicate API/environment issues rather than cache problems | ||
| // Note: Not failing the test as this might be due to API key/environment mismatch | ||
| break | ||
|
|
||
| case (.success(_), .success(_), .success(_)): | ||
| // All succeeded - cache working | ||
| break | ||
|
|
||
| default: | ||
| // Mixed results - partial cache behavior | ||
|
|
||
| // Mixed results with real API key suggest inconsistent cache behavior | ||
| if TestConfig.hasRealApiKey { | ||
| // Warning: Mixed results suggest cache inconsistency | ||
| // Not failing as some mixed results might be acceptable, but noting concern | ||
| } | ||
| } |
There was a problem hiding this comment.
Make this test assertive
Currently any outcome passes. Recommend:
- Without real key: assert three failures.
- With real key: assert first call may hit HTTP, subsequent calls succeed (cache hit). If you adopt URLProtocol, assert zero outbound HTTP after the first success.
Swift sketch:
if !TestConfig.hasRealApiKey {
XCTAssertTrue([firstResult, secondResult, thirdResult].allSatisfy { if case .failure = $0 { return true } else { return false } },
"Without a real key, all three calls should fail (no cache).")
} else {
guard case .success = firstResult else { XCTFail("First call should succeed with real key"); return }
XCTAssertTrue([secondResult, thirdResult].allSatisfy { if case .success = $0 { return true } else { return false } },
"Subsequent calls should succeed via cache with skipAPI=true.")
}🤖 Prompt for AI Agents
In FlagsmithClient/Tests/CustomerCacheUseCaseTests.swift around lines 379-403,
the test currently accepts any outcome; change it to be assertive: when
TestConfig.hasRealApiKey is false assert all three results are failures, and
when TestConfig.hasRealApiKey is true assert the firstResult is .success and
that secondResult and thirdResult are .success (optionally use URLProtocol to
verify zero outbound HTTP after the first success); update XCTAssert/XCTFail
calls accordingly so the test fails on unexpected behavior.
1 participant
Cache Functionality Fixes and Comprehensive Test Suite
Overview
This PR addresses critical cache functionality issues in the Flagsmith iOS SDK and adds a comprehensive test suite to ensure cache behavior works correctly across all scenarios.
Problem Statement
The SDK's caching mechanism was not functioning as expected when
skipAPI=truewas configured. Specifically:skipAPI=truewas setKey Changes
1. APIManager Cache Fixes (
FlagsmithClient/Classes/Internal/APIManager.swift)ensureResponseIsCached()method to manually store successful responses when URLSession's automatic caching failsnilinstead of proposed response)2. SSE Manager Crash Fix (
FlagsmithClient/Classes/Internal/SSEManager.swift)3. Comprehensive Test Suite
Added over 1,600 lines of thorough cache testing:
CacheTests.swift(737 lines)FlagsmithCacheIntegrationTests.swift(521 lines)CustomerCacheUseCaseTests.swift(398 lines)CachedURLResponseTests.swift(183 lines)APIManagerTests.swift(197 lines)4. Testing Infrastructure
TestConfig.swift: Centralized test configuration with environment variable support for API keysREADME_Testing.md: Documentation for running cache tests5. Example App Improvements (
Example/FlagsmithClient/SwiftUIView.swift)Testing
All tests have been updated to:
FLAGSMITH_TEST_API_KEYenvironment variable for integration testsImpact
skipAPI=trueBreaking Changes
None - all changes are backward compatible.
Verification
Run tests with:
The comprehensive test suite ensures cache functionality works correctly across all supported scenarios.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
Documentation