From c182a8641d8263196ef9492225d0497d4c833ccd Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Wed, 25 Mar 2020 16:35:21 +0500 Subject: [PATCH 1/6] Initial commit. --- pkg/decision/rollout_service.go | 65 ++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/pkg/decision/rollout_service.go b/pkg/decision/rollout_service.go index 596a5172a..e9b607f62 100644 --- a/pkg/decision/rollout_service.go +++ b/pkg/decision/rollout_service.go @@ -61,38 +61,43 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return featureDecision, nil } - // For now, Rollouts is just a single experiment layer - experiment := rollout.Experiments[0] - experimentDecisionContext := ExperimentDecisionContext{ - Experiment: &experiment, - ProjectConfig: decisionContext.ProjectConfig, - } - - // if user fails rollout targeting rule we return out of it - if experiment.AudienceConditionTree != nil { - condTreeParams := entities.NewTreeParameters(&userContext, decisionContext.ProjectConfig.GetAudienceMap()) - evalResult, _ := r.audienceTreeEvaluator.Evaluate(experiment.AudienceConditionTree, condTreeParams) - if !evalResult { - featureDecision.Reason = reasons.FailedRolloutTargeting - 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 { + experiment := rollout.Experiments[index] + experimentDecisionContext := ExperimentDecisionContext{ + Experiment: &experiment, + ProjectConfig: decisionContext.ProjectConfig, + } + if experiment.AudienceConditionTree != nil { + condTreeParams := entities.NewTreeParameters(&userContext, decisionContext.ProjectConfig.GetAudienceMap()) + evalResult, _ := r.audienceTreeEvaluator.Evaluate(experiment.AudienceConditionTree, condTreeParams) + if !evalResult { // Evaluate this user for the next rule + featureDecision.Reason = reasons.FailedRolloutTargeting + rsLogger.Debug(fmt.Sprintf(`User "%s" failed targeting for feature rollout with key "%s".`, userContext.ID, feature.Key)) + index++ + continue + } + } + decision, _ := r.experimentBucketerService.GetDecision(experimentDecisionContext, userContext) + if decision.Variation == nil && index < numberOfExperiments-1 { + // Evaluate fall back rule / last rule now + index = numberOfExperiments - 1 + continue + } + // translate the experiment reason into a more rollouts-appropriate reason + switch decision.Reason { + case reasons.NotBucketedIntoVariation: + featureDecision.Decision = Decision{Reason: reasons.FailedRolloutBucketing} + case reasons.BucketedIntoVariation: + featureDecision.Decision = Decision{Reason: reasons.BucketedIntoRollout} + default: + featureDecision.Decision = decision.Decision } - } - decision, _ := r.experimentBucketerService.GetDecision(experimentDecisionContext, userContext) - // translate the experiment reason into a more rollouts-appropriate reason - switch decision.Reason { - case reasons.NotBucketedIntoVariation: - featureDecision.Decision = Decision{Reason: reasons.FailedRolloutBucketing} - case reasons.BucketedIntoVariation: - featureDecision.Decision = Decision{Reason: reasons.BucketedIntoRollout} - default: - featureDecision.Decision = decision.Decision + featureDecision.Experiment = experiment + featureDecision.Variation = decision.Variation + rsLogger.Debug(fmt.Sprintf(`Decision made for user "%s" for feature rollout with key "%s": %s.`, userContext.ID, feature.Key, featureDecision.Reason)) + break } - - featureDecision.Experiment = experiment - featureDecision.Variation = decision.Variation - rsLogger.Debug(fmt.Sprintf(`Decision made for user "%s" for feature rollout with key "%s": %s.`, userContext.ID, feature.Key, featureDecision.Reason)) - return featureDecision, nil } From b047d8558f0b44a90b0c78c51f53279fb2f3f404 Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Thu, 26 Mar 2020 16:58:27 +0500 Subject: [PATCH 2/6] tests added. --- pkg/decision/helpers_test.go | 46 +++++++++++- pkg/decision/rollout_service_test.go | 105 +++++++++++++++++++++++---- 2 files changed, 133 insertions(+), 18 deletions(-) diff --git a/pkg/decision/helpers_test.go b/pkg/decision/helpers_test.go index f4b352832..7a1499050 100644 --- a/pkg/decision/helpers_test.go +++ b/pkg/decision/helpers_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019, Optimizely, Inc. and contributors * + * Copyright 2019-2020, Optimizely, Inc. and contributors * * * * Licensed under the Apache License, Version 2.0 (the "License"); * * you may not use this file except in compliance with the License. * @@ -150,6 +150,48 @@ var testExp1112 = entities.Experiment{ entities.Range{EntityID: "2222", EndOfRange: 10000}, }, } +var testExp1117Var2223 = entities.Variation{ID: "2223", Key: "2223"} +var testAudience5556 = entities.Audience{ID: "5556"} +var testExp1117 = entities.Experiment{ + AudienceConditionTree: &entities.TreeNode{ + Operator: "and", + Nodes: []*entities.TreeNode{ + &entities.TreeNode{Item: "test_audience_5556"}, + }, + }, + ID: "1117", + Key: testExp1111Key, + Variations: map[string]entities.Variation{ + "2223": testExp1117Var2223, + }, + VariationKeyToIDMap: map[string]string{ + "2223": "2223", + }, + TrafficAllocation: []entities.Range{ + entities.Range{EntityID: "2223", EndOfRange: 10000}, + }, +} +var testExp1118Var2224 = entities.Variation{ID: "2224", Key: "2224"} +var testAudience5557 = entities.Audience{ID: "5557"} +var testExp1118 = entities.Experiment{ + AudienceConditionTree: &entities.TreeNode{ + Operator: "and", + Nodes: []*entities.TreeNode{ + &entities.TreeNode{Item: "test_audience_5557"}, + }, + }, + ID: "1118", + Key: testExp1111Key, + Variations: map[string]entities.Variation{ + "2224": testExp1118Var2224, + }, + VariationKeyToIDMap: map[string]string{ + "2224": "2224", + }, + TrafficAllocation: []entities.Range{ + entities.Range{EntityID: "2224", EndOfRange: 10000}, + }, +} const testFeatRollout3334Key = "test_feature_rollout_3334_key" @@ -158,7 +200,7 @@ var testFeatRollout3334 = entities.Feature{ Key: testFeatRollout3334Key, Rollout: entities.Rollout{ ID: "4444", - Experiments: []entities.Experiment{testExp1112}, + Experiments: []entities.Experiment{testExp1112, testExp1117, testExp1118}, }, } diff --git a/pkg/decision/rollout_service_test.go b/pkg/decision/rollout_service_test.go index 241db7a28..729d5ee5b 100644 --- a/pkg/decision/rollout_service_test.go +++ b/pkg/decision/rollout_service_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019, Optimizely, Inc. and contributors * + * Copyright 2019-2020, Optimizely, Inc. and contributors * * * * Licensed under the Apache License, Version 2.0 (the "License"); * * you may not use this file except in compliance with the License. * @@ -31,20 +31,20 @@ import ( type RolloutServiceTestSuite struct { suite.Suite - mockConfig *mockProjectConfig - mockAudienceTreeEvaluator *MockAudienceTreeEvaluator - mockExperimentService *MockExperimentDecisionService - testExperimentDecisionContext ExperimentDecisionContext - testFeatureDecisionContext FeatureDecisionContext - testConditionTreeParams *entities.TreeParameters - testUserContext entities.UserContext + mockConfig *mockProjectConfig + mockAudienceTreeEvaluator *MockAudienceTreeEvaluator + mockExperimentService *MockExperimentDecisionService + testExperiment1112DecisionContext ExperimentDecisionContext + testFeatureDecisionContext FeatureDecisionContext + testConditionTreeParams *entities.TreeParameters + testUserContext entities.UserContext } func (s *RolloutServiceTestSuite) SetupTest() { s.mockConfig = new(mockProjectConfig) s.mockAudienceTreeEvaluator = new(MockAudienceTreeEvaluator) s.mockExperimentService = new(MockExperimentDecisionService) - s.testExperimentDecisionContext = ExperimentDecisionContext{ + s.testExperiment1112DecisionContext = ExperimentDecisionContext{ Experiment: &testExp1112, ProjectConfig: s.mockConfig, } @@ -55,6 +55,8 @@ func (s *RolloutServiceTestSuite) SetupTest() { testAudienceMap := map[string]entities.Audience{ "5555": testAudience5555, + "5556": testAudience5556, + "5557": testAudience5557, } s.testUserContext = entities.UserContext{ ID: "test_user", @@ -70,7 +72,7 @@ func (s *RolloutServiceTestSuite) TestGetDecisionHappyPath() { Decision: Decision{Reason: reasons.BucketedIntoVariation}, } s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(true, true) - s.mockExperimentService.On("GetDecision", s.testExperimentDecisionContext, s.testUserContext).Return(testExperimentBucketerDecision, nil) + s.mockExperimentService.On("GetDecision", s.testExperiment1112DecisionContext, s.testUserContext).Return(testExperimentBucketerDecision, nil) testRolloutService := RolloutService{ audienceTreeEvaluator: s.mockAudienceTreeEvaluator, @@ -88,26 +90,95 @@ func (s *RolloutServiceTestSuite) TestGetDecisionHappyPath() { s.mockExperimentService.AssertExpectations(s.T()) } -func (s *RolloutServiceTestSuite) TestGetDecisionFailsBucketing() { +func (s *RolloutServiceTestSuite) TestGetDecisionFallbacksToLastWhenFailsBucketing() { // Test experiment passes targeting but not bucketing - testExperimentBucketerDecision := ExperimentDecision{ + testExperiment1112BucketerDecision := ExperimentDecision{ Decision: Decision{ Reason: reasons.NotBucketedIntoVariation, }, } - + testExperiment1118BucketerDecision := ExperimentDecision{ + Variation: &testExp1118Var2224, + Decision: Decision{Reason: reasons.BucketedIntoVariation}, + } + experiment1118DecisionContext := ExperimentDecisionContext{ + Experiment: &testExp1118, + ProjectConfig: s.mockConfig, + } s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(true, true) - s.mockExperimentService.On("GetDecision", s.testExperimentDecisionContext, s.testUserContext).Return(testExperimentBucketerDecision, nil) + s.mockAudienceTreeEvaluator.On("Evaluate", testExp1118.AudienceConditionTree, s.testConditionTreeParams).Return(true, true) + s.mockExperimentService.On("GetDecision", s.testExperiment1112DecisionContext, s.testUserContext).Return(testExperiment1112BucketerDecision, nil) + s.mockExperimentService.On("GetDecision", experiment1118DecisionContext, s.testUserContext).Return(testExperiment1118BucketerDecision, nil) + testRolloutService := RolloutService{ audienceTreeEvaluator: s.mockAudienceTreeEvaluator, experimentBucketerService: s.mockExperimentService, } expectedFeatureDecision := FeatureDecision{ + Experiment: testExp1118, + Variation: &testExp1118Var2224, + Source: Rollout, + Decision: Decision{Reason: reasons.BucketedIntoRollout}, + } + decision, _ := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext) + s.Equal(expectedFeatureDecision, decision) + s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) + s.mockExperimentService.AssertExpectations(s.T()) +} + +func (s *RolloutServiceTestSuite) TestGetDecisionWhenFallbackBucketingFails() { + // Test experiment passes targeting but not bucketing + testExperiment1112BucketerDecision := ExperimentDecision{ Decision: Decision{ - Reason: reasons.FailedRolloutBucketing, + Reason: reasons.NotBucketedIntoVariation, }, - Experiment: testExp1112, + } + testExperiment1118DecisionContext := ExperimentDecisionContext{ + Experiment: &testExp1118, + ProjectConfig: s.mockConfig, + } + s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(true, true) + s.mockAudienceTreeEvaluator.On("Evaluate", testExp1118.AudienceConditionTree, s.testConditionTreeParams).Return(true, true) + s.mockExperimentService.On("GetDecision", s.testExperiment1112DecisionContext, s.testUserContext).Return(testExperiment1112BucketerDecision, nil) + s.mockExperimentService.On("GetDecision", testExperiment1118DecisionContext, s.testUserContext).Return(testExperiment1112BucketerDecision, nil) + + testRolloutService := RolloutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + experimentBucketerService: s.mockExperimentService, + } + expectedFeatureDecision := FeatureDecision{ + Experiment: testExp1118, Source: Rollout, + Decision: Decision{Reason: reasons.FailedRolloutBucketing}, + } + decision, _ := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext) + s.Equal(expectedFeatureDecision, decision) + s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) + s.mockExperimentService.AssertExpectations(s.T()) +} + +func (s *RolloutServiceTestSuite) TestEvaluatesNextIfPreviousTargetingFails() { + s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(false, true) + s.mockAudienceTreeEvaluator.On("Evaluate", testExp1117.AudienceConditionTree, s.testConditionTreeParams).Return(true, true) + experiment1117DecisionContext := ExperimentDecisionContext{ + Experiment: &testExp1117, + ProjectConfig: s.mockConfig, + } + testExperimentBucketerDecision := ExperimentDecision{ + Variation: &testExp1117Var2223, + Decision: Decision{Reason: reasons.BucketedIntoVariation}, + } + s.mockExperimentService.On("GetDecision", experiment1117DecisionContext, s.testUserContext).Return(testExperimentBucketerDecision, nil) + + testRolloutService := RolloutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + experimentBucketerService: s.mockExperimentService, + } + expectedFeatureDecision := FeatureDecision{ + Experiment: testExp1117, + Variation: &testExp1117Var2223, + Source: Rollout, + Decision: Decision{Reason: reasons.BucketedIntoRollout}, } decision, _ := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext) s.Equal(expectedFeatureDecision, decision) @@ -117,6 +188,8 @@ func (s *RolloutServiceTestSuite) TestGetDecisionFailsBucketing() { func (s *RolloutServiceTestSuite) TestGetDecisionFailsTargeting() { s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(false, true) + s.mockAudienceTreeEvaluator.On("Evaluate", testExp1117.AudienceConditionTree, s.testConditionTreeParams).Return(false, true) + s.mockAudienceTreeEvaluator.On("Evaluate", testExp1118.AudienceConditionTree, s.testConditionTreeParams).Return(false, true) testRolloutService := RolloutService{ audienceTreeEvaluator: s.mockAudienceTreeEvaluator, experimentBucketerService: s.mockExperimentService, From 83dd14a2f7fbe452297d93aceed221cd9bed3d3d Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Fri, 27 Mar 2020 18:17:54 +0500 Subject: [PATCH 3/6] Unit tests added. --- pkg/decision/rollout_service_test.go | 34 ++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/pkg/decision/rollout_service_test.go b/pkg/decision/rollout_service_test.go index 729d5ee5b..ccde38301 100644 --- a/pkg/decision/rollout_service_test.go +++ b/pkg/decision/rollout_service_test.go @@ -65,6 +65,40 @@ func (s *RolloutServiceTestSuite) SetupTest() { s.mockConfig.On("GetAudienceMap").Return(testAudienceMap) } +func (s *RolloutServiceTestSuite) TestGetDecisionWithEmptyRolloutID() { + + testRolloutService := RolloutService{} + feature := &testFeatRollout3334 + feature.Rollout.ID = "" + featureDecisionContext := FeatureDecisionContext{ + Feature: feature, + ProjectConfig: s.mockConfig, + } + expectedFeatureDecision := FeatureDecision{ + Source: Rollout, + Decision: Decision{Reason: reasons.NoRolloutForFeature}, + } + decision, _ := testRolloutService.GetDecision(featureDecisionContext, s.testUserContext) + s.Equal(expectedFeatureDecision, decision) +} + +func (s *RolloutServiceTestSuite) TestGetDecisionWithNoExperiments() { + + testRolloutService := RolloutService{} + feature := &testFeatRollout3334 + feature.Rollout.Experiments = []entities.Experiment{} + featureDecisionContext := FeatureDecisionContext{ + Feature: feature, + ProjectConfig: s.mockConfig, + } + expectedFeatureDecision := FeatureDecision{ + Source: Rollout, + Decision: Decision{Reason: reasons.RolloutHasNoExperiments}, + } + decision, _ := testRolloutService.GetDecision(featureDecisionContext, s.testUserContext) + s.Equal(expectedFeatureDecision, decision) +} + func (s *RolloutServiceTestSuite) TestGetDecisionHappyPath() { // Test experiment passes targeting and bucketing testExperimentBucketerDecision := ExperimentDecision{ From db3666fe23dfc51ba9e765631874e46840a765fe Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Tue, 31 Mar 2020 10:37:18 +0500 Subject: [PATCH 4/6] nit fixed. --- pkg/decision/rollout_service.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/decision/rollout_service.go b/pkg/decision/rollout_service.go index 9cc401442..14377ada2 100644 --- a/pkg/decision/rollout_service.go +++ b/pkg/decision/rollout_service.go @@ -22,9 +22,8 @@ import ( "github.com/optimizely/go-sdk/pkg/decision/evaluator" "github.com/optimizely/go-sdk/pkg/decision/reasons" - "github.com/optimizely/go-sdk/pkg/logging" - "github.com/optimizely/go-sdk/pkg/entities" + "github.com/optimizely/go-sdk/pkg/logging" ) // RolloutService makes a feature decision for a given feature rollout From edced35606a08b69480cce0cd753d89d798254fa Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Wed, 1 Apr 2020 14:40:55 +0500 Subject: [PATCH 5/6] Updated implementation. --- pkg/decision/rollout_service.go | 85 ++++++++++++++++++---------- pkg/decision/rollout_service_test.go | 2 - 2 files changed, 54 insertions(+), 33 deletions(-) diff --git a/pkg/decision/rollout_service.go b/pkg/decision/rollout_service.go index 14377ada2..68f739374 100644 --- a/pkg/decision/rollout_service.go +++ b/pkg/decision/rollout_service.go @@ -44,11 +44,47 @@ func NewRolloutService(sdkKey string) *RolloutService { // GetDecision returns a decision for the given feature and user context func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext) (FeatureDecision, error) { + featureDecision := FeatureDecision{ Source: Rollout, } feature := decisionContext.Feature rollout := feature.Rollout + + evaluateConditionTree := func(experiment entities.Experiment) bool { + condTreeParams := entities.NewTreeParameters(&userContext, decisionContext.ProjectConfig.GetAudienceMap()) + evalResult, _ := r.audienceTreeEvaluator.Evaluate(experiment.AudienceConditionTree, condTreeParams) + if !evalResult { + featureDecision.Reason = reasons.FailedRolloutTargeting + r.logger.Debug(fmt.Sprintf(`User "%s" failed targeting for feature rollout with key "%s".`, userContext.ID, feature.Key)) + } + return evalResult + } + + getFeatureDecision := func(experiment entities.Experiment, decision ExperimentDecision) (FeatureDecision, error) { + // translate the experiment reason into a more rollouts-appropriate reason + switch decision.Reason { + case reasons.NotBucketedIntoVariation: + featureDecision.Decision = Decision{Reason: reasons.FailedRolloutBucketing} + case reasons.BucketedIntoVariation: + featureDecision.Decision = Decision{Reason: reasons.BucketedIntoRollout} + default: + featureDecision.Decision = decision.Decision + } + + featureDecision.Experiment = experiment + featureDecision.Variation = decision.Variation + r.logger.Debug(fmt.Sprintf(`Decision made for user "%s" for feature rollout with key "%s": %s.`, userContext.ID, feature.Key, featureDecision.Reason)) + return featureDecision, nil + } + + getExperimentDecisionContext := func(experiment entities.Experiment) ExperimentDecisionContext { + return ExperimentDecisionContext{ + Experiment: &experiment, + ProjectConfig: decisionContext.ProjectConfig, + } + } + if rollout.ID == "" { featureDecision.Reason = reasons.NoRolloutForFeature return featureDecision, nil @@ -60,43 +96,30 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return featureDecision, nil } - index := 0 - for index < numberOfExperiments { + for index := 0; index < numberOfExperiments-1; index++ { experiment := rollout.Experiments[index] - experimentDecisionContext := ExperimentDecisionContext{ - Experiment: &experiment, - ProjectConfig: decisionContext.ProjectConfig, - } - if experiment.AudienceConditionTree != nil { - condTreeParams := entities.NewTreeParameters(&userContext, decisionContext.ProjectConfig.GetAudienceMap()) - 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)) - index++ - continue - } + experimentDecisionContext := getExperimentDecisionContext(experiment) + // Move to next evaluation if condition tree is available and evaluation fails + if experiment.AudienceConditionTree != nil && !evaluateConditionTree(experiment) { + // Evaluate this user for the next rule + continue } decision, _ := r.experimentBucketerService.GetDecision(experimentDecisionContext, userContext) - if decision.Variation == nil && index < numberOfExperiments-1 { + if decision.Variation == nil { // Evaluate fall back rule / last rule now - index = numberOfExperiments - 1 - continue - } - // translate the experiment reason into a more rollouts-appropriate reason - switch decision.Reason { - case reasons.NotBucketedIntoVariation: - featureDecision.Decision = Decision{Reason: reasons.FailedRolloutBucketing} - case reasons.BucketedIntoVariation: - featureDecision.Decision = Decision{Reason: reasons.BucketedIntoRollout} - default: - featureDecision.Decision = decision.Decision + break } + return getFeatureDecision(experiment, decision) + } - featureDecision.Experiment = experiment - featureDecision.Variation = decision.Variation - r.logger.Debug(fmt.Sprintf(`Decision made for user "%s" for feature rollout with key "%s": %s.`, userContext.ID, feature.Key, featureDecision.Reason)) - break + // fall back rule / last rule + experiment := rollout.Experiments[numberOfExperiments-1] + experimentDecisionContext := getExperimentDecisionContext(experiment) + // Move to bucketing if conditionTree is unavailable or evaluation passes + if experiment.AudienceConditionTree == nil || evaluateConditionTree(experiment) { + decision, _ := r.experimentBucketerService.GetDecision(experimentDecisionContext, userContext) + return getFeatureDecision(experiment, decision) } + return featureDecision, nil } diff --git a/pkg/decision/rollout_service_test.go b/pkg/decision/rollout_service_test.go index 5a14791ec..48fe33bdd 100644 --- a/pkg/decision/rollout_service_test.go +++ b/pkg/decision/rollout_service_test.go @@ -125,7 +125,6 @@ func (s *RolloutServiceTestSuite) TestGetDecisionHappyPath() { } func (s *RolloutServiceTestSuite) TestGetDecisionFallbacksToLastWhenFailsBucketing() { - // Test experiment passes targeting but not bucketing testExperiment1112BucketerDecision := ExperimentDecision{ Decision: Decision{ Reason: reasons.NotBucketedIntoVariation, @@ -162,7 +161,6 @@ func (s *RolloutServiceTestSuite) TestGetDecisionFallbacksToLastWhenFailsBucketi } func (s *RolloutServiceTestSuite) TestGetDecisionWhenFallbackBucketingFails() { - // Test experiment passes targeting but not bucketing testExperiment1112BucketerDecision := ExperimentDecision{ Decision: Decision{ Reason: reasons.NotBucketedIntoVariation, From 760a86c551e3eced9d03300662e11c7c26847961 Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Thu, 2 Apr 2020 02:39:46 +0500 Subject: [PATCH 6/6] Updated local helper function parameters to references. --- pkg/decision/rollout_service.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/decision/rollout_service.go b/pkg/decision/rollout_service.go index 68f739374..17dca3769 100644 --- a/pkg/decision/rollout_service.go +++ b/pkg/decision/rollout_service.go @@ -51,7 +51,7 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user feature := decisionContext.Feature rollout := feature.Rollout - evaluateConditionTree := func(experiment entities.Experiment) bool { + evaluateConditionTree := func(experiment *entities.Experiment) bool { condTreeParams := entities.NewTreeParameters(&userContext, decisionContext.ProjectConfig.GetAudienceMap()) evalResult, _ := r.audienceTreeEvaluator.Evaluate(experiment.AudienceConditionTree, condTreeParams) if !evalResult { @@ -61,7 +61,7 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return evalResult } - getFeatureDecision := func(experiment entities.Experiment, decision ExperimentDecision) (FeatureDecision, error) { + getFeatureDecision := func(experiment *entities.Experiment, decision *ExperimentDecision) (FeatureDecision, error) { // translate the experiment reason into a more rollouts-appropriate reason switch decision.Reason { case reasons.NotBucketedIntoVariation: @@ -72,15 +72,15 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user featureDecision.Decision = decision.Decision } - featureDecision.Experiment = experiment + featureDecision.Experiment = *experiment featureDecision.Variation = decision.Variation r.logger.Debug(fmt.Sprintf(`Decision made for user "%s" for feature rollout with key "%s": %s.`, userContext.ID, feature.Key, featureDecision.Reason)) return featureDecision, nil } - getExperimentDecisionContext := func(experiment entities.Experiment) ExperimentDecisionContext { + getExperimentDecisionContext := func(experiment *entities.Experiment) ExperimentDecisionContext { return ExperimentDecisionContext{ - Experiment: &experiment, + Experiment: experiment, ProjectConfig: decisionContext.ProjectConfig, } } @@ -97,7 +97,7 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user } for index := 0; index < numberOfExperiments-1; index++ { - experiment := rollout.Experiments[index] + experiment := &rollout.Experiments[index] experimentDecisionContext := getExperimentDecisionContext(experiment) // Move to next evaluation if condition tree is available and evaluation fails if experiment.AudienceConditionTree != nil && !evaluateConditionTree(experiment) { @@ -109,16 +109,16 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user // Evaluate fall back rule / last rule now break } - return getFeatureDecision(experiment, decision) + return getFeatureDecision(experiment, &decision) } // fall back rule / last rule - experiment := rollout.Experiments[numberOfExperiments-1] + experiment := &rollout.Experiments[numberOfExperiments-1] experimentDecisionContext := getExperimentDecisionContext(experiment) // Move to bucketing if conditionTree is unavailable or evaluation passes if experiment.AudienceConditionTree == nil || evaluateConditionTree(experiment) { decision, _ := r.experimentBucketerService.GetDecision(experimentDecisionContext, userContext) - return getFeatureDecision(experiment, decision) + return getFeatureDecision(experiment, &decision) } return featureDecision, nil