diff --git a/lib/optimizely/config/datafile_project_config.rb b/lib/optimizely/config/datafile_project_config.rb index d6f1d27b..fdf3643d 100644 --- a/lib/optimizely/config/datafile_project_config.rb +++ b/lib/optimizely/config/datafile_project_config.rb @@ -115,7 +115,7 @@ def initialize(datafile, logger, error_handler) @variation_id_to_experiment_map = {} @flag_variation_map = {} @holdout_id_map = {} - @global_holdouts = {} + @global_holdouts = [] @included_holdouts = {} @excluded_holdouts = {} @flag_holdouts_map = {} @@ -126,7 +126,7 @@ def initialize(datafile, logger, error_handler) @holdout_id_map[holdout['id']] = holdout if holdout['includedFlags'].nil? || holdout['includedFlags'].empty? - @global_holdouts[holdout['id']] = holdout + @global_holdouts << holdout excluded_flags = holdout['excludedFlags'] if excluded_flags && !excluded_flags.empty? @@ -196,16 +196,28 @@ def initialize(datafile, logger, error_handler) end flag_id = feature_flag['id'] - applicable_holdouts = [] - applicable_holdouts.concat(@included_holdouts[flag_id]) if @included_holdouts[flag_id] + # Collect all applicable holdouts for this flag + # Priority: global holdouts (excluding excluded flags) + included holdouts + applicable_holdouts = [] - @global_holdouts.each_value do |holdout| - excluded_flag_ids = holdout['excludedFlags'] || [] - applicable_holdouts << holdout unless excluded_flag_ids.include?(flag_id) + # Add all global holdouts that don't exclude this flag + excluded_for_flag = @excluded_holdouts[flag_id] || [] + if excluded_for_flag.empty? + # No exclusions, add all global holdouts + applicable_holdouts.concat(@global_holdouts) + else + # Filter out excluded holdouts + @global_holdouts.each do |holdout| + applicable_holdouts << holdout unless excluded_for_flag.include?(holdout) + end end - @flag_holdouts_map[key] = applicable_holdouts unless applicable_holdouts.empty? + # Add all included holdouts for this flag + included = @included_holdouts[flag_id] + applicable_holdouts.concat(included) if included && !included.empty? + + @flag_holdouts_map[flag_id] = applicable_holdouts unless applicable_holdouts.empty? end # Adding Holdout variations in variation id and key maps diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index f1bc92e2..8fa4b7a5 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -312,14 +312,9 @@ def get_variations_for_feature_list(project_config, feature_flags, user_context, decisions = [] feature_flags.each do |feature_flag| - # check if the feature is being experiment on and whether the user is bucketed into the experiment - decision_result = get_variation_for_feature_experiment(project_config, feature_flag, user_context, user_profile_tracker, decide_options) - # Only process rollout if no experiment decision was found and no error - if decision_result.decision.nil? && !decision_result.error - decision_result_rollout = get_variation_for_feature_rollout(project_config, feature_flag, user_context) unless decision_result.decision - decision_result.decision = decision_result_rollout.decision - decision_result.reasons.push(*decision_result_rollout.reasons) - end + # Always use get_decision_for_flag which checks holdouts, experiments, and rollouts in order + # This matches Swift SDK's getDecisionForFlag behavior (DefaultDecisionService.swift:381-419) + decision_result = get_decision_for_flag(feature_flag, user_context, project_config, decide_options, user_profile_tracker) decisions << decision_result end user_profile_tracker&.save_user_profile diff --git a/spec/config/datafile_project_config_spec.rb b/spec/config/datafile_project_config_spec.rb index 84dd4509..6817266f 100644 --- a/spec/config/datafile_project_config_spec.rb +++ b/spec/config/datafile_project_config_spec.rb @@ -1249,40 +1249,60 @@ expect(holdouts).to eq([]) end - it 'should return global holdouts that do not exclude the flag' do - holdouts = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature') + it 'should return all applicable holdouts for the flag' do + # get_holdouts_for_flag expects flag ID, not key + feature_flag = config_with_holdouts.feature_flag_key_map['multi_variate_feature'] + holdouts = config_with_holdouts.get_holdouts_for_flag(feature_flag['id']) + # Should return global holdouts (not excluded) + included holdouts + # global_holdout (not excluded), holdout_empty_1 (global), specific_holdout (included) expect(holdouts.length).to eq(3) - global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' } - expect(global_holdout).not_to be_nil - expect(global_holdout['id']).to eq('holdout_1') - + # Should include the explicit holdout (specific_holdout) specific_holdout = holdouts.find { |h| h['key'] == 'specific_holdout' } expect(specific_holdout).not_to be_nil expect(specific_holdout['id']).to eq('holdout_2') + + # Global holdouts should also be included (Swift SDK alignment) + global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' } + expect(global_holdout).not_to be_nil end it 'should not return global holdouts that exclude the flag' do - holdouts = config_with_holdouts.get_holdouts_for_flag('boolean_single_variable_feature') + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_single_variable_feature'] + holdouts = config_with_holdouts.get_holdouts_for_flag(feature_flag['id']) + # Should only return holdout_empty_1 (global, not excluded) + # holdout_1 excludes this flag, so it shouldn't be included expect(holdouts.length).to eq(1) global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' } expect(global_holdout).to be_nil + + empty_holdout = holdouts.find { |h| h['key'] == 'holdout_empty_1' } + expect(empty_holdout).not_to be_nil end - it 'should cache results for subsequent calls' do - holdouts1 = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature') - holdouts2 = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature') + it 'should return consistent results for subsequent calls' do + feature_flag = config_with_holdouts.feature_flag_key_map['multi_variate_feature'] + holdouts1 = config_with_holdouts.get_holdouts_for_flag(feature_flag['id']) + holdouts2 = config_with_holdouts.get_holdouts_for_flag(feature_flag['id']) + # Results are cached in @flag_holdouts_map expect(holdouts1).to equal(holdouts2) 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') + it 'should return all global holdouts for flags not specifically targeted' do + feature_flag = config_with_holdouts.feature_flag_key_map['string_single_variable_feature'] + holdouts = config_with_holdouts.get_holdouts_for_flag(feature_flag['id']) - # Should only include global holdout (not excluded and no specific targeting) + # Should include all global holdouts (not excluded and no specific targeting) + # global_holdout + holdout_empty_1 expect(holdouts.length).to eq(2) - expect(holdouts.first['key']).to eq('global_holdout') + + global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' } + expect(global_holdout).not_to be_nil + + empty_holdout = holdouts.find { |h| h['key'] == 'holdout_empty_1' } + expect(empty_holdout).not_to be_nil end end @@ -1394,7 +1414,11 @@ it 'should properly categorize holdouts during initialization' do expect(config_with_complex_holdouts.holdout_id_map.keys).to contain_exactly('global_holdout', 'specific_holdout') - expect(config_with_complex_holdouts.global_holdouts.keys).to contain_exactly('global_holdout') + + # global_holdouts is now an Array (Swift alignment) + expect(config_with_complex_holdouts.global_holdouts).to be_an(Array) + expect(config_with_complex_holdouts.global_holdouts.length).to eq(1) + expect(config_with_complex_holdouts.global_holdouts.first['id']).to eq('global_holdout') # Use the correct feature flag IDs boolean_feature_id = '155554' @@ -1416,7 +1440,10 @@ it 'should only process running holdouts during initialization' do expect(config_with_complex_holdouts.holdout_id_map['inactive_holdout']).to be_nil - expect(config_with_complex_holdouts.global_holdouts['inactive_holdout']).to be_nil + + # global_holdouts is now an Array - check it doesn't contain inactive holdout + inactive_in_global = config_with_complex_holdouts.global_holdouts.any? { |h| h['id'] == 'inactive_holdout' } + expect(inactive_in_global).to be false boolean_feature_id = '155554' included_for_boolean = config_with_complex_holdouts.included_holdouts[boolean_feature_id] @@ -1519,8 +1546,9 @@ 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') + feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] + holdouts_1 = config_with_holdouts.get_holdouts_for_flag(feature_flag['id']) + holdouts_2 = config_with_holdouts.get_holdouts_for_flag(feature_flag['id']) expect(holdouts_1).to equal(holdouts_2) end @@ -1594,7 +1622,8 @@ it 'should handle mixed holdout configurations' do # Verify the config has properly categorized holdouts - expect(config_with_holdouts.global_holdouts).to be_a(Hash) + # global_holdouts is an Array (Swift alignment), others are Hashes + expect(config_with_holdouts.global_holdouts).to be_an(Array) expect(config_with_holdouts.included_holdouts).to be_a(Hash) expect(config_with_holdouts.excluded_holdouts).to be_a(Hash) end @@ -1773,8 +1802,9 @@ 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 + # Should treat as global holdout (global_holdouts is an Array) + holdout_found = config.global_holdouts.any? { |h| h['id'] == 'holdout_nil' } + expect(holdout_found).to be true end it 'should only include running holdouts in maps' do diff --git a/spec/decision_service_holdout_comprehensive_spec.rb b/spec/decision_service_holdout_comprehensive_spec.rb new file mode 100644 index 00000000..d9d4a90c --- /dev/null +++ b/spec/decision_service_holdout_comprehensive_spec.rb @@ -0,0 +1,765 @@ +# 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/decision_service' +require 'optimizely/audience' +require 'optimizely/error_handler' +require 'optimizely/logger' + +# Comprehensive holdout tests aligned with Swift SDK DecisionServiceTests_Holdouts.swift +describe 'Optimizely::DecisionService - Comprehensive Holdout Tests' do + let(:error_handler) { Optimizely::NoOpErrorHandler.new } + let(:spy_logger) { spy('logger') } + let(:spy_user_profile_service) { spy('user_profile_service') } + let(:spy_cmab_service) { spy('cmab_service') } + + # Sample data aligned with Swift SDK test fixtures + let(:sample_feature_flag) do + { + 'id' => 'flag_id_1234', + 'key' => 'test_flag', + 'experimentIds' => ['experiment_123'], + 'rolloutId' => '', + 'variables' => [] + } + end + + let(:sample_experiment) do + { + 'id' => 'experiment_123', + 'key' => 'test_experiment', + 'status' => 'Running', + 'layerId' => 'layer_1', + 'trafficAllocation' => [ + {'entityId' => 'variation_a', 'endOfRange' => 10_000} + ], + 'audienceIds' => ['audience_country'], + 'audienceConditions' => ['or', 'audience_country'], + 'variations' => [ + {'id' => 'variation_a', 'key' => 'control', 'variables' => []} + ], + 'forcedVariations' => {} + } + end + + let(:sample_typed_audience) do + { + 'id' => 'audience_country', + 'name' => 'country', + 'conditions' => ['and', ['or', ['or', { + 'type' => 'custom_attribute', + 'name' => 'country', + 'match' => 'exact', + 'value' => 'us' + }]]] + } + end + + let(:global_holdout) do + { + 'id' => 'holdout_global', + 'key' => 'global_holdout', + 'status' => 'Running', + 'trafficAllocation' => [ + {'entityId' => 'global_variation', 'endOfRange' => 500} + ], + 'audienceIds' => ['audience_country'], + 'audienceConditions' => ['or', 'audience_country'], + 'variations' => [ + {'id' => 'global_variation', 'key' => 'global_var', 'featureEnabled' => false} + ], + 'includedFlags' => [], + 'excludedFlags' => [] + } + end + + let(:included_holdout) do + { + 'id' => 'holdout_included', + 'key' => 'included_holdout', + 'status' => 'Running', + 'trafficAllocation' => [ + {'entityId' => 'included_variation', 'endOfRange' => 1000} + ], + 'audienceIds' => ['audience_country'], + 'audienceConditions' => ['or', 'audience_country'], + 'variations' => [ + {'id' => 'included_variation', 'key' => 'included_var', 'featureEnabled' => false} + ], + 'includedFlags' => ['flag_id_1234'], + 'excludedFlags' => [] + } + end + + let(:excluded_holdout) do + { + 'id' => 'holdout_excluded', + 'key' => 'excluded_holdout', + 'status' => 'Running', + 'trafficAllocation' => [ + {'entityId' => 'excluded_variation', 'endOfRange' => 1000} + ], + 'audienceIds' => ['audience_country'], + 'audienceConditions' => ['or', 'audience_country'], + 'variations' => [ + {'id' => 'excluded_variation', 'key' => 'excluded_var', 'featureEnabled' => false} + ], + 'includedFlags' => [], + 'excludedFlags' => ['flag_id_1234'] + } + end + + # MARK: - Audience Evaluation Tests (aligned with Swift lines 221-349) + + describe 'Audience Evaluation' do + let(:config_body) do + { + 'version' => '4', + 'rollouts' => [], + 'typedAudiences' => [sample_typed_audience], + 'anonymizeIP' => false, + 'projectId' => '111001', + 'variables' => [], + 'featureFlags' => [sample_feature_flag], + 'experiments' => [sample_experiment], + 'audiences' => [], + 'groups' => [], + 'attributes' => [], + 'accountId' => '12123', + 'layers' => [], + 'events' => [], + 'revision' => '1', + 'holdouts' => [global_holdout] + } + end + + let(:config) do + Optimizely::DatafileProjectConfig.new( + JSON.dump(config_body), + spy_logger, + error_handler + ) + end + + let(:decision_service) do + Optimizely::DecisionService.new(spy_logger, spy_cmab_service, spy_user_profile_service) + end + + let(:project_instance) do + Optimizely::Project.new( + datafile: JSON.dump(config_body), + logger: spy_logger, + error_handler: error_handler + ) + end + + after(:example) do + project_instance&.close + end + + describe '#user_meets_audience_conditions with audienceConditions' do + it 'should return true when attributes match audienceConditions' do + holdout = config.holdouts.first + user_context = project_instance.create_user_context('test_user', 'country' => 'us') + + result, _reasons = Optimizely::Audience.user_meets_audience_conditions?( + config, + holdout, + user_context, + spy_logger + ) + + expect(result).to be true + end + + it 'should return false when attributes do not match audienceConditions' do + holdout = config.holdouts.first + user_context = project_instance.create_user_context('test_user', 'country' => 'ca') + + result, _reasons = Optimizely::Audience.user_meets_audience_conditions?( + config, + holdout, + user_context, + spy_logger + ) + + expect(result).to be false + end + + it 'should return false when attribute is missing' do + holdout = config.holdouts.first + user_context = project_instance.create_user_context('test_user', 'age' => 30) + + result, _reasons = Optimizely::Audience.user_meets_audience_conditions?( + config, + holdout, + user_context, + spy_logger + ) + + expect(result).to be false + end + end + + describe '#user_meets_audience_conditions with empty arrays' do + it 'should return true when audienceConditions is empty array' do + modified_holdout = global_holdout.dup + modified_holdout['audienceConditions'] = [] + modified_config_body = config_body.dup + modified_config_body['holdouts'] = [modified_holdout] + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + spy_logger, + error_handler + ) + + holdout = modified_config.holdouts.first + user_context = project_instance.create_user_context('test_user', 'country' => 'us') + + result, _reasons = Optimizely::Audience.user_meets_audience_conditions?( + modified_config, + holdout, + user_context, + spy_logger + ) + + expect(result).to be true + end + + it 'should return true when audienceIds is empty array' do + modified_holdout = global_holdout.dup + modified_holdout['audienceIds'] = [] + modified_holdout['audienceConditions'] = nil + modified_config_body = config_body.dup + modified_config_body['holdouts'] = [modified_holdout] + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + spy_logger, + error_handler + ) + + holdout = modified_config.holdouts.first + user_context = project_instance.create_user_context('test_user', {}) + + result, _reasons = Optimizely::Audience.user_meets_audience_conditions?( + modified_config, + holdout, + user_context, + spy_logger + ) + + expect(result).to be true + end + end + end + + # MARK: - Multiple Holdouts Ordering Tests (aligned with Swift lines 497-573) + + describe 'Multiple Holdouts Ordering' do + let(:config_body) do + { + 'version' => '4', + 'rollouts' => [], + 'typedAudiences' => [sample_typed_audience], + 'anonymizeIP' => false, + 'projectId' => '111001', + 'variables' => [], + 'featureFlags' => [sample_feature_flag], + 'experiments' => [sample_experiment], + 'audiences' => [], + 'groups' => [], + 'attributes' => [], + 'accountId' => '12123', + 'layers' => [], + 'events' => [], + 'revision' => '1', + 'holdouts' => [global_holdout, included_holdout, excluded_holdout] + } + end + + let(:config) do + Optimizely::DatafileProjectConfig.new( + JSON.dump(config_body), + spy_logger, + error_handler + ) + end + + let(:decision_service) do + Optimizely::DecisionService.new(spy_logger, spy_cmab_service, spy_user_profile_service) + end + + let(:project_instance) do + Optimizely::Project.new( + datafile: JSON.dump(config_body), + logger: spy_logger, + error_handler: error_handler + ) + end + + after(:example) do + project_instance&.close + end + + it 'should evaluate global holdouts before included holdouts' do + # Get holdouts for the flag + holdouts = config.get_holdouts_for_flag(sample_feature_flag['id']) + + # Verify order: global holdouts come before included holdouts + expect(holdouts).not_to be_empty + + # Find indices + global_index = holdouts.index { |h| h['id'] == 'holdout_global' } + included_index = holdouts.index { |h| h['id'] == 'holdout_included' } + + # Global should come before included + expect(global_index).not_to be_nil + expect(included_index).not_to be_nil + expect(global_index).to be < included_index + end + + it 'should not include excluded holdouts for the flag' do + holdouts = config.get_holdouts_for_flag(sample_feature_flag['id']) + + # Excluded holdout should not be in the list + excluded_found = holdouts.any? { |h| h['id'] == 'holdout_excluded' } + expect(excluded_found).to be false + end + + it 'should fall back to included holdout when global fails bucketing' do + # Modify traffic allocations so global has less traffic + modified_global = global_holdout.dup + modified_global['trafficAllocation'] = [ + {'entityId' => 'global_variation', 'endOfRange' => 100} + ] + + modified_included = included_holdout.dup + modified_included['trafficAllocation'] = [ + {'entityId' => 'included_variation', 'endOfRange' => 10_000} + ] + + modified_config_body = config_body.dup + modified_config_body['holdouts'] = [modified_global, modified_included] + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + spy_logger, + error_handler + ) + + modified_decision_service = Optimizely::DecisionService.new( + spy_logger, + spy_cmab_service, + spy_user_profile_service + ) + + feature_flag = modified_config.feature_flag_key_map['test_flag'] + user_context = project_instance.create_user_context('test_user', 'country' => 'us') + + decision_result = modified_decision_service.get_decision_for_flag( + feature_flag, + user_context, + modified_config, + [] + ) + + # Should bucket into either global or included holdout + # (We can't guarantee which due to real bucketing, but decision should exist) + if decision_result.decision + expect(decision_result.decision.source).to eq(Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT']) + end + end + + it 'should fall back to experiment when all holdouts fail' do + # Modify all holdouts to have 0% traffic + modified_global = global_holdout.dup + modified_global['trafficAllocation'] = [] + + modified_included = included_holdout.dup + modified_included['trafficAllocation'] = [] + + modified_config_body = config_body.dup + modified_config_body['holdouts'] = [modified_global, modified_included] + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + spy_logger, + error_handler + ) + + modified_decision_service = Optimizely::DecisionService.new( + spy_logger, + spy_cmab_service, + spy_user_profile_service + ) + + feature_flag = modified_config.feature_flag_key_map['test_flag'] + user_context = project_instance.create_user_context('test_user', 'country' => 'us') + + decision_result = modified_decision_service.get_decision_for_flag( + feature_flag, + user_context, + modified_config, + [] + ) + + # Should fall back to experiment (or rollout/default if experiment also fails) + # Verify it's NOT a holdout decision + if decision_result.decision + expect(decision_result.decision.source).not_to eq(Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT']) + end + end + end + + # MARK: - Excluded Flag Logic Tests (aligned with Swift lines 476-495) + + describe 'Excluded Flag Logic' do + let(:config_body) do + { + 'version' => '4', + 'rollouts' => [], + 'typedAudiences' => [sample_typed_audience], + 'anonymizeIP' => false, + 'projectId' => '111001', + 'variables' => [], + 'featureFlags' => [sample_feature_flag], + 'experiments' => [sample_experiment], + 'audiences' => [], + 'groups' => [], + 'attributes' => [], + 'accountId' => '12123', + 'layers' => [], + 'events' => [], + 'revision' => '1', + 'holdouts' => [excluded_holdout] + } + end + + let(:config) do + Optimizely::DatafileProjectConfig.new( + JSON.dump(config_body), + spy_logger, + error_handler + ) + end + + let(:decision_service) do + Optimizely::DecisionService.new(spy_logger, spy_cmab_service, spy_user_profile_service) + end + + let(:project_instance) do + Optimizely::Project.new( + datafile: JSON.dump(config_body), + logger: spy_logger, + error_handler: error_handler + ) + end + + after(:example) do + project_instance&.close + end + + it 'should skip holdouts that exclude the flag' do + holdouts = config.get_holdouts_for_flag(sample_feature_flag['id']) + + # Should not include the excluded holdout + expect(holdouts).to be_empty + end + + it 'should use excluded_holdouts map for filtering' do + # Verify the excluded_holdouts map is built correctly + expect(config.excluded_holdouts).to have_key(sample_feature_flag['id']) + expect(config.excluded_holdouts[sample_feature_flag['id']]).to include( + hash_including('id' => 'holdout_excluded') + ) + end + + it 'should apply excluded holdout to non-excluded flags' do + # Add another flag that is NOT excluded + other_flag = sample_feature_flag.dup + other_flag['id'] = 'other_flag_id' + other_flag['key'] = 'other_flag' + + modified_config_body = config_body.dup + modified_config_body['featureFlags'] = [sample_feature_flag, other_flag] + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + spy_logger, + error_handler + ) + + # The excluded holdout should NOT apply to flag_id_1234 + holdouts_excluded_flag = modified_config.get_holdouts_for_flag(sample_feature_flag['id']) + expect(holdouts_excluded_flag).to be_empty + + # But it SHOULD apply to other_flag_id (as a global holdout) + holdouts_other_flag = modified_config.get_holdouts_for_flag('other_flag_id') + expect(holdouts_other_flag).not_to be_empty + expect(holdouts_other_flag.first['id']).to eq('holdout_excluded') + end + end + + # MARK: - Edge Cases (aligned with Swift lines 575-640) + + describe 'Edge Cases' do + let(:config_body) do + { + 'version' => '4', + 'rollouts' => [], + 'typedAudiences' => [sample_typed_audience], + 'anonymizeIP' => false, + 'projectId' => '111001', + 'variables' => [], + 'featureFlags' => [sample_feature_flag], + 'experiments' => [sample_experiment], + 'audiences' => [], + 'groups' => [], + 'attributes' => [], + 'accountId' => '12123', + 'layers' => [], + 'events' => [], + 'revision' => '1', + 'holdouts' => [global_holdout] + } + end + + let(:decision_service) do + Optimizely::DecisionService.new(spy_logger, spy_cmab_service, spy_user_profile_service) + end + + let(:project_instance) do + Optimizely::Project.new( + datafile: JSON.dump(config_body), + logger: spy_logger, + error_handler: error_handler + ) + end + + after(:example) do + project_instance&.close + end + + it 'should handle holdout with no traffic allocation' do + modified_holdout = global_holdout.dup + modified_holdout['trafficAllocation'] = [] + + modified_config_body = config_body.dup + modified_config_body['holdouts'] = [modified_holdout] + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + spy_logger, + error_handler + ) + + feature_flag = modified_config.feature_flag_key_map['test_flag'] + user_context = project_instance.create_user_context('test_user', 'country' => 'us') + + decision_result = decision_service.get_decision_for_flag( + feature_flag, + user_context, + modified_config, + [] + ) + + # Should not bucket into holdout, should fall through + if decision_result.decision + expect(decision_result.decision.source).not_to eq(Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT']) + end + end + + it 'should handle holdout with empty variations array' do + modified_holdout = global_holdout.dup + modified_holdout['variations'] = [] + + modified_config_body = config_body.dup + modified_config_body['holdouts'] = [modified_holdout] + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + spy_logger, + error_handler + ) + + feature_flag = modified_config.feature_flag_key_map['test_flag'] + user_context = project_instance.create_user_context('test_user', 'country' => 'us') + + decision_result = decision_service.get_decision_for_flag( + feature_flag, + user_context, + modified_config, + [] + ) + + # Should not bucket into holdout, should fall through + if decision_result.decision + expect(decision_result.decision.source).not_to eq(Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT']) + end + end + + it 'should handle feature flag with no experiments' do + modified_flag = sample_feature_flag.dup + modified_flag['experimentIds'] = [] + + modified_config_body = config_body.dup + modified_config_body['featureFlags'] = [modified_flag] + modified_config_body['holdouts'] = [included_holdout] + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + spy_logger, + error_handler + ) + + feature_flag = modified_config.feature_flag_key_map['test_flag'] + user_context = project_instance.create_user_context('test_user', 'country' => 'us') + + decision_result = decision_service.get_decision_for_flag( + feature_flag, + user_context, + modified_config, + [] + ) + + # May bucket into holdout or return nil + # Main point is it shouldn't error + expect { decision_result }.not_to raise_error + end + + it 'should handle inactive holdout status' do + modified_holdout = global_holdout.dup + modified_holdout['status'] = 'Paused' + + modified_config_body = config_body.dup + modified_config_body['holdouts'] = [modified_holdout] + + modified_config = Optimizely::DatafileProjectConfig.new( + JSON.dump(modified_config_body), + spy_logger, + error_handler + ) + + feature_flag = modified_config.feature_flag_key_map['test_flag'] + user_context = project_instance.create_user_context('test_user', 'country' => 'us') + + decision_result = decision_service.get_variation_for_holdout( + modified_config.holdouts.first, + user_context, + modified_config + ) + + # Should return nil decision for inactive holdout + expect(decision_result.decision).to be_nil + + # Should log appropriate message + expect(spy_logger).to have_received(:log).with( + Logger::INFO, + a_string_matching(/is not running/i) + ) + end + end + + # MARK: - Tests for Today's Fixes + + describe 'Swift SDK Alignment Validation' do + let(:config_body) do + { + 'version' => '4', + 'rollouts' => [], + 'typedAudiences' => [sample_typed_audience], + 'anonymizeIP' => false, + 'projectId' => '111001', + 'variables' => [], + 'featureFlags' => [sample_feature_flag], + 'experiments' => [sample_experiment], + 'audiences' => [], + 'groups' => [], + 'attributes' => [], + 'accountId' => '12123', + 'layers' => [], + 'events' => [], + 'revision' => '1', + 'holdouts' => [global_holdout, included_holdout] + } + end + + let(:config) do + Optimizely::DatafileProjectConfig.new( + JSON.dump(config_body), + spy_logger, + error_handler + ) + end + + it 'should store global_holdouts as an Array (not Hash)' do + expect(config.global_holdouts).to be_an(Array) + end + + it 'should preserve datafile order in global_holdouts array' do + # Both holdouts are global-ish, but one has includedFlags + # Only the truly global one should be in global_holdouts + expect(config.global_holdouts).to be_an(Array) + + # Verify it contains the global holdout + global_found = config.global_holdouts.any? { |h| h['id'] == 'holdout_global' } + expect(global_found).to be true + end + + it 'should use excluded_holdouts map for efficient filtering' do + # Verify excluded_holdouts is a Hash + expect(config.excluded_holdouts).to be_a(Hash) + end + + it 'should always call get_decision_for_flag in batch operations' do + decision_service = Optimizely::DecisionService.new( + spy_logger, + spy_cmab_service, + spy_user_profile_service + ) + + project_instance = Optimizely::Project.new( + datafile: JSON.dump(config_body), + logger: spy_logger, + error_handler: error_handler + ) + + feature_flag = config.feature_flag_key_map['test_flag'] + user_context = project_instance.create_user_context('test_user', 'country' => 'us') + + # Spy on get_decision_for_flag + allow(decision_service).to receive(:get_decision_for_flag).and_call_original + + # Call get_variations_for_feature_list (batch operation) + decision_service.get_variations_for_feature_list( + config, + [feature_flag], + user_context, + [] + ) + + # Verify get_decision_for_flag was called + expect(decision_service).to have_received(:get_decision_for_flag).at_least(:once) + + project_instance.close + end + end +end diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index fe2cc881..6e6b713e 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -775,7 +775,8 @@ decision_result = decision_service.get_variation_for_feature(config, feature_flag, user_context) expect(decision_result.decision).to eq(expected_decision) - expect(decision_result.reasons).to eq([]) + # Reasons now include rollout bucketing message from get_decision_for_flag + expect(decision_result.reasons).to include(a_string_matching(/is bucketed into a rollout/)) end end