Skip to content

[FSSDK-11649] Fix FSC failed tests for CMAB #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open
14 changes: 13 additions & 1 deletion pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -1199,6 +1199,9 @@ func (o *OptimizelyClient) getAllOptions(options *decide.Options) decide.Options
ExcludeVariables: o.defaultDecideOptions.ExcludeVariables || options.ExcludeVariables,
IgnoreUserProfileService: o.defaultDecideOptions.IgnoreUserProfileService || options.IgnoreUserProfileService,
IncludeReasons: o.defaultDecideOptions.IncludeReasons || options.IncludeReasons,
IgnoreCMABCache: o.defaultDecideOptions.IgnoreCMABCache || options.IgnoreCMABCache,
ResetCMABCache: o.defaultDecideOptions.ResetCMABCache || options.ResetCMABCache,
InvalidateUserCMABCache: o.defaultDecideOptions.InvalidateUserCMABCache || options.InvalidateUserCMABCache,
}
}

Expand Down Expand Up @@ -1252,5 +1255,14 @@ func isNil(v interface{}) bool {
func (o *OptimizelyClient) handleDecisionServiceError(err error, key string, userContext OptimizelyUserContext) OptimizelyDecision {
o.logger.Warning(fmt.Sprintf(`Received error while making a decision for feature %q: %s`, key, err))

return NewErrorDecision(key, userContext, err)
// Return the error decision with the correct format for decision fields
return OptimizelyDecision{
FlagKey: key,
UserContext: userContext,
VariationKey: "",
RuleKey: "",
Enabled: false,
Variables: optimizelyjson.NewOptimizelyJSONfromMap(map[string]interface{}{}),
Reasons: []string{err.Error()},
}
}
30 changes: 30 additions & 0 deletions pkg/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3186,6 +3186,36 @@ func (s *ClientTestSuiteTrackNotification) TestRemoveOnTrackThrowsErrorWhenRemov
mockNotificationCenter.AssertExpectations(s.T())
}

func TestOptimizelyClient_handleDecisionServiceError(t *testing.T) {
// Create the client
client := &OptimizelyClient{
logger: logging.GetLogger("", ""),
}

// Create a CMAB error
cmabErrorMessage := "Failed to fetch CMAB data for experiment exp_1."
cmabError := fmt.Errorf(cmabErrorMessage)

// Create a user context - needs to match the signature expected by handleDecisionServiceError
testUserContext := OptimizelyUserContext{
UserID: "test_user",
Attributes: map[string]interface{}{},
}

// Call the error handler directly
decision := client.handleDecisionServiceError(cmabError, "test_flag", testUserContext)

// Verify the decision is correctly formatted
assert.False(t, decision.Enabled)
assert.Equal(t, "", decision.VariationKey) // Should be empty string, not nil
assert.Equal(t, "", decision.RuleKey) // Should be empty string, not nil
assert.Contains(t, decision.Reasons, cmabErrorMessage)

// Check that reasons contains exactly the expected message
assert.Equal(t, 1, len(decision.Reasons), "Reasons array should have exactly one item")
assert.Equal(t, cmabErrorMessage, decision.Reasons[0], "Error message should be added verbatim")
}

func TestClientTestSuiteAB(t *testing.T) {
suite.Run(t, new(ClientTestSuiteAB))
}
Expand Down
22 changes: 22 additions & 0 deletions pkg/cmab/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/****************************************************************************
* Copyright 2025, 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. *
* You may obtain a copy of the License at *
* *
* http://www.apache.org/licenses/LICENSE-2.0 *
* *
* Unless required by applicable law or agreed to in writing, software *
* distributed under the License is distributed on an "AS IS" BASIS, *
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. *
* See the License for the specific language governing permissions and *
* limitations under the License. *
***************************************************************************/

// Package cmab to define cmab errors//
package cmab

// CmabFetchFailed is the error message format for CMAB fetch failures
// Format required for FSC test compatibility - capitalized and with period
const CmabFetchFailed = "Failed to fetch CMAB data for experiment %s." //nolint:ST1005 // Required exact format for FSC test compatibility
11 changes: 8 additions & 3 deletions pkg/cmab/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package cmab

import (
"encoding/json"
"errors"
"fmt"
"strconv"

Expand Down Expand Up @@ -137,8 +138,9 @@ func (s *DefaultCmabService) GetDecision(
// Fetch new decision
decision, err := s.fetchDecision(ruleID, userContext.ID, filteredAttributes)
if err != nil {
// Append existing reasons and return the error as-is (already formatted correctly)
decision.Reasons = append(reasons, decision.Reasons...)
return decision, fmt.Errorf("CMAB API error: %w", err)
return decision, err
}

// Cache the decision
Expand Down Expand Up @@ -168,8 +170,11 @@ func (s *DefaultCmabService) fetchDecision(

variationID, err := s.cmabClient.FetchDecision(ruleID, userID, attributes, cmabUUID)
if err != nil {
reasons = append(reasons, "Failed to fetch CMAB decision")
return Decision{Reasons: reasons}, fmt.Errorf("CMAB API error: %w", err)
// Use the consistent error message format from errors.go
reason := fmt.Sprintf(CmabFetchFailed, ruleID)
reasons = append(reasons, reason)
// Use same format for Go error - FSC compatibility takes precedence
return Decision{Reasons: reasons}, errors.New(reason) //nolint:ST1005 // Required exact format for FSC test compatibility
}

reasons = append(reasons, "Successfully fetched CMAB decision")
Expand Down
64 changes: 34 additions & 30 deletions pkg/cmab/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -798,34 +798,38 @@ func TestCmabServiceTestSuite(t *testing.T) {
}

func (s *CmabServiceTestSuite) TestGetDecisionApiError() {
// Setup cache key
cacheKey := s.cmabService.getCacheKey(s.testUserID, s.testRuleID)

// Setup cache lookup (cache miss)
s.mockCache.On("Lookup", cacheKey).Return(nil)

// Setup mock to return error for experiment lookup (but this won't stop the flow anymore)
s.mockConfig.On("GetExperimentByID", s.testRuleID).Return(entities.Experiment{}, fmt.Errorf("experiment not found")).Once()

// Mock the FetchDecision call that will now happen
s.mockClient.On("FetchDecision", s.testRuleID, s.testUserID, mock.Anything, mock.Anything).Return("", fmt.Errorf("invalid rule ID"))

// Call the method
userContext := entities.UserContext{
ID: s.testUserID,
Attributes: map[string]interface{}{
"age": 30,
},
}

_, err := s.cmabService.GetDecision(s.mockConfig, userContext, s.testRuleID, nil)

// Should return error from FetchDecision, not from experiment validation
s.Error(err)
s.Contains(err.Error(), "CMAB API error")

// Verify expectations
s.mockConfig.AssertExpectations(s.T())
s.mockCache.AssertExpectations(s.T())
s.mockClient.AssertExpectations(s.T())
// Setup experiment with CMAB config
experiment := entities.Experiment{
ID: "rule-123",
Cmab: &entities.Cmab{
AttributeIds: []string{"attr1"},
},
}
s.mockConfig.On("GetExperimentByID", "rule-123").Return(experiment, nil)
s.mockConfig.On("GetAttributeKeyByID", "attr1").Return("category", nil)

// Configure client to return error
s.mockClient.On("FetchDecision", "rule-123", s.testUserID, mock.Anything, mock.Anything).Return("", errors.New("API error"))

// Setup cache miss
cacheKey := s.cmabService.getCacheKey(s.testUserID, "rule-123")
s.mockCache.On("Lookup", cacheKey).Return(nil)

userContext := entities.UserContext{
ID: s.testUserID,
Attributes: map[string]interface{}{
"category": "cmab",
},
}

decision, err := s.cmabService.GetDecision(s.mockConfig, userContext, "rule-123", nil)

// Should return the exact error format expected by FSC tests
s.Error(err)
s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") // Updated expectation
s.Contains(decision.Reasons, "Failed to fetch CMAB data for experiment rule-123.")

s.mockConfig.AssertExpectations(s.T())
s.mockCache.AssertExpectations(s.T())
s.mockClient.AssertExpectations(s.T())
}
17 changes: 17 additions & 0 deletions pkg/decision/composite_feature_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
package decision

import (
"strings"

"github.com/optimizely/go-sdk/v2/pkg/decide"
"github.com/optimizely/go-sdk/v2/pkg/entities"
"github.com/optimizely/go-sdk/v2/pkg/logging"
Expand Down Expand Up @@ -51,6 +53,21 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont
reasons.Append(decisionReasons)
if err != nil {
f.logger.Debug(err.Error())
// Check if this is a CMAB error - if so, stop the loop and return empty decision
if strings.Contains(err.Error(), "Failed to fetch CMAB data") {
// Add the CMAB error to reasons before returning - use AddError for critical failures
reasons.AddError(err.Error())
return FeatureDecision{}, reasons, nil // Return empty decision for CMAB errors
}
}

// Also check for CMAB errors in decision reasons (when err is nil)
if decisionReasons != nil {
for _, reason := range decisionReasons.ToReport() {
if strings.Contains(reason, "Failed to fetch CMAB data") {
return FeatureDecision{}, reasons, nil
}
}
}

if featureDecision.Variation != nil && err == nil {
Expand Down
37 changes: 35 additions & 2 deletions pkg/decision/composite_feature_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,16 @@ func (s *CompositeFeatureServiceTestSuite) TestGetDecisionFallthrough() {
}

func (s *CompositeFeatureServiceTestSuite) TestGetDecisionReturnsError() {
// test that we move onto the next decision service if an inner service returns an error
// test that we move onto the next decision service if an inner service returns a non-CMAB error
testUserContext := entities.UserContext{
ID: "test_user_1",
}

shouldBeIgnoredDecision := FeatureDecision{
Variation: &testExp1113Var2223,
}
s.mockFeatureService.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return(shouldBeIgnoredDecision, s.reasons, errors.New("Error making decision"))
// Use a non-CMAB error to ensure fallthrough still works for other errors
s.mockFeatureService.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return(shouldBeIgnoredDecision, s.reasons, errors.New("Generic experiment error"))

expectedDecision := FeatureDecision{
Variation: &testExp1113Var2224,
Expand Down Expand Up @@ -165,6 +166,38 @@ func (s *CompositeFeatureServiceTestSuite) TestGetDecisionReturnsLastDecisionWit
s.mockFeatureService2.AssertExpectations(s.T())
}

func (s *CompositeFeatureServiceTestSuite) TestGetDecisionWithCmabError() {
// Test that CMAB errors are terminal and don't fall through to rollout service
testUserContext := entities.UserContext{
ID: "test_user_1",
}

// Mock the first service (FeatureExperimentService) to return a CMAB error
cmabError := errors.New("Failed to fetch CMAB data for experiment exp_1.")
emptyDecision := FeatureDecision{}
s.mockFeatureService.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return(emptyDecision, s.reasons, cmabError)

// The second service (RolloutService) should NOT be called for CMAB errors

compositeFeatureService := &CompositeFeatureService{
featureServices: []FeatureService{
s.mockFeatureService,
s.mockFeatureService2,
},
logger: logging.GetLogger("sdkKey", "CompositeFeatureService"),
}

decision, _, err := compositeFeatureService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options)

// CMAB errors should result in empty decision with no error
s.Equal(FeatureDecision{}, decision)
s.NoError(err, "CMAB errors should not propagate as Go errors")

s.mockFeatureService.AssertExpectations(s.T())
// Verify that the rollout service was NOT called
s.mockFeatureService2.AssertNotCalled(s.T(), "GetDecision")
}

func (s *CompositeFeatureServiceTestSuite) TestNewCompositeFeatureService() {
// Assert that the service is instantiated with the correct child services in the right order
compositeExperimentService := NewCompositeExperimentService("")
Expand Down
9 changes: 6 additions & 3 deletions pkg/decision/experiment_cmab_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,12 @@ func (s *ExperimentCmabService) GetDecision(decisionContext ExperimentDecisionCo
// Get CMAB decision
cmabDecision, err := s.cmabService.GetDecision(projectConfig, userContext, experiment.ID, options)
if err != nil {
message := fmt.Sprintf("Failed to get CMAB decision: %v", err)
decisionReasons.AddInfo(message)
return decision, decisionReasons, fmt.Errorf("failed to get CMAB decision: %w", err)
// Add CMAB error to decision reasons
errorMessage := fmt.Sprintf(cmab.CmabFetchFailed, experiment.Key)
decisionReasons.AddInfo(errorMessage)

// Use same format for Go error - FSC compatibility takes precedence
return decision, decisionReasons, errors.New(errorMessage) //nolint:ST1005 // Required exact format for FSC test compatibility
}

// Find variation by ID
Expand Down
13 changes: 7 additions & 6 deletions pkg/decision/experiment_cmab_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -297,19 +297,20 @@ func (s *ExperimentCmabTestSuite) TestGetDecisionWithNilCmabService() {

func (s *ExperimentCmabTestSuite) TestGetDecisionWithCmabServiceError() {
testDecisionContext := ExperimentDecisionContext{
Experiment: &s.cmabExperiment, // Use s.cmabExperiment from setup
Experiment: &s.cmabExperiment,
ProjectConfig: s.mockProjectConfig,
}

// Mock bucketer to return CMAB dummy entity ID (traffic allocation passes)
s.mockExperimentBucketer.On("BucketToEntityID", "test_user_1", mock.AnythingOfType("entities.Experiment"), entities.Group{}).
Return(CmabDummyEntityID, reasons.BucketedIntoVariation, nil)

// Mock CMAB service to return error
// Mock CMAB service to return error with the exact format expected
expectedError := errors.New("Failed to fetch CMAB data for experiment cmab_exp_1.")
s.mockCmabService.On("GetDecision", s.mockProjectConfig, s.testUserContext, "cmab_exp_1", s.options).
Return(cmab.Decision{}, errors.New("CMAB service error"))
Return(cmab.Decision{}, expectedError)

// Create CMAB service with mocked dependencies (same pattern as TestGetDecisionSuccess)
// Create CMAB service with mocked dependencies
cmabService := &ExperimentCmabService{
bucketer: s.mockExperimentBucketer,
cmabService: s.mockCmabService,
Expand All @@ -318,9 +319,9 @@ func (s *ExperimentCmabTestSuite) TestGetDecisionWithCmabServiceError() {

decision, _, err := cmabService.GetDecision(testDecisionContext, s.testUserContext, s.options)

// Should return the CMAB service error
// Should return the CMAB service error with exact format - updated to match new format
s.Error(err)
s.Contains(err.Error(), "CMAB service error")
s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") // Updated from "failed" to "Failed"
s.Nil(decision.Variation) // No variation when error occurs

s.mockExperimentBucketer.AssertExpectations(s.T())
Expand Down
7 changes: 7 additions & 0 deletions pkg/decision/feature_experiment_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,13 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon
experimentDecision.Reason,
))

// Handle CMAB experiment errors - they should terminate the decision process
if err != nil && experiment.Cmab != nil {
// For CMAB experiments, errors should prevent fallback to other experiments AND rollouts
// Return the error so CompositeFeatureService can detect it
return FeatureDecision{}, reasons, err
}

// Variation not nil means we got a decision and should return it
if experimentDecision.Variation != nil {
featureDecision := FeatureDecision{
Expand Down
Loading