[FSSDK-11649] Fix FSC failed tests for CMAB #411
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mat001
changed the title
raju-opti
requested changes
Jul 17, 2025
| 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) |
There was a problem hiding this comment.
this message processing is also being done in cmab_service. I guess we can do this here and keep the code in cmab_service unchanged?
| @@ -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") { | |||
There was a problem hiding this comment.
if there is an error from the featureDecisonfeatureDecisionService.GetDecision() call, we should just return the error from here. The caller of this method should deal with the error.
| 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) |
There was a problem hiding this comment.
if there was an error, the featureDecisionService.GetDecision() call must return an error, there should be no implicit errors. Please remove this if block
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The issue in the CMAB error handling path within the GetDecision method. When the CMAB API returns an error, the previous implementation attempted to append reasons from the failed API call to the accumulated reasons list. This caused FSC tests to fail because:
Fix
Modified the error handling path to return a clean Decision object with properly initialized reasons
https://jira.sso.episerver.net/browse/FSSDK-11649