Merge branch 'jj-clean-application-experiment' into 'master'

Migrates RoundRobin rollout strategy into the invite_email_experiment

See merge request gitlab-org/gitlab!56419

app/experiments/application_experiment.rb

@@ -35,40 +35,13 @@ class ApplicationExperiment < Gitlab::Experiment # rubocop:disable Gitlab/Namesp
     @excluded = true
   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
 
   def feature_flag_name
     name.tr('/', '_')
   end
 
-  def resolve_variant_name
-    case rollout_strategy
-    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.
+  def experiment_group?
+    Feature.enabled?(feature_flag_name, self, type: :experiment, default_enabled: :yaml)
   end
 end

app/experiments/members/invite_email_experiment.rb

@@ -7,12 +7,8 @@ module Members
 
     INVITE_TYPE = 'initial_email'
 
-    def rollout_strategy
-      :round_robin
-    end
-
-    def variants
-      %i[avatar permission_info control]
+    def resolve_variant_name
+      Strategy::RoundRobin.new(feature_flag_name, %i[avatar permission_info control]).execute
     end
   end
 end

spec/experiments/application_experiment_spec.rb

@@ -114,20 +114,17 @@ RSpec.describe ApplicationExperiment, :experiment do
   end
 
   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
       expect(Feature).to receive(:enabled?).with('namespaced_stub', subject, type: :experiment, default_enabled: :yaml)
 
       expect(subject.variant.name).to eq('control')
     end
 
-      it "returns nil when not rolled out" do
-        stub_feature_flags(namespaced_stub: false)
-
-        expect(subject.variant.name).to eq('control')
+    context "when rolled out to 100%" do
+      before do
+        stub_feature_flags(namespaced_stub: true)
       end
 
-      context "when rolled out to 100%" do
       it "returns the first variant name" do
         subject.try(:variant1) {}
         subject.try(:variant2) {}
@@ -137,58 +134,6 @@ RSpec.describe ApplicationExperiment, :experiment do
     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
     let(:cache) { Gitlab::Experiment::Configuration.cache }
 

spec/experiments/members/invite_email_experiment_spec.rb

@@ -3,26 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe Members::InviteEmailExperiment do
-  subject :invite_email do
-    experiment('members/invite_email', actor: double('Member', created_by: double('User', avatar_url: '_avatar_url_')))
-  end
+  subject(:invite_email) { experiment('members/invite_email', **context) }
+
+  let(:context) { { actor: double('Member', created_by: double('User', avatar_url: '_avatar_url_')) } }
 
   before do
     allow(invite_email).to receive(:enabled?).and_return(true)
   end
 
-  describe "#rollout_strategy" do
-    it "resolves to round_robin" do
-      expect(invite_email.rollout_strategy).to eq(:round_robin)
-    end
-  end
-
-  describe "#variants" do
-    it "has all the expected variants" do
-      expect(invite_email.variants).to match(%i[avatar permission_info control])
-    end
-  end
-
   describe "exclusions", :experiment do
     it "excludes when created by is nil" do
       expect(experiment('members/invite_email')).to exclude(actor: double(created_by: nil))
@@ -34,4 +22,27 @@ RSpec.describe Members::InviteEmailExperiment do
       expect(experiment('members/invite_email')).to exclude(actor: member_without_avatar_url)
     end
   end
+
+  describe "variant resolution", :clean_gitlab_redis_shared_state do
+    it "proves out round robin in variant selection", :aggregate_failures do
+      instance_1 = described_class.new('members/invite_email', **context)
+      allow(instance_1).to receive(:enabled?).and_return(true)
+      instance_2 = described_class.new('members/invite_email', **context)
+      allow(instance_2).to receive(:enabled?).and_return(true)
+      instance_3 = described_class.new('members/invite_email', **context)
+      allow(instance_3).to receive(:enabled?).and_return(true)
+
+      instance_1.try { }
+
+      expect(instance_1.variant.name).to eq('permission_info')
+
+      instance_2.try { }
+
+      expect(instance_2.variant.name).to eq('control')
+
+      instance_3.try { }
+
+      expect(instance_3.variant.name).to eq('avatar')
+    end
+  end
 end