Skip to content

Tidy ups #238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 11, 2014
Merged

Tidy ups #238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2ddf8c
Whitespace and wrapped long lines
Mar 9, 2014
04e0144
Don't need a after block
Mar 10, 2014
e645f0e
Only rescue from known expected error
Mar 10, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/split/experiment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 11 additions & 10 deletions lib/split/helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,35 +6,36 @@ 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
start_trial( Trial.new(:experiment => experiment) )
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)

if Split.configuration.db_failover_allow_parameter_override && override_present?(experiment_name)
ret = override_alternative(experiment_name)
end
ensure
unless ret
ret = control_variable(control)
end
ret ||= control_variable(control)
end

if block_given?
Expand Down
16 changes: 3 additions & 13 deletions spec/alternative_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -248,7 +238,7 @@

alternative2.participant_count = 142
alternative2.set_completed_count(119)

alternative2.z_score.round(2).should eql(2.58)
end

Expand Down
68 changes: 14 additions & 54 deletions spec/helper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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')
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -277,7 +273,6 @@
finished(@experiment_name)
end
end

end

context "finished with config" do
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -452,15 +445,13 @@ 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')
alternative.should eql experiment.control.name
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

Expand Down Expand Up @@ -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|
Expand All @@ -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|
Expand All @@ -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|
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -617,55 +598,35 @@ 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
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
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
Expand All @@ -674,10 +635,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|
Expand All @@ -687,6 +647,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')
Expand Down Expand Up @@ -732,16 +693,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
Expand Down Expand Up @@ -812,7 +773,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
Expand Down Expand Up @@ -842,7 +802,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

Expand Down