Merge branch 'nicolasdular/extract-experiment-from-lib' into 'master'

Extract experiment class from experimentation lib

See merge request gitlab-org/gitlab!47087

doc/development/experiment_guide/index.md

@@ -49,7 +49,6 @@ addressed.
      },
      # Add your experiment here:
      signup_flow: {
-       environment: ::Gitlab.dev_env_or_com?, # Target environment, defaults to enabled for development and GitLab.com
        tracking_category: 'Growth::Activation::Experiment::SignUpFlow' # Used for providing the category when setting up tracking data
      }
    }.freeze

ee/spec/requests/api/experiments_spec.rb

@@ -11,8 +11,7 @@ RSpec.describe API::Experiments do
       let(:experiments) do
         {
           experiment_1: {
-            tracking_category: 'something',
-            environment: true
+            tracking_category: 'something'
           },
           experiment_2: {
             tracking_category: 'something_else'

lib/gitlab/experimentation.rb

@@ -4,7 +4,6 @@
 #
 # Utility module for A/B testing experimental features. Define your experiments in the `EXPERIMENTS` constant.
 # Experiment options:
-# - environment (optional, defaults to enabled for development and GitLab.com)
 # - tracking_category (optional, used to set the category when tracking an experiment event)
 # - use_backwards_compatible_subject_index (optional, set this to true if you need backwards compatibility)
 #
@@ -87,14 +86,13 @@ module Gitlab
 
     class << self
       def experiment(key)
-        Experiment.new(EXPERIMENTS[key].merge(key: key))
+        Gitlab::Experimentation::Experiment.new(key, **EXPERIMENTS[key])
       end
 
       def enabled?(experiment_key)
         return false unless EXPERIMENTS.key?(experiment_key)
 
-        experiment = experiment(experiment_key)
-        experiment.enabled_for_environment? && experiment.enabled?
+        experiment(experiment_key).enabled?
       end
 
       def enabled_for_attribute?(experiment_key, attribute)
@@ -106,36 +104,5 @@ module Gitlab
         enabled?(experiment_key) && experiment(experiment_key).enabled_for_index?(value)
       end
     end
-
-    Experiment = Struct.new(
-      :key,
-      :environment,
-      :tracking_category,
-      :use_backwards_compatible_subject_index,
-      keyword_init: true
-    ) do
-      def enabled?
-        experiment_percentage > 0
-      end
-
-      def enabled_for_environment?
-        return ::Gitlab.dev_env_or_com? if environment.nil?
-
-        environment
-      end
-
-      def enabled_for_index?(index)
-        return false if index.blank?
-
-        index <= experiment_percentage
-      end
-
-      private
-
-      # When a feature does not exist, the `percentage_of_time_value` method will return 0
-      def experiment_percentage
-        @experiment_percentage ||= Feature.get(:"#{key}_experiment_percentage").percentage_of_time_value # rubocop:disable Gitlab/AvoidFeatureGet
-      end
-    end
   end
 end

lib/gitlab/experimentation/experiment.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Experimentation
+    class Experiment
+      attr_reader :tracking_category, :use_backwards_compatible_subject_index
+
+      def initialize(key, **params)
+        @tracking_category = params[:tracking_category]
+        @use_backwards_compatible_subject_index = params[:use_backwards_compatible_subject_index]
+
+        @experiment_percentage = Feature.get(:"#{key}_experiment_percentage").percentage_of_time_value # rubocop:disable Gitlab/AvoidFeatureGet
+      end
+
+      def enabled?
+        ::Gitlab.dev_env_or_com? && experiment_percentage > 0
+      end
+
+      def enabled_for_index?(index)
+        return false if index.blank?
+
+        index <= experiment_percentage
+      end
+
+      private
+
+      attr_reader :experiment_percentage
+    end
+  end
+end

spec/lib/gitlab/experimentation/controller_concern_spec.rb

@@ -6,12 +6,10 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
   before do
     stub_const('Gitlab::Experimentation::EXPERIMENTS', {
         backwards_compatible_test_experiment: {
-          environment: environment,
           tracking_category: 'Team',
           use_backwards_compatible_subject_index: true
         },
         test_experiment: {
-          environment: environment,
           tracking_category: 'Team'
         }
       }
@@ -21,7 +19,6 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
     Feature.enable_percentage_of_time(:test_experiment_experiment_percentage, enabled_percentage)
   end
 
-  let(:environment) { Rails.env.test? }
   let(:enabled_percentage) { 10 }
 
   controller(ApplicationController) do
@@ -391,10 +388,8 @@ RSpec.describe Gitlab::Experimentation::ControllerConcern, type: :controller do
 
     context 'do not track' do
       before do
+        stub_experiment(test_experiment: true)
         allow(controller).to receive(:current_user).and_return(user)
-        allow_next_instance_of(described_class) do |instance|
-          allow(instance).to receive(:experiment_enabled?).with(:test_experiment).and_return(false)
-        end
       end
 
       context 'is disabled' do

spec/lib/gitlab/experimentation/experiment_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Experimentation::Experiment do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:percentage) { 50 }
+  let(:params) do
+    {
+      tracking_category: 'Category1',
+      use_backwards_compatible_subject_index: true
+    }
+  end
+
+  before do
+    feature = double('FeatureFlag', percentage_of_time_value: percentage )
+    expect(Feature).to receive(:get).with(:experiment_key_experiment_percentage).and_return(feature)
+  end
+
+  subject(:experiment) { described_class.new(:experiment_key, **params) }
+
+  describe '#enabled?' do
+    before do
+      allow(Gitlab).to receive(:dev_env_or_com?).and_return(on_gitlab_com)
+    end
+
+    subject { experiment.enabled? }
+
+    where(:on_gitlab_com, :percentage, :is_enabled) do
+      true  | 0  | false
+      true  | 10 | true
+      false | 0  | false
+      false | 10 | false
+    end
+
+    with_them do
+      it { is_expected.to eq(is_enabled) }
+    end
+  end
+
+  describe '#enabled_for_index?' do
+    subject { experiment.enabled_for_index?(index) }
+
+    where(:index, :percentage, :is_enabled) do
+      50  | 40 | false
+      40  | 50 | true
+      nil | 50 | false
+    end
+
+    with_them do
+      it { is_expected.to eq(is_enabled) }
+    end
+  end
+end

spec/lib/gitlab/experimentation_spec.rb

@@ -33,27 +33,25 @@ RSpec.describe Gitlab::Experimentation, :snowplow do
   before do
     stub_const('Gitlab::Experimentation::EXPERIMENTS', {
       backwards_compatible_test_experiment: {
-        environment: environment,
         tracking_category: 'Team',
         use_backwards_compatible_subject_index: true
       },
       test_experiment: {
-        environment: environment,
         tracking_category: 'Team'
       }
     })
 
     Feature.enable_percentage_of_time(:backwards_compatible_test_experiment_experiment_percentage, enabled_percentage)
     Feature.enable_percentage_of_time(:test_experiment_experiment_percentage, enabled_percentage)
+    allow(Gitlab).to receive(:com?).and_return(true)
   end
 
-  let(:environment) { Rails.env.test? }
   let(:enabled_percentage) { 10 }
 
   describe '.enabled?' do
     subject { described_class.enabled?(:test_experiment) }
 
-    context 'feature toggle is enabled, we are on the right environment and we are selected' do
+    context 'feature toggle is enabled and we are selected' do
       it { is_expected.to be_truthy }
     end
 
@@ -68,20 +66,6 @@ RSpec.describe Gitlab::Experimentation, :snowplow do
 
       it { is_expected.to be_falsey }
     end
-
-    describe 'we are on the wrong environment' do
-      let(:environment) { ::Gitlab.com? }
-
-      it { is_expected.to be_falsey }
-
-      it 'ensures the typically less expensive environment is checked before the more expensive call to database for Feature' do
-        expect_next_instance_of(described_class::Experiment) do |experiment|
-          expect(experiment).not_to receive(:enabled?)
-        end
-
-        subject
-      end
-    end
   end
 
   describe '.enabled_for_value?' do