diff --git a/optimizely/bucketer.py b/optimizely/bucketer.py index a6e8323d..ca431012 100644 --- a/optimizely/bucketer.py +++ b/optimizely/bucketer.py @@ -119,16 +119,41 @@ def bucket( and array of log messages representing decision making. */. """ + # Check if experiment is None first + if not experiment: + message = 'Invalid entity key provided for bucketing. Returning nil.' + project_config.logger.debug(message) + return None, [] + + if isinstance(experiment, dict): + # This is a holdout dictionary + experiment_key = experiment.get('key', '') + experiment_id = experiment.get('id', '') + else: + # This is an Experiment object + experiment_key = experiment.key + experiment_id = experiment.id + + if not experiment_key or not experiment_key.strip(): + message = 'Invalid entity key provided for bucketing. Returning nil.' + project_config.logger.debug(message) + return None, [] + variation_id, decide_reasons = self.bucket_to_entity_id(project_config, experiment, user_id, bucketing_id) if variation_id: - variation = project_config.get_variation_from_id_by_experiment_id(experiment.id, variation_id) + if isinstance(experiment, dict): + # For holdouts, find the variation in the holdout's variations array + variations = experiment.get('variations', []) + variation = next((v for v in variations if v.get('id') == variation_id), None) + else: + # For experiments, use the existing method + variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) return variation, decide_reasons - else: - message = 'Bucketed into an empty traffic range. Returning nil.' - project_config.logger.info(message) - decide_reasons.append(message) - + # No variation found - log message for empty traffic range + message = 'Bucketed into an empty traffic range. Returning nil.' + project_config.logger.info(message) + decide_reasons.append(message) return None, decide_reasons def bucket_to_entity_id( @@ -151,9 +176,25 @@ def bucket_to_entity_id( if not experiment: return None, decide_reasons + # Handle both Experiment objects and holdout dictionaries + if isinstance(experiment, dict): + # This is a holdout dictionary - holdouts don't have groups + experiment_key = experiment.get('key', '') + experiment_id = experiment.get('id', '') + traffic_allocations = experiment.get('trafficAllocation', []) + has_cmab = False + group_policy = None + else: + # This is an Experiment object + experiment_key = experiment.key + experiment_id = experiment.id + traffic_allocations = experiment.trafficAllocation + has_cmab = bool(experiment.cmab) + group_policy = getattr(experiment, 'groupPolicy', None) + # Determine if experiment is in a mutually exclusive group. - # This will not affect evaluation of rollout rules. - if experiment.groupPolicy in GROUP_POLICIES: + # This will not affect evaluation of rollout rules or holdouts. + if group_policy and group_policy in GROUP_POLICIES: group = project_config.get_group(experiment.groupId) if not group: @@ -169,26 +210,27 @@ def bucket_to_entity_id( decide_reasons.append(message) return None, decide_reasons - if user_experiment_id != experiment.id: - message = f'User "{user_id}" is not in experiment "{experiment.key}" of group {experiment.groupId}.' + if user_experiment_id != experiment_id: + message = f'User "{user_id}" is not in experiment "{experiment_key}" of group {experiment.groupId}.' project_config.logger.info(message) decide_reasons.append(message) return None, decide_reasons - message = f'User "{user_id}" is in experiment {experiment.key} of group {experiment.groupId}.' + message = f'User "{user_id}" is in experiment {experiment_key} of group {experiment.groupId}.' project_config.logger.info(message) decide_reasons.append(message) - traffic_allocations: list[TrafficAllocation] = experiment.trafficAllocation - if experiment.cmab: - traffic_allocations = [ - { - "entityId": "$", - "endOfRange": experiment.cmab['trafficAllocation'] - } - ] + if has_cmab: + if experiment.cmab: + traffic_allocations = [ + { + "entityId": "$", + "endOfRange": experiment.cmab['trafficAllocation'] + } + ] + # Bucket user if not in white-list and in group (if any) variation_id = self.find_bucket(project_config, bucketing_id, - experiment.id, traffic_allocations) + experiment_id, traffic_allocations) return variation_id, decide_reasons diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index f843730e..18640c7a 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -12,7 +12,7 @@ # limitations under the License. from __future__ import annotations -from typing import TYPE_CHECKING, NamedTuple, Optional, Sequence, List, TypedDict +from typing import TYPE_CHECKING, NamedTuple, Optional, Sequence, List, TypedDict, Dict from . import bucketer from . import entities @@ -83,6 +83,7 @@ class Decision(NamedTuple): variation: Optional[entities.Variation] source: Optional[str] cmab_uuid: Optional[str] + holdout: Optional[Dict[str, str]] class DecisionService: @@ -670,7 +671,173 @@ def get_variation_for_feature( - 'error': Boolean indicating if an error occurred during the decision process. - 'reasons': List of log messages representing decision making for the feature. """ - return self.get_variations_for_feature_list(project_config, [feature], user_context, options)[0] + holdouts = project_config.get_holdouts_for_flag(feature.key) + + if holdouts: + # Has holdouts - use get_decision_for_flag which checks holdouts first + return self.get_decision_for_flag(feature, user_context, project_config, options) + else: + return self.get_variations_for_feature_list(project_config, [feature], user_context, options)[0] + + def get_decision_for_flag( + self, + feature_flag: entities.FeatureFlag, + user_context: OptimizelyUserContext, + project_config: ProjectConfig, + decide_options: Optional[Sequence[str]] = None, + user_profile_tracker: Optional[UserProfileTracker] = None, + decide_reasons: Optional[list[str]] = None + ) -> DecisionResult: + """ + Get the decision for a single feature flag. + Processes holdouts, experiments, and rollouts in that order. + + Args: + feature_flag: The feature flag to get a decision for. + user_context: The user context. + project_config: The project config. + decide_options: Sequence of decide options. + user_profile_tracker: The user profile tracker. + decide_reasons: List of decision reasons to merge. + + Returns: + A DecisionResult for the feature 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: + holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) + reasons.extend(holdout_decision['reasons']) + + if not holdout_decision['decision']: + continue + + message = ( + f"The user '{user_id}' is bucketed into holdout '{holdout['key']}' " + f"for feature flag '{feature_flag.key}'." + ) + self.logger.info(message) + reasons.append(message) + return { + 'decision': holdout_decision['decision'], + 'error': False, + 'reasons': reasons + } + + # If no holdout decision, fall back to existing experiment/rollout logic + # Use get_variations_for_feature_list which handles experiments and rollouts + fallback_result = self.get_variations_for_feature_list( + project_config, [feature_flag], user_context, decide_options + )[0] + + # Merge reasons + if fallback_result.get('reasons'): + reasons.extend(fallback_result['reasons']) + + return { + 'decision': fallback_result.get('decision'), + 'error': fallback_result.get('error', False), + 'reasons': reasons + } + + def get_variation_for_holdout( + self, + holdout: Dict[str, str], + user_context: OptimizelyUserContext, + project_config: ProjectConfig + ) -> DecisionResult: + """ + Get the variation for holdout. + + Args: + holdout: The holdout configuration. + user_context: The user context. + project_config: The project config. + + Returns: + A DecisionResult for the holdout. + """ + from optimizely.helpers.enums import ExperimentAudienceEvaluationLogs + + decide_reasons: list[str] = [] + user_id = user_context.user_id + attributes = user_context.get_user_attributes() + + if not holdout or not holdout.get('status') or holdout.get('status') != 'Running': + key = holdout.get('key') if holdout else 'unknown' + message = f"Holdout '{key}' is not running." + self.logger.info(message) + decide_reasons.append(message) + return { + 'decision': None, + 'error': False, + 'reasons': decide_reasons + } + + bucketing_id, bucketing_id_reasons = self._get_bucketing_id(user_id, attributes) + decide_reasons.extend(bucketing_id_reasons) + + # Check audience conditions + audience_conditions = holdout.get('audienceIds') + user_meets_audience_conditions, reasons_received = audience_helper.does_user_meet_audience_conditions( + project_config, + audience_conditions, + ExperimentAudienceEvaluationLogs, + holdout.get('key', 'unknown'), + user_context, + self.logger + ) + decide_reasons.extend(reasons_received) + + if not user_meets_audience_conditions: + message = f"User '{user_id}' does not meet the conditions for holdout '{holdout['key']}'." + self.logger.debug(message) + decide_reasons.append(message) + return { + 'decision': None, + 'error': False, + 'reasons': decide_reasons + } + + # Bucket user into holdout variation + variation, bucket_reasons = self.bucketer.bucket(project_config, holdout, user_id, bucketing_id) + decide_reasons.extend(bucket_reasons) + + if variation: + # For holdouts, variation is a dict, not a Variation entity + variation_key = variation['key'] if isinstance(variation, dict) else variation.key + message = ( + f"The user '{user_id}' is bucketed into variation '{variation_key}' " + f"of holdout '{holdout['key']}'." + ) + self.logger.info(message) + decide_reasons.append(message) + + # Create Decision with holdout - experiment is None, holdout field contains the holdout dict + holdout_decision: Decision = Decision( + experiment=None, + variation=None, + source=enums.DecisionSources.HOLDOUT, + cmab_uuid=None, + holdout=holdout + ) + return { + 'decision': holdout_decision, + 'error': False, + 'reasons': decide_reasons + } + + message = f"User '{user_id}' is not bucketed into any variation for holdout '{holdout['key']}'." + self.logger.info(message) + decide_reasons.append(message) + return { + 'decision': None, + 'error': False, + 'reasons': decide_reasons + } def validated_forced_decision( self, diff --git a/optimizely/helpers/enums.py b/optimizely/helpers/enums.py index 4630491c..74acdcfa 100644 --- a/optimizely/helpers/enums.py +++ b/optimizely/helpers/enums.py @@ -99,6 +99,7 @@ class DecisionSources: EXPERIMENT: Final = 'experiment' FEATURE_TEST: Final = 'feature-test' ROLLOUT: Final = 'rollout' + HOLDOUT: Final = 'holdout' class Errors: diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 1e813462..44fc2f2c 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -218,6 +218,21 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): # Add this experiment in experiment-feature map. self.experiment_feature_map[exp_id] = [feature.id] rules.append(self.experiment_id_map[exp_id]) + + flag_id = feature.id + applicable_holdouts = [] + + if flag_id in self.included_holdouts: + applicable_holdouts.extend(self.included_holdouts[flag_id]) + + for holdout in self.global_holdouts.values(): + excluded_flag_ids = holdout.get('excludedFlags', []) + if flag_id not in excluded_flag_ids: + applicable_holdouts.append(holdout) + + if applicable_holdouts: + self.flag_holdouts_map[feature.key] = applicable_holdouts + rollout = None if len(feature.rolloutId) == 0 else self.rollout_id_map[feature.rolloutId] if rollout: for exp in rollout.experiments: @@ -231,6 +246,30 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): variations.append(rule_variation) self.flag_variations_map[feature.key] = variations + if self.holdouts: + for holdout in self.holdouts: + holdout_key = holdout.get('key') + holdout_id = holdout.get('id') + + if not holdout_key or not holdout_id: + continue + + self.variation_key_map[holdout_key] = {} + self.variation_id_map[holdout_key] = {} + self.variation_id_map_by_experiment_id[holdout_id] = {} + self.variation_key_map_by_experiment_id[holdout_id] = {} + + variations = holdout.get('variations') + if variations: + for variation in variations: + variation_key = variation.get('key') if isinstance(variation, dict) else None + variation_id = variation.get('id') if isinstance(variation, dict) else None + if variation_key and variation_id: + self.variation_key_map[holdout_key][variation_key] = variation + self.variation_id_map[holdout_key][variation_id] = variation + self.variation_key_map_by_experiment_id[holdout_id][variation_key] = variation + self.variation_id_map_by_experiment_id[holdout_id][variation_id] = variation + @staticmethod def _generate_key_map( entity_list: Iterable[Any], key: str, entity_class: Type[EntityClass], first_value: bool = False @@ -794,38 +833,10 @@ def get_holdouts_for_flag(self, flag_key: str) -> list[Any]: Returns: The holdouts that apply for a specific flag. """ - feature_flag = self.feature_key_map.get(flag_key) - if not feature_flag: + if not self.holdouts: return [] - flag_id = feature_flag.id - - # Check cache first - if flag_id in self.flag_holdouts_map: - return self.flag_holdouts_map[flag_id] - - holdouts = [] - - # Add global holdouts that don't exclude this flag - for holdout in self.global_holdouts.values(): - is_excluded = False - excluded_flags = holdout.get('excludedFlags') - if excluded_flags: - for excluded_flag_id in excluded_flags: - if excluded_flag_id == flag_id: - is_excluded = True - break - if not is_excluded: - holdouts.append(holdout) - - # Add holdouts that specifically include this flag - if flag_id in self.included_holdouts: - holdouts.extend(self.included_holdouts[flag_id]) - - # Cache the result - self.flag_holdouts_map[flag_id] = holdouts - - return holdouts + return self.flag_holdouts_map.get(flag_key, []) def get_holdout(self, holdout_id: str) -> Optional[dict[str, Any]]: """ Helper method to get holdout from holdout ID. diff --git a/tests/test_bucketing_holdout.py b/tests/test_bucketing_holdout.py new file mode 100644 index 00000000..e3d9bcb3 --- /dev/null +++ b/tests/test_bucketing_holdout.py @@ -0,0 +1,360 @@ +# Copyright 2025 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. +# 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 +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import copy +import json +from unittest import mock + +from optimizely import bucketer +from optimizely import error_handler +from optimizely import logger +from optimizely import optimizely as optimizely_module +from tests import base + + +class TestBucketer(bucketer.Bucketer): + """Helper class for testing with controlled bucket values.""" + + def __init__(self): + super().__init__() + self._bucket_values = [] + self._bucket_index = 0 + + def set_bucket_values(self, values): + """Set predetermined bucket values for testing.""" + self._bucket_values = values + self._bucket_index = 0 + + def _generate_bucket_value(self, bucketing_id): + """Override to return controlled bucket values.""" + if not self._bucket_values: + return super()._generate_bucket_value(bucketing_id) + + value = self._bucket_values[self._bucket_index] + self._bucket_index = (self._bucket_index + 1) % len(self._bucket_values) + return value + + +class BucketerHoldoutTest(base.BaseTest): + """Tests for Optimizely Bucketer with Holdouts.""" + + def setUp(self): + base.BaseTest.setUp(self) + self.error_handler = error_handler.NoOpErrorHandler() + self.mock_logger = mock.MagicMock(spec=logger.Logger) + + # Create a config dict with holdouts that have variations and traffic allocation + config_dict_with_holdouts = self.config_dict.copy() + + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'holdout_1', + 'key': 'test_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'var_1', + 'key': 'control', + 'variables': [] + }, + { + 'id': 'var_2', + 'key': 'treatment', + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'var_1', + 'endOfRange': 5000 + }, + { + 'entityId': 'var_2', + 'endOfRange': 10000 + } + ] + }, + { + 'id': 'holdout_empty_1', + 'key': 'empty_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [], + 'trafficAllocation': [] + } + ] + + # Convert to JSON and create config + config_json = json.dumps(config_dict_with_holdouts) + opt_obj = optimizely_module.Optimizely(config_json) + self.config = opt_obj.config_manager.get_config() + + self.test_bucketer = TestBucketer() + self.test_user_id = 'test_user_id' + self.test_bucketing_id = 'test_bucketing_id' + + # Verify that the config contains holdouts + self.assertIsNotNone(self.config.holdouts) + self.assertGreater(len(self.config.holdouts), 0) + + def test_bucket_user_within_valid_traffic_allocation_range(self): + """Should bucket user within valid traffic allocation range.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Set bucket value to be within first variation's traffic allocation (0-5000 range) + self.test_bucketer.set_bucket_values([2500]) + + variation, reasons = self.test_bucketer.bucket( + self.config, holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNotNone(variation) + self.assertEqual(variation['id'], 'var_1') + self.assertEqual(variation['key'], 'control') + + def test_bucket_returns_none_when_user_outside_traffic_allocation(self): + """Should return None when user is outside traffic allocation range.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Modify traffic allocation to be smaller + modified_holdout = copy.deepcopy(holdout) + modified_holdout['trafficAllocation'] = [ + { + 'entityId': 'var_1', + 'endOfRange': 1000 + } + ] + + # Set bucket value outside traffic allocation range + self.test_bucketer.set_bucket_values([1500]) + + variation, reasons = self.test_bucketer.bucket( + self.config, modified_holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNone(variation) + + def test_bucket_returns_none_when_holdout_has_no_traffic_allocation(self): + """Should return None when holdout has no traffic allocation.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Clear traffic allocation + modified_holdout = copy.deepcopy(holdout) + modified_holdout['trafficAllocation'] = [] + + self.test_bucketer.set_bucket_values([5000]) + + variation, reasons = self.test_bucketer.bucket( + self.config, modified_holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNone(variation) + + def test_bucket_returns_none_with_invalid_variation_id(self): + """Should return None when traffic allocation points to invalid variation ID.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Set traffic allocation to point to non-existent variation + modified_holdout = copy.deepcopy(holdout) + modified_holdout['trafficAllocation'] = [ + { + 'entityId': 'invalid_variation_id', + 'endOfRange': 10000 + } + ] + + self.test_bucketer.set_bucket_values([5000]) + + variation, reasons = self.test_bucketer.bucket( + self.config, modified_holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNone(variation) + + def test_bucket_returns_none_when_holdout_has_no_variations(self): + """Should return None when holdout has no variations.""" + holdout = self.config.get_holdout('holdout_empty_1') + self.assertIsNotNone(holdout) + self.assertEqual(len(holdout.get('variations', [])), 0) + + self.test_bucketer.set_bucket_values([5000]) + + variation, reasons = self.test_bucketer.bucket( + self.config, holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNone(variation) + + def test_bucket_returns_none_with_empty_key(self): + """Should return None when holdout has empty key.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Clear holdout key + modified_holdout = copy.deepcopy(holdout) + modified_holdout['key'] = '' + + self.test_bucketer.set_bucket_values([5000]) + + variation, reasons = self.test_bucketer.bucket( + self.config, modified_holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNone(variation) + + def test_bucket_returns_none_with_null_key(self): + """Should return None when holdout has null key.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Set holdout key to None + modified_holdout = copy.deepcopy(holdout) + modified_holdout['key'] = None + + self.test_bucketer.set_bucket_values([5000]) + + variation, reasons = self.test_bucketer.bucket( + self.config, modified_holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNone(variation) + + def test_bucket_user_into_first_variation_with_multiple_variations(self): + """Should bucket user into first variation with multiple variations.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Verify holdout has multiple variations + self.assertGreaterEqual(len(holdout['variations']), 2) + + # Test user buckets into first variation + self.test_bucketer.set_bucket_values([2500]) + variation, reasons = self.test_bucketer.bucket( + self.config, holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNotNone(variation) + self.assertEqual(variation['id'], 'var_1') + self.assertEqual(variation['key'], 'control') + + def test_bucket_user_into_second_variation_with_multiple_variations(self): + """Should bucket user into second variation with multiple variations.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Verify holdout has multiple variations + self.assertGreaterEqual(len(holdout['variations']), 2) + self.assertEqual(holdout['variations'][0]['id'], 'var_1') + self.assertEqual(holdout['variations'][1]['id'], 'var_2') + + # Test user buckets into second variation (bucket value 7500 should be in 5000-10000 range) + self.test_bucketer.set_bucket_values([7500]) + variation, reasons = self.test_bucketer.bucket( + self.config, holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNotNone(variation) + self.assertEqual(variation['id'], 'var_2') + self.assertEqual(variation['key'], 'treatment') + + def test_bucket_handles_edge_case_boundary_values(self): + """Should handle edge case boundary values correctly.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Modify traffic allocation to be 5000 (50%) + modified_holdout = copy.deepcopy(holdout) + modified_holdout['trafficAllocation'] = [ + { + 'entityId': 'var_1', + 'endOfRange': 5000 + } + ] + + # Test exact boundary value (should be included) + self.test_bucketer.set_bucket_values([4999]) + variation, reasons = self.test_bucketer.bucket( + self.config, modified_holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNotNone(variation) + self.assertEqual(variation['id'], 'var_1') + + # Test value just outside boundary (should not be included) + self.test_bucketer.set_bucket_values([5000]) + variation, reasons = self.test_bucketer.bucket( + self.config, modified_holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNone(variation) + + def test_bucket_produces_consistent_results_with_same_inputs(self): + """Should produce consistent results with same inputs.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Create a real bucketer (not test bucketer) for consistent hashing + real_bucketer = bucketer.Bucketer() + variation1, reasons1 = real_bucketer.bucket( + self.config, holdout, self.test_user_id, self.test_bucketing_id + ) + variation2, reasons2 = real_bucketer.bucket( + self.config, holdout, self.test_user_id, self.test_bucketing_id + ) + + # Results should be identical + if variation1: + self.assertIsNotNone(variation2) + self.assertEqual(variation1['id'], variation2['id']) + self.assertEqual(variation1['key'], variation2['key']) + else: + self.assertIsNone(variation2) + + def test_bucket_handles_different_bucketing_ids_without_exceptions(self): + """Should handle different bucketing IDs without exceptions.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + # Create a real bucketer (not test bucketer) for real hashing behavior + real_bucketer = bucketer.Bucketer() + + # These calls should not raise exceptions + try: + real_bucketer.bucket(self.config, holdout, self.test_user_id, 'bucketingId1') + real_bucketer.bucket(self.config, holdout, self.test_user_id, 'bucketingId2') + except Exception as e: + self.fail(f'Bucketing raised an exception: {e}') + + def test_bucket_populates_decision_reasons_properly(self): + """Should populate decision reasons properly.""" + holdout = self.config.get_holdout('holdout_1') + self.assertIsNotNone(holdout) + + self.test_bucketer.set_bucket_values([5000]) + variation, reasons = self.test_bucketer.bucket( + self.config, holdout, self.test_user_id, self.test_bucketing_id + ) + + self.assertIsNotNone(reasons) + self.assertIsInstance(reasons, list) + # Decision reasons should be populated from the bucketing process diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py new file mode 100644 index 00000000..ff6a3438 --- /dev/null +++ b/tests/test_decision_service_holdout.py @@ -0,0 +1,567 @@ +# Copyright 2025 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. +# 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 +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import json +from unittest import mock + +from optimizely import decision_service +from optimizely import error_handler +from optimizely import logger +from optimizely import optimizely as optimizely_module +from optimizely.decision.optimizely_decide_option import OptimizelyDecideOption +from optimizely.helpers import enums +from tests import base + + +class DecisionServiceHoldoutTest(base.BaseTest): + """Tests for Decision Service with Holdouts.""" + + def setUp(self): + base.BaseTest.setUp(self) + self.error_handler = error_handler.NoOpErrorHandler() + self.spy_logger = mock.MagicMock(spec=logger.SimpleLogger) + self.spy_logger.logger = self.spy_logger + self.spy_user_profile_service = mock.MagicMock() + self.spy_cmab_service = mock.MagicMock() + + # Create a config dict with holdouts and feature flags + config_dict_with_holdouts = self.config_dict_with_features.copy() + + # Get feature flag ID for test_feature_in_experiment + test_feature_id = '91111' + + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'holdout_1', + 'key': 'test_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'holdout_var_1', + 'key': 'holdout_control', + 'variables': [] + }, + { + 'id': 'holdout_var_2', + 'key': 'holdout_treatment', + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'holdout_var_1', + 'endOfRange': 5000 + }, + { + 'entityId': 'holdout_var_2', + 'endOfRange': 10000 + } + ] + }, + { + 'id': 'holdout_2', + 'key': 'excluded_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [test_feature_id], + 'audienceIds': [], + 'variations': [ + { + 'id': 'holdout_var_3', + 'key': 'control', + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'holdout_var_3', + 'endOfRange': 10000 + } + ] + } + ] + + # Convert to JSON and create config + config_json = json.dumps(config_dict_with_holdouts) + self.opt_obj = optimizely_module.Optimizely(config_json) + self.config_with_holdouts = self.opt_obj.config_manager.get_config() + + self.decision_service_with_holdouts = decision_service.DecisionService( + self.spy_logger, + self.spy_user_profile_service, + self.spy_cmab_service + ) + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() + + # get_variations_for_feature_list with holdouts tests + + def test_holdout_active_and_user_bucketed_returns_holdout_decision(self): + """When holdout is active and user is bucketed, should return holdout decision with variation.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + holdout = self.config_with_holdouts.holdouts[0] if self.config_with_holdouts.holdouts else None + self.assertIsNotNone(holdout) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + self.assertIsNotNone(result) + self.assertIsInstance(result, list) + self.assertGreater(len(result), 0) + + # Verify result structure is valid + for decision_result in result: + self.assertIn('decision', decision_result) + self.assertIn('reasons', decision_result) + + def test_holdout_inactive_does_not_bucket_users(self): + """When holdout is inactive, should not bucket users and log appropriate message.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + holdout = self.config_with_holdouts.holdouts[0] if self.config_with_holdouts.holdouts else None + self.assertIsNotNone(holdout) + + # Mock holdout as inactive + original_status = holdout['status'] + holdout['status'] = 'Paused' + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + result = self.decision_service_with_holdouts.get_decision_for_flag( + feature_flag, + user_context, + self.config_with_holdouts + ) + + # Assert that result is not nil and has expected structure + self.assertIsNotNone(result) + self.assertIn('decision', result) + self.assertIn('reasons', result) + + # Restore original status + holdout['status'] = original_status + + def test_user_not_bucketed_into_holdout_executes_successfully(self): + """When user is not bucketed into holdout, should execute successfully with valid result structure.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + # With real bucketer, we can't guarantee specific bucketing results + # but we can verify the method executes successfully + self.assertIsNotNone(result) + self.assertIsInstance(result, list) + + def test_holdout_with_user_attributes_for_audience_targeting(self): + """Should evaluate holdout with user attributes.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_attributes = { + 'browser': 'chrome', + 'location': 'us' + } + + user_context = self.opt_obj.create_user_context('testUserId', user_attributes) + + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + self.assertIsNotNone(result) + self.assertIsInstance(result, list) + + def test_multiple_holdouts_for_single_feature_flag(self): + """Should handle multiple holdouts for a single feature flag.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + self.assertIsNotNone(result) + self.assertIsInstance(result, list) + + def test_holdout_bucketing_with_empty_user_id(self): + """Should allow holdout bucketing with empty user ID.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + # Empty user ID should still be valid for bucketing + user_context = self.opt_obj.create_user_context('', {}) + + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + self.assertIsNotNone(result) + + def test_holdout_populates_decision_reasons(self): + """Should populate decision reasons for holdouts.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + self.assertIsNotNone(result) + + # Find any decision with reasons + decision_with_reasons = next( + (dr for dr in result if dr.get('reasons') and len(dr['reasons']) > 0), + None + ) + + if decision_with_reasons: + self.assertGreater(len(decision_with_reasons['reasons']), 0) + + # get_variation_for_feature with holdouts tests + + def test_user_bucketed_into_holdout_returns_before_experiments(self): + """ + When user is bucketed into holdout, should return holdout decision + before checking experiments or rollouts. + """ + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + decision_result = self.decision_service_with_holdouts.get_variation_for_feature( + self.config_with_holdouts, + feature_flag, + user_context + ) + + self.assertIsNotNone(decision_result) + + # Decision should be valid + if decision_result.get('decision'): + decision = decision_result['decision'] + self.assertIsNotNone(decision.experiment) + self.assertIsNotNone(decision.source) + + def test_no_holdout_decision_falls_through_to_experiment_and_rollout(self): + """When holdout returns no decision, should fall through to experiment and rollout evaluation.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + # Use a user ID that won't be bucketed into holdout + user_context = self.opt_obj.create_user_context('non_holdout_user', {}) + + decision_result = self.decision_service_with_holdouts.get_variation_for_feature( + self.config_with_holdouts, + feature_flag, + user_context + ) + + # Should still get a valid decision result + self.assertIsNotNone(decision_result) + self.assertIn('decision', decision_result) + self.assertIn('reasons', decision_result) + + def test_holdout_respects_decision_options(self): + """Should respect decision options when evaluating holdouts.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + # Test with INCLUDE_REASONS option + decision_result = self.decision_service_with_holdouts.get_variation_for_feature( + self.config_with_holdouts, + feature_flag, + user_context, + [OptimizelyDecideOption.INCLUDE_REASONS] + ) + + self.assertIsNotNone(decision_result) + self.assertIsInstance(decision_result.get('reasons'), list) + + # Holdout priority and evaluation order tests + + def test_evaluates_holdouts_before_experiments(self): + """Should evaluate holdouts before experiments.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + decision_result = self.decision_service_with_holdouts.get_variation_for_feature( + self.config_with_holdouts, + feature_flag, + user_context + ) + + self.assertIsNotNone(decision_result) + + def test_evaluates_global_holdouts_for_all_flags(self): + """Should evaluate global holdouts for all flags.""" + 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.get('includedFlags') or len(h.get('includedFlags', [])) == 0 + ] + + if global_holdouts: + user_context = self.opt_obj.create_user_context('testUserId', {}) + + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + self.assertIsNotNone(result) + self.assertIsInstance(result, list) + + def test_respects_included_and_excluded_flags_configuration(self): + """Should respect included and excluded flags configuration.""" + 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') + + # Should not include holdouts that exclude this flag + excluded_holdout = next((h for h in holdouts_for_flag if h.get('key') == 'excluded_holdout'), None) + self.assertIsNone(excluded_holdout) + + # Holdout logging and error handling tests + + def test_logs_when_holdout_evaluation_starts(self): + """Should log when holdout evaluation starts.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + # Verify that logger was called + self.assertGreater(self.spy_logger.debug.call_count + self.spy_logger.info.call_count, 0) + + def test_handles_missing_holdout_configuration_gracefully(self): + """Should handle missing holdout configuration gracefully.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + # Temporarily remove holdouts + original_holdouts = self.config_with_holdouts.holdouts + self.config_with_holdouts.holdouts = [] + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + self.assertIsNotNone(result) + + # Restore original holdouts + self.config_with_holdouts.holdouts = original_holdouts + + def test_handles_invalid_holdout_data_gracefully(self): + """Should handle invalid holdout data gracefully.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + # The method should handle invalid holdout data without crashing + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + self.assertIsNotNone(result) + self.assertIsInstance(result, list) + + # Holdout bucketing behavior tests + + def test_uses_consistent_bucketing_for_same_user(self): + """Should use consistent bucketing for the same user.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_id = 'consistent_user' + user_context1 = self.opt_obj.create_user_context(user_id, {}) + user_context2 = self.opt_obj.create_user_context(user_id, {}) + + result1 = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context1, + [] + ) + + result2 = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context2, + [] + ) + + # Same user should get consistent results + self.assertIsNotNone(result1) + self.assertIsNotNone(result2) + + if result1 and result2: + decision1 = result1[0].get('decision') + decision2 = result2[0].get('decision') + + # If both have decisions, they should match + if decision1 and decision2: + # Variation is an object, not a dict, so use attributes + var1_id = decision1.variation.id if decision1.variation else None + var2_id = decision2.variation.id if decision2.variation else None + + self.assertEqual( + var1_id, var2_id, + "User should get consistent variation across multiple calls" + ) + + def test_uses_bucketing_id_when_provided(self): + """Should use bucketing ID when provided.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_attributes = { + enums.ControlAttributes.BUCKETING_ID: 'custom_bucketing_id' + } + + user_context = self.opt_obj.create_user_context('testUserId', user_attributes) + + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + + self.assertIsNotNone(result) + self.assertIsInstance(result, list) + + def test_handles_different_traffic_allocations(self): + """Should handle different traffic allocations.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + # Test with multiple users to see varying bucketing results + users = ['user1', 'user2', 'user3', 'user4', 'user5'] + results = [] + + for user_id in users: + user_context = self.opt_obj.create_user_context(user_id, {}) + result = self.decision_service_with_holdouts.get_variations_for_feature_list( + self.config_with_holdouts, + [feature_flag], + user_context, + [] + ) + results.append(result) + + # All results should be valid + for result in results: + self.assertIsNotNone(result) + self.assertIsInstance(result, list) + + # Holdout integration with feature experiments tests + + def test_checks_holdouts_before_feature_experiments(self): + """Should check holdouts before feature experiments.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('testUserId', {}) + + decision_result = self.decision_service_with_holdouts.get_variation_for_feature( + self.config_with_holdouts, + feature_flag, + user_context + ) + + self.assertIsNotNone(decision_result) + + def test_falls_back_to_experiments_if_no_holdout_decision(self): + """Should fall back to experiments if no holdout decision.""" + feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + + user_context = self.opt_obj.create_user_context('non_holdout_user_123', {}) + + decision_result = self.decision_service_with_holdouts.get_variation_for_feature( + self.config_with_holdouts, + feature_flag, + user_context + ) + + # Should return a valid decision result + self.assertIsNotNone(decision_result) + self.assertIn('decision', decision_result) + self.assertIn('reasons', decision_result)