From 556d8a0a4fabdf82bb26b6965beb24ac03f29a5f Mon Sep 17 00:00:00 2001 From: Gabriel Schammah Date: Wed, 7 Oct 2015 00:00:22 -0300 Subject: [PATCH 1/2] add missing tests --- lib/split/helper.rb | 34 ++++-- spec/helper_spec.rb | 287 +++++++++++++++++++++++++++++--------------- spec/spec_helper.rb | 1 + split.gemspec | 1 + 4 files changed, 219 insertions(+), 104 deletions(-) diff --git a/lib/split/helper.rb b/lib/split/helper.rb index 3c8dccc5..fdb0b905 100644 --- a/lib/split/helper.rb +++ b/lib/split/helper.rb @@ -95,23 +95,39 @@ def begin_experiment(experiment, alternative_name = nil) # alternative can be either a string or an array of strings def ensure_alternative_or_exclude(experiment_name, alternatives) - return if @alternative_ensured alternatives = Array(alternatives) + experiment = ExperimentCatalog.find(experiment_name) + + # This is called if the experiment is not yet created. Since we can't created (we need the alternatives + # configuration to be passed to this method), we exclude the user, and when `ab_test` is called after + # the experiment is created. This means the first user will be excluded from the test and will have + # the control alternative + if !experiment + experiment = Experiment.new(experiment_name) + exclude_visitor(experiment) + return + end - # if ab_user was not called before, then force the alternative and manually increment the counter. - unless experiment = ExperimentCatalog.find(experiment_name) - ab_user[experiment_name] = default_alternative = alternatives.first - Split::Alternative.new(default_alternative, experiment_name).increment_participation - @alternative_ensured = true + # If the user doesn't have an alternative set, we override the alternative and we manually increment the + # participant count, since split doesn't do it when an alternative is overriden + unless ab_user[experiment.key] + params[experiment_name] = ab_user[experiment.key] = alternatives.sample + Split::Alternative.new(ab_user[experiment.key], experiment_name).increment_participation return end current_alternative = ab_user[experiment.key] - return if exclude_visitor?(experiment) || alternatives.include?(current_alternative) + # We don't decrement the participant count if the visitor is included or if the desired alternative + # was already chosen by split before + if exclude_visitor?(experiment) || alternatives.include?(current_alternative) + params[experiment_name] ||= current_alternative + return + end - Split::Alternative.new(current_alternative, experiment.key).decrement_participation - params[experiment_name] = alternatives.sample + # We decrement the participant count if the desired alternative was not chosen by split before + Split::Alternative.new(current_alternative, experiment_name).decrement_participation + params[experiment_name] = ab_user[experiment.key] = alternatives.sample exclude_visitor(experiment) end diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb index 21367519..bbafe7de 100755 --- a/spec/helper_spec.rb +++ b/spec/helper_spec.rb @@ -488,165 +488,262 @@ def should_finish_experiment(experiment_name, should_finish=true) let(:current_alternative) { ab_user['link_color'] } let(:other_alternative) { (alternatives - [current_alternative]).first } - context 'when ab_test was called before' do + context 'single alternative' do - before { ab_test('link_color', 'blue', 'red') } + context 'when ab_test was called before' do - it 'should decrement the counter if the desired alternative is not the current' do - expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + before { ab_test('link_color', *alternatives) } - ensure_alternative_or_exclude('link_color', other_alternative) + it 'should decrement the counter if the desired alternative is not the current' do + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) - expect(ab_test('link_color', 'blue', 'red')).to eq(other_alternative) - expect(active_experiments).not_to include('link_color') + ensure_alternative_or_exclude('link_color', other_alternative) - expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0) - end + expect(ab_test('link_color', *alternatives)).to eq(other_alternative) + expect(active_experiments).not_to include('link_color') - it 'should not decrement the counter if the desired alternative is the current' do - expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0) - ensure_alternative_or_exclude('link_color', current_alternative) + expect(ab_user['link_color']).to eq(other_alternative) + end - expect(ab_test('link_color', 'blue', 'red')).to eq(current_alternative) - expect(active_experiments.keys).to eq(%w[link_color]) + it 'should not decrement the counter if the desired alternative is the current' do + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) - expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) - end + ensure_alternative_or_exclude('link_color', current_alternative) - it 'should not decrement the counter more than once' do - expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + expect(ab_test('link_color', *alternatives)).to eq(current_alternative) + expect(active_experiments.keys).to eq(%w[link_color]) - ensure_alternative_or_exclude('link_color', other_alternative) - ensure_alternative_or_exclude('link_color', other_alternative) + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) - expect(ab_test('link_color', 'blue', 'red')).to eq(other_alternative) - expect(active_experiments).not_to include('link_color') + expect(ab_user['link_color']).to eq(current_alternative) + end - expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0) - end + it 'should not decrement the counter more than once' do + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) - it 'should increment the conversions if the forced alternative is the current' do - expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(0) + ensure_alternative_or_exclude('link_color', other_alternative) + ensure_alternative_or_exclude('link_color', other_alternative) - ensure_alternative_or_exclude('link_color', current_alternative) + expect(ab_test('link_color', *alternatives)).to eq(other_alternative) + expect(active_experiments).not_to include('link_color') - expect(ab_test('link_color', 'blue', 'red')).to eq(current_alternative) - expect(active_experiments.keys).to eq(%w[link_color]) + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0) + + expect(ab_user['link_color']).to eq(other_alternative) + end - finished(experiment.name, reset: false) + it 'should increment the conversions if the forced alternative is the current' do + expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(0) - expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(1) - end + ensure_alternative_or_exclude('link_color', current_alternative) - it 'should not increment the conversions if the forced alternative is not the current' do - expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(0) + expect(ab_test('link_color', *alternatives)).to eq(current_alternative) + expect(active_experiments.keys).to eq(%w[link_color]) - ensure_alternative_or_exclude('link_color', other_alternative) + finished(experiment.name, reset: false) - expect(ab_test('link_color', 'blue', 'red')).to eq(other_alternative) - expect(active_experiments).not_to include('link_color') + expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(1) - finished(experiment.name, reset: false) + expect(ab_user['link_color']).to eq(current_alternative) + end - expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(0) - end - end + it 'should not increment the conversions if the forced alternative is not the current' do + expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(0) - context 'when ab_test was not previously called' do + ensure_alternative_or_exclude('link_color', other_alternative) - it 'should increment the participant counter' do - ensure_alternative_or_exclude('link_color', 'red') - expect(ab_test('link_color', 'blue', 'red')).to eq('red') - expect(active_experiments.keys).to eq(%w[link_color]) + expect(ab_test('link_color', *alternatives)).to eq(other_alternative) + expect(active_experiments).not_to include('link_color') - expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + finished(experiment.name, reset: false) + + expect(Split::Alternative.new(other_alternative, 'link_color').completed_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').completed_count).to eq(0) + + expect(ab_user['link_color']).to eq(other_alternative) + end end - it 'should not increment the participant counter more than once' do - ensure_alternative_or_exclude('link_color', 'red') - ensure_alternative_or_exclude('link_color', 'red') + context 'when ab_test was not previously called' do - expect(ab_test('link_color', 'blue', 'red')).to eq('red') - expect(active_experiments.keys).to eq(%w[link_color]) + before { Split::ExperimentCatalog.find_or_create('link_color', *alternatives) } - expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) - end + it 'should increment the participant counter' do + expect { + ensure_alternative_or_exclude('link_color', 'red') + expect(ab_test('link_color', *alternatives)).to eq('red') + }.to change { Split::Alternative.new('red', 'link_color').participant_count }.by(1) - it 'should increment the completed counter' do - ensure_alternative_or_exclude('link_color', 'red') - expect(ab_test('link_color', 'blue', 'red')).to eq('red') + expect(active_experiments.keys).to eq(%w[link_color]) + expect(ab_user['link_color']).to eq(current_alternative) + end - finished(experiment.name, reset: false) + it 'should not increment the other alternative participant counter' do + expect { + ensure_alternative_or_exclude('link_color', 'red') + expect(ab_test('link_color', *alternatives)).to eq('red') + }.not_to change { Split::Alternative.new('blue', 'link_color').participant_count } + end + + it 'should not increment the participant counter more than once' do + expect { + ensure_alternative_or_exclude('link_color', 'red') + ensure_alternative_or_exclude('link_color', 'red') + expect(ab_test('link_color', *alternatives)).to eq('red') + }.to change { Split::Alternative.new('red', 'link_color').participant_count }.by(1) + + expect(ab_user['link_color']).to eq(current_alternative) + end + + it 'should increment the completed counter' do + ensure_alternative_or_exclude('link_color', 'red') + expect(ab_test('link_color', *alternatives)).to eq('red') + + expect { + finished(experiment.name, reset: false) + }.to change { Split::Alternative.new(current_alternative, experiment.name).completed_count }.by(1) + + expect(ab_user['link_color']).to eq(current_alternative) + end - expect(Split::Alternative.new(other_alternative, experiment.name).completed_count).to eq(0) - expect(Split::Alternative.new(current_alternative, experiment.name).completed_count).to eq(1) + it 'should not increment the other alternative completed counter' do + ensure_alternative_or_exclude('link_color', 'red') + expect(ab_test('link_color', *alternatives)).to eq('red') + + expect { + finished(experiment.name, reset: false) + }.not_to change { Split::Alternative.new(other_alternative, experiment.name).completed_count } + end end end context 'multiple alternatives' do + let(:alternatives) { %w[blue red black] } let(:other_alternatives) { (alternatives - [current_alternative]) } - before { ab_test('link_color', 'blue', 'red', 'black') } + context 'when ab_test was previously called' do - it 'should decrement the counter if none of the desired alternatives are the current' do - expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + let!(:current_alternative) { ab_test('link_color', *alternatives) } - ensure_alternative_or_exclude('link_color', other_alternatives) + it 'should decrement the counter if the none of the desired alternatives are the current' do - expect(other_alternatives).to include(ab_test('link_color', 'blue', 'red', 'black')) - expect(active_experiments).not_to include('link_color') + expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) - expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0) - end + ensure_alternative_or_exclude('link_color', other_alternatives) - it 'should not decrement the counter if any of the desired alternative are the current' do - expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + expect(other_alternatives).to include(ab_test('link_color', *alternatives)) + expect(active_experiments).not_to include('link_color') - ensure_alternative_or_exclude('link_color', [current_alternative, other_alternatives.first]) + expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0) - expect(ab_test('link_color', 'blue', 'red', 'black')).to eq(current_alternative) - expect(active_experiments.keys).to eq(%w[link_color]) + expect(other_alternatives).to include(ab_user['link_color']) + end - expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0) - expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + it 'should not decrement the counter if any of the desired alternative are the current' do + expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + + ensure_alternative_or_exclude('link_color', [current_alternative, other_alternatives.first]) + + expect(ab_test('link_color', *alternatives)).to eq(current_alternative) + expect(active_experiments.keys).to eq(%w[link_color]) + + expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + + expect(ab_user['link_color']).to eq(current_alternative) + end end context 'when ab_test was not previously called' do let(:valid_alternatives) { %w[red blue] } + before { Split::ExperimentCatalog.find_or_create('link_color', *alternatives) } + it 'should increment the participant counter' do ensure_alternative_or_exclude('link_color', valid_alternatives) - expect(valid_alternatives).to include(ab_test('link_color', 'blue', 'red', 'black')) + expect(valid_alternatives).to include(ab_test('link_color', *alternatives)) expect(active_experiments.keys).to eq(%w[link_color]) expect(Split::Alternative.new(other_alternatives.first, 'link_color').participant_count).to eq(0) expect(Split::Alternative.new(other_alternatives.last, 'link_color').participant_count).to eq(0) expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + + expect(ab_user['link_color']).to eq(current_alternative) end end end + + context 'versioned experiment' do + + before do + experiment.reset + ab_test('link_color', *alternatives) + end + + let(:current_alternative) { ab_user[experiment.key] } + + it 'should decrement the counter if the desired alternative is not the current' do + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + + ensure_alternative_or_exclude('link_color', other_alternative) + expect(ab_test('link_color', *alternatives)).to eq(other_alternative) + expect(active_experiments).not_to include('link_color') + + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(0) + + expect(ab_user['link_color:1']).to eq(other_alternative) + end + + it 'should not decrement the counter if the desired alternative is the current' do + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + + ensure_alternative_or_exclude('link_color', current_alternative) + + expect(ab_test('link_color', *alternatives)).to eq(current_alternative) + expect(active_experiments.keys).to eq(%w[link_color]) + + expect(Split::Alternative.new(other_alternative, 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new(current_alternative, 'link_color').participant_count).to eq(1) + + expect(ab_user['link_color:1']).to eq(current_alternative) + end + end + + context "when the experiment is not created" do + + it "creates the experiment and excludes the user" do + ensure_alternative_or_exclude('link_color', 'red') + expect(ab_test('link_color', 'red', 'blue')).to eq('red') + expect(ab_user['link_color:excluded']).to eq(true) + + expect(Split::Alternative.new('red', 'link_color').participant_count).to eq(0) + expect(Split::Alternative.new('blue', 'link_color').participant_count).to eq(0) + end + + end end describe 'when user is a robot' do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5e0e3ced..0e22eae5 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -2,6 +2,7 @@ require 'rubygems' require 'bundler/setup' +require 'pry' require 'coveralls' Coveralls.wear! diff --git a/split.gemspec b/split.gemspec index b3b40138..7463d95c 100644 --- a/split.gemspec +++ b/split.gemspec @@ -30,4 +30,5 @@ Gem::Specification.new do |s| s.add_development_dependency 'rack-test' s.add_development_dependency 'rake' s.add_development_dependency 'rspec', '~> 3.1.0' + s.add_development_dependency 'pry' end From 17842186ce134a6ec5eb97c0a21a2aad910e96ab Mon Sep 17 00:00:00 2001 From: Gabriel Schammah Date: Wed, 7 Oct 2015 01:02:49 -0300 Subject: [PATCH 2/2] remove pry --- Gemfile | 1 + spec/helper_spec.rb | 2 +- spec/spec_helper.rb | 1 - split.gemspec | 1 - 4 files changed, 2 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 555dc077..ba32740b 100644 --- a/Gemfile +++ b/Gemfile @@ -3,3 +3,4 @@ source "https://rubygems.org" gemspec gem "appraisal" +gem "pry" diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb index bbafe7de..c3309f0f 100755 --- a/spec/helper_spec.rb +++ b/spec/helper_spec.rb @@ -737,7 +737,7 @@ def should_finish_experiment(experiment_name, should_finish=true) it "creates the experiment and excludes the user" do ensure_alternative_or_exclude('link_color', 'red') expect(ab_test('link_color', 'red', 'blue')).to eq('red') - expect(ab_user['link_color:excluded']).to eq(true) + expect(ab_user['link_color:excluded']).to be true expect(Split::Alternative.new('red', 'link_color').participant_count).to eq(0) expect(Split::Alternative.new('blue', 'link_color').participant_count).to eq(0) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 0e22eae5..5e0e3ced 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -2,7 +2,6 @@ require 'rubygems' require 'bundler/setup' -require 'pry' require 'coveralls' Coveralls.wear! diff --git a/split.gemspec b/split.gemspec index 7463d95c..b3b40138 100644 --- a/split.gemspec +++ b/split.gemspec @@ -30,5 +30,4 @@ Gem::Specification.new do |s| s.add_development_dependency 'rack-test' s.add_development_dependency 'rake' s.add_development_dependency 'rspec', '~> 3.1.0' - s.add_development_dependency 'pry' end