wip: fix holdout config tests

Author: muzahidul-opti

Sources/Data Model/HoldoutConfig.swift

@@ -1,10 +1,10 @@
 //
-// Copyright 2022, Optimizely, Inc. and contributors 
-// 
-// Licensed under the Apache License, Version 2.0 (the "License");  
+// Copyright 2022, Optimizely, Inc. and contributors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at   
-// 
+// You may obtain a copy of the License at
+//
 //    http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
@@ -24,11 +24,12 @@ struct HoldoutConfig {
     }
     private(set) var holdoutIdMap: [String: Holdout] = [:]
     private(set) var global: [Holdout] = []
-    private(set) var others: [Holdout] = []
     private(set) var includedHoldouts: [String: [Holdout]] = [:]
     private(set) var excludedHoldouts: [String: [Holdout]] = [:]
     private(set) var flagHoldoutsMap: [String: [Holdout]] = [:]
 
+    let logger = OPTLoggerFactory.getLogger()
+    
     init(allholdouts: [Holdout] = []) {
         self.allHoldouts = allholdouts
         updateHoldoutProperties()
@@ -42,15 +43,19 @@ struct HoldoutConfig {
         }()
         flagHoldoutsMap = [:]
         global = []
-        others = []
+        
         includedHoldouts = [:]
         excludedHoldouts = [:]
 
         for holdout in allHoldouts {
             switch (holdout.includedFlags.isEmpty, holdout.excludedFlags.isEmpty) {
                 case (true, true):
                     global.append(holdout)
-                case (false, _):
+                    
+                case (false, false):
+                    logger.e(.holdoutToFlagMappingError)
+                    
+                case (false, true):
                     holdout.includedFlags.forEach { flagId in
                         if var existing = includedHoldouts[flagId] {
                             existing.append(holdout)
@@ -59,8 +64,10 @@ struct HoldoutConfig {
                             includedHoldouts[flagId] = [holdout]
                         }
                     }
-                case (_, false):
-                    others.append(holdout)
+                    
+                case (true, false):
+                    global.append(holdout)
+                    
                     holdout.excludedFlags.forEach { flagId in
                         if var existing = excludedHoldouts[flagId] {
                             existing.append(holdout)
@@ -76,19 +83,29 @@ struct HoldoutConfig {
     mutating func getHoldoutForFlag(id: String) -> [Holdout] {
         guard !allHoldouts.isEmpty else { return [] }
 
+        // Check cache and return if persist holdouts
         if let holdouts = flagHoldoutsMap[id] {
             return holdouts
         }
 
-        if let included = includedHoldouts[id], !included.isEmpty {
-            flagHoldoutsMap[id] = global + included
-        } else {
-            let excluded = excludedHoldouts[id] ?? []
-            let filteredHoldouts = others.filter { holdout in
+        var activeHoldouts: [Holdout] = []
+        
+        let excluded = excludedHoldouts[id] ?? []
+        
+        if !excluded.isEmpty {
+            activeHoldouts = global.filter { holdout in
                 return !excluded.contains(holdout)
             }
-            flagHoldoutsMap[id] = global + filteredHoldouts
+        } else {
+            activeHoldouts = global
         }
+        
+        let includedHoldouts = includedHoldouts[id] ?? []
+        
+        activeHoldouts += includedHoldouts
+        
+        flagHoldoutsMap[id] = activeHoldouts
+        
         return flagHoldoutsMap[id] ?? []
     }
 

Sources/Utils/LogMessage.swift

@@ -68,6 +68,7 @@ enum LogMessage {
     case failedToAssignValue
     case valueForKeyNotFound(_ key: String)
     case lowPeriodicDownloadInterval
+    case holdoutToFlagMappingError
 }
 
 extension LogMessage: CustomStringConvertible {
@@ -130,6 +131,7 @@ extension LogMessage: CustomStringConvertible {
         case .failedToAssignValue:                                              message = "Value for path could not be assigned to provided type."
         case .valueForKeyNotFound(let key):                                     message = "Value for JSON key (\(key)) not found."
         case .lowPeriodicDownloadInterval:                                      message = "Polling intervals below 30 seconds are not recommended."
+        case .holdoutToFlagMappingError:                                        message = "A holdout cannot have include and exclude flags at the same time."
         }
 
         return message

Tests/OptimizelyTests-DataModel/HoldoutConfigTests.swift

@@ -38,8 +38,7 @@ class HoldoutConfigTests: XCTestCase {
         XCTAssertEqual(holdoutConfig.holdoutIdMap["3333"]?.excludedFlags, ["8888", "9999"])
 
 
-        XCTAssertEqual(holdoutConfig.global, [holdout0])
-        XCTAssertEqual(holdoutConfig.others, [holdout2])
+        XCTAssertEqual(holdoutConfig.global, [holdout0, holdout2])
 
         XCTAssertEqual(holdoutConfig.includedHoldouts["4444"], [holdout1])
         XCTAssertEqual(holdoutConfig.excludedHoldouts["8888"], [holdout2])
@@ -99,10 +98,12 @@ class HoldoutConfigTests: XCTestCase {
         var holdoutConfig = HoldoutConfig(allholdouts: allHoldouts)
 
         // Should maintain order. Global first then lcoal
-        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag1), [holdout3, holdout4, holdout1])
+        
+        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag1), [holdout2, holdout3, holdout4, holdout1])
         XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag2), [holdout3, holdout4])
-        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag3), [holdout3, holdout4, holdout2])
-        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag4), [holdout3, holdout4, holdout2])
+        
+        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag3), [holdout2, holdout3, holdout4])
+        XCTAssertEqual(holdoutConfig.getHoldoutForFlag(id: flag4), [holdout2, holdout3, holdout4])
 
     }