cleanup: add unit test cases

Author: muzahidul-opti

Sources/Data Model/HoldoutConfig.swift

@@ -19,31 +19,34 @@ import Foundation
 struct HoldoutConfig {
     var allHoldouts: [Holdout] {
         didSet {
-            updateHoldoutProperties()
+            updateHoldoutMapping()
         }
     }
-    private(set) var holdoutIdMap: [String: Holdout] = [:]
     private(set) var global: [Holdout] = []
+    private(set) var holdoutIdMap: [String: Holdout] = [:]
+    private(set) var flagHoldoutsMap: [String: [Holdout]] = [:]
     private(set) var includedHoldouts: [String: [Holdout]] = [:]
     private(set) var excludedHoldouts: [String: [Holdout]] = [:]
-    private(set) var flagHoldoutsMap: [String: [Holdout]] = [:]
 
-    let logger = OPTLoggerFactory.getLogger()
+    var cacheWriteCount = 0 // for testing purpose only
+    
+    var logger: OPTLogger = OPTLoggerFactory.getLogger()
 
     init(allholdouts: [Holdout] = []) {
         self.allHoldouts = allholdouts
-        updateHoldoutProperties()
+        updateHoldoutMapping()
     }
 
-    mutating func updateHoldoutProperties() {
+    /// Updates internal mappings of holdouts including the id map, global list, and per-flag inclusion/exclusion maps.
+    mutating func updateHoldoutMapping() {
         holdoutIdMap = {
             var map = [String: Holdout]()
             allHoldouts.forEach { map[$0.id] = $0 }
             return map
         }()
+        
         flagHoldoutsMap = [:]
         global = []
-        
         includedHoldouts = [:]
         excludedHoldouts = [:]
 
@@ -80,10 +83,14 @@ struct HoldoutConfig {
         }
     }
 
+    /// Returns the applicable holdouts for the given flag ID by combining global holdouts (excluding any specified) and included holdouts, in that order.
+    /// Caches the result for future calls.
+    /// - Parameter id: The flag identifier.
+    /// - Returns: An array of `Holdout` objects relevant to the given flag.
     mutating func getHoldoutForFlag(id: String) -> [Holdout] {
         guard !allHoldouts.isEmpty else { return [] }
 
-        // Check cache and return if persist holdouts
+        // Check cache and return persistent holdouts
         if let holdouts = flagHoldoutsMap[id] {
             return holdouts
         }
@@ -105,10 +112,12 @@ struct HoldoutConfig {
         activeHoldouts += includedHoldouts
 
         flagHoldoutsMap[id] = activeHoldouts
+        cacheWriteCount += 1
 
         return flagHoldoutsMap[id] ?? []
     }
 
+    /// Get a Holdout object for an Id.
     func getHoldout(id: String) -> Holdout? {
         return holdoutIdMap[id]
     }

Sources/Data Model/ProjectConfig.swift

@@ -162,33 +162,7 @@ class ProjectConfig {
     func getHoldoutForFlag(id: String) -> [Holdout] {
         return holdoutConfig.getHoldoutForFlag(id: id)
     }
-    
-//    private func updateHoldoutsMapForFlag(id: String) {
-//        if let included = inlcudedHoldoutsMap[id] {
-//            return globalHoldouts + included
-//        }
-//        
-////        for holdout in project.holdouts {
-////            switch (holdout.includedFlags.isEmpty, holdout.excludedFlags.isEmpty) {
-////                case (true, true):
-////                    // Global holdout
-////                    global.append(holdout.id)
-////                    
-////                case (false, _):
-////                    if holdout.includedFlags.contains(id) {
-////                        others.append(holdout.id)
-////                    }
-////                    
-////                case (_, false):
-////                    if !holdout.excludedFlags.contains(id) {
-////                        others.append(holdout.id)
-////                    }
-////            }
-////        }
-////        
-//        flagHoldoutsMap[id] = globalHoldouts + others
-//    }
-    
+        
     func getAllRulesForFlag(_ flag: FeatureFlag) -> [Experiment] {
         var rules = flag.experimentIds.compactMap { experimentIdMap[$0] }
         let rollout = self.rolloutIdMap[flag.rolloutId]

Tests/OptimizelyTests-DataModel/HoldoutConfigTests.swift

@@ -17,15 +17,21 @@
 import XCTest
 
 class HoldoutConfigTests: XCTestCase {
+    func testEmptyHoldouts_shouldHaveEmptyMaps() {
+        let config = HoldoutConfig(allholdouts: [])
+        
+        XCTAssertTrue(config.holdoutIdMap.isEmpty)
+        XCTAssertTrue(config.global.isEmpty)
+        XCTAssertTrue(config.includedHoldouts.isEmpty)
+        XCTAssertTrue(config.excludedHoldouts.isEmpty)
+    }
 
     func testHoldoutMap() {
         let holdout0: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
         let holdout1: Holdout = try! OTUtils.model(from: HoldoutTests.sampleDataWithIncludedFlags)
         let holdout2: Holdout = try! OTUtils.model(from: HoldoutTests.sampleDataWithExcludedFlags)
 
         let allHoldouts =  [holdout0, holdout1, holdout2]
-        
-        
         let holdoutConfig = HoldoutConfig(allholdouts: allHoldouts)
 
         XCTAssertEqual(holdoutConfig.holdoutIdMap["11111"]?.includedFlags, [])
@@ -37,7 +43,6 @@ class HoldoutConfigTests: XCTestCase {
         XCTAssertEqual(holdoutConfig.holdoutIdMap["3333"]?.includedFlags, [])
         XCTAssertEqual(holdoutConfig.holdoutIdMap["3333"]?.excludedFlags, ["8888", "9999"])
 
-        
         XCTAssertEqual(holdoutConfig.global, [holdout0, holdout2])
 
         XCTAssertEqual(holdoutConfig.includedHoldouts["4444"], [holdout1])
@@ -48,16 +53,12 @@ class HoldoutConfigTests: XCTestCase {
     func testGetHoldoutById() {
         var holdout0: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
         holdout0.id = "00000"
-        
         var holdout1: Holdout = try! OTUtils.model(from: HoldoutTests.sampleDataWithIncludedFlags)
         holdout1.id = "11111"
-        
         var holdout2: Holdout = try! OTUtils.model(from: HoldoutTests.sampleDataWithExcludedFlags)
         holdout2.id = "22222"
 
         let allHoldouts =  [holdout0, holdout1, holdout2]
-        
-        
         let holdoutConfig = HoldoutConfig(allholdouts: allHoldouts)
 
         XCTAssertEqual(holdoutConfig.getHoldout(id: "00000"), holdout0)
@@ -66,47 +67,118 @@ class HoldoutConfigTests: XCTestCase {
 
     }
 
-    func testGetHoldoutForFlag() {
+    func testHoldoutOrdering_globalThenIncluded() {
+        var global1: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        global1.id = "g1"
+        
+        var global2: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        global2.id = "g2"
 
+        var included: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        included.id = "i1"
+        included.includedFlags = ["f"]
+        
+        var config = HoldoutConfig(allholdouts: [included, global1, global2])
+        
+        let result = config.getHoldoutForFlag(id: "f").map(\.id)
+        XCTAssertEqual(result, ["g1", "g2", "i1"])
+    }
+    
+    func testHoldoutOrdering_with_Both_IncludedAndExcludedFlags() {
         let flag1 = "11111"
         let flag2 = "22222"
         let flag3 = "33333"
         let flag4 = "44444"
 
-        var holdout1: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
-        holdout1.id = "11"
-        holdout1.includedFlags = [flag1]
-        holdout1.excludedFlags = []
+        var inc: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        inc.id = "i1"
+        inc.includedFlags = [flag1]
 
-        var holdout2: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
-        holdout2.id = "22"
-        holdout2.includedFlags = []
-        holdout2.excludedFlags = [flag2]
+        var exc: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        exc.id = "e1"
+        exc.excludedFlags = [flag2]
 
-        var holdout3: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
-        holdout3.id = "33"
-        holdout3.includedFlags = []
-        holdout3.excludedFlags = []
+        var gh1: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        gh1.id = "gh1"
+        gh1.includedFlags = []
+        gh1.excludedFlags = []
 
-        var holdout4: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
-        holdout4.id = "44"
-        holdout4.includedFlags = []
-        holdout4.excludedFlags = []
+        var gh2: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        gh2.id = "gh2"
+        gh2.includedFlags = []
+        gh2.excludedFlags = []
 
 
-        let allHoldouts =  [holdout1, holdout2, holdout3, holdout4]
+        let allHoldouts =  [inc, exc, gh1, gh2]
         var holdoutConfig = HoldoutConfig(allholdouts: allHoldouts)
 
-        // Should maintain order. Global first then lcoal
+        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag1), [exc, gh1, gh2, inc])
+        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag2), [gh1, gh2])
 
-        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag1), [holdout2, holdout3, holdout4, holdout1])
-        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag2), [holdout3, holdout4])
+        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag3), [exc, gh1, gh2])
+        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag4), [exc, gh1, gh2])
 
-        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag3), [holdout2, holdout3, holdout4])
-        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag4), [holdout2, holdout3, holdout4])
-
     }
 
+    func testExcludedHoldout_shouldNotAppearInGlobalForFlag() {
+        var global: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        global.id = "global"
+        
+        var excluded: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        excluded.id = "excluded"
+        excluded.excludedFlags = ["f"]
+        
+        var config = HoldoutConfig(allholdouts: [global, excluded])
+        
+        let result = config.getHoldoutForFlag(id: "f").map(\.id)
+        XCTAssertEqual(result, ["global"]) // excluded should not appear
+    }
 
+    func testGetHoldoutForFlag_shouldUseCacheOnSecondCall() {
+        var holdout: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        holdout.id = "1"
+        holdout.excludedFlags = ["f1"]
+        
+        var config = HoldoutConfig(allholdouts: [holdout])
+        
+        _ = config.getHoldoutForFlag(id: "f1")
+        let writeCountAfterFirst = config.cacheWriteCount
+        
+        _ = config.getHoldoutForFlag(id: "f1")
+        let writeCountAfterSecond = config.cacheWriteCount
+        
+        XCTAssertEqual(writeCountAfterFirst, 1)
+        XCTAssertEqual(writeCountAfterSecond, 1, "Second call should not increase cache write count")
+    }
 
+    func testHoldoutWithBothIncludedAndExcludedFlags_shouldLogError() {
+        class ConfigMockLogger: OPTLogger {
+            static var logLevel: OptimizelyLogLevel = .info
+            var expectedLog: String = ""
+            var logFound = false
+            
+            required init() {}
+            
+            func log(level: OptimizelyLogLevel, message: String) {
+                if (message == expectedLog) {
+                    logFound = true
+                }
+            }
+        }
+        
+        var holdout: Holdout = try! OTUtils.model(from: HoldoutTests.sampleData)
+        holdout.id = "invalid"
+        holdout.includedFlags = ["f1"]
+        holdout.excludedFlags = ["f2"]
+        
+        let mockLogger = ConfigMockLogger()
+        mockLogger.expectedLog = LogMessage.holdoutToFlagMappingError.description
+        
+        var config = HoldoutConfig(allholdouts: [])
+        config.logger = mockLogger
+        
+        config.allHoldouts = [holdout]
+        
+        XCTAssertTrue(mockLogger.logFound)
+    }
 }

Tests/OptimizelyTests-DataModel/ProjectConfigTests.swift

@@ -98,6 +98,7 @@ class ProjectConfigTests: XCTestCase {
         var holdout2 = HoldoutTests.sampleData
         var holdout3 = HoldoutTests.sampleData
         var holdout4 = HoldoutTests.sampleData
+        
         holdout0["id"] = "3000" // Global holdout (no included or excluded flags)
         holdout1["id"] = "3001" // Global holdout (no included or excluded flags)
         holdout2["id"] = "3002" // Global holdout (no included or excluded flags)