Fix broken tests resulting from conflicts. (#66)

Author: mikeproeng37

lib/optimizely/bucketer.rb

@@ -41,7 +41,7 @@ def bucket(experiment, user_id)
       # experiment - Experiment for which visitor is to be bucketed.
       # user_id - String ID for user.
       #
-      # Returns String variation ID in which visitor with ID user_id has been placed. Nil if no variation.
+      # Returns variation in which visitor with ID user_id has been placed. Nil if no variation.
 
       # check if experiment is in a group; if so, check if user is bucketed into specified experiment
       experiment_id = experiment['id']
@@ -50,12 +50,8 @@ def bucket(experiment, user_id)
       if group_id
         group = @config.group_key_map.fetch(group_id)
         if Helpers::Group.random_policy?(group)
-          bucketing_id = sprintf(BUCKETING_ID_TEMPLATE, user_id: user_id, entity_id: group_id)
           traffic_allocations = group.fetch('trafficAllocation')
-          bucket_value = generate_bucket_value(bucketing_id)
-          @config.logger.log(Logger::DEBUG, "Assigned experiment bucket #{bucket_value} to user '#{user_id}'.")
-          bucketed_experiment_id = find_bucket(bucket_value, traffic_allocations)
-
+          bucketed_experiment_id = find_bucket(user_id, group_id, traffic_allocations)
           # return if the user is not bucketed into any experiment
           unless bucketed_experiment_id
             @config.logger.log(Logger::INFO, "User '#{user_id}' is in no experiment.")
@@ -81,14 +77,14 @@ def bucket(experiment, user_id)
 
       traffic_allocations = experiment['trafficAllocation']
       variation_id = find_bucket(user_id, experiment_id, traffic_allocations)
-
       if variation_id && variation_id != ''
-        variation_key = @config.get_variation_key_from_id(experiment_key, variation_id)
+        variation = @config.get_variation_from_id(experiment_key, variation_id)
+        variation_key = variation ? variation['key'] : nil
         @config.logger.log(
           Logger::INFO,
           "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_key}'."
         )
-        return variation_id
+        return variation
       end
 
       # Handle the case when the traffic range is empty due to sticky bucketing
@@ -100,8 +96,6 @@ def bucket(experiment, user_id)
       nil
     end
 
-    private
-
     def find_bucket(user_id, parent_id, traffic_allocations)
       # Helper function to find the matching entity ID for a given bucketing value in a list of traffic allocations.
       #
@@ -126,6 +120,8 @@ def find_bucket(user_id, parent_id, traffic_allocations)
       nil
     end
 
+    private
+
     def generate_bucket_value(bucketing_id)
       # Helper function to generate bucket value in half-closed interval [0, MAX_TRAFFIC_VALUE).
       #
@@ -147,4 +143,4 @@ def generate_unsigned_hash_code_32_bit(bucketing_id)
       MurmurHash3::V32.str_hash(bucketing_id, @bucket_seed) & UNSIGNED_MAX_32_BIT_VALUE
     end
   end
-end
+end

lib/optimizely/decision_service.rb

@@ -88,7 +88,8 @@ def get_variation(experiment_key, user_id, attributes = nil)
       end
 
       # Bucket normally
-      variation_id = @bucketer.bucket(experiment, user_id)
+      variation = @bucketer.bucket(experiment, user_id)
+      variation_id = variation ? variation['id'] : nil
 
       # Persist bucketing decision
       save_user_profile(user_profile, experiment_id, variation_id)
@@ -156,7 +157,7 @@ def get_user_profile(user_id)
       #
       # user_id - String ID for the user
       #
-      # Returns Hash stored user profile (or a default one if lookup fails or user profile service not provided) 
+      # Returns Hash stored user profile (or a default one if lookup fails or user profile service not provided)
 
       user_profile = {
         :user_id => user_id,

lib/optimizely/project_config.rb

@@ -196,6 +196,28 @@ def get_variation_key_from_id(experiment_key, variation_id)
       nil
     end
 
+    def get_variation_from_id(experiment_key, variation_id)
+      # Get variation given experiment key and variation ID
+      #
+      # experiment_key - Key representing parent experiment of variation
+      # variation_id - ID of the variation
+      #
+      # Returns the variation or nil if not found
+
+      variation_id_map = @variation_id_map[experiment_key]
+      if variation_id_map
+        variation = variation_id_map[variation_id]
+        return variation if variation
+        @logger.log Logger::ERROR, "Variation id '#{variation_id}' is not in datafile."
+        @error_handler.handle_error InvalidVariationError
+        return nil
+      end
+
+      @logger.log Logger::ERROR, "Experiment key '#{experiment_key}' is not in datafile."
+      @error_handler.handle_error InvalidExperimentError
+      nil
+    end
+
     def get_variation_id_from_key(experiment_key, variation_key)
       # Get variation ID given experiment key and variation key
       #
@@ -276,7 +298,7 @@ def get_forced_variation(experiment_key, user_id)
       end
 
       @logger.log(Logger::DEBUG, "Variation '#{variation_key}' is mapped to experiment '#{experiment_key}' and user '#{user_id}' in the forced variation map")
-      
+
       variation
     end
 
@@ -317,7 +339,7 @@ def set_forced_variation(experiment_key, user_id, variation_key)
         return false
       end
 
-      unless @forced_variation_map.has_key? user_id 
+      unless @forced_variation_map.has_key? user_id
         @forced_variation_map[user_id] = {}
       end
       @forced_variation_map[user_id][experiment_id] = variation_id

spec/bucketing_spec.rb

@@ -36,10 +36,12 @@ def get_bucketing_id(user_id, entity_id=nil)
     experiment = config.get_experiment_from_key('test_experiment')
 
     # Variation 1
-    expect(bucketer.bucket(experiment, 'test_user')).to eq('111128')
+    expected_variation_1 = config.get_variation_from_id('test_experiment', '111128')
+    expect(bucketer.bucket(experiment, 'test_user')).to eq(expected_variation_1)
 
     # Variation 2
-    expect(bucketer.bucket(experiment, 'test_user')).to eq('111129')
+    expected_variation_2 = config.get_variation_from_id('test_experiment','111129')
+    expect(bucketer.bucket(experiment, 'test_user')).to eq(expected_variation_2)
 
     # No matching variation
     expect(bucketer.bucket(experiment, 'test_user')).to be_nil
@@ -58,14 +60,13 @@ def get_bucketing_id(user_id, entity_id=nil)
     expect(bucketer).to receive(:generate_bucket_value).twice.and_return(3000)
 
     experiment = config.get_experiment_from_key('group1_exp1')
-    expect(bucketer.bucket(experiment, 'test_user')).to eq('130001')
+    expected_variation = config.get_variation_from_id('group1_exp1','130001')
+    expect(bucketer.bucket(experiment, 'test_user')).to eq(expected_variation)
     expect(spy_logger).to have_received(:log).exactly(4).times
-    expect(spy_logger).to have_received(:log)
-                      .with(Logger::DEBUG, "Assigned experiment bucket 3000 to user 'test_user'.")
+    expect(spy_logger).to have_received(:log).twice
+                      .with(Logger::DEBUG, "Assigned bucket 3000 to user 'test_user'.")
     expect(spy_logger).to have_received(:log)
                       .with(Logger::INFO, "User 'test_user' is in experiment 'group1_exp1' of group 101.")
-    expect(spy_logger).to have_received(:log)
-                      .with(Logger::DEBUG, "Assigned variation bucket 3000 to user 'test_user'.")
     expect(spy_logger).to have_received(:log)
       .with(Logger::INFO, "User 'test_user' is in variation 'g1_e1_v1' of experiment 'group1_exp1'.")
   end
@@ -76,13 +77,12 @@ def get_bucketing_id(user_id, entity_id=nil)
     experiment = config.get_experiment_from_key('group1_exp2')
     expect(bucketer.bucket(experiment, 'test_user')).to be_nil
     expect(spy_logger).to have_received(:log)
-                      .with(Logger::DEBUG, "Assigned experiment bucket 3000 to user 'test_user'.")
+                      .with(Logger::DEBUG, "Assigned bucket 3000 to user 'test_user'.")
     expect(spy_logger).to have_received(:log)
                       .with(Logger::INFO, "User 'test_user' is not in experiment 'group1_exp2' of group 101.")
   end
 
   it 'should return nil when user is not bucketed into any bucket' do
-    expect(bucketer).to receive(:generate_bucket_value).once.and_return(3000)
     expect(bucketer).to receive(:find_bucket).once.and_return(nil)
 
     experiment = config.get_experiment_from_key('group1_exp2')
@@ -95,10 +95,11 @@ def get_bucketing_id(user_id, entity_id=nil)
     expect(bucketer).to receive(:generate_bucket_value).once.and_return(3000)
 
     experiment = config.get_experiment_from_key('group2_exp1')
-    expect(bucketer.bucket(experiment, 'test_user')).to eq('144443')
+    expected_variation = config.get_variation_from_id('group2_exp1','144443')
+    expect(bucketer.bucket(experiment, 'test_user')).to eq(expected_variation)
     expect(spy_logger).to have_received(:log).twice
     expect(spy_logger).to have_received(:log)
-      .with(Logger::DEBUG, "Assigned variation bucket 3000 to user 'test_user'.")
+      .with(Logger::DEBUG, "Assigned bucket 3000 to user 'test_user'.")
     expect(spy_logger).to have_received(:log)
       .with(Logger::INFO, "User 'test_user' is in variation 'g2_e1_v1' of experiment 'group2_exp1'.")
   end
@@ -110,7 +111,7 @@ def get_bucketing_id(user_id, entity_id=nil)
     expect(bucketer.bucket(experiment, 'test_user')).to be_nil
     expect(spy_logger).to have_received(:log).twice
     expect(spy_logger).to have_received(:log)
-      .with(Logger::DEBUG, "Assigned variation bucket 50000 to user 'test_user'.")
+      .with(Logger::DEBUG, "Assigned bucket 50000 to user 'test_user'.")
     expect(spy_logger).to have_received(:log)
       .with(Logger::INFO, "User 'test_user' is in no variation.")
   end
@@ -147,7 +148,7 @@ def get_bucketing_id(user_id, entity_id=nil)
       experiment = config.get_experiment_from_key('test_experiment')
       bucketer.bucket(experiment, 'test_user')
       expect(spy_logger).to have_received(:log).twice
-      expect(spy_logger).to have_received(:log).with(Logger::DEBUG, "Assigned variation bucket 50 to user 'test_user'.")
+      expect(spy_logger).to have_received(:log).with(Logger::DEBUG, "Assigned bucket 50 to user 'test_user'.")
       expect(spy_logger).to have_received(:log).with(
         Logger::INFO,
         "User 'test_user' is in variation 'control' of experiment 'test_experiment'."
@@ -161,7 +162,7 @@ def get_bucketing_id(user_id, entity_id=nil)
       bucketer.bucket(experiment, 'test_user')
       expect(spy_logger).to have_received(:log).twice
       expect(spy_logger).to have_received(:log)
-                        .with(Logger::DEBUG, "Assigned variation bucket 5050 to user 'test_user'.")
+                        .with(Logger::DEBUG, "Assigned bucket 5050 to user 'test_user'.")
       expect(spy_logger).to have_received(:log).with(
         Logger::INFO,
         "User 'test_user' is in variation 'variation' of experiment 'test_experiment'."
@@ -175,9 +176,9 @@ def get_bucketing_id(user_id, entity_id=nil)
       bucketer.bucket(experiment, 'test_user')
       expect(spy_logger).to have_received(:log).twice
       expect(spy_logger).to have_received(:log)
-                        .with(Logger::DEBUG, "Assigned variation bucket 50000 to user 'test_user'.")
+                        .with(Logger::DEBUG, "Assigned bucket 50000 to user 'test_user'.")
       expect(spy_logger).to have_received(:log)
                         .with(Logger::INFO, "User 'test_user' is in no variation.")
     end
   end
-end
+end

spec/project_spec.rb

@@ -121,7 +121,7 @@ class InvalidErrorHandler; end
     before(:example) do
       allow(Time).to receive(:now).and_return(time_now)
       allow(SecureRandom).to receive(:uuid).and_return('a68cf1ad-0393-4e18-af87-efe8f01a7c9c');
-      
+
       @expected_activate_params = {
         account_id: '12001',
         project_id: '111001',
@@ -151,7 +151,8 @@ class InvalidErrorHandler; end
     it 'should properly activate a user, invoke Event object with right params, and return variation' do
       params = @expected_activate_params
 
-      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return('111128')
+      variation_to_return = project_instance.config.get_variation_from_id('test_experiment', '111128')
+      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return)
       allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
       allow(project_instance.config).to receive(:get_audience_ids_for_experiment)
                                        .with('test_experiment')
@@ -196,7 +197,8 @@ class InvalidErrorHandler; end
       }]
       params[:visitors][0][:snapshots][0][:events][0][:entity_id] = '3'
 
-      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return('122228')
+      variation_to_return = project_instance.config.get_variation_from_id('test_experiment_with_audience', '122228')
+      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return)
       allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
 
       expect(project_instance.activate('test_experiment_with_audience', 'test_user', 'browser_type' => 'firefox'))
@@ -257,7 +259,8 @@ class InvalidErrorHandler; end
     it 'should log when an impression event is dispatched' do
       params = @expected_activate_params
 
-      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return('111128')
+      variation_to_return = project_instance.config.get_variation_from_id('test_experiment', '111128')
+      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return)
       allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
       allow(project_instance.config).to receive(:get_audience_ids_for_experiment)
                                         .with('test_experiment')
@@ -268,7 +271,8 @@ class InvalidErrorHandler; end
     end
 
     it 'should log when an exception has occurred during dispatching the impression event' do
-      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return('111128')
+      variation_to_return = project_instance.config.get_variation_from_id('test_experiment', '111128')
+      allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return)
       allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(any_args).and_raise(RuntimeError)
       project_instance.activate('test_experiment', 'test_user')
       expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, "Unable to dispatch impression event. Error: RuntimeError")
@@ -320,7 +324,7 @@ class InvalidErrorHandler; end
     before(:example) do
       allow(Time).to receive(:now).and_return(time_now)
       allow(SecureRandom).to receive(:uuid).and_return('a68cf1ad-0393-4e18-af87-efe8f01a7c9c');
-      
+
       @expected_track_event_params = {
         account_id: '12001',
         project_id: '111001',
@@ -596,7 +600,7 @@ class InvalidErrorHandler; end
       expect(project_instance.get_variation('test_experiment','test_user')). to eq('variation')
     end
 
-    # setForcedVariation on a running experiment with audience enabled and then call getVariation on that same experiment with invalid attributes. 
+    # setForcedVariation on a running experiment with audience enabled and then call getVariation on that same experiment with invalid attributes.
     it 'should return nil when getVariation called on audience enabled running experiment with invalid attributes' do
       project_instance.set_forced_variation('test_experiment_with_audience','test_user','control_with_audience')
       expect { project_instance.get_variation('test_experiment_with_audience', 'test_user', 'invalid') }
@@ -606,7 +610,7 @@ class InvalidErrorHandler; end
     # Adding this test case to cover this in code coverage. All test cases for getForceVariation are present in
     # project_config_spec.rb which test the get_force_variation method in project_config. The one in optimizely.rb
     # only calls the other one
-    
+
     # getForceVariation on a running experiment after setforcevariation
     it 'should return expected variation id  when get_forced_variation is called on a running experiment after setForcedVariation' do
       project_instance.set_forced_variation('test_experiment','test_user','variation')