[FSSDK-12368] Address PR review: simplify types, relocate tests to module-level files

- Remove NotRequired import and use Optional[list[str]] for HoldoutDict.includedRules
- Migrate test fixtures from includedFlags/excludedFlags to includedRules
- Move LocalHoldoutsTest into test_config.py (HoldoutConfigTest + LocalHoldoutConfigTest)
- Move LocalHoldoutsDecisionFlowTest into test_decision_service_holdout.py
- Delete test_local_holdouts.py (violated module-level test convention)
- Tighten weakened assertions to exact counts and content checks

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: FarhanAnjum-opti

optimizely/helpers/types.py

@@ -21,11 +21,6 @@
 else:
     from typing import TypedDict  # type: ignore
 
-if version_info < (3, 11):
-    from typing_extensions import NotRequired
-else:
-    from typing import NotRequired  # type: ignore
-
 
 # Intermediate types for type checking deserialized datafile json before actual class instantiation.
 # These aren't used for anything other than type signatures
@@ -135,4 +130,4 @@ class HoldoutDict(ExperimentDict):
     Extends ExperimentDict with holdout-specific properties.
     """
     holdoutStatus: HoldoutStatus
-    includedRules: NotRequired[Optional[list[str]]]
+    includedRules: Optional[list[str]]

tests/test_config.py

@@ -1384,9 +1384,8 @@ def setUp(self):
         # Create config with holdouts
         config_body_with_holdouts = self.config_dict_with_features.copy()
 
-        # Use correct feature flag IDs from the datafile
-        boolean_feature_id = '91111'  # boolean_single_variable_feature
-        multi_variate_feature_id = '91114'  # test_feature_in_experiment_and_rollout
+        # Rule IDs from the test config experiments
+        rule_id_1 = '111127'  # test_experiment
 
         config_body_with_holdouts['holdouts'] = [
             {
@@ -1396,8 +1395,6 @@ def setUp(self):
                 'variations': [],
                 'trafficAllocation': [],
                 'audienceIds': [],
-                'includedFlags': [],
-                'excludedFlags': [boolean_feature_id]
             },
             {
                 'id': 'holdout_2',
@@ -1406,8 +1403,7 @@ def setUp(self):
                 'variations': [],
                 'trafficAllocation': [],
                 'audienceIds': [],
-                'includedFlags': [multi_variate_feature_id],
-                'excludedFlags': []
+                'includedRules': [rule_id_1],
             },
             {
                 'id': 'holdout_3',
@@ -1416,8 +1412,7 @@ def setUp(self):
                 'variations': [],
                 'trafficAllocation': [],
                 'audienceIds': [],
-                'includedFlags': [boolean_feature_id],
-                'excludedFlags': []
+                'includedRules': [rule_id_1],
             }
         ]
 
@@ -1435,24 +1430,20 @@ def test_get_global_holdouts__returns_global_holdouts(self):
         """ Test that get_global_holdouts returns holdouts with includedRules=None. """
 
         holdouts = self.config_with_holdouts.get_global_holdouts()
-        self.assertGreaterEqual(len(holdouts), 1)
+        self.assertEqual(1, len(holdouts))
 
-        global_holdout = next((h for h in holdouts if h.key == 'global_holdout'), None)
-        self.assertIsNotNone(global_holdout)
+        global_holdout = holdouts[0]
         self.assertEqual('holdout_1', global_holdout.id)
+        self.assertEqual('global_holdout', global_holdout.key)
         self.assertTrue(global_holdout.is_global)
 
     def test_get_holdouts_for_rule__returns_local_holdouts(self):
         """ Test that get_holdouts_for_rule returns holdouts targeting specific rules. """
 
-        # Test with a rule that has a local holdout
-        # First find a rule ID from experiments
-        if self.config_with_holdouts.experiment_id_map:
-            rule_id = list(self.config_with_holdouts.experiment_id_map.keys())[0]
-            # Won't assert on specific holdouts since test data varies
-            holdouts = self.config_with_holdouts.get_holdouts_for_rule(rule_id)
-            # Just verify it returns a list
-            self.assertIsInstance(holdouts, list)
+        holdouts = self.config_with_holdouts.get_holdouts_for_rule('111127')
+        self.assertEqual(1, len(holdouts))
+        self.assertEqual('specific_holdout', holdouts[0].key)
+        self.assertFalse(holdouts[0].is_global)
 
     def test_get_global_holdouts__caches_results(self):
         """ Test that get_global_holdouts returns consistent results. """
@@ -1466,10 +1457,7 @@ def test_get_global_holdouts__caches_results(self):
     def test_get_holdouts_for_rule__empty_for_non_targeted_rules(self):
         """ Test that get_holdouts_for_rule returns empty list for non-targeted rules. """
 
-        # Use a made-up rule ID that definitely doesn't exist
         holdouts = self.config_with_holdouts.get_holdouts_for_rule('definitely_non_existent_rule_id_12345')
-
-        # Should return empty list
         self.assertEqual(0, len(holdouts))
 
     def test_get_holdout__returns_holdout_for_valid_id(self):
@@ -1519,12 +1507,16 @@ def test_holdout_initialization__categorizes_holdouts_properly(self):
 
         self.assertIn('holdout_1', self.config_with_holdouts.holdout_id_map)
         self.assertIn('holdout_2', self.config_with_holdouts.holdout_id_map)
-        # Check if a holdout with ID 'holdout_1' exists in global_holdouts
+
+        # holdout_1 is global (no includedRules)
         holdout_ids_in_global = [h.id for h in self.config_with_holdouts.global_holdouts]
         self.assertIn('holdout_1', holdout_ids_in_global)
+        self.assertNotIn('holdout_2', holdout_ids_in_global)
 
-        # Verify rule-level mapping exists
-        self.assertIsInstance(self.config_with_holdouts.rule_holdouts_map, dict)
+        # holdout_2 is local, mapped to rule '111127'
+        self.assertIn('111127', self.config_with_holdouts.rule_holdouts_map)
+        rule_holdout_ids = [h.id for h in self.config_with_holdouts.rule_holdouts_map['111127']]
+        self.assertIn('holdout_2', rule_holdout_ids)
 
     def test_holdout_initialization__only_processes_running_holdouts(self):
         """ Test that only running holdouts are processed during initialization. """
@@ -1536,6 +1528,215 @@ def test_holdout_initialization__only_processes_running_holdouts(self):
         holdout_ids_in_global = [h.id for h in self.config_with_holdouts.global_holdouts]
         self.assertNotIn('holdout_3', holdout_ids_in_global)
 
+    def test_holdout_entity__is_global_property(self):
+        """ Test Holdout.is_global property. """
+
+        # Global holdout (includedRules is None)
+        global_holdout = entities.Holdout(
+            id='1', key='global', status='Running',
+            variations=[], trafficAllocation=[], audienceIds=[],
+            includedRules=None
+        )
+        self.assertTrue(global_holdout.is_global)
+
+        # Local holdout with rules
+        local_holdout = entities.Holdout(
+            id='2', key='local', status='Running',
+            variations=[], trafficAllocation=[], audienceIds=[],
+            includedRules=['rule1', 'rule2']
+        )
+        self.assertFalse(local_holdout.is_global)
+
+        # Local holdout with empty array (not global)
+        empty_local_holdout = entities.Holdout(
+            id='3', key='empty_local', status='Running',
+            variations=[], trafficAllocation=[], audienceIds=[],
+            includedRules=[]
+        )
+        self.assertFalse(empty_local_holdout.is_global)
+
+    def test_holdout__none_vs_empty_array_distinction(self):
+        """ Test that None (global) and [] (empty local) are handled differently. """
+
+        global_holdouts = self.config_with_holdouts.get_global_holdouts()
+        self.assertEqual(1, len(global_holdouts))
+        self.assertIsNone(global_holdouts[0].includedRules)
+
+        # Empty local holdout should not appear in global list or rule maps
+        empty_holdout = entities.Holdout(
+            id='empty', key='empty', status='Running',
+            variations=[], trafficAllocation=[], audienceIds=[],
+            includedRules=[]
+        )
+        self.assertFalse(empty_holdout.is_global)
+
+
+class LocalHoldoutConfigTest(base.BaseTest):
+    """ Tests for local holdout (rule-level targeting) config. """
+
+    def setUp(self):
+        base.BaseTest.setUp(self)
+
+        config_dict = self.config_dict_with_features.copy()
+
+        rule_id_1 = '111127'  # test_experiment
+        rule_id_2 = '32222'   # group_exp_1
+
+        config_dict['holdouts'] = [
+            {
+                'id': 'global_holdout_1',
+                'key': 'global_holdout',
+                'status': 'Running',
+                'audienceIds': [],
+                'variations': [
+                    {'id': 'global_var_1', 'key': 'global_control', 'variables': []}
+                ],
+                'trafficAllocation': [
+                    {'entityId': 'global_var_1', 'endOfRange': 5000}
+                ]
+            },
+            {
+                'id': 'local_holdout_1',
+                'key': 'local_holdout_single',
+                'status': 'Running',
+                'includedRules': [rule_id_1],
+                'audienceIds': [],
+                'variations': [
+                    {'id': 'local_var_1', 'key': 'local_control', 'variables': []}
+                ],
+                'trafficAllocation': [
+                    {'entityId': 'local_var_1', 'endOfRange': 5000}
+                ]
+            },
+            {
+                'id': 'local_holdout_2',
+                'key': 'local_holdout_multi',
+                'status': 'Running',
+                'includedRules': [rule_id_1, rule_id_2],
+                'audienceIds': [],
+                'variations': [
+                    {'id': 'local_var_2', 'key': 'local_multi_control', 'variables': []}
+                ],
+                'trafficAllocation': [
+                    {'entityId': 'local_var_2', 'endOfRange': 5000}
+                ]
+            },
+            {
+                'id': 'local_holdout_empty',
+                'key': 'local_holdout_empty',
+                'status': 'Running',
+                'includedRules': [],
+                'audienceIds': [],
+                'variations': [
+                    {'id': 'local_var_empty', 'key': 'empty_control', 'variables': []}
+                ],
+                'trafficAllocation': [
+                    {'entityId': 'local_var_empty', 'endOfRange': 10000}
+                ]
+            },
+            {
+                'id': 'draft_holdout',
+                'key': 'draft_holdout',
+                'status': 'Draft',
+                'includedRules': [rule_id_1],
+                'audienceIds': [],
+                'variations': [
+                    {'id': 'draft_var', 'key': 'draft_control', 'variables': []}
+                ],
+                'trafficAllocation': [
+                    {'entityId': 'draft_var', 'endOfRange': 10000}
+                ]
+            },
+            {
+                'id': 'local_holdout_invalid',
+                'key': 'local_holdout_invalid_rule',
+                'status': 'Running',
+                'includedRules': ['non_existent_rule_id'],
+                'audienceIds': [],
+                'variations': [
+                    {'id': 'local_var_invalid', 'key': 'invalid_control', 'variables': []}
+                ],
+                'trafficAllocation': [
+                    {'entityId': 'local_var_invalid', 'endOfRange': 10000}
+                ]
+            }
+        ]
+
+        config_json = json.dumps(config_dict)
+        opt_obj = optimizely.Optimizely(config_json)
+        self.project_config = opt_obj.config_manager.get_config()
+
+    def test_global_holdouts_correctly_identified(self):
+        """ Test that global holdouts are correctly identified. """
+
+        global_holdouts = self.project_config.get_global_holdouts()
+        self.assertEqual(1, len(global_holdouts))
+        self.assertEqual('global_holdout', global_holdouts[0].key)
+        self.assertTrue(global_holdouts[0].is_global)
+
+    def test_rule_holdouts_map(self):
+        """ Test that local holdouts are correctly mapped to rules. """
+
+        # Rule 1 should have 2 local holdouts
+        holdouts_for_rule_1 = self.project_config.get_holdouts_for_rule('111127')
+        self.assertEqual(2, len(holdouts_for_rule_1))
+        holdout_keys_1 = {h.key for h in holdouts_for_rule_1}
+        self.assertIn('local_holdout_single', holdout_keys_1)
+        self.assertIn('local_holdout_multi', holdout_keys_1)
+
+        # Rule 2 should have 1 local holdout
+        holdouts_for_rule_2 = self.project_config.get_holdouts_for_rule('32222')
+        self.assertEqual(1, len(holdouts_for_rule_2))
+        self.assertEqual('local_holdout_multi', holdouts_for_rule_2[0].key)
+
+        # Non-existent rule should return empty list
+        self.assertEqual(0, len(self.project_config.get_holdouts_for_rule('non_existent')))
+
+    def test_empty_included_rules_not_mapped(self):
+        """ Test that holdouts with empty includedRules are not mapped to any rules. """
+
+        for rule_id in ['111127', '32222']:
+            holdouts = self.project_config.get_holdouts_for_rule(rule_id)
+            holdout_keys = {h.key for h in holdouts}
+            self.assertNotIn('local_holdout_empty', holdout_keys)
+
+    def test_draft_holdouts_not_processed(self):
+        """ Test that draft holdouts are not included in global or rule maps. """
+
+        global_keys = {h.key for h in self.project_config.get_global_holdouts()}
+        self.assertNotIn('draft_holdout', global_keys)
+
+        rule_keys = {h.key for h in self.project_config.get_holdouts_for_rule('111127')}
+        self.assertNotIn('draft_holdout', rule_keys)
+
+    def test_invalid_rule_ids_handled_silently(self):
+        """ Test that holdouts with non-existent rule IDs don't cause errors. """
+
+        holdouts = self.project_config.get_holdouts_for_rule('non_existent_rule_id')
+        self.assertEqual(0, len(holdouts))
+
+        # The holdout entity still exists, just not mapped
+        invalid_holdout = self.project_config.get_holdout('local_holdout_invalid')
+        self.assertIsNotNone(invalid_holdout)
+        self.assertEqual('local_holdout_invalid_rule', invalid_holdout.key)
+
+    def test_cross_rule_targeting(self):
+        """ Test that a single holdout can target rules from multiple experiments. """
+
+        holdouts_1 = self.project_config.get_holdouts_for_rule('111127')
+        holdouts_2 = self.project_config.get_holdouts_for_rule('32222')
+
+        holdout_keys_1 = {h.key for h in holdouts_1}
+        holdout_keys_2 = {h.key for h in holdouts_2}
+
+        self.assertIn('local_holdout_multi', holdout_keys_1)
+        self.assertIn('local_holdout_multi', holdout_keys_2)
+
+        # Same holdout object
+        multi_1 = next(h for h in holdouts_1 if h.key == 'local_holdout_multi')
+        multi_2 = next(h for h in holdouts_2 if h.key == 'local_holdout_multi')
+        self.assertEqual(multi_1.id, multi_2.id)
+
 
 class FeatureRolloutConfigTest(base.BaseTest):
     """Tests for Feature Rollout support in ProjectConfig parsing."""

tests/test_decision_service_holdout.py

@@ -1465,3 +1465,48 @@ def test_holdout_with_audience_match(self):
             self.assertIsNotNone(decision_no_match)
         finally:
             opt.close()
+
+
+class LocalHoldoutDecisionFlowTest(base.BaseTest):
+    """Tests for decision flow with local holdouts (rule-level targeting)."""
+
+    def setUp(self):
+        base.BaseTest.setUp(self)
+
+        config_dict = self.config_dict_with_features.copy()
+        rule_id = '111127'  # test_experiment
+
+        config_dict['holdouts'] = [
+            {
+                'id': 'local_test',
+                'key': 'local_test_holdout',
+                'status': 'Running',
+                'includedRules': [rule_id],
+                'audienceIds': [],
+                'variations': [
+                    {
+                        'id': 'local_test_var',
+                        'key': 'local_test_control',
+                        'variables': []
+                    }
+                ],
+                'trafficAllocation': [
+                    {
+                        'entityId': 'local_test_var',
+                        'endOfRange': 10000
+                    }
+                ]
+            }
+        ]
+
+        config_json = json.dumps(config_dict)
+        self.opt_obj = optimizely_module.Optimizely(config_json)
+        self.project_config = self.opt_obj.config_manager.get_config()
+
+    def test_local_holdout_checked_before_rule_evaluation(self):
+        """Test that local holdouts are checked before rule audience/traffic evaluation."""
+        rule_id = '111127'
+
+        holdouts = self.project_config.get_holdouts_for_rule(rule_id)
+        self.assertEqual(1, len(holdouts))
+        self.assertEqual('local_test_holdout', holdouts[0].key)