diff --git a/lib/split/encapsulated_helper.rb b/lib/split/encapsulated_helper.rb index f6970cb4..afc59d4e 100644 --- a/lib/split/encapsulated_helper.rb +++ b/lib/split/encapsulated_helper.rb @@ -26,21 +26,8 @@ def ab_user end end - def ab_test(*arguments) - ret = split_context_shim.ab_test(*arguments) - # TODO there must be a better way to pass a block straight - # through to the original ab_test - if block_given? - if defined?(capture) # a block in a rails view - block = Proc.new { yield(ret) } - concat(capture(ret, &block)) - false - else - yield(ret) - end - else - ret - end + def ab_test(*arguments,&block) + split_context_shim.ab_test(*arguments,&block) end private diff --git a/lib/split/user.rb b/lib/split/user.rb index e528b4fe..2d6cb47d 100644 --- a/lib/split/user.rb +++ b/lib/split/user.rb @@ -21,7 +21,7 @@ def cleanup_old_experiments! def max_experiments_reached?(experiment_key) if Split.configuration.allow_multiple_experiments == 'control' experiments = active_experiments - count_control = experiments.values.count {|v| v == 'control'} + count_control = experiments.count {|k,v| k == experiment_key || v == 'control'} experiments.size > count_control else !Split.configuration.allow_multiple_experiments && diff --git a/spec/encapsulated_helper_spec.rb b/spec/encapsulated_helper_spec.rb index 27541aa4..4ce1d1d5 100644 --- a/spec/encapsulated_helper_spec.rb +++ b/spec/encapsulated_helper_spec.rb @@ -4,18 +4,49 @@ describe Split::EncapsulatedHelper do include Split::EncapsulatedHelper - before do - allow_any_instance_of(Split::EncapsulatedHelper::ContextShim).to receive(:ab_user) - .and_return(mock_user) - end def params raise NoMethodError, 'This method is not really defined' end describe "ab_test" do + before do + allow_any_instance_of(Split::EncapsulatedHelper::ContextShim).to receive(:ab_user) + .and_return(mock_user) + end + it "should not raise an error when params raises an error" do + expect{ params }.to raise_error(NoMethodError) expect(lambda { ab_test('link_color', 'blue', 'red') }).not_to raise_error end + + it "calls the block with selected alternative" do + expect{|block| ab_test('link_color', 'red', 'red', &block) }.to yield_with_args('red', nil) + end + + context "inside a view" do + + it "works inside ERB" do + require 'erb' + template = ERB.new(<<-ERB.split(/\s+/s).map(&:strip).join(' '), nil, "%") + foo <% ab_test(:foo, '1', '2') do |alt, meta| %> + static <%= alt %> + <% end %> + ERB + expect(template.result(binding)).to match /foo static \d/ + end + + end + end + + describe "context" do + it 'is passed in shim' do + ctx = Class.new{ + include Split::EncapsulatedHelper + public :session + }.new + expect(ctx).to receive(:session){{}} + expect{ ctx.ab_test('link_color', 'blue', 'red') }.not_to raise_error + end end end diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb index 6232a0c3..bb79ca8b 100755 --- a/spec/helper_spec.rb +++ b/spec/helper_spec.rb @@ -197,24 +197,51 @@ expect(button_size_alt.participant_count).to eq(1) end - it "should let a user participate in many experiment with one non-'control' alternative with allow_multiple_experiments = 'control'" do - Split.configure do |config| - config.allow_multiple_experiments = 'control' - end - groups = [] - (0..100).each do |n| - alt = ab_test("test#{n}".to_sym, {'control' => (100 - n)}, {'test#{n}-alt' => n}) - groups << alt + context "with allow_multiple_experiments = 'control'" do + it "should let a user participate in many experiment with one non-'control' alternative" do + Split.configure do |config| + config.allow_multiple_experiments = 'control' + end + groups = 100.times.map do |n| + ab_test("test#{n}".to_sym, {'control' => (100 - n)}, {"test#{n}-alt" => n}) + end + + experiments = ab_user.active_experiments + expect(experiments.size).to be > 1 + + count_control = experiments.values.count { |g| g == 'control' } + expect(count_control).to eq(experiments.size - 1) + + count_alts = groups.count { |g| g != 'control' } + expect(count_alts).to eq(1) end - experiments = ab_user.active_experiments - expect(experiments.size).to be > 1 + context "when user already has experiment" do + let(:mock_user){ Split::User.new(self, {'test_0' => 'test-alt'}) } + before{ + Split.configure do |config| + config.allow_multiple_experiments = 'control' + end + Split::ExperimentCatalog.find_or_initialize('test_0', 'control', 'test-alt').save + Split::ExperimentCatalog.find_or_initialize('test_1', 'control', 'test-alt').save + } - count_control = experiments.values.count { |g| g == 'control' } - expect(count_control).to eq(experiments.size - 1) + it "should restore previously selected alternative" do + expect(ab_user.active_experiments.size).to eq 1 + expect(ab_test(:test_0, {'control' => 100}, {"test-alt" => 1})).to eq 'test-alt' + expect(ab_test(:test_0, {'control' => 1}, {"test-alt" => 100})).to eq 'test-alt' + end + + it "lets override existing choice" do + pending "this requires user store reset on first call not depending on whelther it is current trial" + @params = { 'ab_test' => { 'test_1' => 'test-alt' } } + + expect(ab_test(:test_0, {'control' => 0}, {"test-alt" => 100})).to eq 'control' + expect(ab_test(:test_1, {'control' => 100}, {"test-alt" => 1})).to eq 'test-alt' + end + + end - count_alts = groups.count { |g| g != 'control' } - expect(count_alts).to eq(1) end it "should not over-write a finished key when an experiment is on a later version" do @@ -642,6 +669,22 @@ def should_finish_experiment(experiment_name, should_finish=true) it_behaves_like "a disabled test" end + + context "when ignored other address" do + before do + @request = OpenStruct.new(:ip => '1.1.1.1') + Split.configure do |c| + c.ignore_ip_addresses << '81.19.48.130' + end + end + + it "works as usual" do + alternative_name = ab_test('link_color', 'red', 'blue') + expect{ + ab_finished('link_color') + }.to change(Split::Alternative.new(alternative_name, 'link_color'), :completed_count).by(1) + end + end end describe 'versioned experiments' do @@ -774,7 +817,7 @@ def should_finish_experiment(experiment_name, should_finish=true) end end - expect(Split.configuration.db_failover_on_db_error).to receive(:call) + expect(Split.configuration.db_failover_on_db_error).to receive(:call).and_call_original ab_test('link_color', 'blue', 'red') end @@ -833,7 +876,7 @@ def should_finish_experiment(experiment_name, should_finish=true) end end - expect(Split.configuration.db_failover_on_db_error).to receive(:call) + expect(Split.configuration.db_failover_on_db_error).to receive(:call).and_call_original ab_finished('link_color') end end diff --git a/spec/persistence/dual_adapter_spec.rb b/spec/persistence/dual_adapter_spec.rb new file mode 100644 index 00000000..0c490d83 --- /dev/null +++ b/spec/persistence/dual_adapter_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true +require "spec_helper" + +describe Split::Persistence::DualAdapter do + + let(:context){ "some context" } + + let(:just_adapter){ Class.new } + let(:selected_adapter_instance){ double } + let(:selected_adapter){ + c = Class.new + expect(c).to receive(:new){ selected_adapter_instance } + c + } + let(:not_selected_adapter){ + c = Class.new + expect(c).not_to receive(:new) + c + } + + shared_examples_for "forwarding calls" do + it "#[]=" do + expect(selected_adapter_instance).to receive(:[]=).with('my_key', 'my_value') + expect_any_instance_of(not_selected_adapter).not_to receive(:[]=) + subject["my_key"] = "my_value" + end + + it "#[]" do + expect(selected_adapter_instance).to receive(:[]).with('my_key'){'my_value'} + expect_any_instance_of(not_selected_adapter).not_to receive(:[]) + expect(subject["my_key"]).to eq('my_value') + end + + it "#delete" do + expect(selected_adapter_instance).to receive(:delete).with('my_key'){'my_value'} + expect_any_instance_of(not_selected_adapter).not_to receive(:delete) + expect(subject.delete("my_key")).to eq('my_value') + end + + it "#keys" do + expect(selected_adapter_instance).to receive(:keys){'my_value'} + expect_any_instance_of(not_selected_adapter).not_to receive(:keys) + expect(subject.keys).to eq('my_value') + end + end + + context "when logged in" do + subject { + described_class.with_config( + logged_in: -> (context) { true }, + logged_in_adapter: selected_adapter, + logged_out_adapter: not_selected_adapter + ).new(context) + } + + it_should_behave_like "forwarding calls" + end + + context "when not logged in" do + subject { + described_class.with_config( + logged_in: -> (context) { false }, + logged_in_adapter: not_selected_adapter, + logged_out_adapter: selected_adapter + ).new(context) + } + + it_should_behave_like "forwarding calls" + end + + describe "when errors in config" do + before{ + described_class.config.clear + } + let(:some_proc){ ->{} } + it "when no logged in adapter" do + expect{ + described_class.with_config( + logged_in: some_proc, + logged_out_adapter: just_adapter + ).new(context) + }.to raise_error(StandardError, /:logged_in_adapter/) + end + it "when no logged out adapter" do + expect{ + described_class.with_config( + logged_in: some_proc, + logged_in_adapter: just_adapter + ).new(context) + }.to raise_error(StandardError, /:logged_out_adapter/) + end + it "when no logged in detector" do + expect{ + described_class.with_config( + logged_in_adapter: just_adapter, + logged_out_adapter: just_adapter + ).new(context) + }.to raise_error(StandardError, /:logged_in$/) + end + end + +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index bf5b2fc4..6d3f95db 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -16,21 +16,23 @@ require "fakeredis" -fakeredis = Redis.new +G_fakeredis = Redis.new -RSpec.configure do |config| - config.order = 'random' - config.before(:each) do +module GlobalSharedContext + extend RSpec::SharedContext + let(:mock_user){ Split::User.new(double(session: {})) } + before(:each) do Split.configuration = Split::Configuration.new - Split.redis = fakeredis + Split.redis = G_fakeredis Split.redis.flushall @ab_user = mock_user params = nil end end -def mock_user - Split::User.new(double(session: {})) +RSpec.configure do |config| + config.order = 'random' + config.include GlobalSharedContext end def session