From 0edcfe6465e8923a233fdede971ec909f3ef8c6b Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Tue, 15 Aug 2017 16:31:06 -0700 Subject: [PATCH 01/16] added force bucketing --- .../java/com/optimizely/ab/Optimizely.java | 99 +++++++++++++++++-- .../optimizely/ab/config/ProjectConfig.java | 9 ++ .../com/optimizely/ab/OptimizelyTest.java | 35 +++++++ .../ab/event/internal/EventBuilderV2Test.java | 7 +- 4 files changed, 136 insertions(+), 14 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 36d91f77d..5e7162861 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -53,6 +53,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; /** * Top-level container class for Optimizely functionality. @@ -169,7 +170,10 @@ Variation activate(@Nonnull ProjectConfig projectConfig, Map filteredAttributes = filterAttributes(projectConfig, attributes); // bucket the user to the given experiment and dispatch an impression event - Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); + Variation variation = getForcedVariation(experiment.getKey(), userId); + if (variation == null) { + variation = decisionService.getVariation(experiment, userId, filteredAttributes); + } if (variation == null) { logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey()); return null; @@ -251,7 +255,10 @@ public void track(@Nonnull String eventName, Map experimentVariationMap = new HashMap(experimentsForEvent.size()); for (Experiment experiment : experimentsForEvent) { if (experiment.isRunning()) { - Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); + Variation variation = getForcedVariation(experiment.getKey(), userId); + if (variation == null) { + decisionService.getVariation(experiment, userId, filteredAttributes); + } if (variation != null) { experimentVariationMap.put(experiment, variation); } @@ -330,8 +337,10 @@ public void track(@Nonnull String eventName, Map filteredAttributes = filterAttributes(projectConfig, attributes); - Variation variation = decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes); - + Variation variation = getForcedVariation(featureKey, userId); + if (variation == null) { + decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes); + } if (variation != null) { Experiment experiment = projectConfig.getExperimentForVariationId(variation.getId()); if (experiment != null) { @@ -509,8 +518,10 @@ else if (!variable.getType().equals(variableType)) { String variableValue = variable.getDefaultValue(); - Variation variation = decisionService.getVariationForFeature(featureFlag, userId, attributes); - + Variation variation = getForcedVariation(featureKey, userId); + if (variation == null) { + decisionService.getVariationForFeature(featureFlag, userId, attributes); + } if (variation != null) { LiveVariableUsageInstance liveVariableUsageInstance = variation.getVariableIdToLiveVariableUsageInstanceMap().get(variable.getId()); @@ -542,7 +553,11 @@ Variation getVariation(@Nonnull Experiment experiment, Map filteredAttributes = filterAttributes(projectConfig, attributes); - return decisionService.getVariation(experiment, userId, filteredAttributes); + Variation variation = getForcedVariation(experiment.getKey(), userId); + if (variation == null) { + variation = decisionService.getVariation(experiment, userId, filteredAttributes); + } + return variation; } public @Nullable @@ -570,7 +585,75 @@ Variation getVariation(@Nonnull String experimentKey, Map filteredAttributes = filterAttributes(projectConfig, attributes); - return decisionService.getVariation(experiment,userId,filteredAttributes); + Variation variation = getForcedVariation(experimentKey, userId); + if (variation == null) { + variation = decisionService.getVariation(experiment, userId, filteredAttributes); + } + return variation; + } + + /** + * Force a user into a variation for a given experiment. + * The forced variation value does not persist across application launches. + * If the experiment key is not in the project file, this call fails and returns false. + * + * @param experimentKey The key for the experiment. + * @param userId The user ID to be used for bucketing. + * @param variationKey The variation key to force the user into. If the variation key is null + * then the forcedVariation for that experiment is removed. + * + * @return boolean A boolean value that indicates if the set completed successfully. + */ + public boolean setForcedVariation(@Nonnull String experimentKey, + @Nonnull String userId, + @Nullable String variationKey) { + + ProjectConfig currentConfig = getProjectConfig(); + // if the experiment is not a valid experiment key, don't set it. + Experiment experiment = currentConfig.getExperimentKeyMapping().get(experimentKey); + if (experiment == null || !experiment.isRunning()){ + return false; + } + + Map experimentToVariation = currentConfig.getForcedVariationMapping().get(userId); + if (experimentToVariation == null) { + experimentToVariation = new ConcurrentHashMap(); + currentConfig.getForcedVariationMapping().put(userId, experimentToVariation); + } + if (variationKey == null) { + experimentToVariation.remove(experimentKey); + } + else { + experimentToVariation.put(experimentKey, variationKey); + } + + return true; + } + + /** + * Gets the forced variation for a given user and experiment. + * + * @param experimentKey The key for the experiment. + * @param userId The user ID to be used for bucketing. + * + * @return The variation the user was bucketed into. This value can be null if the + * forced variation fails. + */ + public @Nullable Variation getForcedVariation(@Nonnull String experimentKey, + @Nonnull String userId) { + ProjectConfig currentConfig = getProjectConfig(); + + Map experimentToVariation = currentConfig.getForcedVariationMapping().get(userId); + if (experimentToVariation != null) { + String variationKey = experimentToVariation.get(experimentKey); + if (variationKey != null) { + Experiment experiment = currentConfig.getExperimentKeyMapping().get(experimentKey); + if (experiment != null) { + return experiment.getVariationKeyToVariationMap().get(variationKey); + } + } + } + return null; } /** diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index ffb891e29..d89c44f97 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -27,6 +27,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; /** * Represents the Optimizely Project configuration. @@ -84,6 +85,11 @@ public String toString() { private final Map> variationToLiveVariableUsageInstanceMapping; private final Map variationIdToExperimentMapping; + // forced variations supersede any other mappings. They are transient and are not persistent or part of + // the actual datafile. This map should not be accessed directly, but, instead through apis at the + // Optimizely object level. + private transient Map> forcedVariationMapping = new ConcurrentHashMap>(); + // v2 constructor public ProjectConfig(String accountId, String projectId, String version, String revision, List groups, List experiments, List attributes, List eventType, @@ -305,6 +311,8 @@ public Map getFeatureKeyMapping() { return featureKeyMapping; } + public Map> getForcedVariationMapping() { return forcedVariationMapping; } + @Override public String toString() { return "ProjectConfig{" + @@ -328,6 +336,7 @@ public String toString() { ", groupIdMapping=" + groupIdMapping + ", liveVariableIdToExperimentsMapping=" + liveVariableIdToExperimentsMapping + ", variationToLiveVariableUsageInstanceMapping=" + variationToLiveVariableUsageInstanceMapping + + ", forcedVariationMapping=" + forcedVariationMapping + '}'; } } diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 92ffcddcc..2c6acca92 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -296,6 +296,41 @@ public void activateWhenExperimentIsNotInProject() throws Exception { optimizely.activate(unknownExperiment, "userId"); } + /** + * Verify that the {@link Optimizely#activate(String, String, Map)} call + * correctly builds an endpoint url and request params + * and passes them through {@link EventHandler#dispatchEvent(LogEvent)}. + */ + @Test + public void activateWithExperimentKeyForced() throws Exception { + Experiment activatedExperiment = validProjectConfig.getExperiments().get(0); + Variation forcedVariation = activatedExperiment.getVariations().get(1); + EventBuilder mockEventBuilder = mock(EventBuilder.class); + + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withBucketing(mockBucketer) + .withEventBuilder(mockEventBuilder) + .withConfig(validProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", forcedVariation.getKey() ); + + Map testUserAttributes = new HashMap(); + if (datafileVersion == 4) { + testUserAttributes.put(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); + } + else { + testUserAttributes.put("browser_type", "chrome"); + } + + // activate the experiment + Variation actualVariation = optimizely.activate(activatedExperiment.getKey(), "userId", testUserAttributes); + + assertThat(actualVariation, is(forcedVariation)); + + } + /** * Verify that the {@link Optimizely#activate(String, String, Map)} call * correctly builds an endpoint url and request params diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java index 841315924..7ade378c1 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java @@ -68,12 +68,7 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.Matchers.closeTo; import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; From 8c0ffc95f285fc3d636517e4f452ef63fd7dfc7e Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Tue, 15 Aug 2017 16:51:37 -0700 Subject: [PATCH 02/16] get test working --- .../src/main/java/com/optimizely/ab/Optimizely.java | 6 +++++- .../test/java/com/optimizely/ab/OptimizelyTest.java | 10 ++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 5e7162861..b10d2a265 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -611,7 +611,11 @@ public boolean setForcedVariation(@Nonnull String experimentKey, ProjectConfig currentConfig = getProjectConfig(); // if the experiment is not a valid experiment key, don't set it. Experiment experiment = currentConfig.getExperimentKeyMapping().get(experimentKey); - if (experiment == null || !experiment.isRunning()){ + if (experiment == null || !experiment.isActive()){ + return false; + } + + if (!validateUserId(userId)) { return false; } diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 2c6acca92..d78123178 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -324,11 +324,21 @@ public void activateWithExperimentKeyForced() throws Exception { testUserAttributes.put("browser_type", "chrome"); } + Map testParams = new HashMap(); + testParams.put("test", "params"); + + LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); + when(mockEventBuilder.createImpressionEvent(eq(validProjectConfig), eq(activatedExperiment), eq(forcedVariation), + eq("userId"), eq(testUserAttributes))) + .thenReturn(logEventToDispatch); + // activate the experiment Variation actualVariation = optimizely.activate(activatedExperiment.getKey(), "userId", testUserAttributes); assertThat(actualVariation, is(forcedVariation)); + verify(mockEventHandler).dispatchEvent(logEventToDispatch); + } /** From b487cb6c6a99b0d55fac378143515f1bb030dece Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Tue, 15 Aug 2017 19:41:59 -0700 Subject: [PATCH 03/16] fix broken test and fix code errors --- core-api/src/main/java/com/optimizely/ab/Optimizely.java | 6 +++--- .../src/test/java/com/optimizely/ab/OptimizelyTest.java | 4 ++++ .../optimizely/ab/event/internal/EventBuilderV2Test.java | 4 ++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index b10d2a265..814c0ef19 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -257,7 +257,7 @@ public void track(@Nonnull String eventName, if (experiment.isRunning()) { Variation variation = getForcedVariation(experiment.getKey(), userId); if (variation == null) { - decisionService.getVariation(experiment, userId, filteredAttributes); + variation = decisionService.getVariation(experiment, userId, filteredAttributes); } if (variation != null) { experimentVariationMap.put(experiment, variation); @@ -339,7 +339,7 @@ public void track(@Nonnull String eventName, Variation variation = getForcedVariation(featureKey, userId); if (variation == null) { - decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes); + variation = decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes); } if (variation != null) { Experiment experiment = projectConfig.getExperimentForVariationId(variation.getId()); @@ -520,7 +520,7 @@ else if (!variable.getType().equals(variableType)) { Variation variation = getForcedVariation(featureKey, userId); if (variation == null) { - decisionService.getVariationForFeature(featureFlag, userId, attributes); + variation = decisionService.getVariationForFeature(featureFlag, userId, attributes); } if (variation != null) { LiveVariableUsageInstance liveVariableUsageInstance = diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index d78123178..1b091d9fe 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -339,6 +339,10 @@ public void activateWithExperimentKeyForced() throws Exception { verify(mockEventHandler).dispatchEvent(logEventToDispatch); + optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", null ); + + assertEquals(optimizely.getForcedVariation(activatedExperiment.getKey(), "userId"), null); + } /** diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java index 7ade378c1..9662c5c48 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java @@ -167,8 +167,8 @@ public void createImpressionEventIgnoresUnknownAttributes() throws Exception { // verify that no Feature is created for "unknownAtrribute" -> "blahValue" for (Feature feature : impression.getUserFeatures()) { - assertNotEquals(feature.getName(), "unknownAttribute"); - assertNotEquals(feature.getValue(), "blahValue"); + // assertNotEquals(feature.getName(), "unknownAttribute"); + // assertNotEquals(feature.getValue(), "blahValue"); } } From d0909e3db07eeb23e5d25d39b69c61447fa767b5 Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Tue, 15 Aug 2017 19:51:01 -0700 Subject: [PATCH 04/16] unit test forced variations --- .../src/test/java/com/optimizely/ab/OptimizelyTest.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 1b091d9fe..5f40bfdc4 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -298,13 +298,14 @@ public void activateWhenExperimentIsNotInProject() throws Exception { /** * Verify that the {@link Optimizely#activate(String, String, Map)} call - * correctly builds an endpoint url and request params - * and passes them through {@link EventHandler#dispatchEvent(LogEvent)}. + * uses forced variation to force the user into the second variation. The mock bucket returns + * the first variation. Then remove the forced variation and confirm that the forced variation is null. */ @Test public void activateWithExperimentKeyForced() throws Exception { Experiment activatedExperiment = validProjectConfig.getExperiments().get(0); Variation forcedVariation = activatedExperiment.getVariations().get(1); + Variation bucketedVariation = activatedExperiment.getVariations().get(0); EventBuilder mockEventBuilder = mock(EventBuilder.class); Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) @@ -332,6 +333,9 @@ public void activateWithExperimentKeyForced() throws Exception { eq("userId"), eq(testUserAttributes))) .thenReturn(logEventToDispatch); + when(mockBucketer.bucket(activatedExperiment, "userId")) + .thenReturn(bucketedVariation); + // activate the experiment Variation actualVariation = optimizely.activate(activatedExperiment.getKey(), "userId", testUserAttributes); From b9e741af05bd4e5eba9af2989b55d52c3e2b889d Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Tue, 15 Aug 2017 19:58:05 -0700 Subject: [PATCH 05/16] fix EventBuilderV2Test --- .../ab/event/internal/EventBuilderV2Test.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java index 9662c5c48..841315924 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java @@ -68,7 +68,12 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.Matchers.closeTo; import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -167,8 +172,8 @@ public void createImpressionEventIgnoresUnknownAttributes() throws Exception { // verify that no Feature is created for "unknownAtrribute" -> "blahValue" for (Feature feature : impression.getUserFeatures()) { - // assertNotEquals(feature.getName(), "unknownAttribute"); - // assertNotEquals(feature.getValue(), "blahValue"); + assertNotEquals(feature.getName(), "unknownAttribute"); + assertNotEquals(feature.getValue(), "blahValue"); } } From ba2ea7fb5950d9de8dc9d49a7da83d91684112d6 Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Wed, 16 Aug 2017 15:20:46 -0700 Subject: [PATCH 06/16] move force bucketing logic to decision service. enhance decision tests to test forebucketing overrides whitelist, userprofile, audience eval --- .../java/com/optimizely/ab/Optimizely.java | 74 +++---------- .../ab/bucketing/DecisionService.java | 83 +++++++++++++- .../ab/bucketing/DecisionServiceTest.java | 102 +++++++++++++++++- 3 files changed, 193 insertions(+), 66 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 814c0ef19..831b6637e 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -170,10 +170,8 @@ Variation activate(@Nonnull ProjectConfig projectConfig, Map filteredAttributes = filterAttributes(projectConfig, attributes); // bucket the user to the given experiment and dispatch an impression event - Variation variation = getForcedVariation(experiment.getKey(), userId); - if (variation == null) { - variation = decisionService.getVariation(experiment, userId, filteredAttributes); - } + Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); + if (variation == null) { logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey()); return null; @@ -255,10 +253,7 @@ public void track(@Nonnull String eventName, Map experimentVariationMap = new HashMap(experimentsForEvent.size()); for (Experiment experiment : experimentsForEvent) { if (experiment.isRunning()) { - Variation variation = getForcedVariation(experiment.getKey(), userId); - if (variation == null) { - variation = decisionService.getVariation(experiment, userId, filteredAttributes); - } + Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); if (variation != null) { experimentVariationMap.put(experiment, variation); } @@ -337,10 +332,7 @@ public void track(@Nonnull String eventName, Map filteredAttributes = filterAttributes(projectConfig, attributes); - Variation variation = getForcedVariation(featureKey, userId); - if (variation == null) { - variation = decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes); - } + Variation variation = decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes); if (variation != null) { Experiment experiment = projectConfig.getExperimentForVariationId(variation.getId()); if (experiment != null) { @@ -518,10 +510,7 @@ else if (!variable.getType().equals(variableType)) { String variableValue = variable.getDefaultValue(); - Variation variation = getForcedVariation(featureKey, userId); - if (variation == null) { - variation = decisionService.getVariationForFeature(featureFlag, userId, attributes); - } + Variation variation = decisionService.getVariationForFeature(featureFlag, userId, attributes); if (variation != null) { LiveVariableUsageInstance liveVariableUsageInstance = variation.getVariableIdToLiveVariableUsageInstanceMap().get(variable.getId()); @@ -553,10 +542,8 @@ Variation getVariation(@Nonnull Experiment experiment, Map filteredAttributes = filterAttributes(projectConfig, attributes); - Variation variation = getForcedVariation(experiment.getKey(), userId); - if (variation == null) { - variation = decisionService.getVariation(experiment, userId, filteredAttributes); - } + Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); + return variation; } @@ -585,10 +572,8 @@ Variation getVariation(@Nonnull String experimentKey, Map filteredAttributes = filterAttributes(projectConfig, attributes); - Variation variation = getForcedVariation(experimentKey, userId); - if (variation == null) { - variation = decisionService.getVariation(experiment, userId, filteredAttributes); - } + Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); + return variation; } @@ -596,6 +581,7 @@ Variation getVariation(@Nonnull String experimentKey, * Force a user into a variation for a given experiment. * The forced variation value does not persist across application launches. * If the experiment key is not in the project file, this call fails and returns false. + * This method just calls into the decision service method of the same signature. * * @param experimentKey The key for the experiment. * @param userId The user ID to be used for bucketing. @@ -608,35 +594,13 @@ public boolean setForcedVariation(@Nonnull String experimentKey, @Nonnull String userId, @Nullable String variationKey) { - ProjectConfig currentConfig = getProjectConfig(); - // if the experiment is not a valid experiment key, don't set it. - Experiment experiment = currentConfig.getExperimentKeyMapping().get(experimentKey); - if (experiment == null || !experiment.isActive()){ - return false; - } - - if (!validateUserId(userId)) { - return false; - } - - Map experimentToVariation = currentConfig.getForcedVariationMapping().get(userId); - if (experimentToVariation == null) { - experimentToVariation = new ConcurrentHashMap(); - currentConfig.getForcedVariationMapping().put(userId, experimentToVariation); - } - if (variationKey == null) { - experimentToVariation.remove(experimentKey); - } - else { - experimentToVariation.put(experimentKey, variationKey); - } - return true; + return decisionService == null? false : decisionService.setForcedVariation(experimentKey, userId, variationKey); } /** * Gets the forced variation for a given user and experiment. - * + * This method just calls into the decision service method of the same signature. * @param experimentKey The key for the experiment. * @param userId The user ID to be used for bucketing. * @@ -645,19 +609,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, */ public @Nullable Variation getForcedVariation(@Nonnull String experimentKey, @Nonnull String userId) { - ProjectConfig currentConfig = getProjectConfig(); - - Map experimentToVariation = currentConfig.getForcedVariationMapping().get(userId); - if (experimentToVariation != null) { - String variationKey = experimentToVariation.get(experimentKey); - if (variationKey != null) { - Experiment experiment = currentConfig.getExperimentKeyMapping().get(experimentKey); - if (experiment != null) { - return experiment.getVariationKeyToVariationMap().get(variationKey); - } - } - } - return null; + return decisionService == null ? null :decisionService.getForcedVariation(experimentKey, userId); } /** diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java index 8d6137321..c43400ed0 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java @@ -30,6 +30,7 @@ import javax.annotation.Nullable; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; /** * Optimizely's decision service that determines which variation of an experiment the user will be allocated to. @@ -82,8 +83,14 @@ public DecisionService(@Nonnull Bucketer bucketer, return null; } + // look for forced bucketing first. + Variation variation = getForcedVariation(experiment.getKey(), userId); + // check for whitelisting - Variation variation = getWhitelistedVariation(experiment, userId); + if (variation == null) { + variation = getWhitelistedVariation(experiment, userId); + } + if (variation != null) { return variation; } @@ -263,4 +270,78 @@ void saveVariation(@Nonnull Experiment experiment, } } } + + /** + * Force a user into a variation for a given experiment. + * The forced variation value does not persist across application launches. + * If the experiment key is not in the project file, this call fails and returns false. + * + * @param experimentKey The key for the experiment. + * @param userId The user ID to be used for bucketing. + * @param variationKey The variation key to force the user into. If the variation key is null + * then the forcedVariation for that experiment is removed. + * + * @return boolean A boolean value that indicates if the set completed successfully. + */ + public boolean setForcedVariation(@Nonnull String experimentKey, + @Nonnull String userId, + @Nullable String variationKey) { + + ProjectConfig currentConfig = projectConfig; + // if the experiment is not a valid experiment key, don't set it. + Experiment experiment = currentConfig.getExperimentKeyMapping().get(experimentKey); + if (experiment == null || !experiment.isActive()){ + return false; + } + + // if the variation is not part of the experiment, return false. + if (variationKey != null && experiment.getVariationKeyToVariationMap().get(variationKey) == null) { + return false; + } + + // if the user id is invalid, return false. + if (userId == null || userId.trim().isEmpty()) { + return false; + } + + Map experimentToVariation = currentConfig.getForcedVariationMapping().get(userId); + if (experimentToVariation == null) { + experimentToVariation = new ConcurrentHashMap(); + currentConfig.getForcedVariationMapping().put(userId, experimentToVariation); + } + if (variationKey == null) { + experimentToVariation.remove(experimentKey); + } + else { + experimentToVariation.put(experimentKey, variationKey); + } + + return true; + } + + /** + * Gets the forced variation for a given user and experiment. + * + * @param experimentKey The key for the experiment. + * @param userId The user ID to be used for bucketing. + * + * @return The variation the user was bucketed into. This value can be null if the + * forced variation fails. + */ + public @Nullable Variation getForcedVariation(@Nonnull String experimentKey, + @Nonnull String userId) { + ProjectConfig currentConfig = projectConfig; + + Map experimentToVariation = currentConfig.getForcedVariationMapping().get(userId); + if (experimentToVariation != null) { + String variationKey = experimentToVariation.get(experimentKey); + if (variationKey != null) { + Experiment experiment = currentConfig.getExperimentKeyMapping().get(experimentKey); + if (experiment != null) { + return experiment.getVariationKeyToVariationMap().get(variationKey); + } + } + } + return null; + } } diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java index d5f731877..d6a71f409 100644 --- a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java +++ b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java @@ -94,7 +94,7 @@ public static void setUp() throws Exception { * over audience evaluation. */ @Test - public void getVariationForcedVariationPrecedesAudienceEval() throws Exception { + public void getVariationWhitelistedPrecedesAudienceEval() throws Exception { Bucketer bucketer = spy(new Bucketer(validProjectConfig)); DecisionService decisionService = spy(new DecisionService(bucketer, mockErrorHandler, validProjectConfig, null)); Experiment experiment = validProjectConfig.getExperiments().get(0); @@ -112,6 +112,99 @@ public void getVariationForcedVariationPrecedesAudienceEval() throws Exception { verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class)); } + /** + * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} gives precedence to forced variation bucketing + * over audience evaluation. + */ + @Test + public void getForcedVariationBeforeWhitelisting() throws Exception { + Bucketer bucketer = spy(new Bucketer(validProjectConfig)); + DecisionService decisionService = spy(new DecisionService(bucketer, mockErrorHandler, validProjectConfig, null)); + Experiment experiment = validProjectConfig.getExperiments().get(0); + Variation whitelistVariation = experiment.getVariations().get(0); + Variation expectedVariation = experiment.getVariations().get(1); + + // user excluded without audiences and whitelisting + assertNull(decisionService.getVariation(experiment, genericUserId, Collections.emptyMap())); + + logbackVerifier.expectMessage(Level.INFO, "User \"" + genericUserId + "\" does not meet conditions to be in experiment \"etag1\"."); + + // set the runtimeForcedVariation + decisionService.setForcedVariation(experiment.getKey(), whitelistedUserId, expectedVariation.getKey()); + // no attributes provided for a experiment that has an audience + assertThat(decisionService.getVariation(experiment, whitelistedUserId, Collections.emptyMap()), is(expectedVariation)); + + //verify(decisionService).getForcedVariation(experiment.getKey(), whitelistedUserId); + verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class)); + assertEquals(decisionService.getWhitelistedVariation(experiment, whitelistedUserId), whitelistVariation); + assertEquals(decisionService.setForcedVariation(experiment.getKey(), whitelistedUserId,null), true); + assertNull(decisionService.getForcedVariation(experiment.getKey(), whitelistedUserId)); + } + + /** + * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} gives precedence to forced variation bucketing + * over audience evaluation. + */ + @Test + public void getVariationForcedPrecedesAudienceEval() throws Exception { + Bucketer bucketer = spy(new Bucketer(validProjectConfig)); + DecisionService decisionService = spy(new DecisionService(bucketer, mockErrorHandler, validProjectConfig, null)); + Experiment experiment = validProjectConfig.getExperiments().get(0); + Variation expectedVariation = experiment.getVariations().get(1); + + // user excluded without audiences and whitelisting + assertNull(decisionService.getVariation(experiment, genericUserId, Collections.emptyMap())); + + logbackVerifier.expectMessage(Level.INFO, "User \"" + genericUserId + "\" does not meet conditions to be in experiment \"etag1\"."); + + // set the runtimeForcedVariation + decisionService.setForcedVariation(experiment.getKey(), genericUserId, expectedVariation.getKey()); + // no attributes provided for a experiment that has an audience + assertThat(decisionService.getVariation(experiment, genericUserId, Collections.emptyMap()), is(expectedVariation)); + + verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class)); + assertEquals(decisionService.setForcedVariation(experiment.getKey(), genericUserId,null), true); + assertNull(decisionService.getForcedVariation(experiment.getKey(), genericUserId)); + } + + /** + * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} + * gives precedence to user profile over audience evaluation. + */ + @Test + public void getVariationForcedBeforeUserProfile() throws Exception { + Experiment experiment = validProjectConfig.getExperiments().get(0); + Variation variation = experiment.getVariations().get(0); + Bucketer bucketer = spy(new Bucketer(validProjectConfig)); + Decision decision = new Decision(variation.getId()); + UserProfile userProfile = new UserProfile(userProfileId, + Collections.singletonMap(experiment.getId(), decision)); + UserProfileService userProfileService = mock(UserProfileService.class); + when(userProfileService.lookup(userProfileId)).thenReturn(userProfile.toMap()); + + DecisionService decisionService = spy(new DecisionService(bucketer, + mockErrorHandler, validProjectConfig, userProfileService)); + + // ensure that normal users still get excluded from the experiment when they fail audience evaluation + assertNull(decisionService.getVariation(experiment, genericUserId, Collections.emptyMap())); + + logbackVerifier.expectMessage(Level.INFO, + "User \"" + genericUserId + "\" does not meet conditions to be in experiment \"" + + experiment.getKey() + "\"."); + + // ensure that a user with a saved user profile, sees the same variation regardless of audience evaluation + assertEquals(variation, + decisionService.getVariation(experiment, userProfileId, Collections.emptyMap())); + + Variation forcedVariation = experiment.getVariations().get(1); + decisionService.setForcedVariation(experiment.getKey(), userProfileId, forcedVariation.getKey()); + assertEquals(forcedVariation, + decisionService.getVariation(experiment, userProfileId, Collections.emptyMap())); + assertEquals(decisionService.setForcedVariation(experiment.getKey(), userProfileId, null), true); + assertNull(decisionService.getForcedVariation(experiment.getKey(), userProfileId)); + + + } /** * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} @@ -141,6 +234,7 @@ public void getVariationEvaluatesUserProfileBeforeAudienceTargeting() throws Exc // ensure that a user with a saved user profile, sees the same variation regardless of audience evaluation assertEquals(variation, decisionService.getVariation(experiment, userProfileId, Collections.emptyMap())); + } //========== get Variation for Feature tests ==========// @@ -252,7 +346,7 @@ public void getVariationForFeatureReturnsVariationReturnedFromGetVarition() { * Test {@link DecisionService#getWhitelistedVariation(Experiment, String)} correctly returns a whitelisted variation. */ @Test - public void getForcedVariationReturnsForcedVariation() { + public void getWhitelistedReturnsForcedVariation() { Bucketer bucketer = new Bucketer(validProjectConfig); DecisionService decisionService = new DecisionService(bucketer, mockErrorHandler, validProjectConfig, null); @@ -266,7 +360,7 @@ public void getForcedVariationReturnsForcedVariation() { * when an invalid variation key is found in the forced variations mapping. */ @Test - public void getForcedVariationWithInvalidVariation() throws Exception { + public void getWhitelistedWithInvalidVariation() throws Exception { String userId = "testUser1"; String invalidVariationKey = "invalidVarKey"; @@ -297,7 +391,7 @@ public void getForcedVariationWithInvalidVariation() throws Exception { * Verify that {@link DecisionService#getWhitelistedVariation(Experiment, String)} returns null when user is not whitelisted. */ @Test - public void getForcedVariationReturnsNullWhenUserIsNotWhitelisted() throws Exception { + public void getWhitelistedReturnsNullWhenUserIsNotWhitelisted() throws Exception { Bucketer bucketer = new Bucketer(validProjectConfig); DecisionService decisionService = new DecisionService(bucketer, mockErrorHandler, validProjectConfig, null); From 766116458fe739cc1dbd2006c1e7fbd020605377 Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Wed, 16 Aug 2017 17:44:48 -0700 Subject: [PATCH 07/16] move methods to project config. decision logic in decision service --- .../java/com/optimizely/ab/Optimizely.java | 4 +- .../ab/bucketing/DecisionService.java | 75 +------------------ .../optimizely/ab/config/ProjectConfig.java | 73 ++++++++++++++++++ .../ab/bucketing/DecisionServiceTest.java | 18 ++--- 4 files changed, 85 insertions(+), 85 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 831b6637e..7e87e6045 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -595,7 +595,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, @Nullable String variationKey) { - return decisionService == null? false : decisionService.setForcedVariation(experimentKey, userId, variationKey); + return projectConfig.setForcedVariation(experimentKey, userId, variationKey); } /** @@ -609,7 +609,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, */ public @Nullable Variation getForcedVariation(@Nonnull String experimentKey, @Nonnull String userId) { - return decisionService == null ? null :decisionService.getForcedVariation(experimentKey, userId); + return projectConfig.getForcedVariation(experimentKey, userId); } /** diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java index c43400ed0..dba2aa3cc 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java @@ -84,7 +84,7 @@ public DecisionService(@Nonnull Bucketer bucketer, } // look for forced bucketing first. - Variation variation = getForcedVariation(experiment.getKey(), userId); + Variation variation = projectConfig.getForcedVariation(experiment.getKey(), userId); // check for whitelisting if (variation == null) { @@ -271,77 +271,4 @@ void saveVariation(@Nonnull Experiment experiment, } } - /** - * Force a user into a variation for a given experiment. - * The forced variation value does not persist across application launches. - * If the experiment key is not in the project file, this call fails and returns false. - * - * @param experimentKey The key for the experiment. - * @param userId The user ID to be used for bucketing. - * @param variationKey The variation key to force the user into. If the variation key is null - * then the forcedVariation for that experiment is removed. - * - * @return boolean A boolean value that indicates if the set completed successfully. - */ - public boolean setForcedVariation(@Nonnull String experimentKey, - @Nonnull String userId, - @Nullable String variationKey) { - - ProjectConfig currentConfig = projectConfig; - // if the experiment is not a valid experiment key, don't set it. - Experiment experiment = currentConfig.getExperimentKeyMapping().get(experimentKey); - if (experiment == null || !experiment.isActive()){ - return false; - } - - // if the variation is not part of the experiment, return false. - if (variationKey != null && experiment.getVariationKeyToVariationMap().get(variationKey) == null) { - return false; - } - - // if the user id is invalid, return false. - if (userId == null || userId.trim().isEmpty()) { - return false; - } - - Map experimentToVariation = currentConfig.getForcedVariationMapping().get(userId); - if (experimentToVariation == null) { - experimentToVariation = new ConcurrentHashMap(); - currentConfig.getForcedVariationMapping().put(userId, experimentToVariation); - } - if (variationKey == null) { - experimentToVariation.remove(experimentKey); - } - else { - experimentToVariation.put(experimentKey, variationKey); - } - - return true; - } - - /** - * Gets the forced variation for a given user and experiment. - * - * @param experimentKey The key for the experiment. - * @param userId The user ID to be used for bucketing. - * - * @return The variation the user was bucketed into. This value can be null if the - * forced variation fails. - */ - public @Nullable Variation getForcedVariation(@Nonnull String experimentKey, - @Nonnull String userId) { - ProjectConfig currentConfig = projectConfig; - - Map experimentToVariation = currentConfig.getForcedVariationMapping().get(userId); - if (experimentToVariation != null) { - String variationKey = experimentToVariation.get(experimentKey); - if (variationKey != null) { - Experiment experiment = currentConfig.getExperimentKeyMapping().get(experimentKey); - if (experiment != null) { - return experiment.getVariationKeyToVariationMap().get(variationKey); - } - } - } - return null; - } } diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index d89c44f97..a8eb70138 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -20,6 +20,7 @@ import com.optimizely.ab.config.audience.Audience; import com.optimizely.ab.config.audience.Condition; +import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; import java.util.ArrayList; @@ -313,6 +314,78 @@ public Map getFeatureKeyMapping() { public Map> getForcedVariationMapping() { return forcedVariationMapping; } + /** + * Force a user into a variation for a given experiment. + * The forced variation value does not persist across application launches. + * If the experiment key is not in the project file, this call fails and returns false. + * + * @param experimentKey The key for the experiment. + * @param userId The user ID to be used for bucketing. + * @param variationKey The variation key to force the user into. If the variation key is null + * then the forcedVariation for that experiment is removed. + * + * @return boolean A boolean value that indicates if the set completed successfully. + */ + public boolean setForcedVariation(@Nonnull String experimentKey, + @Nonnull String userId, + @Nullable String variationKey) { + + // if the experiment is not a valid experiment key, don't set it. + Experiment experiment = getExperimentKeyMapping().get(experimentKey); + if (experiment == null || !experiment.isActive()){ + return false; + } + + // if the variation is not part of the experiment, return false. + if (variationKey != null && experiment.getVariationKeyToVariationMap().get(variationKey) == null) { + return false; + } + + // if the user id is invalid, return false. + if (userId == null || userId.trim().isEmpty()) { + return false; + } + + Map experimentToVariation = getForcedVariationMapping().get(userId); + if (experimentToVariation == null) { + experimentToVariation = new ConcurrentHashMap(); + getForcedVariationMapping().put(userId, experimentToVariation); + } + if (variationKey == null) { + experimentToVariation.remove(experimentKey); + } + else { + experimentToVariation.put(experimentKey, variationKey); + } + + return true; + } + + /** + * Gets the forced variation for a given user and experiment. + * + * @param experimentKey The key for the experiment. + * @param userId The user ID to be used for bucketing. + * + * @return The variation the user was bucketed into. This value can be null if the + * forced variation fails. + */ + public @Nullable Variation getForcedVariation(@Nonnull String experimentKey, + @Nonnull String userId) { + + Map experimentToVariation = getForcedVariationMapping().get(userId); + if (experimentToVariation != null) { + String variationKey = experimentToVariation.get(experimentKey); + if (variationKey != null) { + Experiment experiment = getExperimentKeyMapping().get(experimentKey); + if (experiment != null) { + return experiment.getVariationKeyToVariationMap().get(variationKey); + } + } + } + return null; + } + @Override public String toString() { return "ProjectConfig{" + diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java index d6a71f409..d8d1a3353 100644 --- a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java +++ b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java @@ -130,15 +130,15 @@ public void getForcedVariationBeforeWhitelisting() throws Exception { logbackVerifier.expectMessage(Level.INFO, "User \"" + genericUserId + "\" does not meet conditions to be in experiment \"etag1\"."); // set the runtimeForcedVariation - decisionService.setForcedVariation(experiment.getKey(), whitelistedUserId, expectedVariation.getKey()); + validProjectConfig.setForcedVariation(experiment.getKey(), whitelistedUserId, expectedVariation.getKey()); // no attributes provided for a experiment that has an audience assertThat(decisionService.getVariation(experiment, whitelistedUserId, Collections.emptyMap()), is(expectedVariation)); //verify(decisionService).getForcedVariation(experiment.getKey(), whitelistedUserId); verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class)); assertEquals(decisionService.getWhitelistedVariation(experiment, whitelistedUserId), whitelistVariation); - assertEquals(decisionService.setForcedVariation(experiment.getKey(), whitelistedUserId,null), true); - assertNull(decisionService.getForcedVariation(experiment.getKey(), whitelistedUserId)); + assertEquals(validProjectConfig.setForcedVariation(experiment.getKey(), whitelistedUserId,null), true); + assertNull(validProjectConfig.getForcedVariation(experiment.getKey(), whitelistedUserId)); } /** @@ -158,13 +158,13 @@ public void getVariationForcedPrecedesAudienceEval() throws Exception { logbackVerifier.expectMessage(Level.INFO, "User \"" + genericUserId + "\" does not meet conditions to be in experiment \"etag1\"."); // set the runtimeForcedVariation - decisionService.setForcedVariation(experiment.getKey(), genericUserId, expectedVariation.getKey()); + validProjectConfig.setForcedVariation(experiment.getKey(), genericUserId, expectedVariation.getKey()); // no attributes provided for a experiment that has an audience assertThat(decisionService.getVariation(experiment, genericUserId, Collections.emptyMap()), is(expectedVariation)); verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class)); - assertEquals(decisionService.setForcedVariation(experiment.getKey(), genericUserId,null), true); - assertNull(decisionService.getForcedVariation(experiment.getKey(), genericUserId)); + assertEquals(validProjectConfig.setForcedVariation(experiment.getKey(), genericUserId,null), true); + assertNull(validProjectConfig.getForcedVariation(experiment.getKey(), genericUserId)); } /** @@ -197,11 +197,11 @@ public void getVariationForcedBeforeUserProfile() throws Exception { decisionService.getVariation(experiment, userProfileId, Collections.emptyMap())); Variation forcedVariation = experiment.getVariations().get(1); - decisionService.setForcedVariation(experiment.getKey(), userProfileId, forcedVariation.getKey()); + validProjectConfig.setForcedVariation(experiment.getKey(), userProfileId, forcedVariation.getKey()); assertEquals(forcedVariation, decisionService.getVariation(experiment, userProfileId, Collections.emptyMap())); - assertEquals(decisionService.setForcedVariation(experiment.getKey(), userProfileId, null), true); - assertNull(decisionService.getForcedVariation(experiment.getKey(), userProfileId)); + assertEquals(validProjectConfig.setForcedVariation(experiment.getKey(), userProfileId, null), true); + assertNull(validProjectConfig.getForcedVariation(experiment.getKey(), userProfileId)); } From 6ecb1e1f9e78ec967cb6b811fcee79880e794cdd Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Wed, 16 Aug 2017 19:20:17 -0700 Subject: [PATCH 08/16] update to reflect spacing issues with my cut and paste --- .../java/com/optimizely/ab/Optimizely.java | 21 +++++++++---------- .../optimizely/ab/config/ProjectConfig.java | 10 +++++---- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 7e87e6045..59d7c7b00 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -171,7 +171,6 @@ Variation activate(@Nonnull ProjectConfig projectConfig, // bucket the user to the given experiment and dispatch an impression event Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); - if (variation == null) { logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey()); return null; @@ -253,7 +252,7 @@ public void track(@Nonnull String eventName, Map experimentVariationMap = new HashMap(experimentsForEvent.size()); for (Experiment experiment : experimentsForEvent) { if (experiment.isRunning()) { - Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); + Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); if (variation != null) { experimentVariationMap.put(experiment, variation); } @@ -333,6 +332,7 @@ public void track(@Nonnull String eventName, Map filteredAttributes = filterAttributes(projectConfig, attributes); Variation variation = decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes); + if (variation != null) { Experiment experiment = projectConfig.getExperimentForVariationId(variation.getId()); if (experiment != null) { @@ -510,7 +510,7 @@ else if (!variable.getType().equals(variableType)) { String variableValue = variable.getDefaultValue(); - Variation variation = decisionService.getVariationForFeature(featureFlag, userId, attributes); + Variation variation = decisionService.getVariationForFeature(featureFlag, userId, attributes); if (variation != null) { LiveVariableUsageInstance liveVariableUsageInstance = variation.getVariableIdToLiveVariableUsageInstanceMap().get(variable.getId()); @@ -542,9 +542,7 @@ Variation getVariation(@Nonnull Experiment experiment, Map filteredAttributes = filterAttributes(projectConfig, attributes); - Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); - - return variation; + return decisionService.getVariation(experiment, userId, filteredAttributes); } public @Nullable @@ -572,16 +570,15 @@ Variation getVariation(@Nonnull String experimentKey, Map filteredAttributes = filterAttributes(projectConfig, attributes); - Variation variation = decisionService.getVariation(experiment, userId, filteredAttributes); - - return variation; + return decisionService.getVariation(experiment, userId, filteredAttributes); } /** * Force a user into a variation for a given experiment. * The forced variation value does not persist across application launches. * If the experiment key is not in the project file, this call fails and returns false. - * This method just calls into the decision service method of the same signature. + * This method just calls into the {@link com.optimizely.ab.config.ProjectConfig#setForcedVariation(String, String, String)} + * method of the same signature. * * @param experimentKey The key for the experiment. * @param userId The user ID to be used for bucketing. @@ -600,7 +597,9 @@ public boolean setForcedVariation(@Nonnull String experimentKey, /** * Gets the forced variation for a given user and experiment. - * This method just calls into the decision service method of the same signature. + * This method just calls into the {@link com.optimizely.ab.config.ProjectConfig#getForcedVariation(String, String)} + * method of the same signature. + * * @param experimentKey The key for the experiment. * @param userId The user ID to be used for bucketing. * diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index a8eb70138..45c03847b 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -86,9 +86,11 @@ public String toString() { private final Map> variationToLiveVariableUsageInstanceMapping; private final Map variationIdToExperimentMapping; - // forced variations supersede any other mappings. They are transient and are not persistent or part of - // the actual datafile. This map should not be accessed directly, but, instead through apis at the - // Optimizely object level. + /* Forced variations supersede any other mappings. They are transient and are not persistent or part of + * the actual datafile. This contains all the forced variations + * set by the user by calling setForcedVariation (it is not the same as the + * whitelisting forcedVariations data structure in the Experiments class). + */ private transient Map> forcedVariationMapping = new ConcurrentHashMap>(); // v2 constructor @@ -332,7 +334,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, // if the experiment is not a valid experiment key, don't set it. Experiment experiment = getExperimentKeyMapping().get(experimentKey); - if (experiment == null || !experiment.isActive()){ + if (experiment == null){ return false; } From 70f6e050cbeb09a2acdaada168375584676c4267 Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Wed, 16 Aug 2017 19:46:27 -0700 Subject: [PATCH 09/16] more line cleanup --- .../java/com/optimizely/ab/Optimizely.java | 3 ++- .../ab/bucketing/DecisionService.java | 2 -- .../ab/bucketing/DecisionServiceTest.java | 18 +++++++++--------- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 59d7c7b00..0e1a3870b 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -511,6 +511,7 @@ else if (!variable.getType().equals(variableType)) { String variableValue = variable.getDefaultValue(); Variation variation = decisionService.getVariationForFeature(featureFlag, userId, attributes); + if (variation != null) { LiveVariableUsageInstance liveVariableUsageInstance = variation.getVariableIdToLiveVariableUsageInstanceMap().get(variable.getId()); @@ -570,7 +571,7 @@ Variation getVariation(@Nonnull String experimentKey, Map filteredAttributes = filterAttributes(projectConfig, attributes); - return decisionService.getVariation(experiment, userId, filteredAttributes); + return decisionService.getVariation(experiment,userId,filteredAttributes); } /** diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java index dba2aa3cc..50b9241b8 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java @@ -30,7 +30,6 @@ import javax.annotation.Nullable; import java.util.HashMap; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; /** * Optimizely's decision service that determines which variation of an experiment the user will be allocated to. @@ -270,5 +269,4 @@ void saveVariation(@Nonnull Experiment experiment, } } } - } diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java index d8d1a3353..0ea651644 100644 --- a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java +++ b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java @@ -45,8 +45,8 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyMap; import static org.mockito.Matchers.anyMapOf; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; @@ -90,8 +90,8 @@ public static void setUp() throws Exception { //========= getVariation tests =========/ /** - * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} gives precedence to forced variation bucketing - * over audience evaluation. + * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} + * gives precedence to forced variation bucketing over audience evaluation. */ @Test public void getVariationWhitelistedPrecedesAudienceEval() throws Exception { @@ -113,8 +113,8 @@ public void getVariationWhitelistedPrecedesAudienceEval() throws Exception { } /** - * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} gives precedence to forced variation bucketing - * over audience evaluation. + * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} + * gives precedence to forced variation bucketing over whitelisting. */ @Test public void getForcedVariationBeforeWhitelisting() throws Exception { @@ -142,8 +142,8 @@ public void getForcedVariationBeforeWhitelisting() throws Exception { } /** - * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} gives precedence to forced variation bucketing - * over audience evaluation. + * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} + * gives precedence to forced variation bucketing over audience evaluation. */ @Test public void getVariationForcedPrecedesAudienceEval() throws Exception { @@ -169,7 +169,7 @@ public void getVariationForcedPrecedesAudienceEval() throws Exception { /** * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} - * gives precedence to user profile over audience evaluation. + * gives precedence to forced variation bucketing over user profile. */ @Test public void getVariationForcedBeforeUserProfile() throws Exception { @@ -200,7 +200,7 @@ public void getVariationForcedBeforeUserProfile() throws Exception { validProjectConfig.setForcedVariation(experiment.getKey(), userProfileId, forcedVariation.getKey()); assertEquals(forcedVariation, decisionService.getVariation(experiment, userProfileId, Collections.emptyMap())); - assertEquals(validProjectConfig.setForcedVariation(experiment.getKey(), userProfileId, null), true); + assertTrue(validProjectConfig.setForcedVariation(experiment.getKey(), userProfileId, null)); assertNull(validProjectConfig.getForcedVariation(experiment.getKey(), userProfileId)); From bf0efd93afdf348854a02bf161b3fed04b83243e Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Thu, 17 Aug 2017 13:35:55 -0700 Subject: [PATCH 10/16] use variation and experiment ids instead of key for storage. use putIfAbsent --- .../optimizely/ab/config/ProjectConfig.java | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 45c03847b..41c974314 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -338,8 +338,9 @@ public boolean setForcedVariation(@Nonnull String experimentKey, return false; } + Variation variation = (variationKey == null) ? null : experiment.getVariationKeyToVariationMap().get(variationKey); // if the variation is not part of the experiment, return false. - if (variationKey != null && experiment.getVariationKeyToVariationMap().get(variationKey) == null) { + if (variationKey != null && variation == null) { return false; } @@ -348,16 +349,26 @@ public boolean setForcedVariation(@Nonnull String experimentKey, return false; } - Map experimentToVariation = getForcedVariationMapping().get(userId); + Map> userForcedVariationMapping = getForcedVariationMapping(); + Map experimentToVariation = userForcedVariationMapping.get(userId); if (experimentToVariation == null) { experimentToVariation = new ConcurrentHashMap(); - getForcedVariationMapping().put(userId, experimentToVariation); + // sanity check... + if (userForcedVariationMapping instanceof ConcurrentHashMap) { + Map experimentToVariationExists = (Map) ((ConcurrentHashMap)userForcedVariationMapping).putIfAbsent(userId, experimentToVariation); + if (experimentToVariationExists != null) { + experimentToVariation = experimentToVariationExists; + } + } + else { + userForcedVariationMapping.put(userId, experimentToVariation); + } } if (variationKey == null) { - experimentToVariation.remove(experimentKey); + experimentToVariation.remove(experiment.getId()); } else { - experimentToVariation.put(experimentKey, variationKey); + experimentToVariation.put(experiment.getId(), variation.getId()); } return true; @@ -377,12 +388,13 @@ public boolean setForcedVariation(@Nonnull String experimentKey, Map experimentToVariation = getForcedVariationMapping().get(userId); if (experimentToVariation != null) { - String variationKey = experimentToVariation.get(experimentKey); - if (variationKey != null) { - Experiment experiment = getExperimentKeyMapping().get(experimentKey); - if (experiment != null) { - return experiment.getVariationKeyToVariationMap().get(variationKey); - } + Experiment experiment = getExperimentKeyMapping().get(experimentKey); + if (experiment == null) { + return null; + } + String variationId = experimentToVariation.get(experiment.getId()); + if (variationId != null) { + return experiment.getVariationIdToVariationMap().get(variationId); } } return null; From a5bae5d125879a8eaf7b209f7e0376aab4ff96cc Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Fri, 18 Aug 2017 15:09:16 -0700 Subject: [PATCH 11/16] added all unit tests and code coverage report from jacocoTestReport --- build.gradle | 8 + .../optimizely/ab/config/ProjectConfig.java | 11 +- .../com/optimizely/ab/OptimizelyTest.java | 171 +++++++++++++++++- .../ab/config/ProjectConfigTest.java | 141 ++++++++++++++- 4 files changed, 320 insertions(+), 11 deletions(-) diff --git a/build.gradle b/build.gradle index 8d6721170..9e700f9c5 100644 --- a/build.gradle +++ b/build.gradle @@ -47,6 +47,14 @@ subprojects { jcenter() } + jacocoTestReport { + reports { + xml.enabled false + csv.enabled false + html.destination file("${buildDir}/jacocoHtml") + } + } + task sourcesJar(type: Jar, dependsOn: classes) { classifier = 'sources' from sourceSets.main.allSource diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 41c974314..13b5e755e 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -364,14 +364,15 @@ public boolean setForcedVariation(@Nonnull String experimentKey, userForcedVariationMapping.put(userId, experimentToVariation); } } + boolean retVal = true; if (variationKey == null) { - experimentToVariation.remove(experiment.getId()); + retVal = (experimentToVariation.remove(experiment.getId()) != null); } else { experimentToVariation.put(experiment.getId(), variation.getId()); } - return true; + return retVal; } /** @@ -386,6 +387,12 @@ public boolean setForcedVariation(@Nonnull String experimentKey, public @Nullable Variation getForcedVariation(@Nonnull String experimentKey, @Nonnull String userId) { + // if the user id is invalid, return false. + if (userId == null || userId.trim().isEmpty() || + experimentKey == null || experimentKey.isEmpty()) { + return null; + } + Map experimentToVariation = getForcedVariationMapping().get(userId); if (experimentToVariation != null) { Experiment experiment = getExperimentKeyMapping().get(experimentKey); diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 5f40bfdc4..a5603b332 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -53,12 +53,7 @@ import org.mockito.junit.MockitoRule; import java.io.IOException; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import static com.optimizely.ab.config.ProjectConfigTestUtils.noAudienceProjectConfigJsonV2; import static com.optimizely.ab.config.ProjectConfigTestUtils.noAudienceProjectConfigJsonV3; @@ -349,6 +344,103 @@ public void activateWithExperimentKeyForced() throws Exception { } + /** + * Verify that the {@link Optimizely#getVariation(String, String, Map)} call + * uses forced variation to force the user into the second variation. The mock bucket returns + * the first variation. Then remove the forced variation and confirm that the forced variation is null. + */ + @Test + public void getVariationWithExperimentKeyForced() throws Exception { + Experiment activatedExperiment = validProjectConfig.getExperiments().get(0); + Variation forcedVariation = activatedExperiment.getVariations().get(1); + Variation bucketedVariation = activatedExperiment.getVariations().get(0); + EventBuilder mockEventBuilder = mock(EventBuilder.class); + + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withBucketing(mockBucketer) + .withEventBuilder(mockEventBuilder) + .withConfig(validProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", forcedVariation.getKey() ); + + Map testUserAttributes = new HashMap(); + if (datafileVersion == 4) { + testUserAttributes.put(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); + } + else { + testUserAttributes.put("browser_type", "chrome"); + } + + Map testParams = new HashMap(); + testParams.put("test", "params"); + + LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); + when(mockEventBuilder.createImpressionEvent(eq(validProjectConfig), eq(activatedExperiment), eq(forcedVariation), + eq("userId"), eq(testUserAttributes))) + .thenReturn(logEventToDispatch); + + when(mockBucketer.bucket(activatedExperiment, "userId")) + .thenReturn(bucketedVariation); + + // activate the experiment + Variation actualVariation = optimizely.getVariation(activatedExperiment.getKey(), "userId", testUserAttributes); + + assertThat(actualVariation, is(forcedVariation)); + + optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", null ); + + assertEquals(optimizely.getForcedVariation(activatedExperiment.getKey(), "userId"), null); + + } + + /** + * Verify that the {@link Optimizely#activate(String, String, Map)} call + * uses forced variation to force the user into the second variation. The mock bucket returns + * the first variation. Then remove the forced variation and confirm that the forced variation is null. + */ + @Test + public void isFeatureEnabledWithExperimentKeyForced() throws Exception { + assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString())); + + Experiment activatedExperiment = validProjectConfig.getExperimentKeyMapping().get(EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY); + Variation forcedVariation = activatedExperiment.getVariations().get(1); + EventBuilder mockEventBuilder = mock(EventBuilder.class); + + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withBucketing(mockBucketer) + .withEventBuilder(mockEventBuilder) + .withConfig(validProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", forcedVariation.getKey() ); + + Map testUserAttributes = new HashMap(); + if (datafileVersion != 4) { + testUserAttributes.put("browser_type", "chrome"); + } + + Map testParams = new HashMap(); + testParams.put("test", "params"); + + LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); + when(mockEventBuilder.createImpressionEvent(eq(validProjectConfig), eq(activatedExperiment), eq(forcedVariation), + eq("userId"), eq(testUserAttributes))) + .thenReturn(logEventToDispatch); + + // activate the experiment + assertTrue(optimizely.isFeatureEnabled(FEATURE_FLAG_MULTI_VARIATE_FEATURE.getKey(), "userId")); + + verify(mockEventHandler).dispatchEvent(logEventToDispatch); + + optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", null ); + + assertEquals(optimizely.getForcedVariation(activatedExperiment.getKey(), "userId"), null); + + } + /** * Verify that the {@link Optimizely#activate(String, String, Map)} call * correctly builds an endpoint url and request params @@ -966,6 +1058,73 @@ public void activateLaunchedExperimentDoesNotDispatchEvent() throws Exception { //======== track tests ========// + /** + * Verify that the {@link Optimizely#track(String, String)} call correctly builds a V2 event and passes it + * through {@link EventHandler#dispatchEvent(LogEvent)}. + */ + @Test + public void trackEventEndToEndForced() throws Exception { + EventType eventType; + String datafile; + ProjectConfig config; + if (datafileVersion == 4) { + config = validProjectConfig; + eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); + datafile = validDatafile; + } + else { + config = noAudienceProjectConfig; + eventType = noAudienceProjectConfig.getEventTypes().get(0); + datafile = noAudienceDatafile; + } + List allExperiments = new ArrayList(); + allExperiments.add(config.getExperiments().get(0)); + EventBuilder eventBuilderV2 = new EventBuilderV2(); + DecisionService spyDecisionService = spy(new DecisionService(mockBucketer, + mockErrorHandler, + validProjectConfig, + null)); + + Optimizely optimizely = Optimizely.builder(datafile, mockEventHandler) + .withDecisionService(spyDecisionService) + .withEventBuilder(eventBuilderV2) + .withConfig(noAudienceProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + // Force to the first variation for all experiments. However, only a subset of the experiments will actually + // call get forced. + for (Experiment experiment : allExperiments) { + optimizely.projectConfig.setForcedVariation(experiment.getKey(), "userId", experiment.getVariations().get(0).getKey()); + } + + // call track + optimizely.track(eventType.getKey(), "userId"); + + // verify that the bucketing algorithm was called only on experiments corresponding to the specified goal. + List experimentsForEvent = config.getExperimentsForEventKey(eventType.getKey()); + for (Experiment experiment : allExperiments) { + if (experiment.isRunning() && experimentsForEvent.contains(experiment)) { + verify(spyDecisionService).getVariation(experiment, "userId", + Collections.emptyMap()); + } else { + verify(spyDecisionService, never()).getVariation(experiment, "userId", + Collections.emptyMap()); + } + } + + // verify that dispatchEvent was called + if (datafileVersion != 4) { + verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); + } + else { + verify(mockEventHandler).dispatchEvent(any(LogEvent.class)); + } + for (Experiment experiment : allExperiments) { + optimizely.projectConfig.setForcedVariation(experiment.getKey(), "userId", null); + } + } + /** * Verify that the {@link Optimizely#track(String, String)} call correctly builds a V2 event and passes it * through {@link EventHandler#dispatchEvent(LogEvent)}. diff --git a/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java b/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java index c0cedc238..430c8e324 100644 --- a/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java +++ b/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java @@ -30,14 +30,13 @@ import static java.util.Arrays.asList; import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThat; +import static org.junit.Assert.*; import org.junit.Before; import org.junit.Test; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import org.junit.experimental.theories.suppliers.TestedOn; /** * Tests for {@link ProjectConfig}. @@ -195,4 +194,140 @@ public void verifyAnonymizeIPIsFalseByDefault() throws Exception { ProjectConfig v2ProjectConfig = ProjectConfigTestUtils.validProjectConfigV2(); assertFalse(v2ProjectConfig.getAnonymizeIP()); } + + /** + * Invalid User IDs + + User ID is null + User ID is an empty string + Invalid Experiment IDs + + Experiment key does not exist in the datafile + Experiment key is null + Experiment key is an empty string + Invalid Variation IDs [set only] + + Variation key does not exist in the datafile + Variation key is null + Variation key is an empty string + Multiple set calls [set only] + + Call set variation with different variations on one user/experiment to confirm that each set is expected. + Set variation on multiple variations for one user. + Set variations for multiple users. + */ + /* UserID test */ + @Test + @SuppressFBWarnings("NP") + public void setForcedVariationNullUserId() { + boolean b = projectConfig.setForcedVariation("etag1", null, "vtag1"); + assertFalse(b); + } + @Test + @SuppressFBWarnings("NP") + public void getForcedVariationNullUserId() { + assertNull(projectConfig.getForcedVariation("etag1", null)); + } + + @Test + public void setForcedVariationEmptyUserId() { + assertFalse(projectConfig.setForcedVariation("etag1", "", "vtag1")); + } + @Test + public void getForcedVariationEmptyUserId() { + assertNull(projectConfig.getForcedVariation("etag1", "")); + } + + /* Invalid Experiement */ + @Test + @SuppressFBWarnings("NP") + public void setForcedVariationNullExperimentKey() { + assertFalse(projectConfig.setForcedVariation(null, "testUser1", "vtag1")); + } + @Test + @SuppressFBWarnings("NP") + public void getForcedVariationNullExperimentKey() { + assertNull(projectConfig.getForcedVariation(null, "testUser1")); + } + + @Test + public void setForcedVariationWrongExperimentKey() { + assertFalse(projectConfig.setForcedVariation("wrongKey", "testUser1", "vtag1")); + + } + @Test + public void getForcedVariationWrongExperimentKey() { + assertNull(projectConfig.getForcedVariation("wrongKey", "testUser1")); + } + + @Test + public void setForcedVariationEmptyExperimentKey() { + assertFalse(projectConfig.setForcedVariation("", "testUser1", "vtag1")); + + } + @Test + public void getForcedVariationEmptyExperimentKey() { + assertNull(projectConfig.getForcedVariation("", "testUser1")); + } + + /* Invalid Variation Id (set only */ + @Test + public void setForcedVariationWrongVariationKey() { + assertFalse(projectConfig.setForcedVariation("etag1", "testUser1", "vtag3")); + } + + @Test + public void setForcedVariationNullVariationKey() { + assertFalse(projectConfig.setForcedVariation("etag1", "testUser1", null)); + } + + @Test + public void setForcedVariationEmptyVariationKey() { + assertFalse(projectConfig.setForcedVariation("etag1", "testUser1", "")); + } + + /* Multiple set calls (set only */ + @Test + public void setForcedVariationDifferentVariations() { + assertTrue(projectConfig.setForcedVariation("etag1", "testUser1", "vtag1")); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser1", "vtag2")); + assertEquals(projectConfig.getForcedVariation("etag1", "testUser1").getKey(), "vtag2"); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser1", null)); + } + + @Test + public void setForcedVariationMultipleVariationsExperiments() { + assertTrue(projectConfig.setForcedVariation("etag1", "testUser1", "vtag1")); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser2", "vtag2")); + assertTrue(projectConfig.setForcedVariation("etag2", "testUser1", "vtag3")); + assertTrue(projectConfig.setForcedVariation("etag2", "testUser2", "vtag4")); + assertEquals(projectConfig.getForcedVariation("etag1", "testUser1").getKey(), "vtag1"); + assertEquals(projectConfig.getForcedVariation("etag1", "testUser2").getKey(), "vtag2"); + assertEquals(projectConfig.getForcedVariation("etag2", "testUser1").getKey(), "vtag3"); + assertEquals(projectConfig.getForcedVariation("etag2", "testUser2").getKey(), "vtag4"); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser1", null)); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser2", null)); + assertTrue(projectConfig.setForcedVariation("etag2", "testUser1", null)); + assertTrue(projectConfig.setForcedVariation("etag2", "testUser2", null)); + + } + + @Test + public void setForcedVariationMultipleUsers() { + assertTrue(projectConfig.setForcedVariation("etag1", "testUser1", "vtag1")); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser2", "vtag1")); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser3", "vtag1")); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser4", "vtag1")); + + assertEquals(projectConfig.getForcedVariation("etag1", "testUser1").getKey(), "vtag1"); + assertEquals(projectConfig.getForcedVariation("etag1", "testUser2").getKey(), "vtag1"); + assertEquals(projectConfig.getForcedVariation("etag1", "testUser3").getKey(), "vtag1"); + assertEquals(projectConfig.getForcedVariation("etag1", "testUser4").getKey(), "vtag1"); + + assertTrue(projectConfig.setForcedVariation("etag1", "testUser1", null)); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser2", null)); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser3", null)); + assertTrue(projectConfig.setForcedVariation("etag1", "testUser4", null)); + + } } \ No newline at end of file From e6e24463cc3e2205ca1e3135ce1595c1a0b2aab6 Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Mon, 21 Aug 2017 12:33:33 -0700 Subject: [PATCH 12/16] added logging for projectConfig, better testing, no import star, test for version 4 or greater --- build.gradle | 8 -- .../java/com/optimizely/ab/Optimizely.java | 4 +- .../optimizely/ab/config/ProjectConfig.java | 87 +++++++++++++----- .../com/optimizely/ab/OptimizelyTest.java | 92 +++++++++++-------- .../ab/config/ProjectConfigTest.java | 17 +++- .../ab/event/internal/EventBuilderV2Test.java | 6 +- 6 files changed, 138 insertions(+), 76 deletions(-) diff --git a/build.gradle b/build.gradle index 9e700f9c5..8d6721170 100644 --- a/build.gradle +++ b/build.gradle @@ -47,14 +47,6 @@ subprojects { jcenter() } - jacocoTestReport { - reports { - xml.enabled false - csv.enabled false - html.destination file("${buildDir}/jacocoHtml") - } - } - task sourcesJar(type: Jar, dependsOn: classes) { classifier = 'sources' from sourceSets.main.allSource diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 0e1a3870b..a830165ed 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -578,9 +578,7 @@ Variation getVariation(@Nonnull String experimentKey, * Force a user into a variation for a given experiment. * The forced variation value does not persist across application launches. * If the experiment key is not in the project file, this call fails and returns false. - * This method just calls into the {@link com.optimizely.ab.config.ProjectConfig#setForcedVariation(String, String, String)} - * method of the same signature. - * + * If the variationKey is not in the experiment, this call fails. * @param experimentKey The key for the experiment. * @param userId The user ID to be used for bucketing. * @param variationKey The variation key to force the user into. If the variation key is null diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 13b5e755e..aa604fc7a 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -19,6 +19,8 @@ import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.optimizely.ab.config.audience.Audience; import com.optimizely.ab.config.audience.Condition; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -56,6 +58,10 @@ public String toString() { } } + // logger + private static final Logger logger = LoggerFactory.getLogger(ProjectConfig.class); + + // ProjectConfig properties private final String accountId; private final String projectId; private final String revision; @@ -86,12 +92,13 @@ public String toString() { private final Map> variationToLiveVariableUsageInstanceMapping; private final Map variationIdToExperimentMapping; - /* Forced variations supersede any other mappings. They are transient and are not persistent or part of + /** + * Forced variations supersede any other mappings. They are transient and are not persistent or part of * the actual datafile. This contains all the forced variations - * set by the user by calling setForcedVariation (it is not the same as the + * set by the user by calling {@link ProjectConfig#setForcedVariation(String, String, String)} (it is not the same as the * whitelisting forcedVariations data structure in the Experiments class). */ - private transient Map> forcedVariationMapping = new ConcurrentHashMap>(); + private transient ConcurrentHashMap> forcedVariationMapping = new ConcurrentHashMap>(); // v2 constructor public ProjectConfig(String accountId, String projectId, String version, String revision, List groups, @@ -314,7 +321,7 @@ public Map getFeatureKeyMapping() { return featureKeyMapping; } - public Map> getForcedVariationMapping() { return forcedVariationMapping; } + public ConcurrentHashMap> getForcedVariationMapping() { return forcedVariationMapping; } /** * Force a user into a variation for a given experiment. @@ -335,41 +342,62 @@ public boolean setForcedVariation(@Nonnull String experimentKey, // if the experiment is not a valid experiment key, don't set it. Experiment experiment = getExperimentKeyMapping().get(experimentKey); if (experiment == null){ + logger.error("Experiment {} does not exist in ProjectConfig for project {}", experimentKey, projectId); return false; } - Variation variation = (variationKey == null) ? null : experiment.getVariationKeyToVariationMap().get(variationKey); - // if the variation is not part of the experiment, return false. - if (variationKey != null && variation == null) { - return false; + Variation variation = null; + + // keep in mind that you can pass in a variationKey that is null if you want to + // remove the variation. + if (variationKey != null) { + variation = experiment.getVariationKeyToVariationMap().get(variationKey); + // if the variation is not part of the experiment, return false. + if (variation == null) { + logger.error("Variation {} does not exist for experiment {}", variationKey, experimentKey); + return false; + } } // if the user id is invalid, return false. if (userId == null || userId.trim().isEmpty()) { + logger.error("Invalid userId: either empty or null"); return false; } - Map> userForcedVariationMapping = getForcedVariationMapping(); - Map experimentToVariation = userForcedVariationMapping.get(userId); - if (experimentToVariation == null) { - experimentToVariation = new ConcurrentHashMap(); - // sanity check... - if (userForcedVariationMapping instanceof ConcurrentHashMap) { - Map experimentToVariationExists = (Map) ((ConcurrentHashMap)userForcedVariationMapping).putIfAbsent(userId, experimentToVariation); - if (experimentToVariationExists != null) { - experimentToVariation = experimentToVariationExists; + ConcurrentHashMap experimentToVariation; + if (!forcedVariationMapping.containsKey(userId)) { + forcedVariationMapping.putIfAbsent(userId, new ConcurrentHashMap()); + } + experimentToVariation = forcedVariationMapping.get(userId); + + boolean retVal = true; + // if it is null remove the variation if it exists. + if (variationKey == null) { + String removedVariationId = experimentToVariation.remove(experiment.getId()); + if (removedVariationId != null) { + Variation removedVariation = experiment.getVariationIdToVariationMap().get(removedVariationId); + if (removedVariation != null) { + logger.info("Removed forced variation {}", removedVariation.getKey()); + } + else { + logger.info("Removed forced variation that did not exist in experiment"); } } else { - userForcedVariationMapping.put(userId, experimentToVariation); + logger.info("No variation for experiment"); + retVal = false; } } - boolean retVal = true; - if (variationKey == null) { - retVal = (experimentToVariation.remove(experiment.getId()) != null); - } else { - experimentToVariation.put(experiment.getId(), variation.getId()); + String previous = experimentToVariation.put(experiment.getId(), variation.getId()); + if (previous != null) { + Variation previousVariation = experiment.getVariationIdToVariationMap().get(previous); + if (previousVariation != null) { + logger.info("forced variation {} replaced forced variation {}", + variation.getKey(), previousVariation.getKey()); + } + } } return retVal; @@ -388,8 +416,13 @@ public boolean setForcedVariation(@Nonnull String experimentKey, @Nonnull String userId) { // if the user id is invalid, return false. - if (userId == null || userId.trim().isEmpty() || - experimentKey == null || experimentKey.isEmpty()) { + if (userId == null || userId.trim().isEmpty()) { + logger.error("Invalid userId: either null or empty"); + return null; + } + + if (experimentKey == null || experimentKey.isEmpty()) { + logger.error("Invalid experiment key"); return null; } @@ -397,12 +430,16 @@ public boolean setForcedVariation(@Nonnull String experimentKey, if (experimentToVariation != null) { Experiment experiment = getExperimentKeyMapping().get(experimentKey); if (experiment == null) { + logger.info("No forced variation for user {} in experiment {} ", userId, experimentKey, projectId); return null; } String variationId = experimentToVariation.get(experiment.getId()); if (variationId != null) { return experiment.getVariationIdToVariationMap().get(variationId); } + else { + logger.info("No forced variation for user {} in experiment {} ", userId, experimentKey, projectId); + } } return null; } diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index a5603b332..496e14f02 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -53,7 +53,13 @@ import org.mockito.junit.MockitoRule; import java.io.IOException; -import java.util.*; +import java.util.Arrays; +import java.util.Collection; +import java.util.Map; +import java.util.HashMap; +import java.util.List; +import java.util.Collections; +import java.util.ArrayList; import static com.optimizely.ab.config.ProjectConfigTestUtils.noAudienceProjectConfigJsonV2; import static com.optimizely.ab.config.ProjectConfigTestUtils.noAudienceProjectConfigJsonV3; @@ -188,7 +194,7 @@ public OptimizelyTest(int datafileVersion, public void activateEndToEnd() throws Exception { Experiment activatedExperiment; Map testUserAttributes = new HashMap(); - if(datafileVersion == 4) { + if(datafileVersion >= 4) { activatedExperiment = validProjectConfig.getExperimentKeyMapping().get(EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY); testUserAttributes.put(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); } @@ -207,6 +213,7 @@ public void activateEndToEnd() throws Exception { .build(); Map testParams = new HashMap(); + testParams.put("test", "params"); LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); when(mockEventBuilder.createImpressionEvent(validProjectConfig, activatedExperiment, bucketedVariation, "userId", @@ -313,7 +320,7 @@ public void activateWithExperimentKeyForced() throws Exception { optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", forcedVariation.getKey() ); Map testUserAttributes = new HashMap(); - if (datafileVersion == 4) { + if (datafileVersion >= 4) { testUserAttributes.put(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); } else { @@ -366,7 +373,7 @@ public void getVariationWithExperimentKeyForced() throws Exception { optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", forcedVariation.getKey() ); Map testUserAttributes = new HashMap(); - if (datafileVersion == 4) { + if (datafileVersion >= 4) { testUserAttributes.put(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); } else { @@ -393,6 +400,9 @@ public void getVariationWithExperimentKeyForced() throws Exception { assertEquals(optimizely.getForcedVariation(activatedExperiment.getKey(), "userId"), null); + actualVariation = optimizely.getVariation(activatedExperiment.getKey(), "userId", testUserAttributes); + + assertThat(actualVariation, is(bucketedVariation)); } /** @@ -418,7 +428,7 @@ public void isFeatureEnabledWithExperimentKeyForced() throws Exception { optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", forcedVariation.getKey() ); Map testUserAttributes = new HashMap(); - if (datafileVersion != 4) { + if (datafileVersion < 4) { testUserAttributes.put("browser_type", "chrome"); } @@ -439,6 +449,8 @@ public void isFeatureEnabledWithExperimentKeyForced() throws Exception { assertEquals(optimizely.getForcedVariation(activatedExperiment.getKey(), "userId"), null); + assertFalse(optimizely.isFeatureEnabled(FEATURE_FLAG_MULTI_VARIATE_FEATURE.getKey(), "userId")); + } /** @@ -460,7 +472,7 @@ public void activateWithExperimentKey() throws Exception { .build(); Map testUserAttributes = new HashMap(); - if (datafileVersion == 4) { + if (datafileVersion >= 4) { testUserAttributes.put(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); } else { @@ -603,7 +615,7 @@ public void activateWithUnknownAttribute() throws Exception { .build(); Map testUserAttributes = new HashMap(); - if (datafileVersion == 4) { + if (datafileVersion >= 4) { testUserAttributes.put(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); } else { @@ -1067,13 +1079,13 @@ public void trackEventEndToEndForced() throws Exception { EventType eventType; String datafile; ProjectConfig config; - if (datafileVersion == 4) { - config = validProjectConfig; + if (datafileVersion >= 4) { + config = spy(validProjectConfig); eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); datafile = validDatafile; } else { - config = noAudienceProjectConfig; + config = spy(noAudienceProjectConfig); eventType = noAudienceProjectConfig.getEventTypes().get(0); datafile = noAudienceDatafile; } @@ -1082,7 +1094,7 @@ public void trackEventEndToEndForced() throws Exception { EventBuilder eventBuilderV2 = new EventBuilderV2(); DecisionService spyDecisionService = spy(new DecisionService(mockBucketer, mockErrorHandler, - validProjectConfig, + config, null)); Optimizely optimizely = Optimizely.builder(datafile, mockEventHandler) @@ -1092,10 +1104,17 @@ public void trackEventEndToEndForced() throws Exception { .withErrorHandler(mockErrorHandler) .build(); + // Bucket to null for all experiments. However, only a subset of the experiments will actually + // call the bucket function. + for (Experiment experiment : allExperiments) { + when(mockBucketer.bucket(experiment, "userId")) + .thenReturn(null); + } // Force to the first variation for all experiments. However, only a subset of the experiments will actually // call get forced. for (Experiment experiment : allExperiments) { - optimizely.projectConfig.setForcedVariation(experiment.getKey(), "userId", experiment.getVariations().get(0).getKey()); + optimizely.projectConfig.setForcedVariation(experiment.getKey(), + "userId", experiment.getVariations().get(0).getKey()); } // call track @@ -1107,6 +1126,7 @@ public void trackEventEndToEndForced() throws Exception { if (experiment.isRunning() && experimentsForEvent.contains(experiment)) { verify(spyDecisionService).getVariation(experiment, "userId", Collections.emptyMap()); + verify(config).getForcedVariation(experiment.getKey(), "userId"); } else { verify(spyDecisionService, never()).getVariation(experiment, "userId", Collections.emptyMap()); @@ -1114,15 +1134,14 @@ public void trackEventEndToEndForced() throws Exception { } // verify that dispatchEvent was called - if (datafileVersion != 4) { - verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); - } - else { - verify(mockEventHandler).dispatchEvent(any(LogEvent.class)); - } + verify(mockEventHandler).dispatchEvent(any(LogEvent.class)); + for (Experiment experiment : allExperiments) { + assertEquals(optimizely.projectConfig.getForcedVariation(experiment.getKey(), "userId"), experiment.getVariations().get(0)); optimizely.projectConfig.setForcedVariation(experiment.getKey(), "userId", null); + assertNull(optimizely.projectConfig.getForcedVariation(experiment.getKey(), "userId")); } + } /** @@ -1134,13 +1153,13 @@ public void trackEventEndToEnd() throws Exception { EventType eventType; String datafile; ProjectConfig config; - if (datafileVersion == 4) { - config = validProjectConfig; + if (datafileVersion >= 4) { + config = spy(validProjectConfig); eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); datafile = validDatafile; } else { - config = noAudienceProjectConfig; + config = spy(noAudienceProjectConfig); eventType = noAudienceProjectConfig.getEventTypes().get(0); datafile = noAudienceDatafile; } @@ -1149,7 +1168,7 @@ public void trackEventEndToEnd() throws Exception { EventBuilder eventBuilderV2 = new EventBuilderV2(); DecisionService spyDecisionService = spy(new DecisionService(mockBucketer, mockErrorHandler, - validProjectConfig, + config, null)); Optimizely optimizely = Optimizely.builder(datafile, mockEventHandler) @@ -1175,6 +1194,7 @@ public void trackEventEndToEnd() throws Exception { if (experiment.isRunning() && experimentsForEvent.contains(experiment)) { verify(spyDecisionService).getVariation(experiment, "userId", Collections.emptyMap()); + verify(config).getForcedVariation(experiment.getKey(), "userId"); } else { verify(spyDecisionService, never()).getVariation(experiment, "userId", Collections.emptyMap()); @@ -1233,7 +1253,7 @@ public void trackEventWithUnknownEventKeyAndRaiseExceptionErrorHandler() throws public void trackEventWithAttributes() throws Exception { Attribute attribute = validProjectConfig.getAttributes().get(0); EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); } else { @@ -1306,7 +1326,7 @@ public void trackEventWithAttributes() throws Exception { justification="testing nullness contract violation") public void trackEventWithNullAttributes() throws Exception { EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); } else { @@ -1378,7 +1398,7 @@ public void trackEventWithNullAttributes() throws Exception { @Test public void trackEventWithNullAttributeValues() throws Exception { EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); } else { @@ -1450,7 +1470,7 @@ public void trackEventWithNullAttributeValues() throws Exception { @SuppressWarnings("unchecked") public void trackEventWithUnknownAttribute() throws Exception { EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); } else { @@ -1521,7 +1541,7 @@ public void trackEventWithUnknownAttribute() throws Exception { @Test public void trackEventWithEventTags() throws Exception { EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); } else { @@ -1604,7 +1624,7 @@ public void trackEventWithEventTags() throws Exception { justification="testing nullness contract violation") public void trackEventWithNullEventTags() throws Exception { EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); } else { @@ -1668,7 +1688,7 @@ public void trackEventWithNullEventTags() throws Exception { @Test public void trackEventWithNoValidExperiments() throws Exception { EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); } else { @@ -1718,7 +1738,7 @@ public void trackDispatchEventThrowsException() throws Exception { @Test public void trackDoesNotSendEventWhenExperimentsAreLaunchedOnly() throws Exception { EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { eventType = validProjectConfig.getEventNameMapping().get(EVENT_LAUNCHED_EXPERIMENT_ONLY_KEY); } else { @@ -1775,7 +1795,7 @@ public void trackDoesNotSendEventWhenExperimentsAreLaunchedOnly() throws Excepti public void trackDispatchesWhenEventHasLaunchedAndRunningExperiments() throws Exception { EventBuilder mockEventBuilder = mock(EventBuilder.class); EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); } else { @@ -1975,7 +1995,7 @@ public void getVariationWithAudiences() throws Exception { @Test public void getVariationWithAudiencesNoAttributes() throws Exception { Experiment experiment; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { experiment = validProjectConfig.getExperimentKeyMapping().get(EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY); } else { @@ -2065,7 +2085,7 @@ public void getVariationForGroupExperimentWithMatchingAttributes() throws Except Variation variation = experiment.getVariations().get(0); Map attributes = new HashMap(); - if (datafileVersion == 4) { + if (datafileVersion >= 4) { attributes.put(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); } else { @@ -2109,7 +2129,7 @@ public void getVariationForGroupExperimentWithNonMatchingAttributes() throws Exc @Test public void getVariationExperimentStatusPrecedesForcedVariation() throws Exception { Experiment experiment; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { experiment = validProjectConfig.getExperimentKeyMapping().get(EXPERIMENT_PAUSED_EXPERIMENT_KEY); } else { @@ -2136,7 +2156,7 @@ public void getVariationExperimentStatusPrecedesForcedVariation() throws Excepti public void addNotificationListener() throws Exception { Experiment activatedExperiment; EventType eventType; - if (datafileVersion == 4) { + if (datafileVersion >= 4) { activatedExperiment = validProjectConfig.getExperimentKeyMapping().get(EXPERIMENT_BASIC_EXPERIMENT_KEY); eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); } @@ -2286,7 +2306,7 @@ public void removeNotificationListener() throws Exception { public void clearNotificationListeners() throws Exception { Experiment activatedExperiment; Map attributes = new HashMap(); - if (datafileVersion == 4) { + if (datafileVersion >= 4) { activatedExperiment = validProjectConfig.getExperimentKeyMapping().get(EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY); attributes.put(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE); } diff --git a/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java b/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java index 430c8e324..d5682f7f4 100644 --- a/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java +++ b/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java @@ -30,7 +30,12 @@ import static java.util.Arrays.asList; import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.*; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertEquals; + import org.junit.Before; import org.junit.Test; @@ -309,6 +314,11 @@ public void setForcedVariationMultipleVariationsExperiments() { assertTrue(projectConfig.setForcedVariation("etag1", "testUser2", null)); assertTrue(projectConfig.setForcedVariation("etag2", "testUser1", null)); assertTrue(projectConfig.setForcedVariation("etag2", "testUser2", null)); + assertNull(projectConfig.getForcedVariation("etag1", "testUser1")); + assertNull(projectConfig.getForcedVariation("etag1", "testUser2")); + assertNull(projectConfig.getForcedVariation("etag2", "testUser1")); + assertNull(projectConfig.getForcedVariation("etag2", "testUser2")); + } @@ -329,5 +339,10 @@ public void setForcedVariationMultipleUsers() { assertTrue(projectConfig.setForcedVariation("etag1", "testUser3", null)); assertTrue(projectConfig.setForcedVariation("etag1", "testUser4", null)); + assertNull(projectConfig.getForcedVariation("etag1", "testUser1")); + assertNull(projectConfig.getForcedVariation("etag1", "testUser2")); + assertNull(projectConfig.getForcedVariation("etag2", "testUser1")); + assertNull(projectConfig.getForcedVariation("etag2", "testUser2")); + } } \ No newline at end of file diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java index 841315924..fdd210ba9 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java @@ -70,7 +70,7 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotEquals; +//import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -172,8 +172,8 @@ public void createImpressionEventIgnoresUnknownAttributes() throws Exception { // verify that no Feature is created for "unknownAtrribute" -> "blahValue" for (Feature feature : impression.getUserFeatures()) { - assertNotEquals(feature.getName(), "unknownAttribute"); - assertNotEquals(feature.getValue(), "blahValue"); + //assertNotEquals(feature.getName(), "unknownAttribute"); + //assertNotEquals(feature.getValue(), "blahValue"); } } From 884c77e0e4b752839c3009fae3129bdb677aa691 Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Mon, 21 Aug 2017 16:25:41 -0700 Subject: [PATCH 13/16] added missing decisionService test for non-running experiement with forced variations and getVariation called. more cleanup --- .../optimizely/ab/config/ProjectConfig.java | 19 ++++++---- .../com/optimizely/ab/OptimizelyTest.java | 4 +-- .../ab/bucketing/DecisionServiceTest.java | 36 +++++++++++++++++-- .../ab/event/internal/EventBuilderV2Test.java | 5 ++- 4 files changed, 50 insertions(+), 14 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index aa604fc7a..103356fb9 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -378,14 +378,14 @@ public boolean setForcedVariation(@Nonnull String experimentKey, if (removedVariationId != null) { Variation removedVariation = experiment.getVariationIdToVariationMap().get(removedVariationId); if (removedVariation != null) { - logger.info("Removed forced variation {}", removedVariation.getKey()); + logger.debug("Removed forced variation {}", removedVariation.getKey()); } else { - logger.info("Removed forced variation that did not exist in experiment"); + logger.debug("Removed forced variation that did not exist in experiment"); } } else { - logger.info("No variation for experiment"); + logger.debug("No variation for experiment {}", experimentKey); retVal = false; } } @@ -394,7 +394,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, if (previous != null) { Variation previousVariation = experiment.getVariationIdToVariationMap().get(previous); if (previousVariation != null) { - logger.info("forced variation {} replaced forced variation {}", + logger.debug("forced variation {} replaced forced variation {} in forced variation map.", variation.getKey(), previousVariation.getKey()); } } @@ -430,15 +430,20 @@ public boolean setForcedVariation(@Nonnull String experimentKey, if (experimentToVariation != null) { Experiment experiment = getExperimentKeyMapping().get(experimentKey); if (experiment == null) { - logger.info("No forced variation for user {} in experiment {} ", userId, experimentKey, projectId); + logger.debug("No experiment \"%s\" mapped to user \"%s\" in the forced variation map ", experimentKey, userId); return null; } String variationId = experimentToVariation.get(experiment.getId()); if (variationId != null) { - return experiment.getVariationIdToVariationMap().get(variationId); + Variation variation = experiment.getVariationIdToVariationMap().get(variationId); + if (variation != null) { + logger.debug("Variation \"%s\" is mapped to experiment \"%s\" and user \"%s\" in the forced variation map", + variation.getKey(), experimentKey, userId); + return variation; + } } else { - logger.info("No forced variation for user {} in experiment {} ", userId, experimentKey, projectId); + logger.debug("No variation for experiment \"%s\" mapped to user \"%s\" in the forced variation map ", experimentKey, userId); } } return null; diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 496e14f02..49c76ebf7 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -445,9 +445,9 @@ public void isFeatureEnabledWithExperimentKeyForced() throws Exception { verify(mockEventHandler).dispatchEvent(logEventToDispatch); - optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", null ); + assertTrue(optimizely.setForcedVariation(activatedExperiment.getKey(), "userId", null )); - assertEquals(optimizely.getForcedVariation(activatedExperiment.getKey(), "userId"), null); + assertNull(optimizely.getForcedVariation(activatedExperiment.getKey(), "userId")); assertFalse(optimizely.isFeatureEnabled(FEATURE_FLAG_MULTI_VARIATE_FEATURE.getKey(), "userId")); diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java index 0ea651644..4dee30504 100644 --- a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java +++ b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java @@ -39,6 +39,7 @@ import java.util.Map; import static com.optimizely.ab.config.ProjectConfigTestUtils.noAudienceProjectConfigV3; +import static com.optimizely.ab.config.ProjectConfigTestUtils.validConfigJsonV2; import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV3; import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV4; import static org.hamcrest.CoreMatchers.is; @@ -46,6 +47,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertFalse; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyMapOf; import static org.mockito.Matchers.anyString; @@ -58,6 +60,7 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.times; public class DecisionServiceTest { @@ -118,7 +121,7 @@ public void getVariationWhitelistedPrecedesAudienceEval() throws Exception { */ @Test public void getForcedVariationBeforeWhitelisting() throws Exception { - Bucketer bucketer = spy(new Bucketer(validProjectConfig)); + Bucketer bucketer = new Bucketer(validProjectConfig); DecisionService decisionService = spy(new DecisionService(bucketer, mockErrorHandler, validProjectConfig, null)); Experiment experiment = validProjectConfig.getExperiments().get(0); Variation whitelistVariation = experiment.getVariations().get(0); @@ -137,8 +140,9 @@ public void getForcedVariationBeforeWhitelisting() throws Exception { //verify(decisionService).getForcedVariation(experiment.getKey(), whitelistedUserId); verify(decisionService, never()).getStoredVariation(eq(experiment), any(UserProfile.class)); assertEquals(decisionService.getWhitelistedVariation(experiment, whitelistedUserId), whitelistVariation); - assertEquals(validProjectConfig.setForcedVariation(experiment.getKey(), whitelistedUserId,null), true); + assertTrue(validProjectConfig.setForcedVariation(experiment.getKey(), whitelistedUserId,null)); assertNull(validProjectConfig.getForcedVariation(experiment.getKey(), whitelistedUserId)); + assertThat(decisionService.getVariation(experiment, whitelistedUserId, Collections.emptyMap()), is(whitelistVariation)); } /** @@ -237,6 +241,34 @@ public void getVariationEvaluatesUserProfileBeforeAudienceTargeting() throws Exc } + @Test + public void getVariationOnNonRunningExperimentWithForcedVariation() { + Experiment experiment = validProjectConfig.getExperiments().get(1); + assertFalse(experiment.isRunning()); + Variation variation = experiment.getVariations().get(0); + Bucketer bucketer = new Bucketer(validProjectConfig); + + DecisionService decisionService = spy(new DecisionService(bucketer, + mockErrorHandler, validProjectConfig, null)); + + // ensure that normal users still get excluded from the experiment when they fail audience evaluation + assertNull(decisionService.getVariation(experiment, genericUserId, Collections.emptyMap())); + + logbackVerifier.expectMessage(Level.INFO, + "Experiment \"etag2\" is not running.", times(3)); + + + assertTrue(validProjectConfig.setForcedVariation(experiment.getKey(), "userId", variation.getKey())); + + // ensure that a user with a saved user profile, sees the same variation regardless of audience evaluation + assertNull(decisionService.getVariation(experiment, "userId", Collections.emptyMap())); + + assertTrue(validProjectConfig.setForcedVariation(experiment.getKey(), "userId", null)); + assertNull(decisionService.getVariation(experiment, "userId", Collections.emptyMap())); + + + } + //========== get Variation for Feature tests ==========// /** diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java index fdd210ba9..53719f720 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventBuilderV2Test.java @@ -70,7 +70,6 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -//import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -172,8 +171,8 @@ public void createImpressionEventIgnoresUnknownAttributes() throws Exception { // verify that no Feature is created for "unknownAtrribute" -> "blahValue" for (Feature feature : impression.getUserFeatures()) { - //assertNotEquals(feature.getName(), "unknownAttribute"); - //assertNotEquals(feature.getValue(), "blahValue"); + assertFalse(feature.getName() == "unknownAttribute"); + assertFalse(feature.getValue() == "blahValue"); } } From b40368ba76a17484c6d873ab7fd854c351f6f38f Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Mon, 21 Aug 2017 16:32:37 -0700 Subject: [PATCH 14/16] add getForceVariation check --- .../test/java/com/optimizely/ab/config/ProjectConfigTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java b/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java index d5682f7f4..609dfddba 100644 --- a/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java +++ b/core-api/src/test/java/com/optimizely/ab/config/ProjectConfigTest.java @@ -41,7 +41,6 @@ import org.junit.Test; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; -import org.junit.experimental.theories.suppliers.TestedOn; /** * Tests for {@link ProjectConfig}. @@ -284,6 +283,7 @@ public void setForcedVariationWrongVariationKey() { @Test public void setForcedVariationNullVariationKey() { assertFalse(projectConfig.setForcedVariation("etag1", "testUser1", null)); + assertNull(projectConfig.getForcedVariation("etag1", "testUser1")); } @Test From 1ffc359ba4ce145def0836d6349238046accb65e Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Tue, 22 Aug 2017 10:55:26 -0700 Subject: [PATCH 15/16] fix comments for test of non-running experiment --- .../ab/bucketing/DecisionServiceTest.java | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java index 4dee30504..eef46cc5a 100644 --- a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java +++ b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java @@ -241,6 +241,12 @@ public void getVariationEvaluatesUserProfileBeforeAudienceTargeting() throws Exc } + /** + * Verify that {@link DecisionService#getVariation(Experiment, String, Map)} + * gives a null variation on a Experiment that is not running. Set the forced variation. + * And, test to make sure that after setting forced variation, the getVariation still returns + * null. + */ @Test public void getVariationOnNonRunningExperimentWithForcedVariation() { Experiment experiment = validProjectConfig.getExperiments().get(1); @@ -251,19 +257,23 @@ public void getVariationOnNonRunningExperimentWithForcedVariation() { DecisionService decisionService = spy(new DecisionService(bucketer, mockErrorHandler, validProjectConfig, null)); - // ensure that normal users still get excluded from the experiment when they fail audience evaluation - assertNull(decisionService.getVariation(experiment, genericUserId, Collections.emptyMap())); + // ensure that the not running variation returns null with no forced variation set. + assertNull(decisionService.getVariation(experiment, "userId", Collections.emptyMap())); + // we call getVariation 3 times on an experiment that is not running. logbackVerifier.expectMessage(Level.INFO, "Experiment \"etag2\" is not running.", times(3)); - + // set a forced variation on the user that got back null assertTrue(validProjectConfig.setForcedVariation(experiment.getKey(), "userId", variation.getKey())); - // ensure that a user with a saved user profile, sees the same variation regardless of audience evaluation + // ensure that a user with a forced variation set + // still gets back a null variation if the variation is not running. assertNull(decisionService.getVariation(experiment, "userId", Collections.emptyMap())); + // set the forced variation back to null assertTrue(validProjectConfig.setForcedVariation(experiment.getKey(), "userId", null)); + // test one more time that the getVariation returns null for the experiment that is not running. assertNull(decisionService.getVariation(experiment, "userId", Collections.emptyMap())); From 951f161489e4bdd5b227349824cf262857167216 Mon Sep 17 00:00:00 2001 From: Thomas Zurkan Date: Tue, 22 Aug 2017 13:39:36 -0700 Subject: [PATCH 16/16] update comments to be more consistent with php sdk --- .../java/com/optimizely/ab/config/ProjectConfig.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 103356fb9..9fce1f1ba 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -361,7 +361,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, // if the user id is invalid, return false. if (userId == null || userId.trim().isEmpty()) { - logger.error("Invalid userId: either empty or null"); + logger.error("User ID is invalid"); return false; } @@ -378,7 +378,7 @@ public boolean setForcedVariation(@Nonnull String experimentKey, if (removedVariationId != null) { Variation removedVariation = experiment.getVariationIdToVariationMap().get(removedVariationId); if (removedVariation != null) { - logger.debug("Removed forced variation {}", removedVariation.getKey()); + logger.debug("Variation mapped to experiment \"%s\" has been removed for user \"%s\"", experiment.getKey(), userId); } else { logger.debug("Removed forced variation that did not exist in experiment"); @@ -391,6 +391,8 @@ public boolean setForcedVariation(@Nonnull String experimentKey, } else { String previous = experimentToVariation.put(experiment.getId(), variation.getId()); + logger.debug("Set variation \"%s\" for experiment \"%s\" and user \"%s\" in the forced variation map.", + variation.getKey(), experiment.getKey(), userId); if (previous != null) { Variation previousVariation = experiment.getVariationIdToVariationMap().get(previous); if (previousVariation != null) { @@ -417,12 +419,12 @@ public boolean setForcedVariation(@Nonnull String experimentKey, // if the user id is invalid, return false. if (userId == null || userId.trim().isEmpty()) { - logger.error("Invalid userId: either null or empty"); + logger.error("User ID is invalid"); return null; } if (experimentKey == null || experimentKey.isEmpty()) { - logger.error("Invalid experiment key"); + logger.error("experiment key is invalid"); return null; }