Commit ad96546c authored by jejacks0n's avatar jejacks0n

Remove rollout concepts from ApplicationExperiment

- These ideas inspired the concept to be put into the base library and
changed the interface — the boolean inclusion/exclusion is now provided
by the `experiment_group?` method.
parent 7b257c7c
...@@ -35,40 +35,13 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp ...@@ -35,40 +35,13 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
@excluded = true @excluded = true
end end
def rollout_strategy
# no-op override in inherited class as desired
end
def variants
# override as desired in inherited class with all variants + control
# %i[variant1 variant2 control]
#
# this will make sure we supply variants as these go together - rollout_strategy of :round_robin must have variants
raise NotImplementedError, "Inheriting class must supply variants as an array if :round_robin strategy is used" if rollout_strategy == :round_robin
end
private private
def feature_flag_name def feature_flag_name
name.tr('/', '_') name.tr('/', '_')
end end
def resolve_variant_name def experiment_group?
case rollout_strategy Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
when :round_robin
round_robin_rollout
else
percentage_rollout
end
end
def round_robin_rollout
Strategy::RoundRobin.new(feature_flag_name, variants).execute
end
def percentage_rollout
return variant_names.first if Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
nil # Returning nil vs. :control is important for not caching and rollouts.
end end
end end
...@@ -114,20 +114,17 @@ RSpec.describe ApplicationExperiment, :experiment do ...@@ -114,20 +114,17 @@ RSpec.describe ApplicationExperiment, :experiment do
end end
describe "variant resolution" do describe "variant resolution" do
context "when using the default feature flag percentage rollout" do
it "uses the default value as specified in the yaml" 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', subject, type: :experiment, default_enabled: :yaml)
expect(subject.variant.name).to eq('control') expect(subject.variant.name).to eq('control')
end end
it "returns nil when not rolled out" do context "when rolled out to 100%" do
stub_feature_flags(namespaced_stub: false) before do
stub_feature_flags(namespaced_stub: true)
expect(subject.variant.name).to eq('control')
end end
context "when rolled out to 100%" do
it "returns the first variant name" do it "returns the first variant name" do
subject.try(:variant1) {} subject.try(:variant1) {}
subject.try(:variant2) {} subject.try(:variant2) {}
...@@ -137,58 +134,6 @@ RSpec.describe ApplicationExperiment, :experiment do ...@@ -137,58 +134,6 @@ RSpec.describe ApplicationExperiment, :experiment do
end end
end end
context "when using the round_robin strategy", :clean_gitlab_redis_shared_state do
context "when variants aren't supplied" do
subject :inheriting_class do
Class.new(described_class) do
def rollout_strategy
:round_robin
end
end.new('namespaced/stub')
end
it "raises an error" do
expect { inheriting_class.variants }.to raise_error(NotImplementedError)
end
end
context "when variants are supplied" do
let(:inheriting_class) do
Class.new(described_class) do
def rollout_strategy
:round_robin
end
def variants
%i[variant1 variant2 control]
end
end
end
it "proves out round robin in variant selection", :aggregate_failures do
instance_1 = inheriting_class.new('namespaced/stub')
allow(instance_1).to receive(:enabled?).and_return(true)
instance_2 = inheriting_class.new('namespaced/stub')
allow(instance_2).to receive(:enabled?).and_return(true)
instance_3 = inheriting_class.new('namespaced/stub')
allow(instance_3).to receive(:enabled?).and_return(true)
instance_1.try {}
expect(instance_1.variant.name).to eq('variant2')
instance_2.try {}
expect(instance_2.variant.name).to eq('control')
instance_3.try {}
expect(instance_3.variant.name).to eq('variant1')
end
end
end
end
context "when caching" do context "when caching" do
let(:cache) { Gitlab::Experiment::Configuration.cache } let(:cache) { Gitlab::Experiment::Configuration.cache }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment