diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index be2be2c5..1d00bea5 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -1,4 +1,4 @@ -# Copyright 2017-2022, Optimizely +# Copyright 2017-2022, 2026, Optimizely # 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 @@ -610,6 +610,23 @@ def get_variation_for_rollout( return Decision(experiment=rule, variation=forced_decision_variation, source=enums.DecisionSources.ROLLOUT, cmab_uuid=None), decide_reasons + # Check local holdouts targeting this rollout rule + local_holdouts = project_config.get_holdouts_for_rule(rule.id) + for holdout in local_holdouts: + holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) + decide_reasons.extend(holdout_decision['reasons']) + + decision = holdout_decision['decision'] + # Check if user was bucketed into holdout (has a variation) + if decision.variation is not None: + message = ( + f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " + f"for rollout rule '{rule.key}' in feature flag '{feature.key}'." + ) + self.logger.info(message) + decide_reasons.append(message) + return decision, decide_reasons + bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes) decide_reasons += bucket_reasons @@ -733,9 +750,9 @@ def get_decision_for_flag( reasons = decide_reasons.copy() if decide_reasons else [] user_id = user_context.user_id - # Check holdouts - holdouts = project_config.get_holdouts_for_flag(feature_flag.key) - for holdout in holdouts: + # Check global holdouts (evaluated before any rules) + global_holdouts = project_config.get_global_holdouts() + for holdout in global_holdouts: holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) reasons.extend(holdout_decision['reasons']) @@ -745,7 +762,7 @@ def get_decision_for_flag( continue message = ( - f"The user '{user_id}' is bucketed into holdout '{holdout.key}' " + f"The user '{user_id}' is bucketed into global holdout '{holdout.key}' " f"for feature flag '{feature_flag.key}'." ) self.logger.info(message) @@ -756,7 +773,8 @@ def get_decision_for_flag( 'reasons': reasons } - # If no holdout decision, check experiments then rollouts + # If no global holdout decision, check experiments then rollouts + # Local holdouts are evaluated within each rule's evaluation if feature_flag.experimentIds: for experiment_id in feature_flag.experimentIds: experiment = project_config.get_experiment_from_id(experiment_id) @@ -778,6 +796,27 @@ def get_decision_for_flag( 'reasons': reasons } + # Check local holdouts targeting this rule + local_holdouts = project_config.get_holdouts_for_rule(experiment.id) + for holdout in local_holdouts: + holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) + reasons.extend(holdout_decision['reasons']) + + decision = holdout_decision['decision'] + # Check if user was bucketed into holdout (has a variation) + if decision.variation is not None: + message = ( + f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " + f"for rule '{experiment.key}' in feature flag '{feature_flag.key}'." + ) + self.logger.info(message) + reasons.append(message) + return { + 'decision': holdout_decision['decision'], + 'error': False, + 'reasons': reasons + } + # Get variation for experiment variation_result = self.get_variation( project_config, experiment, user_context, user_profile_tracker, reasons, decide_options diff --git a/optimizely/entities.py b/optimizely/entities.py index 589ca984..cb68055f 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -1,4 +1,4 @@ -# Copyright 2016-2022, Optimizely +# Copyright 2016-2022, 2026, Optimizely # 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 @@ -222,8 +222,7 @@ def __init__( variations: list[VariationDict], trafficAllocation: list[TrafficAllocation], audienceIds: list[str], - includedFlags: Optional[list[str]] = None, - excludedFlags: Optional[list[str]] = None, + includedRules: Optional[list[str]] = None, audienceConditions: Optional[Sequence[str | list[str]]] = None, **kwargs: Any ): @@ -234,8 +233,10 @@ def __init__( self.trafficAllocation = trafficAllocation self.audienceIds = audienceIds self.audienceConditions = audienceConditions - self.includedFlags = includedFlags or [] - self.excludedFlags = excludedFlags or [] + # None = global holdout (applies to all rules) + # [] = local holdout with no rules (effectively no holdout) + # [rule_id, ...] = local holdout targeting specific rules + self.includedRules = includedRules def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: """Returns audienceConditions if present, otherwise audienceIds. @@ -257,6 +258,15 @@ def is_activated(self) -> bool: """ return self.status == self.Status.RUNNING + @property + def is_global(self) -> bool: + """Check if the holdout is global (applies to all rules). + + Returns: + True if includedRules is None (global), False otherwise (local). + """ + return self.includedRules is None + def __str__(self) -> str: return self.key diff --git a/optimizely/helpers/types.py b/optimizely/helpers/types.py index 7076dc4d..1414f904 100644 --- a/optimizely/helpers/types.py +++ b/optimizely/helpers/types.py @@ -1,4 +1,4 @@ -# Copyright 2022, Optimizely +# Copyright 2022, 2026, Optimizely # 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 @@ -130,5 +130,4 @@ class HoldoutDict(ExperimentDict): Extends ExperimentDict with holdout-specific properties. """ holdoutStatus: HoldoutStatus - includedFlags: list[str] - excludedFlags: list[str] + includedRules: Optional[list[str]] diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 5b752538..5d9f61a7 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -1,4 +1,4 @@ -# Copyright 2016-2019, 2021-2022, Optimizely +# Copyright 2016-2019, 2021-2022, 2026, Optimizely # 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 @@ -89,47 +89,12 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): region_value = config.get('region') self.region: str = region_value or 'US' - # Parse holdouts from datafile and convert to Holdout entities + # Parse holdouts from datafile (processing deferred until after experiments are loaded) holdouts_data: list[types.HoldoutDict] = config.get('holdouts', []) self.holdouts: list[entities.Holdout] = [] self.holdout_id_map: dict[str, entities.Holdout] = {} self.global_holdouts: list[entities.Holdout] = [] - self.included_holdouts: dict[str, list[entities.Holdout]] = {} - self.excluded_holdouts: dict[str, list[entities.Holdout]] = {} - self.flag_holdouts_map: dict[str, list[entities.Holdout]] = {} - - # Convert holdout dicts to Holdout entities - for holdout_data in holdouts_data: - # Create Holdout entity - holdout = entities.Holdout(**holdout_data) - self.holdouts.append(holdout) - - # Only process Running holdouts but doing it here for efficiency like the original Python implementation) - if not holdout.is_activated: - continue - - # Map by ID for quick lookup - self.holdout_id_map[holdout.id] = holdout - - # Categorize as global vs flag-specific - # Global holdouts: apply to all flags unless explicitly excluded - # Flag-specific holdouts: only apply to explicitly included flags - if not holdout.includedFlags: - # This is a global holdout - self.global_holdouts.append(holdout) - - # Track which flags this global holdout excludes - if holdout.excludedFlags: - for flag_id in holdout.excludedFlags: - if flag_id not in self.excluded_holdouts: - self.excluded_holdouts[flag_id] = [] - self.excluded_holdouts[flag_id].append(holdout) - else: - # This holdout applies to specific flags only - for flag_id in holdout.includedFlags: - if flag_id not in self.included_holdouts: - self.included_holdouts[flag_id] = [] - self.included_holdouts[flag_id].append(holdout) + self.rule_holdouts_map: dict[str, list[entities.Holdout]] = {} # Utility maps for quick lookup self.group_id_map: dict[str, entities.Group] = self._generate_key_map(self.groups, 'id', entities.Group) @@ -263,21 +228,10 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): everyone_else_variation.variables, 'id', entities.Variation.VariableUsage ) - flag_id = feature.id - applicable_holdouts: list[entities.Holdout] = [] - - # Add global holdouts first, excluding any that are explicitly excluded for this flag - excluded_holdouts = self.excluded_holdouts.get(flag_id, []) - for holdout in self.global_holdouts: - if holdout not in excluded_holdouts: - applicable_holdouts.append(holdout) - - # Add flag-specific local holdouts AFTER global holdouts - if flag_id in self.included_holdouts: - applicable_holdouts.extend(self.included_holdouts[flag_id]) - - if applicable_holdouts: - self.flag_holdouts_map[feature.key] = applicable_holdouts + # Note: Holdout evaluation is now done at the rule level, + # not the flag level. Global holdouts are checked before any rule evaluation, + # and local holdouts are checked within each rule's evaluation. + # The flag_holdouts_map is no longer used. rollout = None if len(feature.rolloutId) == 0 else self.rollout_id_map[feature.rolloutId] if rollout: @@ -294,7 +248,38 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): variations.append(rule_var) self.flag_variations_map[feature.key] = variations - # Process holdout variations are converted to Variation entities just like experiment variations + # Process holdouts: Convert holdout dicts to Holdout entities and build mappings + # This happens after experiment_id_map is fully populated so we can validate rule IDs + for holdout_data in holdouts_data: + # Create Holdout entity + holdout = entities.Holdout(**holdout_data) + self.holdouts.append(holdout) + + # Only process Running holdouts (for efficiency) + if not holdout.is_activated: + continue + + # Map by ID for quick lookup + self.holdout_id_map[holdout.id] = holdout + + # Categorize as global vs local (rule-specific) + # Global holdouts: includedRules is None (applies to all rules) + # Local holdouts: includedRules is a list (applies to specific rules) + if holdout.is_global: + # This is a global holdout + self.global_holdouts.append(holdout) + else: + # This holdout applies to specific rules only + # includedRules contains rule IDs - validate they exist before mapping + if holdout.includedRules: + for rule_id in holdout.includedRules: + # Only map if the rule exists (silently skip invalid rule IDs) + if rule_id in self.experiment_id_map: + if rule_id not in self.rule_holdouts_map: + self.rule_holdouts_map[rule_id] = [] + self.rule_holdouts_map[rule_id].append(holdout) + + # Process holdout variations - convert to Variation entities just like experiment variations if self.holdouts: for holdout in self.holdouts: # Initialize variation maps for this holdout @@ -912,19 +897,26 @@ def get_flag_variation( return None - def get_holdouts_for_flag(self, flag_key: str) -> list[entities.Holdout]: - """ Helper method to get holdouts from an applied feature flag. + def get_global_holdouts(self) -> list[entities.Holdout]: + """ Helper method to get all global holdouts. - Args: - flag_key: Key of the feature flag. + Global holdouts apply to all rules unless specifically targeted by local holdouts. Returns: - The holdouts that apply for a specific flag as Holdout entity objects. + List of global Holdout entity objects. """ - if not self.holdouts: - return [] + return self.global_holdouts + + def get_holdouts_for_rule(self, rule_id: str) -> list[entities.Holdout]: + """ Helper method to get local holdouts targeting a specific rule. - return self.flag_holdouts_map.get(flag_key, []) + Args: + rule_id: ID of the rule (experiment or rollout rule). + + Returns: + List of Holdout entity objects that target this specific rule. + """ + return self.rule_holdouts_map.get(rule_id, []) def get_holdout(self, holdout_id: str) -> Optional[entities.Holdout]: """ Helper method to get holdout from holdout ID. diff --git a/tests/test_config.py b/tests/test_config.py index c21f9b34..b72acd56 100644 --- a/tests/test_config.py +++ b/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], } ] @@ -1425,54 +1420,45 @@ def setUp(self): opt_obj = optimizely.Optimizely(self.config_json_with_holdouts) self.config_with_holdouts = opt_obj.config_manager.get_config() - def test_get_holdouts_for_flag__non_existent_flag(self): - """ Test that get_holdouts_for_flag returns empty array for non-existent flag. """ + def test_get_holdouts_for_rule__non_existent_rule(self): + """ Test that get_holdouts_for_rule returns empty array for non-existent rule. """ - holdouts = self.config_with_holdouts.get_holdouts_for_flag('non_existent_flag') + holdouts = self.config_with_holdouts.get_holdouts_for_rule('non_existent_rule') self.assertEqual([], holdouts) - def test_get_holdouts_for_flag__returns_global_and_specific_holdouts(self): - """ Test that get_holdouts_for_flag returns global holdouts that do not exclude the flag - and specific holdouts that include the flag. """ + 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_holdouts_for_flag('test_feature_in_experiment_and_rollout') - self.assertEqual(2, len(holdouts)) - - global_holdout = next((h for h in holdouts if h['key'] == 'global_holdout'), None) - self.assertIsNotNone(global_holdout) - self.assertEqual('holdout_1', global_holdout['id']) - - specific_holdout = next((h for h in holdouts if h['key'] == 'specific_holdout'), None) - self.assertIsNotNone(specific_holdout) - self.assertEqual('holdout_2', specific_holdout['id']) + holdouts = self.config_with_holdouts.get_global_holdouts() + self.assertEqual(1, len(holdouts)) - def test_get_holdouts_for_flag__excludes_global_holdouts_for_excluded_flags(self): - """ Test that get_holdouts_for_flag does not return global holdouts that exclude the flag. """ + global_holdout = holdouts[0] + self.assertEqual('holdout_1', global_holdout.id) + self.assertEqual('global_holdout', global_holdout.key) + self.assertTrue(global_holdout.is_global) - holdouts = self.config_with_holdouts.get_holdouts_for_flag('boolean_single_variable_feature') - self.assertEqual(0, len(holdouts)) + def test_get_holdouts_for_rule__returns_local_holdouts(self): + """ Test that get_holdouts_for_rule returns holdouts targeting specific rules. """ - global_holdout = next((h for h in holdouts if h['key'] == 'global_holdout'), None) - self.assertIsNone(global_holdout) + 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_holdouts_for_flag__caches_results(self): - """ Test that get_holdouts_for_flag caches results for subsequent calls. """ + def test_get_global_holdouts__caches_results(self): + """ Test that get_global_holdouts returns consistent results. """ - holdouts1 = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment_and_rollout') - holdouts2 = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment_and_rollout') + holdouts1 = self.config_with_holdouts.get_global_holdouts() + holdouts2 = self.config_with_holdouts.get_global_holdouts() - # Should be the same object (cached) + # Should be the same list object self.assertIs(holdouts1, holdouts2) - self.assertEqual(2, len(holdouts1)) - - def test_get_holdouts_for_flag__returns_only_global_for_non_targeted_flags(self): - """ Test that get_holdouts_for_flag returns only global holdouts for flags not specifically targeted. """ - holdouts = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_rollout') + 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. """ - # Should only include global holdout (not excluded and no specific targeting) - self.assertEqual(1, len(holdouts)) - self.assertEqual('global_holdout', holdouts[0]['key']) + holdouts = self.config_with_holdouts.get_holdouts_for_rule('definitely_non_existent_rule_id_12345') + self.assertEqual(0, len(holdouts)) def test_get_holdout__returns_holdout_for_valid_id(self): """ Test that get_holdout returns holdout when valid ID is provided. """ @@ -1517,34 +1503,239 @@ def test_get_holdout__does_not_log_when_found(self): mock_logger.error.assert_not_called() def test_holdout_initialization__categorizes_holdouts_properly(self): - """ Test that holdouts are properly categorized during initialization. """ + """ Test that holdouts are properly categorized during initialization (rule-level). """ 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) - # Use correct feature flag IDs - boolean_feature_id = '91111' - multi_variate_feature_id = '91114' - - self.assertIn(multi_variate_feature_id, self.config_with_holdouts.included_holdouts) - self.assertTrue(len(self.config_with_holdouts.included_holdouts[multi_variate_feature_id]) > 0) - self.assertNotIn(boolean_feature_id, self.config_with_holdouts.included_holdouts) - - self.assertIn(boolean_feature_id, self.config_with_holdouts.excluded_holdouts) - self.assertTrue(len(self.config_with_holdouts.excluded_holdouts[boolean_feature_id]) > 0) + # 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. """ + # Inactive holdouts should not be in the holdout_id_map self.assertNotIn('holdout_3', self.config_with_holdouts.holdout_id_map) - self.assertNotIn('holdout_3', self.config_with_holdouts.global_holdouts) - boolean_feature_id = '91111' - included_for_boolean = self.config_with_holdouts.included_holdouts.get(boolean_feature_id) - self.assertIsNone(included_for_boolean) + # Inactive holdouts should not be in global_holdouts + 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): diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index e286ba5c..b9b12a26 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -1,4 +1,4 @@ -# Copyright 2025 Optimizely and contributors +# Copyright 2026 Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -298,10 +298,10 @@ def test_user_bucketed_into_holdout_returns_before_experiments(self): 'reasons': [] } - # Mock get_holdouts_for_flag to return holdouts so the holdout path is taken + # Mock get_global_holdouts to return holdouts so the holdout path is taken with mock.patch.object( self.config_with_holdouts, - 'get_holdouts_for_flag', + 'get_global_holdouts', return_value=[holdout] ): with mock.patch.object( @@ -381,11 +381,8 @@ def test_evaluates_global_holdouts_for_all_flags(self): feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') self.assertIsNotNone(feature_flag) - # Get global holdouts - global_holdouts = [ - h for h in self.config_with_holdouts.holdouts - if not h.includedFlags or len(h.includedFlags) == 0 - ] + # Get global holdouts (includedRules is None for global holdouts) + global_holdouts = self.config_with_holdouts.get_global_holdouts() if global_holdouts: user_context = self.opt_obj.create_user_context('testUserId', {}) @@ -401,16 +398,16 @@ def test_evaluates_global_holdouts_for_all_flags(self): self.assertIsInstance(result, list) def test_respects_included_and_excluded_flags_configuration(self): - """Should respect included and excluded flags configuration.""" + """Should respect local holdout rule-level targeting (replaces flag-level include/exclude).""" feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') if feature_flag: - # Get holdouts for this flag - holdouts_for_flag = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment') + # In the new model, holdouts target specific rules, not flags + # Global holdouts apply to all rules + global_holdouts = self.config_with_holdouts.get_global_holdouts() - # Should not include holdouts that exclude this flag - excluded_holdout = next((h for h in holdouts_for_flag if h.key == 'excluded_holdout'), None) - self.assertIsNone(excluded_holdout) + # Global holdouts should apply to all rules (no exclusion at flag level anymore) + self.assertIsNotNone(global_holdouts) # Holdout logging and error handling tests @@ -1468,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)