diff --git a/firebase-abt/CHANGELOG.md b/firebase-abt/CHANGELOG.md index 931ae65ff4d..d54afdaa8e6 100644 --- a/firebase-abt/CHANGELOG.md +++ b/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. diff --git a/firebase-abt/src/main/java/com/google/firebase/abt/FirebaseABTesting.java b/firebase-abt/src/main/java/com/google/firebase/abt/FirebaseABTesting.java index 15ce789b78d..d3f89e99bbf 100644 --- a/firebase-abt/src/main/java/com/google/firebase/abt/FirebaseABTesting.java +++ b/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. * *

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 runningExperiments) throws AbtException { throwAbtExceptionIfAnalyticsIsNull(); - Set 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 experimentsToRemove = - getExperimentsToRemove(getAllExperimentsInAnalytics(), runningExperimentIds); + getExperimentsToRemove(getAllExperiments(), runningExperiments); removeExperiments(experimentsToRemove); } @@ -245,34 +241,29 @@ private void replaceAllExperimentsWith(List replacementExperi return; } - Set replacementExperimentIds = new HashSet<>(); - for (AbtExperimentInfo replacementExperiment : replacementExperiments) { - replacementExperimentIds.add(replacementExperiment.getExperimentId()); - } - - List experimentsInAnalytics = getAllExperimentsInAnalytics(); - Set idsOfExperimentsInAnalytics = new HashSet<>(); - for (ConditionalUserProperty experimentInAnalytics : experimentsInAnalytics) { - idsOfExperimentsInAnalytics.add(experimentInAnalytics.name); - } + // Get all experiments in Analytics. + List experimentsInAnalytics = getAllExperiments(); + // Remove experiments no longer assigned. List experimentsToRemove = - getExperimentsToRemove(experimentsInAnalytics, replacementExperimentIds); + getExperimentsToRemove(experimentsInAnalytics, replacementExperiments); removeExperiments(experimentsToRemove); + // Add newly assigned or updated (changed variant id). List 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 getExperimentsToRemove( - List experimentsInAnalytics, Set replacementExperimentIds) { + List experimentsInAnalytics, + List replacementExperiments) { ArrayList 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 getExperimentsToRemove( * to this origin's list of experiments in Analytics. */ private ArrayList getExperimentsToAdd( - List replacementExperiments, Set idsOfExperimentsInAnalytics) { + List replacementExperiments, + List experimentInfoFromAnalytics) { ArrayList 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 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 experimentsToAdd) { @@ -369,7 +376,7 @@ private int getMaxUserPropertiesInAnalytics() { * Returns a list of all this origin's experiments in this App's Analytics SDK. * *

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 getAllExperimentsInAnalytics() { diff --git a/firebase-abt/src/test/java/com/google/firebase/abt/FirebaseABTestingTest.java b/firebase-abt/src/test/java/com/google/firebase/abt/FirebaseABTestingTest.java index a98c05f317f..c8e2c6d8fdd 100644 --- a/firebase-abt/src/test/java/com/google/firebase/abt/FirebaseABTestingTest.java +++ b/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 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 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 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,7 +323,7 @@ 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 abtExperimentInfoList = firebaseAbt.getAllExperiments(); @@ -274,7 +331,7 @@ public void getAllExperiments_existExperimentsInAnalytics_returnAllExperiments() 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,