Merge a2cc36f into 7848cc1

Author: milaGGL

firebase-abt/CHANGELOG.md

@@ -1,5 +1,7 @@
 # Unreleased
 
+* [changed] Internal changes to improve experiment reporting.
+
 # 21.1.0
 * [changed] Internal changes to ensure functionality alignment with other
   SDK releases.

firebase-abt/src/main/java/com/google/firebase/abt/FirebaseABTesting.java

@@ -30,10 +30,8 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Deque;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 /**
  * Manages Firebase A/B Testing Experiments.
@@ -144,11 +142,11 @@ public void removeAllExperiments() throws AbtException {
   }
 
   /**
-   * Gets the origin service's list of experiments in the app.
+   * Gets the origin service's list of experiments in the app via the Analytics SDK.
    *
    * <p>Note: This is a blocking call and therefore should be called from a worker thread.
    *
-   * @return the origin service's list of experiments in the app.
+   * @return the origin service's list of experiments in the app as {@link AbtExperimentInfo}s.
    * @throws AbtException If there is no Analytics SDK.
    */
   @WorkerThread
@@ -204,12 +202,10 @@ public void reportActiveExperiment(AbtExperimentInfo activeExperiment) throws Ab
   public void validateRunningExperiments(List<AbtExperimentInfo> runningExperiments)
       throws AbtException {
     throwAbtExceptionIfAnalyticsIsNull();
-    Set<String> runningExperimentIds = new HashSet<>();
-    for (AbtExperimentInfo runningExperiment : runningExperiments) {
-      runningExperimentIds.add(runningExperiment.getExperimentId());
-    }
+
+    // Get all experiments in Analytics and remove the ones that aren't running.
     List<ConditionalUserProperty> experimentsToRemove =
-        getExperimentsToRemove(getAllExperimentsInAnalytics(), runningExperimentIds);
+        getExperimentsToRemove(getAllExperiments(), runningExperiments);
     removeExperiments(experimentsToRemove);
   }
 
@@ -245,34 +241,29 @@ private void replaceAllExperimentsWith(List<AbtExperimentInfo> replacementExperi
       return;
     }
 
-    Set<String> replacementExperimentIds = new HashSet<>();
-    for (AbtExperimentInfo replacementExperiment : replacementExperiments) {
-      replacementExperimentIds.add(replacementExperiment.getExperimentId());
-    }
-
-    List<ConditionalUserProperty> experimentsInAnalytics = getAllExperimentsInAnalytics();
-    Set<String> idsOfExperimentsInAnalytics = new HashSet<>();
-    for (ConditionalUserProperty experimentInAnalytics : experimentsInAnalytics) {
-      idsOfExperimentsInAnalytics.add(experimentInAnalytics.name);
-    }
+    // Get all experiments in Analytics.
+    List<AbtExperimentInfo> experimentsInAnalytics = getAllExperiments();
 
+    // Remove experiments no longer assigned.
     List<ConditionalUserProperty> experimentsToRemove =
-        getExperimentsToRemove(experimentsInAnalytics, replacementExperimentIds);
+        getExperimentsToRemove(experimentsInAnalytics, replacementExperiments);
     removeExperiments(experimentsToRemove);
 
+    // Add newly assigned or updated (changed variant id).
     List<AbtExperimentInfo> experimentsToAdd =
-        getExperimentsToAdd(replacementExperiments, idsOfExperimentsInAnalytics);
+        getExperimentsToAdd(replacementExperiments, experimentsInAnalytics);
     addExperiments(experimentsToAdd);
   }
 
   /** Returns this origin's experiments in Analytics that are no longer assigned to this App. */
   private ArrayList<ConditionalUserProperty> getExperimentsToRemove(
-      List<ConditionalUserProperty> experimentsInAnalytics, Set<String> replacementExperimentIds) {
+      List<AbtExperimentInfo> experimentsInAnalytics,
+      List<AbtExperimentInfo> replacementExperiments) {
 
     ArrayList<ConditionalUserProperty> experimentsToRemove = new ArrayList<>();
-    for (ConditionalUserProperty experimentInAnalytics : experimentsInAnalytics) {
-      if (!replacementExperimentIds.contains(experimentInAnalytics.name)) {
-        experimentsToRemove.add(experimentInAnalytics);
+    for (AbtExperimentInfo experimentInAnalytics : experimentsInAnalytics) {
+      if (!experimentsListContainsExperiment(replacementExperiments, experimentInAnalytics)) {
+        experimentsToRemove.add(experimentInAnalytics.toConditionalUserProperty(originService));
       }
     }
     return experimentsToRemove;
@@ -283,17 +274,33 @@ private ArrayList<ConditionalUserProperty> getExperimentsToRemove(
    * to this origin's list of experiments in Analytics.
    */
   private ArrayList<AbtExperimentInfo> getExperimentsToAdd(
-      List<AbtExperimentInfo> replacementExperiments, Set<String> idsOfExperimentsInAnalytics) {
+      List<AbtExperimentInfo> replacementExperiments,
+      List<AbtExperimentInfo> experimentInfoFromAnalytics) {
 
     ArrayList<AbtExperimentInfo> experimentsToAdd = new ArrayList<>();
     for (AbtExperimentInfo replacementExperiment : replacementExperiments) {
-      if (!idsOfExperimentsInAnalytics.contains(replacementExperiment.getExperimentId())) {
+      if (!experimentsListContainsExperiment(experimentInfoFromAnalytics, replacementExperiment)) {
         experimentsToAdd.add(replacementExperiment);
       }
     }
     return experimentsToAdd;
   }
 
+  private boolean experimentsListContainsExperiment(
+      List<AbtExperimentInfo> experiments, AbtExperimentInfo experiment) {
+    String experimentId = experiment.getExperimentId();
+    String variantId = experiment.getVariantId();
+
+    for (AbtExperimentInfo experimentInfo : experiments) {
+      if (experimentInfo.getExperimentId().equals(experimentId)
+          && experimentInfo.getVariantId().equals(variantId)) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
   /** Adds the given experiments to the origin's list in Analytics. */
   private void addExperiments(List<AbtExperimentInfo> experimentsToAdd) {
 
@@ -369,7 +376,7 @@ private int getMaxUserPropertiesInAnalytics() {
    * Returns a list of all this origin's experiments in this App's Analytics SDK.
    *
    * <p>The list is sorted chronologically by the experiment start time, with the oldest experiment
-   * at index 0.
+   * at index 0. Experiments are stored as {@link ConditionalUserProperty}s in Analytics.
    */
   @WorkerThread
   private List<ConditionalUserProperty> getAllExperimentsInAnalytics() {

firebase-abt/src/test/java/com/google/firebase/abt/FirebaseABTestingTest.java

@@ -54,14 +54,27 @@ public class FirebaseABTestingTest {
   private static final String TEST_EXPERIMENT_1_ID = "1";
   private static final String TEST_EXPERIMENT_2_ID = "2";
 
+  private static final String TEST_VARIANT_ID_A = "VARIANT_A";
+  private static final String TEST_VARIANT_ID_B = "VARIANT_B";
+
   private static final AbtExperimentInfo TEST_ABT_EXPERIMENT_1 =
       createExperimentInfo(
           TEST_EXPERIMENT_1_ID,
+          TEST_VARIANT_ID_A,
           /*triggerEventName=*/ "",
           /*experimentStartTimeInEpochMillis=*/ 1000L);
-  private static final AbtExperimentInfo TEST_ABT_EXPERIMENT_2 =
+  private static final AbtExperimentInfo TEST_ABT_EXPERIMENT_2_VARIANT_A =
+      createExperimentInfo(
+          TEST_EXPERIMENT_2_ID,
+          TEST_VARIANT_ID_A,
+          "trigger_event_2",
+          /*experimentStartTimeInEpochMillis=*/ 2000L);
+  private static final AbtExperimentInfo TEST_ABT_EXPERIMENT_2_VARIANT_B =
       createExperimentInfo(
-          TEST_EXPERIMENT_2_ID, "trigger_event_2", /*experimentStartTimeInEpochMillis=*/ 2000L);
+          TEST_EXPERIMENT_2_ID,
+          TEST_VARIANT_ID_B,
+          "trigger_event_2",
+          /*experimentStartTimeInEpochMillis=*/ 2000L);
 
   private static final int MAX_ALLOWED_EXPERIMENTS_IN_ANALYTICS = 100;
 
@@ -91,7 +104,7 @@ public void replaceAllExperiments_noExperimentsInAnalytics_experimentsCorrectlyS
 
     firebaseAbt.replaceAllExperiments(
         Lists.newArrayList(
-            TEST_ABT_EXPERIMENT_1.toStringMap(), TEST_ABT_EXPERIMENT_2.toStringMap()));
+            TEST_ABT_EXPERIMENT_1.toStringMap(), TEST_ABT_EXPERIMENT_2_VARIANT_A.toStringMap()));
 
     ArgumentCaptor<ConditionalUserProperty> analyticsExperimentArgumentCaptor =
         ArgumentCaptor.forClass(ConditionalUserProperty.class);
@@ -107,7 +120,8 @@ public void replaceAllExperiments_noExperimentsInAnalytics_experimentsCorrectlyS
 
     // Validates that TEST_ABT_EXPERIMENT_1 and TEST_ABT_EXPERIMENT_2 have been set in Analytics.
     assertThat(analyticsExperiment1.toStringMap()).isEqualTo(TEST_ABT_EXPERIMENT_1.toStringMap());
-    assertThat(analyticsExperiment2.toStringMap()).isEqualTo(TEST_ABT_EXPERIMENT_2.toStringMap());
+    assertThat(analyticsExperiment2.toStringMap())
+        .isEqualTo(TEST_ABT_EXPERIMENT_2_VARIANT_A.toStringMap());
   }
 
   @Test
@@ -117,10 +131,11 @@ public void replaceAllExperiments_existExperimentsInAnalytics_experimentsCorrect
         .thenReturn(
             Lists.newArrayList(
                 TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
-                TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
+                TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));
 
-    AbtExperimentInfo newExperiment3 = createExperimentInfo("3", "", 1000L);
-    AbtExperimentInfo newExperiment4 = createExperimentInfo("4", "trigger_event_4", 1000L);
+    AbtExperimentInfo newExperiment3 = createExperimentInfo("3", TEST_VARIANT_ID_A, "", 1000L);
+    AbtExperimentInfo newExperiment4 =
+        createExperimentInfo("4", TEST_VARIANT_ID_A, "trigger_event_4", 1000L);
 
     // Simulates the case where experiment 1 is assigned (as before), experiment 2 is no longer
     // assigned; experiment 3 and experiment 4 are newly assigned.
@@ -146,6 +161,47 @@ public void replaceAllExperiments_existExperimentsInAnalytics_experimentsCorrect
         .isEqualTo(newExperiment4.toStringMap());
   }
 
+  @Test
+  public void
+      replaceAllExperiments_existExperimentsInAnalyticsWithDifferentVariants_experimentsCorrectlySetInAnalytics()
+          throws Exception {
+    when(mockAnalyticsConnector.getConditionalUserProperties(ORIGIN_SERVICE, ""))
+        .thenReturn(
+            Lists.newArrayList(
+                TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
+                TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));
+
+    AbtExperimentInfo newExperiment3 = createExperimentInfo("3", "b", "", 1000L);
+    AbtExperimentInfo newExperiment4 = createExperimentInfo("4", "a", "trigger_event_4", 1000L);
+
+    // Simulates the case where experiments 1 and 2 are removed,
+    // experiment 2 is re-set with a new variant, and experiments 3 and 4 are newly added.
+    firebaseAbt.replaceAllExperiments(
+        Lists.newArrayList(
+            TEST_ABT_EXPERIMENT_2_VARIANT_B.toStringMap(),
+            newExperiment3.toStringMap(),
+            newExperiment4.toStringMap()));
+
+    // Validates that experiment 1 is cleared, experiment 2 is updated,
+    // and experiment 3 and experiment 4 are set in Analytics.
+    ArgumentCaptor<ConditionalUserProperty> analyticsExperimentArgumentCaptor =
+        ArgumentCaptor.forClass(ConditionalUserProperty.class);
+    verify(mockAnalyticsConnector, times(1))
+        .clearConditionalUserProperty(TEST_EXPERIMENT_1_ID, null, null);
+    verify(mockAnalyticsConnector, times(1))
+        .clearConditionalUserProperty(TEST_EXPERIMENT_2_ID, null, null);
+    verify(mockAnalyticsConnector, times(3))
+        .setConditionalUserProperty(analyticsExperimentArgumentCaptor.capture());
+
+    List<ConditionalUserProperty> actualValues = analyticsExperimentArgumentCaptor.getAllValues();
+    assertThat(AbtExperimentInfo.fromConditionalUserProperty(actualValues.get(0)).toStringMap())
+        .isEqualTo(TEST_ABT_EXPERIMENT_2_VARIANT_B.toStringMap());
+    assertThat(AbtExperimentInfo.fromConditionalUserProperty(actualValues.get(1)).toStringMap())
+        .isEqualTo(newExperiment3.toStringMap());
+    assertThat(AbtExperimentInfo.fromConditionalUserProperty(actualValues.get(2)).toStringMap())
+        .isEqualTo(newExperiment4.toStringMap());
+  }
+
   @Test
   public void replaceAllExperiments_totalExperimentsExceedsAnalyticsLimit_oldExperimentsDiscarded()
       throws Exception {
@@ -155,17 +211,18 @@ public void replaceAllExperiments_totalExperimentsExceedsAnalyticsLimit_oldExper
         .thenReturn(
             Lists.newArrayList(
                 TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
-                TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
+                TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));
 
-    AbtExperimentInfo newExperiment3 = createExperimentInfo("3", "", 1000L);
-    AbtExperimentInfo newExperiment4 = createExperimentInfo("4", "trigger_event_4", 1000L);
+    AbtExperimentInfo newExperiment3 = createExperimentInfo("3", TEST_VARIANT_ID_A, "", 1000L);
+    AbtExperimentInfo newExperiment4 =
+        createExperimentInfo("4", TEST_VARIANT_ID_A, "trigger_event_4", 1000L);
 
     // Simulates the case where experiment 1 and 2 are assigned (as before), experiment 3 and
     // experiment 4 are newly assigned.
     firebaseAbt.replaceAllExperiments(
         Lists.newArrayList(
             TEST_ABT_EXPERIMENT_1.toStringMap(),
-            TEST_ABT_EXPERIMENT_2.toStringMap(),
+            TEST_ABT_EXPERIMENT_2_VARIANT_A.toStringMap(),
             newExperiment3.toStringMap(),
             newExperiment4.toStringMap()));
 
@@ -231,7 +288,7 @@ public void removeAllExperiments_existExperimentsInAnalytics_experimentsClearedF
         .thenReturn(
             Lists.newArrayList(
                 TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
-                TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
+                TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));
 
     firebaseAbt.removeAllExperiments();
 
@@ -266,15 +323,15 @@ public void getAllExperiments_existExperimentsInAnalytics_returnAllExperiments()
         .thenReturn(
             Lists.newArrayList(
                 TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
-                TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
+                TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));
 
     List<AbtExperimentInfo> abtExperimentInfoList = firebaseAbt.getAllExperiments();
 
     assertThat(abtExperimentInfoList).hasSize(2);
     assertThat(abtExperimentInfoList.get(0).toStringMap())
         .isEqualTo(TEST_ABT_EXPERIMENT_1.toStringMap());
     assertThat(abtExperimentInfoList.get(1).toStringMap())
-        .isEqualTo(TEST_ABT_EXPERIMENT_2.toStringMap());
+        .isEqualTo(TEST_ABT_EXPERIMENT_2_VARIANT_A.toStringMap());
   }
 
   @Test
@@ -298,7 +355,7 @@ public void getAllExperiments_analyticsSdkUnavailable_throwsAbtException() {
         .thenReturn(
             Lists.newArrayList(
                 TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
-                TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
+                TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));
 
     // Update to just one experiment running
     firebaseAbt.validateRunningExperiments(Lists.newArrayList(TEST_ABT_EXPERIMENT_1));
@@ -315,11 +372,11 @@ public void validateRunningExperiments_noinactiveExperimentsInAnalytics_cleansUp
         .thenReturn(
             Lists.newArrayList(
                 TEST_ABT_EXPERIMENT_1.toConditionalUserProperty(ORIGIN_SERVICE),
-                TEST_ABT_EXPERIMENT_2.toConditionalUserProperty(ORIGIN_SERVICE)));
+                TEST_ABT_EXPERIMENT_2_VARIANT_A.toConditionalUserProperty(ORIGIN_SERVICE)));
 
     // Update still says the same two experiments are running
     firebaseAbt.validateRunningExperiments(
-        Lists.newArrayList(TEST_ABT_EXPERIMENT_1, TEST_ABT_EXPERIMENT_2));
+        Lists.newArrayList(TEST_ABT_EXPERIMENT_1, TEST_ABT_EXPERIMENT_2_VARIANT_A));
 
     // Verify nothing cleared
     verify(mockAnalyticsConnector, never()).clearConditionalUserProperty(any(), any(), any());
@@ -346,11 +403,14 @@ public void reportActiveExperiment_setsNullTriggerCondition() throws Exception {
   }
 
   private static AbtExperimentInfo createExperimentInfo(
-      String experimentId, String triggerEventName, long experimentStartTimeInEpochMillis) {
+      String experimentId,
+      String variantId,
+      String triggerEventName,
+      long experimentStartTimeInEpochMillis) {
 
     return new AbtExperimentInfo(
         experimentId,
-        VARIANT_ID_VALUE,
+        variantId,
         triggerEventName,
         new Date(experimentStartTimeInEpochMillis),
         TRIGGER_TIMEOUT_IN_MILLIS_VALUE,