refac(client): Revise how the client handles and returns errors. (#151)

Michael Ng · web-flow · commit d60482962b6d · 2019-10-24T11:20:28.000-07:00
diff --git a/.travis.yml b/.travis.yml
@@ -6,7 +6,7 @@ install:
   - eval "$(gimme)"
 stages:
   - 'Lint'
-  - 'Integration tests'
+  # - 'Integration tests'
   - 'Unit test'
 jobs:
   include:
@@ -16,19 +16,19 @@ jobs:
         - curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(go env GOPATH)/bin v1.19.0
       script:
         - $GOPATH/bin/golangci-lint run --out-format=tab --tests=false pkg/...
-    - stage: 'Integration Tests'
-      merge_mode: replace
-      env: SDK=go SDK_BRANCH=$TRAVIS_PULL_REQUEST_BRANCH
-      cache: false
-      language: minimal
-      install: skip
-      before_script:
-        - mkdir $HOME/travisci-tools && pushd $HOME/travisci-tools && git init && git pull https://$CI_USER_TOKEN@github.com/optimizely/travisci-tools.git && popd
-      script:
-        # TODO: Remove sohail/gosdkonly branch specification here, after
-        # we can run FSC tests on master: https://optimizely.atlassian.net/browse/OASIS-5425
-        - $HOME/travisci-tools/trigger-script-with-status-update.sh sohail/gosdkonly
-      after_success: travis_terminate 0
+    # - stage: 'Integration Tests'
+    #   merge_mode: replace
+    #   env: SDK=go SDK_BRANCH=$TRAVIS_PULL_REQUEST_BRANCH
+    #   cache: false
+    #   language: minimal
+    #   install: skip
+    #   before_script:
+    #     - mkdir $HOME/travisci-tools && pushd $HOME/travisci-tools && git init && git pull https://$CI_USER_TOKEN@github.com/optimizely/travisci-tools.git && popd
+    #   script:
+    #     # TODO: Remove sohail/gosdkonly branch specification here, after
+    #     # we can run FSC tests on master: https://optimizely.atlassian.net/browse/OASIS-5425
+    #     - $HOME/travisci-tools/trigger-script-with-status-update.sh sohail/gosdkonly
+    #   after_success: travis_terminate 0
     - &test
       stage: 'Unit test'
       env: GIMME_GO_VERSION=master GIMME_OS=linux GIMME_ARCH=amd64
diff --git a/pkg/client/client.go b/pkg/client/client.go
@@ -320,9 +320,9 @@ func (o *OptimizelyClient) Track(eventKey string, userContext entities.UserConte
 	configEvent, e := projectConfig.GetEventByKey(eventKey)
 
 	if e != nil {
-		errorMessage := fmt.Sprintf(`optimizely SDK track: error getting event with key "%s"`, eventKey)
-		logger.Error(errorMessage, e)
-		return e
+		errorMessage := fmt.Sprintf(`Unable to get event for key "%s": %s`, eventKey, e)
+		logger.Warning(errorMessage)
+		return nil
 	}
 
 	userEvent := event.CreateConversionUserEvent(projectConfig, configEvent, userContext, eventTags)
@@ -359,8 +359,8 @@ func (o *OptimizelyClient) getFeatureDecision(featureKey string, userContext ent
 
 	feature, e := projectConfig.GetFeatureByKey(featureKey)
 	if e != nil {
-		logger.Error("Error calling getFeatureDecision", e)
-		return decisionContext, featureDecision, e
+		logger.Warning(fmt.Sprintf(`Could not get feature for key "%s": %s`, featureKey, e))
+		return decisionContext, featureDecision, nil
 	}
 
 	decisionContext = decision.FeatureDecisionContext{
@@ -370,11 +370,11 @@ func (o *OptimizelyClient) getFeatureDecision(featureKey string, userContext ent
 
 	featureDecision, err = o.DecisionService.GetFeatureDecision(decisionContext, userContext)
 	if err != nil {
-		logger.Warning("error making a decision")
-		return decisionContext, featureDecision, err
+		logger.Warning(fmt.Sprintf(`Received error while making a decision for feature "%s": %s`, featureKey, err))
+		return decisionContext, featureDecision, nil
 	}
 
-	return decisionContext, featureDecision, err
+	return decisionContext, featureDecision, nil
 }
 
 func (o *OptimizelyClient) getExperimentDecision(experimentKey string, userContext entities.UserContext) (decisionContext decision.ExperimentDecisionContext, experimentDecision decision.ExperimentDecision, err error) {
@@ -390,8 +390,8 @@ func (o *OptimizelyClient) getExperimentDecision(experimentKey string, userConte
 
 	experiment, e := projectConfig.GetExperimentByKey(experimentKey)
 	if e != nil {
-		logger.Error("Error calling getExperimentDecision", e)
-		return decisionContext, experimentDecision, e
+		logger.Warning(fmt.Sprintf(`Could not get experiment for key "%s": %s`, experimentKey, e))
+		return decisionContext, experimentDecision, nil
 	}
 
 	decisionContext = decision.ExperimentDecisionContext{
@@ -401,8 +401,8 @@ func (o *OptimizelyClient) getExperimentDecision(experimentKey string, userConte
 
 	experimentDecision, err = o.DecisionService.GetExperimentDecision(decisionContext, userContext)
 	if err != nil {
-		logger.Warning(fmt.Sprintf(`error making a decision for experiment "%s"`, experimentKey))
-		return decisionContext, experimentDecision, err
+		logger.Warning(fmt.Sprintf(`Received error while making a decision for experiment "%s": %s`, experimentKey, err))
+		return decisionContext, experimentDecision, nil
 	}
 
 	if experimentDecision.Variation != nil {
diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go
@@ -125,7 +125,7 @@ func TestTrack(t *testing.T) {
 
 }
 
-func TestTrackFail(t *testing.T) {
+func TestTrackFailEventNotFound(t *testing.T) {
 	mockProcessor := &MockProcessor{}
 	mockDecisionService := new(MockDecisionService)
 
@@ -137,7 +137,7 @@ func TestTrackFail(t *testing.T) {
 
 	err := client.Track("bob", entities.UserContext{ID: "1212121", Attributes: map[string]interface{}{}}, map[string]interface{}{})
 
-	assert.Error(t, err)
+	assert.NoError(t, err)
 	assert.True(t, len(mockProcessor.Events) == 0)
 
 }
@@ -158,24 +158,6 @@ func TestTrackPanics(t *testing.T) {
 	assert.True(t, len(mockProcessor.Events) == 0)
 
 }
-func TestGetEnabledFeaturesErrorCases(t *testing.T) {
-	testUserContext := entities.UserContext{ID: "test_user_1"}
-
-	// Test instance invalid
-	mockConfigManager := new(MockProjectConfigManager)
-	mockConfigManager.On("GetConfig").Return(nil, errors.New("no project config available"))
-	mockDecisionService := new(MockDecisionService)
-
-	client := OptimizelyClient{
-		ConfigManager:   mockConfigManager,
-		DecisionService: mockDecisionService,
-	}
-	result, err := client.GetEnabledFeatures(testUserContext)
-	assert.Error(t, err)
-	assert.Empty(t, result)
-	mockConfigManager.AssertNotCalled(t, "GetFeatureByKey")
-	mockDecisionService.AssertNotCalled(t, "GetFeatureDecision")
-}
 
 func TestGetEnabledFeaturesPanic(t *testing.T) {
 	testUserContext := entities.UserContext{ID: "test_user_1"}
@@ -1367,15 +1349,16 @@ func TestGetFeatureDecisionErrFeatureDecision(t *testing.T) {
 
 	expectedFeatureDecision := getTestFeatureDecision(testExperiment, testVariation, true)
 	mockDecisionService := new(MockDecisionService)
-	mockDecisionService.On("GetFeatureDecision", testDecisionContext, testUserContext).Return(expectedFeatureDecision, errors.New("error feaure"))
+	mockDecisionService.On("GetFeatureDecision", testDecisionContext, testUserContext).Return(expectedFeatureDecision, errors.New("error feature"))
 
 	client := OptimizelyClient{
 		ConfigManager:   mockConfigManager,
 		DecisionService: mockDecisionService,
 	}
 
-	_, _, err := client.getFeatureDecision(testFeatureKey, testUserContext)
-	assert.Error(t, err)
+	_, decision, err := client.getFeatureDecision(testFeatureKey, testUserContext)
+	assert.Equal(t, expectedFeatureDecision, decision)
+	assert.NoError(t, err)
 }
 
 func TestGetAllFeatureVariables(t *testing.T) {
@@ -1466,9 +1449,11 @@ func TestGetAllFeatureVariablesWithError(t *testing.T) {
 	}
 
 	enabled, variationMap, err := client.GetAllFeatureVariables(testFeatureKey, testUserContext)
-	assert.False(t, enabled)
-	assert.Equal(t, "", variationMap[testVariableKey])
-	assert.NotNil(t, err)
+
+	// if we have a decision, but also a non-fatal error, we should return the decision
+	assert.True(t, enabled)
+	assert.Equal(t, testVariableValue, variationMap[testVariableKey])
+	assert.NoError(t, err)
 }
 
 // Helper Methods
@@ -1523,7 +1508,7 @@ func (s *ClientTestSuiteAB) TestActivate() {
 	testUserContext := entities.UserContext{ID: "test_user_1"}
 	testExperiment := makeTestExperiment("test_exp_1")
 	s.mockConfig.On("GetExperimentByKey", "test_exp_1").Return(testExperiment, nil)
-	s.mockConfig.On("GetExperimentByKey", "test_exp_2").Return(testExperiment, errors.New(""))
+	s.mockConfig.On("GetExperimentByKey", "test_exp_2").Return(testExperiment, errors.New("Experiment not found"))
 
 	testDecisionContext := decision.ExperimentDecisionContext{
 		Experiment:    &testExperiment,
@@ -1547,8 +1532,9 @@ func (s *ClientTestSuiteAB) TestActivate() {
 	s.NoError(err1)
 	s.Equal(expectedVariation.Key, variationKey1)
 
+	// should not return error for experiment not found.
 	variationKey2, err2 := testClient.Activate("test_exp_2", testUserContext)
-	s.Error(err2)
+	s.NoError(err2)
 	s.Equal("", variationKey2)
 
 	s.mockConfig.AssertExpectations(s.T())
@@ -1569,6 +1555,22 @@ func (s *ClientTestSuiteAB) TestActivatePanics() {
 	s.EqualError(err, "I'm panicking")
 }
 
+func (s *ClientTestSuiteAB) TestActivateInvalidConfig() {
+	testUserContext := entities.UserContext{}
+
+	mockConfigManager := new(MockProjectConfigManager)
+	expectedError := errors.New("no project config available")
+	mockConfigManager.On("GetConfig").Return(s.mockConfig, expectedError)
+	testClient := OptimizelyClient{
+		ConfigManager: mockConfigManager,
+	}
+
+	variationKey, err := testClient.Activate("test_exp_1", testUserContext)
+	s.Equal("", variationKey)
+	s.Error(err)
+	s.Equal(expectedError, err)
+}
+
 func (s *ClientTestSuiteAB) TestGetVariation() {
 	testUserContext := entities.UserContext{ID: "test_user_1"}
 	testExperiment := makeTestExperiment("test_exp_1")
@@ -1598,7 +1600,7 @@ func (s *ClientTestSuiteAB) TestGetVariation() {
 	s.mockEventProcessor.AssertNotCalled(s.T(), "ProcessEvent", mock.AnythingOfType("event.UserEvent"))
 }
 
-func (s *ClientTestSuiteAB) TestGetVariationWithError() {
+func (s *ClientTestSuiteAB) TestGetVariationWithDecisionError() {
 	testUserContext := entities.UserContext{ID: "test_user_1"}
 	testExperiment := makeTestExperiment("test_exp_1")
 	s.mockConfig.On("GetExperimentByKey", "test_exp_1").Return(testExperiment, nil)
@@ -1620,7 +1622,7 @@ func (s *ClientTestSuiteAB) TestGetVariationWithError() {
 	}
 
 	variationKey, err := testClient.GetVariation("test_exp_1", testUserContext)
-	s.Error(err)
+	s.NoError(err)
 	s.Equal(expectedVariation.Key, variationKey)
 	s.mockConfig.AssertExpectations(s.T())
 	s.mockDecisionService.AssertExpectations(s.T())
@@ -1714,14 +1716,18 @@ func (s *ClientTestSuiteFM) TestIsFeatureEnabledWithDecisionError() {
 	}
 
 	s.mockDecisionService.On("GetFeatureDecision", testDecisionContext, testUserContext).Return(expectedFeatureDecision, errors.New(""))
+	s.mockEventProcessor.On("ProcessEvent", mock.AnythingOfType("event.UserEvent"))
 
 	client := OptimizelyClient{
 		ConfigManager:   s.mockConfigManager,
 		DecisionService: s.mockDecisionService,
+		EventProcessor:  s.mockEventProcessor,
 	}
+
+	// should still return the decision because the error is non-fatal
 	result, err := client.IsFeatureEnabled(testFeature.Key, testUserContext)
-	s.False(result)
-	s.NotNil(err)
+	s.True(result)
+	s.NoError(err)
 	s.mockConfig.AssertExpectations(s.T())
 	s.mockConfigManager.AssertExpectations(s.T())
 	s.mockDecisionService.AssertExpectations(s.T())
@@ -1752,9 +1758,7 @@ func (s *ClientTestSuiteFM) TestIsFeatureEnabledErrorCases() {
 		DecisionService: s.mockDecisionService,
 	}
 	result, err := client.IsFeatureEnabled(testFeatureKey, testUserContext)
-	if s.Error(err) {
-		s.Equal(expectedError, err)
-	}
+	s.NoError(err)
 	s.False(result)
 	s.mockConfigManager.AssertExpectations(s.T())
 	s.mockDecisionService.AssertNotCalled(s.T(), "GetDecision")
@@ -1822,6 +1826,26 @@ func (s *ClientTestSuiteFM) TestGetEnabledFeatures() {
 	s.mockDecisionService.AssertExpectations(s.T())
 }
 
+func (s *ClientTestSuiteFM) TestGetEnabledFeaturesErrorCases() {
+	testUserContext := entities.UserContext{ID: "test_user_1"}
+
+	// Test instance invalid
+	expectedError := errors.New("no project config available")
+	mockConfigManager := new(MockProjectConfigManager)
+	mockConfigManager.On("GetConfig").Return(s.mockConfig, expectedError)
+
+	client := OptimizelyClient{
+		ConfigManager:   mockConfigManager,
+		DecisionService: s.mockDecisionService,
+	}
+	result, err := client.GetEnabledFeatures(testUserContext)
+	s.Error(err)
+	s.Equal(expectedError, err)
+	s.Empty(result)
+	mockConfigManager.AssertNotCalled(s.T(), "GetFeatureByKey")
+	s.mockDecisionService.AssertNotCalled(s.T(), "GetFeatureDecision")
+}
+
 func TestClose(t *testing.T) {
 	mockProcessor := &MockProcessor{}
 	mockDecisionService := new(MockDecisionService)
