Fix variations reset across page loads for multiple=control and improve coverage

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

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 &&

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

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

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

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