feature (multi-rollout): Added support for multiple rollouts. #247
Conversation
pkg/decision/rollout_service.go
Outdated
| rsLogger.Debug(fmt.Sprintf(`User "%s" failed targeting for feature rollout with key "%s".`, userContext.ID, feature.Key)) | ||
| return featureDecision, nil | ||
| index := 0 | ||
| for index < numberOfExperiments { |
There was a problem hiding this comment.
it should be numberOfExperiments-1
pkg/decision/rollout_service.go
Outdated
| if decision.Variation == nil && index < numberOfExperiments-1 { | ||
| // Evaluate fall back rule / last rule now | ||
| index = numberOfExperiments - 1 | ||
| continue |
There was a problem hiding this comment.
Please add everyoneElse rule.
pkg/decision/rollout_service.go
Outdated
| "github.com/optimizely/go-sdk/pkg/logging" | ||
|
|
||
| "github.com/optimizely/go-sdk/pkg/entities" |
pkg/decision/rollout_service.go
Outdated
| evalResult, _ := r.audienceTreeEvaluator.Evaluate(experiment.AudienceConditionTree, condTreeParams) | ||
| if !evalResult { // Evaluate this user for the next rule | ||
| featureDecision.Reason = reasons.FailedRolloutTargeting | ||
| r.logger.Debug(fmt.Sprintf(`User "%s" failed targeting for feature rollout with key "%s".`, userContext.ID, feature.Key)) |
There was a problem hiding this comment.
are we loggin in other sdks?
| mockConfig *mockProjectConfig | ||
| mockAudienceTreeEvaluator *MockAudienceTreeEvaluator | ||
| mockExperimentService *MockExperimentDecisionService | ||
| testExperiment1112DecisionContext ExperimentDecisionContext |
There was a problem hiding this comment.
it represents the experimentID.
| } | ||
| decision, _ := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext) | ||
| s.Equal(expectedFeatureDecision, decision) | ||
| s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) |
There was a problem hiding this comment.
Can we also assert, number of rolloutrules experiments were in it? and the returned mocked roleout rule was the last one. Also throw exception for all other roleout rules so I can be sure, it didn't iterate once fallen into audience evaluation but didn't bucket.
There was a problem hiding this comment.
Since there are 3 rollout rules experiments in this test, we have only mocked response for Evaluate and GetDecision methods of 1st and last rule, This makes sure that if these methods were called for any other experiments, the test will fail. We assert that the returned rollout rule was the last one already by providing testExp1118 in expectedFeatureDecision.
optimizely-codereview-bot
requested review from
msohailhussain and
pawels-optimizely
pkg/decision/rollout_service.go
Outdated
| featureDecision := FeatureDecision{ | ||
| Source: Rollout, | ||
| } | ||
| feature := decisionContext.Feature | ||
| rollout := feature.Rollout | ||
|
|
||
| evaluateConditionTree := func(experiment entities.Experiment) bool { |
There was a problem hiding this comment.
since this is a help function, can you try passing by reference instead of value?
feature (multi-rollout): Added support for multiple rollouts
Summary
Added support for multiple rollouts in
rollout_service, this includes: