From b2ddf8c8cb077b78a360283fcb0ac176a2a250f4 Mon Sep 17 00:00:00 2001 From: Ian Vaughan Date: Sun, 9 Mar 2014 15:07:07 +0000 Subject: [PATCH 1/3] Whitespace and wrapped long lines --- lib/split/experiment.rb | 1 - lib/split/helper.rb | 19 ++++++------ spec/alternative_spec.rb | 16 ++--------- spec/helper_spec.rb | 62 +++++++++------------------------------- 4 files changed, 27 insertions(+), 71 deletions(-) diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb index 74f98622..85acc591 100644 --- a/lib/split/experiment.rb +++ b/lib/split/experiment.rb @@ -79,7 +79,6 @@ def save @alternatives.reverse.each {|a| Split.redis.lpush(name, a.name)} @goals.reverse.each {|a| Split.redis.lpush(goals_key, a)} unless @goals.nil? else - existing_alternatives = load_alternatives_from_redis existing_goals = load_goals_from_redis unless existing_alternatives == @alternatives.map(&:name) && existing_goals == @goals diff --git a/lib/split/helper.rb b/lib/split/helper.rb index d47067e2..27881825 100644 --- a/lib/split/helper.rb +++ b/lib/split/helper.rb @@ -6,16 +6,20 @@ def ab_test(metric_descriptor, control=nil, *alternatives) puts 'WARNING: You should always pass the control alternative through as the second argument with any other alternatives as the third because the order of the hash is not preserved in ruby 1.8' end - # Check if array is passed to ab_test: ab_test('name', ['Alt 1', 'Alt 2', 'Alt 3']) + # Check if array is passed to ab_test + # e.g. ab_test('name', ['Alt 1', 'Alt 2', 'Alt 3']) if control.is_a? Array and alternatives.length.zero? control, alternatives = control.first, control[1..-1] end begin - experiment_name_with_version, goals = normalize_experiment(metric_descriptor) - experiment_name = experiment_name_with_version.to_s.split(':')[0] - experiment = Split::Experiment.new(experiment_name, :alternatives => [control].compact + alternatives, :goals => goals) - control ||= experiment.control && experiment.control.name + experiment_name_with_version, goals = normalize_experiment(metric_descriptor) + experiment_name = experiment_name_with_version.to_s.split(':')[0] + experiment = Split::Experiment.new( + experiment_name, + :alternatives => [control].compact + alternatives, + :goals => goals) + control ||= experiment.control && experiment.control.name ret = if Split.configuration.enabled experiment.save @@ -23,7 +27,6 @@ def ab_test(metric_descriptor, control=nil, *alternatives) else control_variable(control) end - rescue => e raise(e) unless Split.configuration.db_failover Split.configuration.db_failover_on_db_error.call(e) @@ -32,9 +35,7 @@ def ab_test(metric_descriptor, control=nil, *alternatives) ret = override_alternative(experiment_name) end ensure - unless ret - ret = control_variable(control) - end + ret ||= control_variable(control) end if block_given? diff --git a/spec/alternative_spec.rb b/spec/alternative_spec.rb index 34d310e9..a9614a83 100644 --- a/spec/alternative_spec.rb +++ b/spec/alternative_spec.rb @@ -11,32 +11,22 @@ Split::Alternative.new('Cart', 'basket_text') } - let(:experiment) { + let!(:experiment) { Split::Experiment.find_or_create({"basket_text" => ["purchase", "refund"]}, "Basket", "Cart") } let(:goal1) { "purchase" } let(:goal2) { "refund" } - # setup experiment - before do - experiment - end - it "should have goals" do alternative.goals.should eql(["purchase", "refund"]) end - it "should have a name" do - alternative.name.should eql('Basket') - end - - it "return only the name" do + it "should have and only return the name" do alternative.name.should eql('Basket') end describe 'weights' do - it "should set the weights" do experiment = Split::Experiment.new('basket_text', :alternatives => [{'Basket' => 0.6}, {"Cart" => 0.4}]) first = experiment.alternatives[0] @@ -248,7 +238,7 @@ alternative2.participant_count = 142 alternative2.set_completed_count(119) - + alternative2.z_score.round(2).should eql(2.58) end diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb index 6061a81d..5cc92ddf 100755 --- a/spec/helper_spec.rb +++ b/spec/helper_spec.rb @@ -10,7 +10,6 @@ } describe "ab_test" do - it "should not raise an error when passed strings for alternatives" do lambda { ab_test('xyz', '1', '2', '3') }.should_not raise_error end @@ -41,7 +40,6 @@ end it "should increment the participation counter after assignment to a new user" do - previous_red_count = Split::Alternative.new('red', 'link_color').participant_count previous_blue_count = Split::Alternative.new('blue', 'link_color').participant_count @@ -81,9 +79,7 @@ end it "should return the given alternative for an existing user" do - alternative = ab_test('link_color', 'blue', 'red') - repeat_alternative = ab_test('link_color', 'blue', 'red') - alternative.should eql repeat_alternative + ab_test('link_color', 'blue', 'red').should eql ab_test('link_color', 'blue', 'red') end it 'should always return the winner if one is present' do @@ -105,7 +101,7 @@ alternative.should eql('red') end - it "should not store the via params forced alternative" do + it "should not store the split when a param forced alternative" do @params = {'link_color' => 'blue'} ab_user.should_not_receive(:[]=) ab_test('link_color', 'blue', 'red') @@ -135,7 +131,7 @@ ret.should eql("shared/#{alt}") end - it "should allow the share of visitors see an alternative to be specificed" do + it "should allow the share of visitors see an alternative to be specified" do ab_test('link_color', {'blue' => 0.8}, {'red' => 20}) ['red', 'blue'].should include(ab_user['link_color']) end @@ -277,7 +273,6 @@ finished(@experiment_name) end end - end context "finished with config" do @@ -374,7 +369,6 @@ def should_finish_experiment(experiment_name, should_finish=true) ab_user[exp.key].should == alternative_name ab_user[exp.finished_key].should == true end - end describe 'conversions' do @@ -431,7 +425,6 @@ def should_finish_experiment(experiment_name, should_finish=true) end end - describe 'when providing custom ignore logic' do context "using a proc to configure custom logic" do @@ -452,7 +445,6 @@ def should_finish_experiment(experiment_name, should_finish=true) end shared_examples_for "a disabled test" do - describe 'ab_test' do it 'should return the control' do alternative = ab_test('link_color', 'blue', 'red') @@ -460,7 +452,6 @@ def should_finish_experiment(experiment_name, should_finish=true) end it "should not increment the participation count" do - previous_red_count = Split::Alternative.new('red', 'link_color').participant_count previous_blue_count = Split::Alternative.new('blue', 'link_color').participant_count @@ -489,9 +480,7 @@ def should_finish_experiment(experiment_name, should_finish=true) end describe 'when ip address is ignored' do - context "individually" do - before(:each) do @request = OpenStruct.new(:ip => '81.19.48.130') Split.configure do |c| @@ -500,11 +489,9 @@ def should_finish_experiment(experiment_name, should_finish=true) end it_behaves_like "a disabled test" - end context "for a range" do - before(:each) do @request = OpenStruct.new(:ip => '81.19.48.129') Split.configure do |c| @@ -513,11 +500,9 @@ def should_finish_experiment(experiment_name, should_finish=true) end it_behaves_like "a disabled test" - end context "using both a range and a specific value" do - before(:each) do @request = OpenStruct.new(:ip => '81.19.48.128') Split.configure do |c| @@ -527,9 +512,7 @@ def should_finish_experiment(experiment_name, should_finish=true) end it_behaves_like "a disabled test" - end - end describe 'versioned experiments' do @@ -602,13 +585,11 @@ def should_finish_experiment(experiment_name, should_finish=true) end context 'when redis is not available' do - before(:each) do Split.stub(:redis).and_raise(Errno::ECONNREFUSED.new) end context 'and db_failover config option is turned off' do - before(:each) do Split.configure do |config| config.db_failover = false @@ -617,22 +598,17 @@ def should_finish_experiment(experiment_name, should_finish=true) describe 'ab_test' do it 'should raise an exception' do - lambda { - ab_test('link_color', 'blue', 'red') - }.should raise_error + lambda { ab_test('link_color', 'blue', 'red') }.should raise_error end end describe 'finished' do it 'should raise an exception' do - lambda { - finished('link_color') - }.should raise_error + lambda { finished('link_color') }.should raise_error end end describe "disable split testing" do - before(:each) do Split.configure do |config| config.enabled = false @@ -646,26 +622,17 @@ def should_finish_experiment(experiment_name, should_finish=true) end it "should not attempt to connect to redis" do - - lambda { - ab_test('link_color', 'blue', 'red') - }.should_not raise_error + lambda { ab_test('link_color', 'blue', 'red') }.should_not raise_error end it "should return control variable" do ab_test('link_color', 'blue', 'red').should eq('blue') - lambda { - finished('link_color') - }.should_not raise_error + lambda { finished('link_color') }.should_not raise_error end - end - - end context 'and db_failover config option is turned on' do - before(:each) do Split.configure do |config| config.db_failover = true @@ -674,10 +641,9 @@ def should_finish_experiment(experiment_name, should_finish=true) describe 'ab_test' do it 'should not raise an exception' do - lambda { - ab_test('link_color', 'blue', 'red') - }.should_not raise_error + lambda { ab_test('link_color', 'blue', 'red') }.should_not raise_error end + it 'should call db_failover_on_db_error proc with error as parameter' do Split.configure do |config| config.db_failover_on_db_error = proc do |error| @@ -687,6 +653,7 @@ def should_finish_experiment(experiment_name, should_finish=true) Split.configuration.db_failover_on_db_error.should_receive(:call) ab_test('link_color', 'blue', 'red') end + it 'should always use first alternative' do ab_test('link_color', 'blue', 'red').should eq('blue') ab_test('link_color', {'blue' => 0.01}, 'red' => 0.2).should eq('blue') @@ -732,16 +699,16 @@ def should_finish_experiment(experiment_name, should_finish=true) describe 'finished' do it 'should not raise an exception' do - lambda { - finished('link_color') - }.should_not raise_error + lambda { finished('link_color') }.should_not raise_error end + it 'should call db_failover_on_db_error proc with error as parameter' do Split.configure do |config| config.db_failover_on_db_error = proc do |error| error.should be_a(Errno::ECONNREFUSED) end end + Split.configuration.db_failover_on_db_error.should_receive(:call) finished('link_color') end @@ -812,7 +779,6 @@ def should_finish_experiment(experiment_name, should_finish=true) ab_test :my_experiment experiment = Split::Experiment.new(:my_experiment) experiment.alternatives.collect{|a| [a.name, a.weight]}.should == [['control_opt', 0.67], ['second_opt', 0.1], ['third_opt', 0.23]] - end it "accepts probability on some alternatives" do @@ -842,7 +808,7 @@ def should_finish_experiment(experiment_name, should_finish=true) ab_test :my_experiment experiment = Split::Experiment.new(:my_experiment) names_and_weights = experiment.alternatives.collect{|a| [a.name, a.weight]} - names_and_weights.should == [['control_opt', 0.18], ['second_opt', 0.18], ['third_opt', 0.64]] + names_and_weights.should == [['control_opt', 0.18], ['second_opt', 0.18], ['third_opt', 0.64]] names_and_weights.inject(0){|sum, nw| sum + nw[1]}.should == 1.0 end From 04e01441e08e7e066280e5fe160fde2bcb84d257 Mon Sep 17 00:00:00 2001 From: Ian Vaughan Date: Mon, 10 Mar 2014 08:51:49 +0000 Subject: [PATCH 2/3] Don't need a after block --- spec/helper_spec.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb index 5cc92ddf..baac05c8 100755 --- a/spec/helper_spec.rb +++ b/spec/helper_spec.rb @@ -615,12 +615,6 @@ def should_finish_experiment(experiment_name, should_finish=true) end end - after(:each) do - Split.configure do |config| - config.enabled = true - end - end - it "should not attempt to connect to redis" do lambda { ab_test('link_color', 'blue', 'red') }.should_not raise_error end From e645f0e537fb870b5a6cf7d956dbc46b097b4690 Mon Sep 17 00:00:00 2001 From: Ian Vaughan Date: Mon, 10 Mar 2014 08:48:32 +0000 Subject: [PATCH 3/3] Only rescue from known expected error --- lib/split/helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/split/helper.rb b/lib/split/helper.rb index 27881825..794e1f8e 100644 --- a/lib/split/helper.rb +++ b/lib/split/helper.rb @@ -27,7 +27,7 @@ def ab_test(metric_descriptor, control=nil, *alternatives) else control_variable(control) end - rescue => e + rescue Errno::ECONNREFUSED => e raise(e) unless Split.configuration.db_failover Split.configuration.db_failover_on_db_error.call(e)