diff --git a/app/experiments/application_experiment.rb b/app/experiments/application_experiment.rb
index 37d87baf30b6b476d2b49a5ab646afa4fa094996..22c632a08aa2fd3aa710ea047673a0d1b9d5c55b 100644
--- a/app/experiments/application_experiment.rb
+++ b/app/experiments/application_experiment.rb
@@ -32,8 +32,8 @@ def publish_to_database
subject = value[:namespace] || value[:group] || value[:project] || value[:user] || value[:actor]
return unless ExperimentSubject.valid_subject?(subject)
- variant = :experimental if @variant_name != :control
- Experiment.add_subject(name, variant: variant || :control, subject: subject)
+ variant_name = :experimental if variant&.name != 'control'
+ Experiment.add_subject(name, variant: variant_name || :control, subject: subject)
end
def record!
diff --git a/spec/experiments/application_experiment_spec.rb b/spec/experiments/application_experiment_spec.rb
index b0788eec808a40782e3b18cbef3f9954df2778c0..dda8ca66eeea2162eb4c16cceb078b3381b40b1e 100644
--- a/spec/experiments/application_experiment_spec.rb
+++ b/spec/experiments/application_experiment_spec.rb
@@ -3,7 +3,7 @@
require 'spec_helper'
RSpec.describe ApplicationExperiment, :experiment do
- subject { described_class.new('namespaced/stub', **context) }
+ subject(:application_experiment) { described_class.new('namespaced/stub', **context) }
let(:context) { {} }
let(:feature_definition) { { name: 'namespaced_stub', type: 'experiment', default_enabled: false } }
@@ -15,7 +15,7 @@
end
before do
- allow(subject).to receive(:enabled?).and_return(true)
+ allow(application_experiment).to receive(:enabled?).and_return(true)
end
it "doesn't raise an exception without a defined control" do
@@ -26,7 +26,7 @@
describe "#enabled?" do
before do
- allow(subject).to receive(:enabled?).and_call_original
+ allow(application_experiment).to receive(:enabled?).and_call_original
allow(Feature::Definition).to receive(:get).and_return('_instance_')
allow(Gitlab).to receive(:dev_env_or_com?).and_return(true)
@@ -34,25 +34,25 @@
end
it "is enabled when all criteria are met" do
- expect(subject).to be_enabled
+ expect(application_experiment).to be_enabled
end
it "isn't enabled if the feature definition doesn't exist" do
expect(Feature::Definition).to receive(:get).with('namespaced_stub').and_return(nil)
- expect(subject).not_to be_enabled
+ expect(application_experiment).not_to be_enabled
end
it "isn't enabled if we're not in dev or dotcom environments" do
expect(Gitlab).to receive(:dev_env_or_com?).and_return(false)
- expect(subject).not_to be_enabled
+ expect(application_experiment).not_to be_enabled
end
it "isn't enabled if the feature flag state is :off" do
expect(Feature).to receive(:get).with('namespaced_stub').and_return(double(state: :off))
- expect(subject).not_to be_enabled
+ expect(application_experiment).not_to be_enabled
end
end
@@ -60,11 +60,11 @@
let(:should_track) { true }
before do
- allow(subject).to receive(:should_track?).and_return(should_track)
+ allow(application_experiment).to receive(:should_track?).and_return(should_track)
end
it "tracks the assignment", :snowplow do
- subject.publish
+ application_experiment.publish
expect_snowplow_event(
category: 'namespaced/stub',
@@ -74,24 +74,24 @@
end
it "publishes to the client" do
- expect(subject).to receive(:publish_to_client)
+ expect(application_experiment).to receive(:publish_to_client)
- subject.publish
+ application_experiment.publish
end
it "publishes to the database if we've opted for that" do
- subject.record!
+ application_experiment.record!
- expect(subject).to receive(:publish_to_database)
+ expect(application_experiment).to receive(:publish_to_database)
- subject.publish
+ application_experiment.publish
end
context 'when we should not track' do
let(:should_track) { false }
it 'does not track an event to Snowplow', :snowplow do
- subject.publish
+ application_experiment.publish
expect_no_snowplow_event
end
@@ -102,13 +102,13 @@
signature = { key: '86208ac54ca798e11f127e8b23ec396a', variant: 'control' }
expect(Gon).to receive(:push).with({ experiment: { 'namespaced/stub' => hash_including(signature) } }, true)
- subject.publish_to_client
+ application_experiment.publish_to_client
end
it "handles when Gon raises exceptions (like when it can't be pushed into)" do
expect(Gon).to receive(:push).and_raise(NoMethodError)
- expect { subject.publish_to_client }.not_to raise_error
+ expect { application_experiment.publish_to_client }.not_to raise_error
end
context 'when we should not track' do
@@ -117,7 +117,7 @@
it 'returns early' do
expect(Gon).not_to receive(:push)
- subject.publish_to_client
+ application_experiment.publish_to_client
end
end
end
@@ -125,13 +125,15 @@
describe '#publish_to_database' do
using RSpec::Parameterized::TableSyntax
+ let(:publish_to_database) { application_experiment.publish_to_database }
+
shared_examples 'does not record to the database' do
it 'does not create an experiment record' do
- expect { subject.publish_to_database }.not_to change(Experiment, :count)
+ expect { publish_to_database }.not_to change(Experiment, :count)
end
it 'does not create an experiment subject record' do
- expect { subject.publish_to_database }.not_to change(ExperimentSubject, :count)
+ expect { publish_to_database }.not_to change(ExperimentSubject, :count)
end
end
@@ -139,16 +141,16 @@
let(:context) { { context_key => context_value } }
where(:context_key, :context_value, :object_type) do
- :namespace | build(:namespace) | :namespace
- :group | build(:namespace) | :namespace
- :project | build(:project) | :project
- :user | build(:user) | :user
- :actor | build(:user) | :user
+ :namespace | build(:namespace, id: non_existing_record_id) | :namespace
+ :group | build(:namespace, id: non_existing_record_id) | :namespace
+ :project | build(:project, id: non_existing_record_id) | :project
+ :user | build(:user, id: non_existing_record_id) | :user
+ :actor | build(:user, id: non_existing_record_id) | :user
end
with_them do
it 'creates an experiment and experiment subject record' do
- expect { subject.publish_to_database }.to change(Experiment, :count).by(1)
+ expect { publish_to_database }.to change(Experiment, :count).by(1)
expect(Experiment.last.name).to eq('namespaced/stub')
expect(ExperimentSubject.last.send(object_type)).to eq(context[context_key])
@@ -156,6 +158,16 @@
end
end
+ context "when experiment hasn't ran" do
+ let(:context) { { user: create(:user) } }
+
+ it 'sets a variant on the experiment subject' do
+ publish_to_database
+
+ expect(ExperimentSubject.last.variant).to eq('control')
+ end
+ end
+
context 'when there is not a usable subject' do
let(:context) { { context_key => context_value } }
@@ -183,15 +195,15 @@
end
it "doesn't track if we shouldn't track" do
- allow(subject).to receive(:should_track?).and_return(false)
+ allow(application_experiment).to receive(:should_track?).and_return(false)
- subject.track(:action)
+ application_experiment.track(:action)
expect_no_snowplow_event
end
it "tracks the event with the expected arguments and merged contexts" do
- subject.track(:action, property: '_property_', context: [fake_context])
+ application_experiment.track(:action, property: '_property_', context: [fake_context])
expect_snowplow_event(
category: 'namespaced/stub',
@@ -233,7 +245,7 @@
describe "#key_for" do
it "generates MD5 hashes" do
- expect(subject.key_for(foo: :bar)).to eq('6f9ac12afdb9b58c2f19a136d09f9153')
+ expect(application_experiment.key_for(foo: :bar)).to eq('6f9ac12afdb9b58c2f19a136d09f9153')
end
end
@@ -256,26 +268,26 @@
with_them do
it "returns the url or nil if invalid" do
allow(Gitlab).to receive(:dev_env_or_com?).and_return(true)
- expect(subject.process_redirect_url(url)).to eq(processed_url)
+ expect(application_experiment.process_redirect_url(url)).to eq(processed_url)
end
it "considers all urls invalid when not on dev or com" do
allow(Gitlab).to receive(:dev_env_or_com?).and_return(false)
- expect(subject.process_redirect_url(url)).to be_nil
+ expect(application_experiment.process_redirect_url(url)).to be_nil
end
end
it "generates the correct urls based on where the engine was mounted" do
- url = Rails.application.routes.url_helpers.experiment_redirect_url(subject, url: 'https://docs.gitlab.com')
- expect(url).to include("/-/experiment/namespaced%2Fstub:#{subject.context.key}?https://docs.gitlab.com")
+ url = Rails.application.routes.url_helpers.experiment_redirect_url(application_experiment, url: 'https://docs.gitlab.com')
+ expect(url).to include("/-/experiment/namespaced%2Fstub:#{application_experiment.context.key}?https://docs.gitlab.com")
end
end
context "when resolving variants" do
it "uses the default value as specified in the yaml" do
- expect(Feature).to receive(:enabled?).with('namespaced_stub', subject, type: :experiment, default_enabled: :yaml)
+ expect(Feature).to receive(:enabled?).with('namespaced_stub', application_experiment, type: :experiment, default_enabled: :yaml)
- expect(subject.variant.name).to eq('control')
+ expect(application_experiment.variant.name).to eq('control')
end
context "when rolled out to 100%" do
@@ -284,10 +296,10 @@
end
it "returns the first variant name" do
- subject.try(:variant1) {}
- subject.try(:variant2) {}
+ application_experiment.try(:variant1) {}
+ application_experiment.try(:variant2) {}
- expect(subject.variant.name).to eq('variant1')
+ expect(application_experiment.variant.name).to eq('variant1')
end
end
end
@@ -298,18 +310,18 @@
before do
allow(Gitlab::Experiment::Configuration).to receive(:cache).and_call_original
- cache.clear(key: subject.name)
+ cache.clear(key: application_experiment.name)
- subject.use { } # setup the control
- subject.try { } # setup the candidate
+ application_experiment.use { } # setup the control
+ application_experiment.try { } # setup the candidate
end
it "caches the variant determined by the variant resolver" do
- expect(subject.variant.name).to eq('candidate') # we should be in the experiment
+ expect(application_experiment.variant.name).to eq('candidate') # we should be in the experiment
- subject.run
+ application_experiment.run
- expect(subject.cache.read).to eq('candidate')
+ expect(application_experiment.cache.read).to eq('candidate')
end
it "doesn't cache a variant if we don't explicitly provide one" do
@@ -320,11 +332,11 @@
# the control.
stub_feature_flags(namespaced_stub: false) # simulate being not rolled out
- expect(subject.variant.name).to eq('control') # if we ask, it should be control
+ expect(application_experiment.variant.name).to eq('control') # if we ask, it should be control
- subject.run
+ application_experiment.run
- expect(subject.cache.read).to be_nil
+ expect(application_experiment.cache.read).to be_nil
end
it "caches a control variant if we assign it specifically" do
@@ -332,27 +344,27 @@
# that this context will always get the control variant unless we delete
# the field from the cache (or clear the entire experiment cache) -- or
# write code that would specify a different variant.
- subject.run(:control)
+ application_experiment.run(:control)
- expect(subject.cache.read).to eq('control')
+ expect(application_experiment.cache.read).to eq('control')
end
context "arbitrary attributes" do
before do
- subject.cache.store.clear(key: subject.name + '_attrs')
+ application_experiment.cache.store.clear(key: application_experiment.name + '_attrs')
end
it "sets and gets attributes about an experiment" do
- subject.cache.attr_set(:foo, :bar)
+ application_experiment.cache.attr_set(:foo, :bar)
- expect(subject.cache.attr_get(:foo)).to eq('bar')
+ expect(application_experiment.cache.attr_get(:foo)).to eq('bar')
end
it "increments a value for an experiment" do
- expect(subject.cache.attr_get(:foo)).to be_nil
+ expect(application_experiment.cache.attr_get(:foo)).to be_nil
- expect(subject.cache.attr_inc(:foo)).to eq(1)
- expect(subject.cache.attr_inc(:foo)).to eq(2)
+ expect(application_experiment.cache.attr_inc(:foo)).to eq(1)
+ expect(application_experiment.cache.attr_inc(:foo)).to eq(2)
end
end
end