refac(decision): moved various components around. (#111)

Author: Michael Ng

optimizely/decision/composite_experiment_service.go

@@ -18,15 +18,12 @@
 package decision
 
 import (
-	"github.com/optimizely/go-sdk/optimizely/decision/evaluator"
-	"github.com/optimizely/go-sdk/optimizely/decision/reasons"
 	"github.com/optimizely/go-sdk/optimizely/entities"
 )
 
 // CompositeExperimentService bridges together the various experiment decision services that ship by default with the SDK
 type CompositeExperimentService struct {
-	audienceTreeEvaluator evaluator.TreeEvaluator
-	experimentServices    []ExperimentService
+	experimentServices []ExperimentService
 }
 
 // NewCompositeExperimentService creates a new instance of the CompositeExperimentService
@@ -35,7 +32,6 @@ func NewCompositeExperimentService() *CompositeExperimentService {
 	// 1. Bucketing
 	// @TODO(mng): Prepend forced variation and whitelisting services
 	return &CompositeExperimentService{
-		audienceTreeEvaluator: evaluator.NewMixedTreeEvaluator(),
 		experimentServices: []ExperimentService{
 			NewExperimentBucketerService(),
 		},
@@ -46,19 +42,8 @@ func NewCompositeExperimentService() *CompositeExperimentService {
 func (s CompositeExperimentService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) {
 
 	experimentDecision := ExperimentDecision{}
-	experiment := decisionContext.Experiment
 
-	// Determine if user can be part of the experiment
-	if experiment.AudienceConditionTree != nil {
-		condTreeParams := entities.NewTreeParameters(&userContext, decisionContext.ProjectConfig.GetAudienceMap())
-		evalResult := s.audienceTreeEvaluator.Evaluate(experiment.AudienceConditionTree, condTreeParams)
-		if !evalResult {
-			experimentDecision.Reason = reasons.FailedAudienceTargeting
-			return experimentDecision, nil
-		}
-	}
-
-	// User passed targeting (or the experiment is untargeted), so run through the various decision services
+	// Run through the various decision services until we get a decision
 	for _, experimentService := range s.experimentServices {
 		if decision, err := experimentService.GetDecision(decisionContext, userContext); decision.Variation != nil {
 			return decision, err

optimizely/decision/composite_experiment_service_test.go

@@ -19,18 +19,33 @@ package decision
 import (
 	"testing"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/suite"
 
 	"github.com/optimizely/go-sdk/optimizely/entities"
 )
 
-func TestCompositeExperimentServiceGetDecision(t *testing.T) {
-	mockProjectConfig := new(mockProjectConfig)
-	testDecisionContext := ExperimentDecisionContext{
+type CompositeExperimentTestSuite struct {
+	suite.Suite
+	mockConfig             *mockProjectConfig
+	mockExperimentService  *MockExperimentDecisionService
+	mockExperimentService2 *MockExperimentDecisionService
+	testDecisionContext    ExperimentDecisionContext
+}
+
+func (s *CompositeExperimentTestSuite) SetupTest() {
+	s.mockConfig = new(mockProjectConfig)
+	s.mockExperimentService = new(MockExperimentDecisionService)
+	s.mockExperimentService2 = new(MockExperimentDecisionService)
+
+	// Setup test data
+	s.testDecisionContext = ExperimentDecisionContext{
 		Experiment:    &testExp1111,
-		ProjectConfig: mockProjectConfig,
+		ProjectConfig: s.mockConfig,
 	}
+}
 
+func (s *CompositeExperimentTestSuite) TestGetDecision() {
+	// test that we return out of the decision making and the next one does not get called
 	testUserContext := entities.UserContext{
 		ID: "test_user_1",
 	}
@@ -39,102 +54,67 @@ func TestCompositeExperimentServiceGetDecision(t *testing.T) {
 	expectedExperimentDecision := ExperimentDecision{
 		Variation: &expectedVariation,
 	}
-	// test that we return out of the decision making and the next one doesn't get called
-	mockExperimentDecisionService := new(MockExperimentDecisionService)
-	mockExperimentDecisionService.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
+	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
 
-	mockExperimentDecisionService2 := new(MockExperimentDecisionService)
 	compositeExperimentService := &CompositeExperimentService{
-		experimentServices: []ExperimentService{mockExperimentDecisionService, mockExperimentDecisionService2},
+		experimentServices: []ExperimentService{s.mockExperimentService, s.mockExperimentService2},
+	}
+	decision, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext)
+	s.Equal(expectedExperimentDecision, decision)
+	s.NoError(err)
+	s.mockExperimentService.AssertExpectations(s.T())
+	s.mockExperimentService2.AssertNotCalled(s.T(), "GetDecision")
+
+}
+
+func (s *CompositeExperimentTestSuite) TestGetDecisionFallthrough() {
+	// test that we move onto the next decision service if no decision is made
+	testUserContext := entities.UserContext{
+		ID: "test_user_1",
 	}
-	decision, err := compositeExperimentService.GetDecision(testDecisionContext, testUserContext)
-	mockExperimentDecisionService.AssertExpectations(t)
-	mockExperimentDecisionService2.AssertNotCalled(t, "GetDecision")
 
-	// test that we move on to the next decision service if no decision is made
-	mockExperimentDecisionService = new(MockExperimentDecisionService)
-	expectedExperimentDecision = ExperimentDecision{}
-	mockExperimentDecisionService.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
+	expectedVariation := testExp1111.Variations["2222"]
+	expectedExperimentDecision := ExperimentDecision{}
+	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
 
-	mockExperimentDecisionService2 = new(MockExperimentDecisionService)
 	expectedExperimentDecision2 := ExperimentDecision{
 		Variation: &expectedVariation,
 	}
-	mockExperimentDecisionService2.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision2, nil)
+	s.mockExperimentService2.On("GetDecision", s.testDecisionContext, testUserContext).Return(expectedExperimentDecision2, nil)
 
-	compositeExperimentService = &CompositeExperimentService{
-		experimentServices: []ExperimentService{mockExperimentDecisionService, mockExperimentDecisionService2},
+	compositeExperimentService := &CompositeExperimentService{
+		experimentServices: []ExperimentService{s.mockExperimentService, s.mockExperimentService2},
 	}
-	decision, err = compositeExperimentService.GetDecision(testDecisionContext, testUserContext)
+	decision, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext)
 
-	assert.NoError(t, err)
-	assert.Equal(t, expectedExperimentDecision2, decision)
-	mockExperimentDecisionService.AssertExpectations(t)
-	mockExperimentDecisionService2.AssertExpectations(t)
+	s.NoError(err)
+	s.Equal(expectedExperimentDecision2, decision)
+	s.mockExperimentService.AssertExpectations(s.T())
+	s.mockExperimentService2.AssertExpectations(s.T())
+}
 
+func (s *CompositeExperimentTestSuite) TestGetDecisionNoDecisionsMade() {
 	// test when no decisions are made
-	mockExperimentDecisionService = new(MockExperimentDecisionService)
-	expectedExperimentDecision = ExperimentDecision{}
-	mockExperimentDecisionService.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
-
-	mockExperimentDecisionService2 = new(MockExperimentDecisionService)
-	expectedExperimentDecision2 = ExperimentDecision{}
-	mockExperimentDecisionService2.On("GetDecision", testDecisionContext, testUserContext).Return(expectedExperimentDecision2, nil)
-
-	compositeExperimentService = &CompositeExperimentService{
-		experimentServices: []ExperimentService{mockExperimentDecisionService, mockExperimentDecisionService2},
+	testUserContext := entities.UserContext{
+		ID: "test_user_1",
 	}
-	decision, err = compositeExperimentService.GetDecision(testDecisionContext, testUserContext)
+	expectedExperimentDecision := ExperimentDecision{}
+	s.mockExperimentService.On("GetDecision", s.testDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
 
-	assert.NoError(t, err)
-	assert.Equal(t, expectedExperimentDecision2, decision)
-	mockExperimentDecisionService.AssertExpectations(t)
-	mockExperimentDecisionService2.AssertExpectations(t)
-}
+	expectedExperimentDecision2 := ExperimentDecision{}
+	s.mockExperimentService2.On("GetDecision", s.testDecisionContext, testUserContext).Return(expectedExperimentDecision2, nil)
 
-func TestCompositeExperimentServiceGetDecisionTargeting(t *testing.T) {
-	testUserContext := entities.UserContext{
-		ID: "test_user",
-	}
-	testAudienceMap := map[string]entities.Audience{
-		"5555": testAudience5555,
+	compositeExperimentService := &CompositeExperimentService{
+		experimentServices: []ExperimentService{s.mockExperimentService, s.mockExperimentService2},
 	}
-	mockProjectConfig := new(mockProjectConfig)
-	mockProjectConfig.On("GetAudienceMap").Return(testAudienceMap)
+	decision, err := compositeExperimentService.GetDecision(s.testDecisionContext, testUserContext)
 
-	testExperimentDecisionContext := ExperimentDecisionContext{
-		Experiment:    &testExp1112,
-		ProjectConfig: mockProjectConfig,
-	}
-	testCondTreeParams := entities.NewTreeParameters(&testUserContext, testAudienceMap)
-
-	// Test user fails targeting
-	mockAudienceTreeEvaluator := new(MockAudienceTreeEvaluator)
-	mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, testCondTreeParams).Return(false)
-	mockExperimentDecisionService := new(MockExperimentDecisionService)
-	testCompositeExperimentService := &CompositeExperimentService{
-		audienceTreeEvaluator: mockAudienceTreeEvaluator,
-		experimentServices:    []ExperimentService{mockExperimentDecisionService},
-	}
-	decision, _ := testCompositeExperimentService.GetDecision(testExperimentDecisionContext, testUserContext)
-	assert.Nil(t, decision.Variation)
-	mockAudienceTreeEvaluator.AssertExpectations(t)
-	mockExperimentDecisionService.AssertNotCalled(t, "GetDecision")
+	s.NoError(err)
+	s.Equal(expectedExperimentDecision2, decision)
+	s.mockExperimentService.AssertExpectations(s.T())
+	s.mockExperimentService2.AssertExpectations(s.T())
+}
 
-	// Test user passes targeting, moves on to children decision services
-	expectedExperimentDecision := ExperimentDecision{
-		Variation: &testExp1112Var2222,
-	}
-	mockAudienceTreeEvaluator = new(MockAudienceTreeEvaluator)
-	mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, testCondTreeParams).Return(true)
-	mockExperimentDecisionService = new(MockExperimentDecisionService)
-	mockExperimentDecisionService.On("GetDecision", testExperimentDecisionContext, testUserContext).Return(expectedExperimentDecision, nil)
-	testCompositeExperimentService = &CompositeExperimentService{
-		audienceTreeEvaluator: mockAudienceTreeEvaluator,
-		experimentServices:    []ExperimentService{mockExperimentDecisionService},
-	}
-	decision, _ = testCompositeExperimentService.GetDecision(testExperimentDecisionContext, testUserContext)
-	assert.Equal(t, decision, expectedExperimentDecision)
-	mockAudienceTreeEvaluator.AssertExpectations(t)
-	mockExperimentDecisionService.AssertExpectations(t)
+func TestCompositeExperimentTestSuite(t *testing.T) {
+	suite.Run(t, new(CompositeExperimentTestSuite))
 }

optimizely/decision/composite_feature_service.go

@@ -28,65 +28,27 @@ var cfLogger = logging.GetLogger("CompositeFeatureService")
 
 // CompositeFeatureService is the default out-of-the-box feature decision service
 type CompositeFeatureService struct {
-	featureExperimentService ExperimentService
-	rolloutDecisionService   FeatureService
+	featureServices []FeatureService
 }
 
 // NewCompositeFeatureService returns a new instance of the CompositeFeatureService
 func NewCompositeFeatureService() *CompositeFeatureService {
 	return &CompositeFeatureService{
-		featureExperimentService: NewFeatureExperimentService(),
-		rolloutDecisionService:   NewRolloutService(),
+		featureServices: []FeatureService{
+			NewFeatureExperimentService(),
+			NewRolloutService(),
+		},
 	}
 }
 
 // GetDecision returns a decision for the given feature and user context
 func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext) (FeatureDecision, error) {
-	feature := decisionContext.Feature
-
-	// Check if user is bucketed in feature experiment
-	if f.featureExperimentService != nil && len(feature.FeatureExperiments) > 0 {
-		// @TODO this can be improved by getting group ID first and determining experiment and then bucketing in experiment
-		for _, experiment := range feature.FeatureExperiments {
-			featureExperiment := experiment
-			experimentDecisionContext := ExperimentDecisionContext{
-				Experiment:    &featureExperiment,
-				ProjectConfig: decisionContext.ProjectConfig,
-			}
-
-			experimentDecision, err := f.featureExperimentService.GetDecision(experimentDecisionContext, userContext)
-			// Variation not nil means we got a decision and should return it
-			if experimentDecision.Variation != nil {
-				featureDecision := FeatureDecision{
-					Experiment: experiment,
-					Decision:   experimentDecision.Decision,
-					Variation:  experimentDecision.Variation,
-					Source:     FeatureTest,
-				}
-
-				cfLogger.Debug(fmt.Sprintf(
-					`Decision made for feature test with key "%s" for user "%s" with the following reason: "%s".`,
-					feature.Key,
-					userContext.ID,
-					featureDecision.Reason,
-				))
-				return featureDecision, err
-			}
+	for _, featureDecisionService := range f.featureServices {
+		featureDecision, err := featureDecisionService.GetDecision(decisionContext, userContext)
+		if featureDecision.Variation != nil {
+			return featureDecision, err
 		}
 	}
 
-	featureDecisionContext := FeatureDecisionContext{
-		Feature:       feature,
-		ProjectConfig: decisionContext.ProjectConfig,
-	}
-	featureDecision, err := f.rolloutDecisionService.GetDecision(featureDecisionContext, userContext)
-	featureDecision.Source = Rollout
-	cfLogger.Debug(fmt.Sprintf(
-		`Decision made for feature rollout with key "%s" for user "%s" with the following reason: "%s".`,
-		feature.Key,
-		userContext.ID,
-		featureDecision.Reason,
-	))
-
-	return featureDecision, err
+	return FeatureDecision{}, fmt.Errorf("no decision was made for feature %s", decisionContext.Feature.Key)
 }