Add latest changes from gitlab-org/gitlab@13-10-stable-eev13.10.0-rc40

3 files changed, 84 insertions, 155 deletions

diff --git a/spec/experiments/application_experiment/cache_spec.rb b/spec/experiments/application_experiment/cache_spec.rb
deleted file mode 100644
index 4caa91e6ac4..00000000000
--- a/spec/experiments/application_experiment/cache_spec.rb
+++ /dev/null
@@ -1,54 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe ApplicationExperiment::Cache do
-  let(:key_name) { 'experiment_name' }
-  let(:field_name) { 'abc123' }
-  let(:key_field) { [key_name, field_name].join(':') }
-  let(:shared_state) { Gitlab::Redis::SharedState }
-
-  around do |example|
-    shared_state.with { |r| r.del(key_name) }
-    example.run
-    shared_state.with { |r| r.del(key_name) }
-  end
-
-  it "allows reading, writing and deleting", :aggregate_failures do
-    # we test them all together because they are largely interdependent
-
-    expect(subject.read(key_field)).to be_nil
-    expect(shared_state.with { |r| r.hget(key_name, field_name) }).to be_nil
-
-    subject.write(key_field, 'value')
-
-    expect(subject.read(key_field)).to eq('value')
-    expect(shared_state.with { |r| r.hget(key_name, field_name) }).to eq('value')
-
-    subject.delete(key_field)
-
-    expect(subject.read(key_field)).to be_nil
-    expect(shared_state.with { |r| r.hget(key_name, field_name) }).to be_nil
-  end
-
-  it "handles the fetch with a block behavior (which is read/write)" do
-    expect(subject.fetch(key_field) { 'value1' }).to eq('value1') # rubocop:disable Style/RedundantFetchBlock
-    expect(subject.fetch(key_field) { 'value2' }).to eq('value1') # rubocop:disable Style/RedundantFetchBlock
-  end
-
-  it "can clear a whole experiment cache key" do
-    subject.write(key_field, 'value')
-    subject.clear(key: key_field)
-
-    expect(shared_state.with { |r| r.get(key_name) }).to be_nil
-  end
-
-  it "doesn't allow clearing a key from the cache that's not a hash (definitely not an experiment)" do
-    shared_state.with { |r| r.set(key_name, 'value') }
-
-    expect { subject.clear(key: key_name) }.to raise_error(
-      ArgumentError,
-      'invalid call to clear a non-hash cache key'
-    )
-  end
-end
diff --git a/spec/experiments/application_experiment_spec.rb b/spec/experiments/application_experiment_spec.rb
index 501d344e920..2481ee5a806 100644
--- a/spec/experiments/application_experiment_spec.rb
+++ b/spec/experiments/application_experiment_spec.rb
@@ -58,30 +58,38 @@ RSpec.describe ApplicationExperiment, :experiment do
   end
 
   describe "publishing results" do
+    it "doesn't track or push data to the client if we shouldn't track", :snowplow do
+      allow(subject).to receive(:should_track?).and_return(false)
+      expect(Gon).not_to receive(:push)
+
+      subject.publish(:action)
+
+      expect_no_snowplow_event
+    end
+
     it "tracks the assignment" do
       expect(subject).to receive(:track).with(:assignment)
 
-      subject.publish(nil)
+      subject.publish
     end
 
-    it "pushes the experiment knowledge into the client using Gon.global" do
-      expect(Gon.global).to receive(:push).with(
-        {
-          experiment: {
-            'namespaced/stub' => { # string key because it can be namespaced
-              experiment: 'namespaced/stub',
-              key: '86208ac54ca798e11f127e8b23ec396a',
-              variant: 'control'
-            }
-          }
-        },
-        true
-      )
+    it "pushes the experiment knowledge into the client using Gon" do
+      expect(Gon).to receive(:push).with({ experiment: { 'namespaced/stub' => subject.signature } }, true)
+
+      subject.publish
+    end
 
-      subject.publish(nil)
+    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 }.not_to raise_error
     end
   end
 
+  it "can exclude from within the block" do
+    expect(described_class.new('namespaced/stub') { |e| e.exclude! }).to be_excluded
+  end
+
   describe "tracking events", :snowplow do
     it "doesn't track if we shouldn't track" do
       allow(subject).to receive(:should_track?).and_return(false)
@@ -115,91 +123,36 @@ 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 "uses the default value as specified in the yaml" do
+      expect(Feature).to receive(:enabled?).with('namespaced_stub', subject, type: :experiment, default_enabled: :yaml)
 
-      it "returns nil when not rolled out" do
-        stub_feature_flags(namespaced_stub: false)
-
-        expect(subject.variant.name).to eq('control')
-      end
-
-      context "when rolled out to 100%" do
-        it "returns the first variant name" do
-          subject.try(:variant1) {}
-          subject.try(:variant2) {}
-
-          expect(subject.variant.name).to eq('variant1')
-        end
-      end
+      expect(subject.variant.name).to eq('control')
     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
+    context "when rolled out to 100%" do
+      before do
+        stub_feature_flags(namespaced_stub: true)
       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')
+      it "returns the first variant name" do
+        subject.try(:variant1) {}
+        subject.try(:variant2) {}
 
-          instance_2.try {}
-
-          expect(instance_2.variant.name).to eq('control')
-
-          instance_3.try {}
-
-          expect(instance_3.variant.name).to eq('variant1')
-        end
+        expect(subject.variant.name).to eq('variant1')
       end
     end
   end
 
   context "when caching" do
-    let(:cache) { ApplicationExperiment::Cache.new }
+    let(:cache) { Gitlab::Experiment::Configuration.cache }
 
     before do
+      allow(Gitlab::Experiment::Configuration).to receive(:cache).and_call_original
+
       cache.clear(key: subject.name)
 
       subject.use { } # setup the control
       subject.try { } # setup the candidate
-
-      allow(Gitlab::Experiment::Configuration).to receive(:cache).and_return(cache)
     end
 
     it "caches the variant determined by the variant resolver" do
@@ -207,7 +160,7 @@ RSpec.describe ApplicationExperiment, :experiment do
 
       subject.run
 
-      expect(cache.read(subject.cache_key)).to eq('candidate')
+      expect(subject.cache.read).to eq('candidate')
     end
 
     it "doesn't cache a variant if we don't explicitly provide one" do
@@ -222,7 +175,7 @@ RSpec.describe ApplicationExperiment, :experiment do
 
       subject.run
 
-      expect(cache.read(subject.cache_key)).to be_nil
+      expect(subject.cache.read).to be_nil
     end
 
     it "caches a control variant if we assign it specifically" do
@@ -232,7 +185,26 @@ RSpec.describe ApplicationExperiment, :experiment do
       # write code that would specify a different variant.
       subject.run(:control)
 
-      expect(cache.read(subject.cache_key)).to eq('control')
+      expect(subject.cache.read).to eq('control')
+    end
+
+    context "arbitrary attributes" do
+      before do
+        subject.cache.store.clear(key: subject.name + '_attrs')
+      end
+
+      it "sets and gets attributes about an experiment" do
+        subject.cache.attr_set(:foo, :bar)
+
+        expect(subject.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(subject.cache.attr_inc(:foo)).to eq(1)
+        expect(subject.cache.attr_inc(:foo)).to eq(2)
+      end
     end
   end
 end
diff --git a/spec/experiments/members/invite_email_experiment_spec.rb b/spec/experiments/members/invite_email_experiment_spec.rb
index 4376c021385..539230e39b9 100644
--- a/spec/experiments/members/invite_email_experiment_spec.rb
+++ b/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