diff --git a/lib/optimizely/audience.rb b/lib/optimizely/audience.rb index 6ea523e4..77e5d179 100644 --- a/lib/optimizely/audience.rb +++ b/lib/optimizely/audience.rb @@ -49,7 +49,7 @@ def user_meets_audience_conditions?(config, experiment, user_context, logger, lo logger.log(Logger::DEBUG, message) # Return true if there are no audiences - if audience_conditions.empty? + if audience_conditions.nil? || audience_conditions.empty? message = format(logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'], logging_key, 'TRUE') logger.log(Logger::INFO, message) decide_reasons.push(message) diff --git a/lib/optimizely/bucketer.rb b/lib/optimizely/bucketer.rb index d62e088d..4943d38c 100644 --- a/lib/optimizely/bucketer.rb +++ b/lib/optimizely/bucketer.rb @@ -45,6 +45,12 @@ def bucket(project_config, experiment, bucketing_id, user_id) # # Returns variation in which visitor with ID user_id has been placed. Nil if no variation. + if experiment.nil? || experiment['key'].to_s.strip.empty? + message = 'Invalid entity key provided for bucketing. Returning nil.' + @logger.log(Logger::DEBUG, message) + return nil, [] + end + variation_id, decide_reasons = bucket_to_entity_id(project_config, experiment, bucketing_id, user_id) if variation_id && variation_id != '' experiment_id = experiment['id'] diff --git a/lib/optimizely/config/datafile_project_config.rb b/lib/optimizely/config/datafile_project_config.rb index dfbe7522..3b872443 100644 --- a/lib/optimizely/config/datafile_project_config.rb +++ b/lib/optimizely/config/datafile_project_config.rb @@ -194,6 +194,41 @@ def initialize(datafile, logger, error_handler) feature_flag['experimentIds'].each do |experiment_id| @experiment_feature_map[experiment_id] = [feature_flag['id']] end + + flag_id = feature_flag['id'] + applicable_holdouts = [] + + applicable_holdouts.concat(@included_holdouts[flag_id]) if @included_holdouts[flag_id] + + @global_holdouts.each_value do |holdout| + excluded_flag_ids = holdout['excludedFlags'] || [] + applicable_holdouts << holdout unless excluded_flag_ids.include?(flag_id) + end + + @flag_holdouts_map[key] = applicable_holdouts unless applicable_holdouts.empty? + end + + # Adding Holdout variations in variation id and key maps + return unless @holdouts && !@holdouts.empty? + + @holdouts.each do |holdout| + holdout_key = holdout['key'] + holdout_id = holdout['id'] + + @variation_key_map[holdout_key] = {} + @variation_id_map[holdout_key] = {} + @variation_id_map_by_experiment_id[holdout_id] = {} + @variation_key_map_by_experiment_id[holdout_id] = {} + + variations = holdout['variations'] + next unless variations && !variations.empty? + + variations.each do |variation| + @variation_key_map[holdout_key][variation['key']] = variation + @variation_id_map[holdout_key][variation['id']] = variation + @variation_key_map_by_experiment_id[holdout_id][variation['key']] = variation + @variation_id_map_by_experiment_id[holdout_id][variation['id']] = variation + end end end @@ -605,38 +640,9 @@ def get_holdouts_for_flag(flag_key) # # Returns the holdouts that apply for a specific flag - feature_flag = @feature_flag_key_map[flag_key] - return [] unless feature_flag - - flag_id = feature_flag['id'] - - # Check catch first - return @flag_holdouts_map[flag_id] if @flag_holdouts_map.key?(flag_id) - - holdouts = [] - - # Add global holdouts that don't exclude this flag - @global_holdouts.each_value do |holdout| - is_excluded = false - excluded_flags = holdout['excludedFlags'] - if excluded_flags && !excluded_flags.empty? - excluded_flags.each do |excluded_flag_id| - if excluded_flag_id == flag_id - is_excluded = true - break - end - end - end - holdouts << holdout unless is_excluded - end - - # Add holdouts that specifically include this flag - holdouts.concat(@included_holdouts[flag_id]) if @included_holdouts.key?(flag_id) - - # Cache the result - @flag_holdouts_map[flag_id] = holdouts + return [] if @holdouts.nil? || @holdouts.empty? - holdouts + @flag_holdouts_map[flag_key] || [] end def get_holdout(holdout_id) diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index a97bf4d6..1b39357b 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -46,7 +46,8 @@ class DecisionService DECISION_SOURCES = { 'EXPERIMENT' => 'experiment', 'FEATURE_TEST' => 'feature-test', - 'ROLLOUT' => 'rollout' + 'ROLLOUT' => 'rollout', + 'HOLDOUT' => 'holdout' }.freeze def initialize(logger, cmab_service, user_profile_service = nil) @@ -166,7 +167,127 @@ def get_variation_for_feature(project_config, feature_flag, user_context, decide # user_context - Optimizely user context instance # # Returns DecisionResult struct. - get_variations_for_feature_list(project_config, [feature_flag], user_context, decide_options).first + holdouts = project_config.get_holdouts_for_flag(feature_flag['key']) + + if holdouts && !holdouts.empty? + # Has holdouts - use get_decision_for_flag which checks holdouts first + get_decision_for_flag(feature_flag, user_context, project_config, decide_options) + else + get_variations_for_feature_list(project_config, [feature_flag], user_context, decide_options).first + end + end + + def get_decision_for_flag(feature_flag, user_context, project_config, decide_options = [], user_profile_tracker = nil, decide_reasons = nil) + # Get the decision for a single feature flag. + # Processes holdouts, experiments, and rollouts in that order. + # + # feature_flag - The feature flag to get a decision for + # user_context - The user context + # project_config - The project config + # decide_options - Array of decide options + # user_profile_tracker - The user profile tracker + # decide_reasons - Array of decision reasons to merge + # + # Returns a DecisionResult for the feature flag + + reasons = decide_reasons ? decide_reasons.dup : [] + user_id = user_context.user_id + + # Check holdouts + holdouts = project_config.get_holdouts_for_flag(feature_flag['key']) + holdouts.each do |holdout| + holdout_decision = get_variation_for_holdout(holdout, user_context, project_config) + reasons.push(*holdout_decision.reasons) + + next unless holdout_decision.decision + + message = "The user '#{user_id}' is bucketed into holdout '#{holdout['key']}' for feature flag '#{feature_flag['key']}'." + @logger.log(Logger::INFO, message) + reasons.push(message) + return DecisionResult.new(holdout_decision.decision, false, reasons) + end + + # Check if the feature flag has an experiment and the user is bucketed into that experiment + experiment_decision = get_variation_for_feature_experiment(project_config, feature_flag, user_context, user_profile_tracker, decide_options) + reasons.push(*experiment_decision.reasons) + + return DecisionResult.new(experiment_decision.decision, experiment_decision.error, reasons) if experiment_decision.decision + + # Check if the feature flag has a rollout and the user is bucketed into that rollout + rollout_decision = get_variation_for_feature_rollout(project_config, feature_flag, user_context) + reasons.push(*rollout_decision.reasons) + + if rollout_decision.decision + # Check if this was a forced decision (last reason contains "forced decision map") + is_forced_decision = reasons.last&.include?('forced decision map') + + unless is_forced_decision + # Only add the "bucketed into rollout" message for normal bucketing + message = "The user '#{user_id}' is bucketed into a rollout for feature flag '#{feature_flag['key']}'." + @logger.log(Logger::INFO, message) + reasons.push(message) + end + + DecisionResult.new(rollout_decision.decision, rollout_decision.error, reasons) + else + message = "The user '#{user_id}' is not bucketed into a rollout for feature flag '#{feature_flag['key']}'." + @logger.log(Logger::INFO, message) + DecisionResult.new(nil, false, reasons) + end + end + + def get_variation_for_holdout(holdout, user_context, project_config) + # Get the variation for holdout + # + # holdout - The holdout configuration + # user_context - The user context + # project_config - The project config + # + # Returns a DecisionResult for the holdout + + decide_reasons = [] + user_id = user_context.user_id + attributes = user_context.user_attributes + + if holdout.nil? || holdout['status'].nil? || holdout['status'] != 'Running' + key = holdout && holdout['key'] ? holdout['key'] : 'unknown' + message = "Holdout '#{key}' is not running." + @logger.log(Logger::INFO, message) + decide_reasons.push(message) + return DecisionResult.new(nil, false, decide_reasons) + end + + bucketing_id, bucketing_id_reasons = get_bucketing_id(user_id, attributes) + decide_reasons.push(*bucketing_id_reasons) + + # Check audience conditions + user_meets_audience_conditions, reasons_received = Audience.user_meets_audience_conditions?(project_config, holdout, user_context, @logger) + decide_reasons.push(*reasons_received) + + unless user_meets_audience_conditions + message = "User '#{user_id}' does not meet the conditions for holdout '#{holdout['key']}'." + @logger.log(Logger::DEBUG, message) + decide_reasons.push(message) + return DecisionResult.new(nil, false, decide_reasons) + end + + # Bucket user into holdout variation + variation, bucket_reasons = @bucketer.bucket(project_config, holdout, bucketing_id, user_id) + decide_reasons.push(*bucket_reasons) + + if variation + message = "The user '#{user_id}' is bucketed into variation '#{variation['key']}' of holdout '#{holdout['key']}'." + @logger.log(Logger::INFO, message) + decide_reasons.push(message) + + holdout_decision = Decision.new(holdout, variation, DECISION_SOURCES['HOLDOUT'], nil) + DecisionResult.new(holdout_decision, false, decide_reasons) + else + message = "The user '#{user_id}' is not bucketed into holdout '#{holdout['key']}'." + @logger.log(Logger::DEBUG, message) + decide_reasons.push(message) + DecisionResult.new(nil, false, decide_reasons) + end end def get_variations_for_feature_list(project_config, feature_flags, user_context, decide_options = []) @@ -183,9 +304,11 @@ def get_variations_for_feature_list(project_config, feature_flags, user_context, ignore_ups = decide_options.include? Optimizely::Decide::OptimizelyDecideOption::IGNORE_USER_PROFILE_SERVICE user_profile_tracker = nil unless ignore_ups && @user_profile_service - user_profile_tracker = UserProfileTracker.new(user_context.user_id, @user_profile_service, @logger) + user_id = user_context.user_id + user_profile_tracker = UserProfileTracker.new(user_id, @user_profile_service, @logger) user_profile_tracker.load_user_profile end + decisions = [] feature_flags.each do |feature_flag| # check if the feature is being experiment on and whether the user is bucketed into the experiment diff --git a/spec/bucketing_holdout_spec.rb b/spec/bucketing_holdout_spec.rb new file mode 100644 index 00000000..4ddf1f67 --- /dev/null +++ b/spec/bucketing_holdout_spec.rb @@ -0,0 +1,318 @@ +# frozen_string_literal: true + +# +# 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. +# +require 'spec_helper' +require 'optimizely/bucketer' +require 'optimizely/error_handler' +require 'optimizely/logger' + +# Helper class for testing with controlled bucket values +class TestBucketer < Optimizely::Bucketer + def initialize(logger) + super(logger) + @bucket_values = [] + @bucket_index = 0 + end + + def bucket_values(values) + @bucket_values = values + @bucket_index = 0 + end + + def generate_bucket_value(bucketing_id) + return super(bucketing_id) if @bucket_values.empty? + + value = @bucket_values[@bucket_index] + @bucket_index = (@bucket_index + 1) % @bucket_values.length + value + end +end + +describe 'Optimizely::Bucketer - Holdout Tests' do + let(:config_body) { OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS } + let(:config_body_JSON) { OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_JSON } + let(:error_handler) { Optimizely::NoOpErrorHandler.new } + let(:spy_logger) { spy('logger') } + let(:test_user_id) { 'test_user_id' } + let(:test_bucketing_id) { 'test_bucketing_id' } + let(:config) do + Optimizely::DatafileProjectConfig.new( + OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_JSON, + spy_logger, + error_handler + ) + end + let(:test_bucketer) { TestBucketer.new(spy_logger) } + + before do + # Verify that the config contains holdouts + expect(config.holdouts).not_to be_nil + expect(config.holdouts.length).to be > 0 + end + + describe '#bucket with holdouts' do + it 'should bucket user within valid traffic allocation range' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Set bucket value to be within first variation's traffic allocation (0-5000 range) + test_bucketer.bucket_values([2500]) + + variation, _reasons = test_bucketer.bucket(config, holdout, test_bucketing_id, test_user_id) + + expect(variation).not_to be_nil + expect(variation['id']).to eq('var_1') + expect(variation['key']).to eq('control') + + # Verify logging + expect(spy_logger).to have_received(:log).with( + Logger::DEBUG, + "Assigned bucket 2500 to user '#{test_user_id}' with bucketing ID: '#{test_bucketing_id}'." + ) + end + + it 'should return nil when user is outside traffic allocation range' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Modify traffic allocation to be smaller by creating a modified holdout + modified_holdout = OptimizelySpec.deep_clone(holdout) + modified_holdout['trafficAllocation'] = [ + { + 'entityId' => 'var_1', + 'endOfRange' => 1000 + } + ] + + # Set bucket value outside traffic allocation range + test_bucketer.bucket_values([1500]) + + variation, _reasons = test_bucketer.bucket(config, modified_holdout, test_bucketing_id, test_user_id) + + expect(variation).to be_nil + + # Verify user was assigned bucket value but no variation was found + expect(spy_logger).to have_received(:log).with( + Logger::DEBUG, + "Assigned bucket 1500 to user '#{test_user_id}' with bucketing ID: '#{test_bucketing_id}'." + ) + end + + it 'should return nil when holdout has no traffic allocation' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Clear traffic allocation + modified_holdout = OptimizelySpec.deep_clone(holdout) + modified_holdout['trafficAllocation'] = [] + + test_bucketer.bucket_values([5000]) + + variation, _reasons = test_bucketer.bucket(config, modified_holdout, test_bucketing_id, test_user_id) + + expect(variation).to be_nil + + # Verify bucket was assigned + expect(spy_logger).to have_received(:log).with( + Logger::DEBUG, + "Assigned bucket 5000 to user '#{test_user_id}' with bucketing ID: '#{test_bucketing_id}'." + ) + end + + it 'should return nil when traffic allocation points to invalid variation ID' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Set traffic allocation to point to non-existent variation + modified_holdout = OptimizelySpec.deep_clone(holdout) + modified_holdout['trafficAllocation'] = [ + { + 'entityId' => 'invalid_variation_id', + 'endOfRange' => 10_000 + } + ] + + test_bucketer.bucket_values([5000]) + + variation, _reasons = test_bucketer.bucket(config, modified_holdout, test_bucketing_id, test_user_id) + + expect(variation).to be_nil + + # Verify bucket was assigned + expect(spy_logger).to have_received(:log).with( + Logger::DEBUG, + "Assigned bucket 5000 to user '#{test_user_id}' with bucketing ID: '#{test_bucketing_id}'." + ) + end + + it 'should return nil when holdout has no variations' do + holdout = config.get_holdout('holdout_empty_1') + expect(holdout).not_to be_nil + expect(holdout['variations']&.length || 0).to eq(0) + + test_bucketer.bucket_values([5000]) + + variation, _reasons = test_bucketer.bucket(config, holdout, test_bucketing_id, test_user_id) + + expect(variation).to be_nil + + # Verify bucket was assigned + expect(spy_logger).to have_received(:log).with( + Logger::DEBUG, + "Assigned bucket 5000 to user '#{test_user_id}' with bucketing ID: '#{test_bucketing_id}'." + ) + end + + it 'should return nil when holdout has empty key' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Clear holdout key + modified_holdout = OptimizelySpec.deep_clone(holdout) + modified_holdout['key'] = '' + + test_bucketer.bucket_values([5000]) + + variation, _reasons = test_bucketer.bucket(config, modified_holdout, test_bucketing_id, test_user_id) + + # Should return nil for invalid experiment key + expect(variation).to be_nil + end + + it 'should return nil when holdout has null key' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Set holdout key to nil + modified_holdout = OptimizelySpec.deep_clone(holdout) + modified_holdout['key'] = nil + + test_bucketer.bucket_values([5000]) + + variation, _reasons = test_bucketer.bucket(config, modified_holdout, test_bucketing_id, test_user_id) + + # Should return nil for null experiment key + expect(variation).to be_nil + end + + it 'should bucket user into first variation with multiple variations' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Verify holdout has multiple variations + expect(holdout['variations'].length).to be >= 2 + + # Test user buckets into first variation + test_bucketer.bucket_values([2500]) + variation, _reasons = test_bucketer.bucket(config, holdout, test_bucketing_id, test_user_id) + + expect(variation).not_to be_nil + expect(variation['id']).to eq('var_1') + expect(variation['key']).to eq('control') + end + + it 'should bucket user into second variation with multiple variations' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Verify holdout has multiple variations + expect(holdout['variations'].length).to be >= 2 + expect(holdout['variations'][0]['id']).to eq('var_1') + expect(holdout['variations'][1]['id']).to eq('var_2') + + # Test user buckets into second variation (bucket value 7500 should be in 5000-10000 range) + test_bucketer.bucket_values([7500]) + variation, _reasons = test_bucketer.bucket(config, holdout, test_bucketing_id, test_user_id) + + expect(variation).not_to be_nil + expect(variation['id']).to eq('var_2') + expect(variation['key']).to eq('treatment') + end + + it 'should handle edge case boundary values correctly' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Modify traffic allocation to be 5000 (50%) + modified_holdout = OptimizelySpec.deep_clone(holdout) + modified_holdout['trafficAllocation'] = [ + { + 'entityId' => 'var_1', + 'endOfRange' => 5000 + } + ] + + # Test exact boundary value (should be included) + test_bucketer.bucket_values([4999]) + variation, _reasons = test_bucketer.bucket(config, modified_holdout, test_bucketing_id, test_user_id) + + expect(variation).not_to be_nil + expect(variation['id']).to eq('var_1') + + # Test value just outside boundary (should not be included) + test_bucketer.bucket_values([5000]) + variation, _reasons = test_bucketer.bucket(config, modified_holdout, test_bucketing_id, test_user_id) + + expect(variation).to be_nil + end + + it 'should produce consistent results with same inputs' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Create a real bucketer (not test bucketer) for consistent hashing + real_bucketer = Optimizely::Bucketer.new(spy_logger) + variation1, _reasons1 = real_bucketer.bucket(config, holdout, test_bucketing_id, test_user_id) + variation2, _reasons2 = real_bucketer.bucket(config, holdout, test_bucketing_id, test_user_id) + + # Results should be identical + if variation1 + expect(variation2).not_to be_nil + expect(variation1['id']).to eq(variation2['id']) + expect(variation1['key']).to eq(variation2['key']) + else + expect(variation2).to be_nil + end + end + + it 'should handle different bucketing IDs without exceptions' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + # Create a real bucketer (not test bucketer) for real hashing behavior + real_bucketer = Optimizely::Bucketer.new(spy_logger) + + # These calls should not raise exceptions + expect do + real_bucketer.bucket(config, holdout, 'bucketingId1', test_user_id) + real_bucketer.bucket(config, holdout, 'bucketingId2', test_user_id) + end.not_to raise_error + end + + it 'should populate decision reasons properly' do + holdout = config.get_holdout('holdout_1') + expect(holdout).not_to be_nil + + test_bucketer.bucket_values([5000]) + _variation, reasons = test_bucketer.bucket(config, holdout, test_bucketing_id, test_user_id) + + expect(reasons).not_to be_nil + # Decision reasons should be populated from the bucketing process + # The exact content depends on whether the user was bucketed or not + end + end +end diff --git a/spec/config/datafile_project_config_spec.rb b/spec/config/datafile_project_config_spec.rb index d30bb47f..84dd4509 100644 --- a/spec/config/datafile_project_config_spec.rb +++ b/spec/config/datafile_project_config_spec.rb @@ -1251,7 +1251,7 @@ it 'should return global holdouts that do not exclude the flag' do holdouts = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature') - expect(holdouts.length).to eq(2) + expect(holdouts.length).to eq(3) global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' } expect(global_holdout).not_to be_nil @@ -1264,7 +1264,7 @@ it 'should not return global holdouts that exclude the flag' do holdouts = config_with_holdouts.get_holdouts_for_flag('boolean_single_variable_feature') - expect(holdouts.length).to eq(0) + expect(holdouts.length).to eq(1) global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' } expect(global_holdout).to be_nil @@ -1274,14 +1274,14 @@ holdouts1 = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature') holdouts2 = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature') expect(holdouts1).to equal(holdouts2) - expect(holdouts1.length).to eq(2) + expect(holdouts1.length).to eq(3) end it 'should return only global holdouts for flags not specifically targeted' do holdouts = config_with_holdouts.get_holdouts_for_flag('string_single_variable_feature') # Should only include global holdout (not excluded and no specific targeting) - expect(holdouts.length).to eq(1) + expect(holdouts.length).to eq(2) expect(holdouts.first['key']).to eq('global_holdout') end end @@ -1423,4 +1423,376 @@ expect(included_for_boolean).to be_nil end end + + describe 'Holdout Decision Functionality' do + let(:config_with_holdouts) do + Optimizely::DatafileProjectConfig.new( + OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_JSON, + logger, + error_handler + ) + end + + describe '#decide with global holdout' do + it 'should return valid decision for global holdout' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + # Verify holdouts are loaded + expect(config_with_holdouts.holdouts).not_to be_nil + expect(config_with_holdouts.holdouts.length).to be > 0 + end + + it 'should handle decision with global holdout configuration' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + expect(feature_flag['id']).not_to be_empty + end + end + + describe '#decide with included flags holdout' do + it 'should return valid decision for included flags' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + # Check if there's a holdout that includes this flag + included_holdout = config_with_holdouts.holdouts.find do |h| + h['includedFlags']&.include?(feature_flag['id']) + end + + if included_holdout + expect(included_holdout['key']).not_to be_empty + expect(included_holdout['status']).to eq('Running') + end + end + + it 'should properly filter holdouts based on includedFlags' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag('boolean_feature') + expect(holdouts_for_flag).to be_an(Array) + end + end + + describe '#decide with excluded flags holdout' do + it 'should not return excluded holdout for excluded flag' do + # boolean_feature is excluded by holdout_excluded_1 + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + + if feature_flag + holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag('boolean_feature') + + # Should not include holdouts that exclude this flag + excluded_holdout = holdouts_for_flag.find { |h| h['key'] == 'excluded_holdout' } + expect(excluded_holdout).to be_nil + end + end + + it 'should return holdouts for non-excluded flag' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag('boolean_feature') + expect(holdouts_for_flag).to be_an(Array) + end + end + + describe '#decide with multiple holdouts' do + it 'should handle multiple holdouts for different flags' do + flag_keys = %w[boolean_feature multi_variate_feature string_single_variable_feature empty_feature] + + flag_keys.each do |flag_key| + feature_flag = config_with_holdouts.feature_flag_key_map[flag_key] + next unless feature_flag + + holdouts = config_with_holdouts.get_holdouts_for_flag(flag_key) + expect(holdouts).to be_an(Array) + + # Each holdout should have proper structure + holdouts.each do |holdout| + expect(holdout).to have_key('id') + expect(holdout).to have_key('key') + expect(holdout).to have_key('status') + end + end + end + + it 'should properly cache holdout lookups' do + holdouts_1 = config_with_holdouts.get_holdouts_for_flag('boolean_feature') + holdouts_2 = config_with_holdouts.get_holdouts_for_flag('boolean_feature') + + expect(holdouts_1).to equal(holdouts_2) + end + end + + describe '#decide with inactive holdout' do + it 'should not include inactive holdouts in decision process' do + # Find a holdout and verify status handling + holdout = config_with_holdouts.holdouts.first + + if holdout + # Temporarily modify status to test behavior + original_status = holdout['status'] + holdout['status'] = 'Paused' + + # Recreate config to process the modified holdout + modified_config_body = OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS.dup + modified_config_body['holdouts'] = config_with_holdouts.holdouts.map(&:dup) + modified_config_body['holdouts'].first['status'] = 'Paused' + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + logger, + error_handler + ) + + # Should not be in active holdouts map + expect(modified_config.holdout_id_map[holdout['id']]).to be_nil + + # Restore original status + holdout['status'] = original_status + end + end + + it 'should only process running holdouts' do + running_holdouts = config_with_holdouts.holdouts.select { |h| h['status'] == 'Running' } + + running_holdouts.each do |holdout| + expect(config_with_holdouts.holdout_id_map[holdout['id']]).not_to be_nil + end + end + end + + describe '#decide with empty user id' do + it 'should handle empty user id without error' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + # Empty user ID should be valid for bucketing + # This test verifies the config structure supports this + expect(feature_flag['key']).to eq('boolean_feature') + end + end + + describe '#holdout priority evaluation' do + it 'should evaluate global holdouts for flags without specific targeting' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + global_holdouts = config_with_holdouts.holdouts.select do |h| + h['includedFlags'].nil? || h['includedFlags'].empty? + end + + included_holdouts = config_with_holdouts.holdouts.select do |h| + h['includedFlags']&.include?(feature_flag['id']) + end + + # Should have either global or included holdouts + expect(global_holdouts.length + included_holdouts.length).to be >= 0 + end + + it 'should handle mixed holdout configurations' do + # Verify the config has properly categorized holdouts + expect(config_with_holdouts.global_holdouts).to be_a(Hash) + expect(config_with_holdouts.included_holdouts).to be_a(Hash) + expect(config_with_holdouts.excluded_holdouts).to be_a(Hash) + end + end + end + + describe 'Holdout Decision Reasons' do + let(:config_with_holdouts) do + Optimizely::DatafileProjectConfig.new( + OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_JSON, + logger, + error_handler + ) + end + + describe 'decision reasons structure' do + it 'should support decision reasons for holdout decisions' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + # Verify the feature flag has proper structure for decision reasons + expect(feature_flag).to have_key('id') + expect(feature_flag).to have_key('key') + end + + it 'should include holdout information in config' do + expect(config_with_holdouts.holdouts).not_to be_empty + + config_with_holdouts.holdouts.each do |holdout| + expect(holdout).to have_key('id') + expect(holdout).to have_key('key') + expect(holdout).to have_key('status') + end + end + end + + describe 'holdout bucketing messages' do + it 'should have holdout configuration for bucketing decisions' do + holdout = config_with_holdouts.holdouts.first + + if holdout + expect(holdout['status']).to eq('Running').or eq('Inactive') + expect(holdout).to have_key('id') + expect(holdout).to have_key('key') + end + end + + it 'should support audience evaluation for holdouts' do + holdout = config_with_holdouts.holdouts.first + + if holdout + # Holdouts may or may not have audiences - both are valid + expect(holdout).to have_key('id') + expect(holdout).to have_key('key') + expect(holdout).to have_key('status') + end + end + end + + describe 'holdout status messages' do + it 'should differentiate between running and non-running holdouts' do + running_holdouts = config_with_holdouts.holdouts.select { |h| h['status'] == 'Running' } + non_running_holdouts = config_with_holdouts.holdouts.reject { |h| h['status'] == 'Running' } + + # Only running holdouts should be in the holdout_id_map + running_holdouts.each do |holdout| + expect(config_with_holdouts.holdout_id_map[holdout['id']]).not_to be_nil + end + + non_running_holdouts.each do |holdout| + expect(config_with_holdouts.holdout_id_map[holdout['id']]).to be_nil + end + end + end + + describe 'audience condition evaluation' do + it 'should support audience conditions in holdouts' do + holdout = config_with_holdouts.holdouts.first + + if holdout + expect(holdout).to have_key('id') + expect(holdout).to have_key('key') + expect(holdout.key?('audienceIds') || holdout.key?('audiences')).to be true + end + end + + it 'should handle holdouts with empty audience conditions' do + # Empty audience conditions should evaluate to TRUE (match everyone) + holdouts_with_empty_audiences = config_with_holdouts.holdouts.select do |h| + !h.key?('audiences') || h['audiences'].nil? || h['audiences'].empty? + end + + # These holdouts should match all users + holdouts_with_empty_audiences.each do |holdout| + expect(holdout['status']).to eq('Running').or eq('Inactive') + end + end + end + + describe 'holdout evaluation reasoning' do + it 'should provide holdout configuration for evaluation' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag('boolean_feature') + + holdouts_for_flag.each do |holdout| + # Each holdout should have necessary info for decision reasoning + expect(holdout['id']).not_to be_empty + expect(holdout['key']).not_to be_empty + expect(holdout['status']).to eq('Running') + end + end + + it 'should support relevant holdout decision information' do + holdout = config_with_holdouts.holdouts.first + + if holdout + # Verify holdout has all necessary fields for decision reasoning + expect(holdout).to have_key('id') + expect(holdout).to have_key('key') + expect(holdout).to have_key('status') + end + end + end + end + + describe 'Holdout Edge Cases' do + let(:config_with_holdouts) do + config_body_with_holdouts = config_body.dup + config_body_with_holdouts['holdouts'] = [ + { + 'id' => 'holdout_1', + 'key' => 'test_holdout', + 'status' => 'Running', + 'audiences' => [], + 'includedFlags' => [], + 'excludedFlags' => [] + }, + { + 'id' => 'holdout_2', + 'key' => 'paused_holdout', + 'status' => 'Paused', + 'audiences' => [], + 'includedFlags' => [], + 'excludedFlags' => [] + } + ] + config_json = JSON.dump(config_body_with_holdouts) + Optimizely::DatafileProjectConfig.new(config_json, logger, error_handler) + end + + it 'should handle datafile without holdouts' do + config_without_holdouts = Optimizely::DatafileProjectConfig.new( + config_body_JSON, + logger, + error_handler + ) + + holdouts_for_flag = config_without_holdouts.get_holdouts_for_flag('boolean_feature') + expect(holdouts_for_flag).to eq([]) + end + + it 'should handle holdouts with nil included/excluded flags' do + config_body_with_nil = config_body.dup + config_body_with_nil['holdouts'] = [ + { + 'id' => 'holdout_nil', + 'key' => 'nil_holdout', + 'status' => 'Running', + 'audiences' => [], + 'includedFlags' => nil, + 'excludedFlags' => nil + } + ] + config_json = JSON.dump(config_body_with_nil) + config = Optimizely::DatafileProjectConfig.new(config_json, logger, error_handler) + + # Should treat as global holdout + expect(config.global_holdouts['holdout_nil']).not_to be_nil + end + + it 'should only include running holdouts in maps' do + running_count = config_with_holdouts.holdout_id_map.length + total_count = config_with_holdouts.holdouts.length + + # Only running holdouts should be in the map + expect(running_count).to be < total_count + expect(config_with_holdouts.holdout_id_map['holdout_1']).not_to be_nil + expect(config_with_holdouts.holdout_id_map['holdout_2']).to be_nil + end + + it 'should handle mixed status holdouts correctly' do + running_holdouts = config_with_holdouts.holdouts.select { |h| h['status'] == 'Running' } + + running_holdouts.each do |holdout| + expect(config_with_holdouts.get_holdout(holdout['id'])).not_to be_nil + end + end + end end diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index fe2cc881..30f52882 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -1167,4 +1167,560 @@ end end end + + describe 'Holdout Decision Service Tests' do + let(:config_with_holdouts) do + Optimizely::DatafileProjectConfig.new( + OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_JSON, + spy_logger, + error_handler + ) + end + + let(:project_with_holdouts) do + Optimizely::Project.new( + datafile: OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_JSON, + logger: spy_logger, + error_handler: error_handler + ) + end + + let(:decision_service_with_holdouts) do + Optimizely::DecisionService.new(spy_logger, spy_cmab_service, spy_user_profile_service) + end + + after(:example) do + project_with_holdouts&.close + end + + describe '#get_variations_for_feature_list with holdouts' do + describe 'when holdout is active and user is bucketed' do + it 'should return holdout decision with variation' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + holdout = config_with_holdouts.holdouts.first + expect(holdout).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + {} + ) + + expect(result).not_to be_nil + expect(result).to be_an(Array) + expect(result.length).to be > 0 + + # Check if any decision is from holdout source + _holdout_decision = result.find do |decision_result| + decision_result.decision&.source == Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'] + end + + # With real bucketer, we can't guarantee holdout bucketing + # but we can verify the result structure is valid + result.each do |decision_result| + expect(decision_result).to respond_to(:decision) + expect(decision_result).to respond_to(:reasons) + end + end + end + + describe 'when holdout is inactive' do + it 'should not bucket users and log appropriate message' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + holdout = config_with_holdouts.holdouts.first + expect(holdout).not_to be_nil + + # Mock holdout as inactive + original_status = holdout['status'] + holdout['status'] = 'Paused' + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + result = decision_service_with_holdouts.get_decision_for_flag( + feature_flag, + user_context, + config_with_holdouts + ) + + # Assert that result is not nil and has expected structure + expect(result).not_to be_nil + expect(result).to respond_to(:decision) + expect(result).to respond_to(:reasons) + + # Verify log message for inactive holdout + expect(spy_logger).to have_received(:log).with( + Logger::INFO, + a_string_matching(/Holdout.*is not running/) + ) + + # Restore original status + holdout['status'] = original_status + end + end + + describe 'when user is not bucketed into holdout' do + it 'should execute successfully with valid result structure' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + holdout = config_with_holdouts.holdouts.first + expect(holdout).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + result = decision_service_with_holdouts.get_variations_for_feature_list( + 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 + expect(result).not_to be_nil + expect(result).to be_an(Array) + end + end + + describe 'with user attributes for audience targeting' do + it 'should evaluate holdout with user attributes' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + holdout = config_with_holdouts.holdouts.first + expect(holdout).not_to be_nil + + user_attributes = { + 'browser' => 'chrome', + 'location' => 'us' + } + + user_context = project_with_holdouts.create_user_context('testUserId', user_attributes) + + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + user_attributes + ) + + expect(result).not_to be_nil + expect(result).to be_an(Array) + + # With real bucketer, we can't guarantee specific variations + # but can verify execution completes successfully + end + end + + describe 'with multiple holdouts' do + it 'should handle multiple holdouts for a single feature flag' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + {} + ) + + expect(result).not_to be_nil + expect(result).to be_an(Array) + + # With real bucketer, we can't guarantee specific bucketing results + # but we can verify the method executes successfully + end + end + + describe 'with empty user ID' do + it 'should allow holdout bucketing with empty user ID' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + # Empty user ID should still be valid for bucketing + user_context = project_with_holdouts.create_user_context('', {}) + + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + {} + ) + + expect(result).not_to be_nil + + # Empty user ID should not log error about invalid user ID + expect(spy_logger).not_to have_received(:log).with( + Logger::ERROR, + a_string_matching(/User ID.*(?:null|empty)/) + ) + end + end + + describe 'with decision reasons' do + it 'should populate decision reasons for holdouts' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + holdout = config_with_holdouts.holdouts.first + expect(holdout).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + {} + ) + + expect(result).not_to be_nil + + # With real bucketer, we expect proper decision reasons to be generated + # Find any decision with reasons + decision_with_reasons = result.find do |decision_result| + decision_result.reasons && !decision_result.reasons.empty? + end + + expect(decision_with_reasons.reasons).not_to be_empty if decision_with_reasons + end + end + end + + describe '#get_variation_for_feature with holdouts' do + describe 'when user is bucketed into holdout' do + it 'should return holdout decision before checking experiments or rollouts' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + # The get_variation_for_feature method should check holdouts first + decision_result = decision_service_with_holdouts.get_variation_for_feature( + config_with_holdouts, + feature_flag, + user_context + ) + + expect(decision_result).not_to be_nil + + # Decision should be valid (from holdout, experiment, or rollout) + if decision_result.decision + expect(decision_result.decision).to respond_to(:experiment) + expect(decision_result.decision).to respond_to(:variation) + expect(decision_result.decision).to respond_to(:source) + end + end + end + + describe 'when holdout returns no decision' do + it 'should fall through to experiment and rollout evaluation' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + # Use a user ID that won't be bucketed into holdout + user_context = project_with_holdouts.create_user_context('non_holdout_user', {}) + + decision_result = decision_service_with_holdouts.get_variation_for_feature( + config_with_holdouts, + feature_flag, + user_context + ) + + # Should still get a valid decision result (even if decision is nil) + expect(decision_result).not_to be_nil + expect(decision_result).to respond_to(:decision) + expect(decision_result).to respond_to(:reasons) + end + end + + describe 'with decision options' do + it 'should respect decision options when evaluating holdouts' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + # Test with INCLUDE_REASONS option + decision_result = decision_service_with_holdouts.get_variation_for_feature( + config_with_holdouts, + feature_flag, + user_context, + [Optimizely::Decide::OptimizelyDecideOption::INCLUDE_REASONS] + ) + + expect(decision_result).not_to be_nil + expect(decision_result.reasons).to be_an(Array) + end + end + end + + describe 'holdout priority and evaluation order' do + it 'should evaluate holdouts before experiments' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + # Mock the get_variation_for_feature_experiment to track if it's called + allow(decision_service_with_holdouts).to receive(:get_variation_for_feature_experiment) + .and_call_original + + decision_result = decision_service_with_holdouts.get_variation_for_feature( + config_with_holdouts, + feature_flag, + user_context + ) + + expect(decision_result).not_to be_nil + + # If user is bucketed into holdout, experiment evaluation should be skipped + # We can verify this through the decision source if a decision is made + if decision_result.decision && decision_result.decision.source == Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'] + # Holdout decision was made, so experiment evaluation should have been skipped + expect(decision_service_with_holdouts).not_to have_received(:get_variation_for_feature_experiment) + end + end + + it 'should evaluate global holdouts for all flags' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + # Get global holdouts + global_holdouts = config_with_holdouts.holdouts.select do |h| + h['includedFlags'].nil? || h['includedFlags'].empty? + end + + unless global_holdouts.empty? + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + {} + ) + + expect(result).not_to be_nil + expect(result).to be_an(Array) + end + end + + it 'should respect included and excluded flags configuration' do + # Test that flags in excludedFlags are not affected by that holdout + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + + if feature_flag + # Get holdouts for this flag + holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag('boolean_feature') + + # Should not include holdouts that exclude this flag + excluded_holdout = holdouts_for_flag.find { |h| h['key'] == 'excluded_holdout' } + expect(excluded_holdout).to be_nil + end + end + end + + describe 'holdout logging and error handling' do + it 'should log when holdout evaluation starts' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + {} + ) + + # Verify that appropriate log messages are generated + # (specific messages depend on implementation) + expect(spy_logger).to have_received(:log).at_least(:once) + end + + it 'should handle missing holdout configuration gracefully' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + # Temporarily remove holdouts + original_holdouts = config_with_holdouts.instance_variable_get(:@holdouts) + config_with_holdouts.instance_variable_set(:@holdouts, []) + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + {} + ) + + expect(result).not_to be_nil + + # Restore original holdouts + config_with_holdouts.instance_variable_set(:@holdouts, original_holdouts) + end + + it 'should handle invalid holdout data gracefully' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + # The method should handle invalid holdout data without crashing + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + {} + ) + + expect(result).not_to be_nil + expect(result).to be_an(Array) + end + end + + describe 'holdout bucketing behavior' do + it 'should use consistent bucketing for the same user' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_id = 'consistent_user' + user_context1 = project_with_holdouts.create_user_context(user_id, {}) + user_context2 = project_with_holdouts.create_user_context(user_id, {}) + + result1 = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context1, + {} + ) + + result2 = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context2, + {} + ) + + # Same user should get consistent results + expect(result1).not_to be_nil + expect(result2).not_to be_nil + + if !result1.empty? && !result2.empty? + # Compare the first decision from each result + decision1 = result1[0].decision + decision2 = result2[0].decision + + # If both have decisions, they should match + if decision1 && decision2 + expect(decision1.variation&.fetch('id', nil)).to eq(decision2.variation&.fetch('id', nil)) + expect(decision1.source).to eq(decision2.source) + end + end + end + + it 'should use bucketing ID when provided' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_attributes = { + Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'custom_bucketing_id' + } + + user_context = project_with_holdouts.create_user_context('testUserId', user_attributes) + + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + user_attributes + ) + + expect(result).not_to be_nil + expect(result).to be_an(Array) + + # Bucketing should work with custom bucketing ID + end + + it 'should handle different traffic allocations' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + # Test with multiple users to see varying bucketing results + users = %w[user1 user2 user3 user4 user5] + results = [] + + users.each do |user_id| + user_context = project_with_holdouts.create_user_context(user_id, {}) + result = decision_service_with_holdouts.get_variations_for_feature_list( + config_with_holdouts, + [feature_flag], + user_context, + {} + ) + results << result + end + + # All results should be valid + results.each do |result| + expect(result).not_to be_nil + expect(result).to be_an(Array) + end + end + end + + describe 'holdout integration with feature experiments' do + it 'should check holdouts before feature experiments' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_context = project_with_holdouts.create_user_context('testUserId', {}) + + # Mock feature experiment method to track calls + allow(decision_service_with_holdouts).to receive(:get_variation_for_feature_experiment) + .and_call_original + + decision_result = decision_service_with_holdouts.get_variation_for_feature( + config_with_holdouts, + feature_flag, + user_context + ) + + expect(decision_result).not_to be_nil + + # Holdout evaluation happens in get_variations_for_feature_list + # which is called before experiment evaluation + end + + it 'should fall back to experiments if no holdout decision' do + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + expect(feature_flag).not_to be_nil + + user_context = project_with_holdouts.create_user_context('non_holdout_user_123', {}) + + decision_result = decision_service_with_holdouts.get_variation_for_feature( + config_with_holdouts, + feature_flag, + user_context + ) + + # Should return a valid decision result + expect(decision_result).not_to be_nil + expect(decision_result).to respond_to(:decision) + expect(decision_result).to respond_to(:reasons) + end + end + end end diff --git a/spec/spec_params.rb b/spec/spec_params.rb index a906f288..90d31f27 100644 --- a/spec/spec_params.rb +++ b/spec/spec_params.rb @@ -1946,22 +1946,83 @@ module OptimizelySpec 'id' => 'holdout_1', 'key' => 'global_holdout', 'status' => 'Running', + 'audiences' => [], 'includedFlags' => [], - 'excludedFlags' => ['155554'] + 'excludedFlags' => ['155554'], + 'variations' => [ + { + 'id' => 'var_1', + 'key' => 'control', + 'featureEnabled' => true + }, + { + 'id' => 'var_2', + 'key' => 'treatment', + 'featureEnabled' => true + } + ], + 'trafficAllocation' => [ + { + 'entityId' => 'var_1', + 'endOfRange' => 5000 + }, + { + 'entityId' => 'var_2', + 'endOfRange' => 10_000 + } + ] + }, + { + 'id' => 'holdout_empty_1', + 'key' => 'holdout_empty_1', + 'status' => 'Running', + 'audiences' => [], + 'includedFlags' => [], + 'excludedFlags' => [], + 'variations' => [], + 'trafficAllocation' => [] }, { 'id' => 'holdout_2', 'key' => 'specific_holdout', 'status' => 'Running', + 'audiences' => [], 'includedFlags' => ['155559'], - 'excludedFlags' => [] + 'excludedFlags' => [], + 'variations' => [ + { + 'id' => 'var_3', + 'key' => 'control', + 'featureEnabled' => false + } + ], + 'trafficAllocation' => [ + { + 'entityId' => 'var_3', + 'endOfRange' => 10_000 + } + ] }, { 'id' => 'holdout_3', 'key' => 'inactive_holdout', 'status' => 'Inactive', + 'audiences' => [], 'includedFlags' => ['155554'], - 'excludedFlags' => [] + 'excludedFlags' => [], + 'variations' => [ + { + 'id' => 'var_4', + 'key' => 'off', + 'featureEnabled' => false + } + ], + 'trafficAllocation' => [ + { + 'entityId' => 'var_4', + 'endOfRange' => 10_000 + } + ] } ] }