Skip to content

Commit ca1e7c8

Browse files
mfahadahmedmikeproeng37
mfahadahmed
authored and
mikeproeng37
committed
Fixed issues in IsFeatueEnabled method (#45)
1 parent 77c9d27 commit ca1e7c8

File tree

7 files changed

+161
-60
lines changed

7 files changed

+161
-60
lines changed

‎OptimizelySDK.Tests/BucketerTest.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,5 +225,18 @@ public void TestBucketVariationGroupedExperimentsWithBucketingId()
225225
bucketer.Bucket(Config, Config.GetExperimentFromKey("group_experiment_2"),
226226
TestBucketingIdGroupExp2Var2, TestUserIdBucketsToNoGroup));
227227
}
228+
229+
// Make sure that user gets bucketed into the rollout rule.
230+
[Test]
231+
public void TestBucketRolloutRule()
232+
{
233+
var bucketer = new Bucketer(LoggerMock.Object);
234+
var rollout = Config.GetRolloutFromId("166660");
235+
var rolloutRule = rollout.Experiments[1];
236+
var expectedVariation = Config.GetVariationFromId(rolloutRule.Key, "177773");
237+
238+
Assert.True(TestData.CompareObjects(expectedVariation,
239+
bucketer.Bucket(Config, rolloutRule, "testBucketingId", TestUserId)));
240+
}
228241
}
229242
}

‎OptimizelySDK.Tests/DecisionServiceTest.cs‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -682,6 +682,63 @@ public void TestGetVariationForFeatureRolloutWhenUserDoesNotQualifyForAnyTargeti
682682
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, $"User \"user_1\" does not meet the conditions to be in rollout rule for audience \"{ProjectConfig.AudienceIdMap[experiment0.AudienceIds[0]].Name}\"."));
683683
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, $"User \"user_1\" does not meet the conditions to be in rollout rule for audience \"{ProjectConfig.AudienceIdMap[experiment1.AudienceIds[0]].Name}\"."));
684684
}
685+
686+
[Test]
687+
public void TestGetVariationForFeatureRolloutAudienceAndTrafficeAllocationCheck()
688+
{
689+
var featureFlag = ProjectConfig.GetFeatureFlagFromKey("boolean_single_variable_feature");
690+
var rolloutId = featureFlag.RolloutId;
691+
var rollout = ProjectConfig.GetRolloutFromId(rolloutId);
692+
var expWithAudienceiPhoneUsers = rollout.Experiments[1];
693+
var expWithAudienceChromeUsers = rollout.Experiments[0];
694+
var expWithNoAudience = rollout.Experiments[2];
695+
var varWithAudienceiPhoneUsers = expWithAudienceiPhoneUsers.Variations[0];
696+
var varWithAudienceChromeUsers = expWithAudienceChromeUsers.Variations[0];
697+
var varWithNoAudience = expWithNoAudience.Variations[0];
698+
699+
var mockBucketer = new Mock<Bucketer>(LoggerMock.Object) { CallBase = true };
700+
mockBucketer.Setup(bm => bm.GenerateBucketValue(It.IsAny<string>())).Returns(980);
701+
var decisionService = new DecisionService(mockBucketer.Object, ErrorHandlerMock.Object, ProjectConfig, null, LoggerMock.Object);
702+
703+
// Calling with audience iPhone users in San Francisco.
704+
var actualDecision = decisionService.GetVariationForFeatureRollout(featureFlag, GenericUserId, new UserAttributes
705+
{
706+
{ "device_type", "iPhone" },
707+
{ "location", "San Francisco" }
708+
});
709+
710+
// Returned variation id should be '177773' because of audience 'iPhone users in San Francisco'.
711+
var expectedDecision = new FeatureDecision(expWithAudienceiPhoneUsers.Id, varWithAudienceiPhoneUsers.Id, FeatureDecision.DECISION_SOURCE_ROLLOUT);
712+
Assert.IsTrue(TestData.CompareObjects(expectedDecision, actualDecision));
713+
714+
// Calling with audience Chrome users.
715+
actualDecision = decisionService.GetVariationForFeatureRollout(featureFlag, GenericUserId, new UserAttributes
716+
{
717+
{ "browser_type", "chrome" }
718+
});
719+
720+
// Returned variation id should be '177771' because of audience 'Chrome users'.
721+
expectedDecision = new FeatureDecision(expWithAudienceChromeUsers.Id, varWithAudienceChromeUsers.Id, FeatureDecision.DECISION_SOURCE_ROLLOUT);
722+
Assert.IsTrue(TestData.CompareObjects(expectedDecision, actualDecision));
723+
724+
// Calling with no audience.
725+
mockBucketer.Setup(bm => bm.GenerateBucketValue(It.IsAny<string>())).Returns(8000);
726+
actualDecision = decisionService.GetVariationForFeatureRollout(featureFlag, GenericUserId, new UserAttributes());
727+
728+
// Returned variation id should be of everyone else rule because of no audience.
729+
expectedDecision = new FeatureDecision(expWithNoAudience.Id, varWithNoAudience.Id, FeatureDecision.DECISION_SOURCE_ROLLOUT);
730+
Assert.IsTrue(TestData.CompareObjects(expectedDecision, actualDecision));
731+
732+
// Calling with audience 'Chrome users' and traffice allocation '9500'.
733+
mockBucketer.Setup(bm => bm.GenerateBucketValue(It.IsAny<string>())).Returns(9500);
734+
actualDecision = decisionService.GetVariationForFeatureRollout(featureFlag, GenericUserId, new UserAttributes
735+
{
736+
{ "browser_type", "chrome" }
737+
});
738+
739+
// Returned decision entity should be null because bucket value exceeds traffice allocation of everyone else rule.
740+
Assert.Null(actualDecision);
741+
}
685742

686743
#endregion // GetVariationForFeatureRollout Tests
687744

‎OptimizelySDK.Tests/OptimizelyTest.cs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1513,6 +1513,29 @@ public void TestIsFeatureEnabledGivenFeatureFlagIsEnabledAndUserIsBeingExperimen
15131513
$@"Feature flag ""{featureKey}"" is enabled for user ""{TestUserId}""."));
15141514
}
15151515

1516+
// Verify that IsFeatureEnabled returns true if a variation does not get found in the feature
1517+
// flag experiment but found in the rollout rule.
1518+
[Test]
1519+
public void TestIsFeatureEnabledGivenVariationNotFoundInFeatureExperimentButInRolloutRule()
1520+
{
1521+
var featureKey = "boolean_single_variable_feature";
1522+
var userAttributes = new UserAttributes
1523+
{
1524+
{ "device_type", "iPhone" },
1525+
{ "company", "Optimizely" }
1526+
};
1527+
1528+
Assert.True(Optimizely.IsFeatureEnabled(featureKey, TestUserId, userAttributes));
1529+
1530+
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "The feature flag \"boolean_single_variable_feature\" is not used in any experiments."), Times.Once);
1531+
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "User \"testUserId\" does not meet the conditions to be in rollout rule for audience \"Chrome users\"."), Times.Once);
1532+
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "User \"testUserId\" does not meet the conditions to be in rollout rule for audience \"iPhone users in San Francisco\"."), Times.Once);
1533+
LoggerMock.Verify(l => l.Log(LogLevel.DEBUG, "Assigned bucket [8408] to user [testUserId] with bucketing ID [testUserId]."), Times.Once);
1534+
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "The user \"testUserId\" is bucketed into a rollout for feature flag \"boolean_single_variable_feature\"."), Times.Once);
1535+
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "The user \"testUserId\" is not being experimented on feature \"boolean_single_variable_feature\"."), Times.Once);
1536+
LoggerMock.Verify(l => l.Log(LogLevel.INFO, "Feature flag \"boolean_single_variable_feature\" is enabled for user \"testUserId\"."), Times.Once);
1537+
}
1538+
15161539
#endregion // Test IsFeatureEnabled method
15171540

15181541
#region Test NotificationCenter

‎OptimizelySDK.Tests/ProjectConfigTest.cs‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,31 @@ public void TestInit()
169169
{"control", Config.GetVariationFromKey("test_experiment_integer_feature", "control") },
170170
{ "variation", Config.GetVariationFromKey("test_experiment_integer_feature", "variation") }
171171
}
172+
},
173+
{ "177770", new Dictionary<string, object>
174+
{
175+
{"177771", Config.GetVariationFromKey("177770", "177771") }
176+
}
177+
},
178+
{ "177772", new Dictionary<string, object>
179+
{
180+
{"177773", Config.GetVariationFromKey("177772", "177773") }
181+
}
182+
},
183+
{ "177776", new Dictionary<string, object>
184+
{
185+
{"177778", Config.GetVariationFromKey("177776", "177778") }
186+
}
187+
},
188+
{ "177774", new Dictionary<string, object>
189+
{
190+
{"177775", Config.GetVariationFromKey("177774", "177775") }
191+
}
192+
},
193+
{ "177779", new Dictionary<string, object>
194+
{
195+
{"177780", Config.GetVariationFromKey("177779", "177780") }
196+
}
172197
}
173198
};
174199

@@ -221,10 +246,36 @@ public void TestInit()
221246
{ "7722360022", Config.GetVariationFromId("group_experiment_1", "7722360022")}
222247
}
223248
},
224-
{ "group_experiment_2", new Dictionary<string, object> {
249+
{ "group_experiment_2", new Dictionary<string, object>
250+
{
225251
{"7713030086", Config.GetVariationFromId("group_experiment_2", "7713030086") },
226252
{ "7725250007", Config.GetVariationFromId("group_experiment_2", "7725250007")}
227253
}
254+
},
255+
{ "177770", new Dictionary<string, object>
256+
{
257+
{"177771", Config.GetVariationFromId("177770", "177771") }
258+
}
259+
},
260+
{ "177772", new Dictionary<string, object>
261+
{
262+
{"177773", Config.GetVariationFromId("177772", "177773") }
263+
}
264+
},
265+
{ "177776", new Dictionary<string, object>
266+
{
267+
{"177778", Config.GetVariationFromId("177776", "177778") }
268+
}
269+
},
270+
{ "177774", new Dictionary<string, object>
271+
{
272+
{"177775", Config.GetVariationFromId("177774", "177775") }
273+
}
274+
},
275+
{ "177779", new Dictionary<string, object>
276+
{
277+
{"177780", Config.GetVariationFromId("177779", "177780") }
278+
}
228279
}
229280
};
230281

‎OptimizelySDK.Tests/TestData.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,7 @@
423423
"key": "177772",
424424
"status": "Running",
425425
"layerId": "166660",
426-
"audienceIds": ["11154"],
426+
"audienceIds": ["7718080042"],
427427
"variations": [{
428428
"id": "177773",
429429
"key": "177773",
@@ -452,7 +452,7 @@
452452
}],
453453
"trafficAllocation": [{
454454
"entityId": "177778",
455-
"endOfRange": 10000
455+
"endOfRange": 9000
456456
}]
457457
}]
458458
}, {

‎OptimizelySDK/Bucketing/DecisionService.cs‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ public virtual FeatureDecision GetVariationForFeatureRollout(FeatureFlag feature
296296
Logger.Log(LogLevel.DEBUG, $"Attempting to bucket user \"{userId}\" into rollout rule \"{rolloutRule.Key}\".");
297297
variation = Bucketer.Bucket(ProjectConfig, rolloutRule, bucketingId, userId);
298298

299-
if (variation == null)
299+
if (variation == null || string.IsNullOrEmpty(variation.Id))
300300
{
301301
Logger.Log(LogLevel.DEBUG, $"User \"{userId}\" is excluded due to traffic allocation. Checking \"Eveyrone Else\" rule now.");
302302
break;
@@ -314,13 +314,11 @@ public virtual FeatureDecision GetVariationForFeatureRollout(FeatureFlag feature
314314
var everyoneElseRolloutRule = rollout.Experiments[rolloutRulesLength - 1];
315315
variation = Bucketer.Bucket(ProjectConfig, everyoneElseRolloutRule, bucketingId, userId);
316316

317-
if (variation == null)
318-
{
319-
Logger.Log(LogLevel.DEBUG, $"User \"{userId}\" is excluded from \"Everyone Else\" rule for feature flag \"{featureFlag.Key}\".");
320-
return null;
321-
}
317+
if (variation != null && !string.IsNullOrEmpty(variation.Id))
318+
return new FeatureDecision(everyoneElseRolloutRule.Id, variation.Id, FeatureDecision.DECISION_SOURCE_ROLLOUT);
322319

323-
return new FeatureDecision(everyoneElseRolloutRule.Id, variation.Id, FeatureDecision.DECISION_SOURCE_ROLLOUT);
320+
Logger.Log(LogLevel.DEBUG, $"User \"{userId}\" is excluded from \"Everyone Else\" rule for feature flag \"{featureFlag.Key}\".");
321+
return null;
324322
}
325323

326324
/// <summary>
@@ -354,7 +352,7 @@ public virtual FeatureDecision GetVariationForFeatureExperiment(FeatureFlag feat
354352

355353
var variation = GetVariation(experiment, userId, filteredAttributes);
356354

357-
if (variation != null)
355+
if (variation != null && !string.IsNullOrEmpty(variation.Id))
358356
{
359357
Logger.Log(LogLevel.INFO, $"The user \"{userId}\" is bucketed into experiment \"{experiment.Key}\" of feature \"{featureFlag.Key}\".");
360358
return new FeatureDecision(experimentId, variation.Id, FeatureDecision.DECISION_SOURCE_EXPERIMENT);

‎OptimizelySDK/ProjectConfig.cs‎

Lines changed: 8 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -126,18 +126,6 @@ private Dictionary<string, Dictionary<string, Variation>> _VariationIdMap
126126
private Dictionary<string, Rollout> _RolloutIdMap;
127127
public Dictionary<string, Rollout> RolloutIdMap { get { return _RolloutIdMap; } }
128128

129-
/// <summary>
130-
/// Associative array of Variation ID to Experiments(s) in the datafile
131-
/// </summary>
132-
private Dictionary<string, Experiment> _VariationIdToExperimentMap = new Dictionary<string, Experiment>();
133-
public Dictionary<string, Experiment> VariationIdToExperimentMap{ get { return _VariationIdToExperimentMap; } }
134-
135-
/// <summary>
136-
/// Associative array of Variation ID to Rollout Rules(s) in the datafile
137-
/// </summary>
138-
private Dictionary<string, Experiment> _VariationIdToRolloutRuleMap = new Dictionary<string, Experiment>();
139-
public Dictionary<string, Experiment> VariationIdToRolloutRuleMap { get { return _VariationIdToRolloutRuleMap; } }
140-
141129
//========================= Callbacks ===========================
142130

143131
/// <summary>
@@ -248,22 +236,25 @@ private void Initialize()
248236
{
249237
_VariationKeyMap[experiment.Key][variation.Key] = variation;
250238
_VariationIdMap[experiment.Key][variation.Id] = variation;
251-
252-
// Generate Variation ID to Experiment map.
253-
_VariationIdToExperimentMap[variation.Id] = experiment;
254239
}
255240
}
256241
}
257242

258-
// Generate Variation ID to Rollout Rule map.
243+
// Adding Rollout variations in variation id and key maps.
259244
foreach (var rollout in Rollouts)
260245
{
261246
foreach (var rolloutRule in rollout.Experiments)
262247
{
248+
_VariationKeyMap[rolloutRule.Key] = new Dictionary<string, Variation>();
249+
_VariationIdMap[rolloutRule.Key] = new Dictionary<string, Variation>();
250+
263251
if (rolloutRule.Variations != null)
264252
{
265253
foreach (var variation in rolloutRule.Variations)
266-
_VariationIdToRolloutRuleMap[variation.Id] = rolloutRule;
254+
{
255+
_VariationKeyMap[rolloutRule.Key][variation.Key] = variation;
256+
_VariationIdMap[rolloutRule.Key][variation.Id] = variation;
257+
}
267258
}
268259
}
269260
}
@@ -560,37 +551,5 @@ public Rollout GetRolloutFromId(string rolloutId)
560551
ErrorHandler.HandleError(new Exceptions.InvalidRolloutException("Provided rollout is not in datafile."));
561552
return new Rollout();
562553
}
563-
564-
/// <summary>
565-
/// Get experiment from variation ID.
566-
/// </summary>
567-
/// <param name="variationId">Variation ID</param>
568-
/// <returns>Experiment Entity corresponding to the variation ID or a dummy entity if ID is invalid</returns>
569-
public Experiment GetExperimentForVariationId(string variationId)
570-
{
571-
if (_VariationIdToExperimentMap.ContainsKey(variationId))
572-
return _VariationIdToExperimentMap[variationId];
573-
574-
Logger.Log(LogLevel.ERROR, $@"No experiment has been defined in datafile for variation ""{variationId}"".");
575-
ErrorHandler.HandleError(new Exceptions.InvalidVariationException("No experiment has been found for provided variation Id in the datafile."));
576-
577-
return new Experiment();
578-
}
579-
580-
/// <summary>
581-
/// Get rollout rule for variation.
582-
/// </summary>
583-
/// <param name="variationId">Variation ID</param>
584-
/// <returns>Rollout Rule corresponding to the variation ID or a dummy entity if ID is invalid</returns>
585-
public Experiment GetRolloutRuleForVariationId(string variationId)
586-
{
587-
if (_VariationIdToRolloutRuleMap.ContainsKey(variationId))
588-
return _VariationIdToRolloutRuleMap[variationId];
589-
590-
Logger.Log(LogLevel.ERROR, $@"No rollout rule has been defined in datafile for variation ""{variationId}"".");
591-
ErrorHandler.HandleError(new Exceptions.InvalidVariationException("No rollout rule has been found for provided variation Id in the datafile."));
592-
593-
return new Experiment();
594-
}
595554
}
596555
}

0 commit comments

Comments
 (0)