-
Notifications
You must be signed in to change notification settings - Fork 26
[FSSDK-11558] Mpirnovar add holdouts support #452
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
base: master
Are you sure you want to change the base?
Conversation
This change adds holdout support to Agent by: 1. Updating to use local go-sdk with holdouts implementation 2. Adding GetHoldoutList and GetHoldoutsForFlag methods to TestProjectConfig mock 3. Migrating CMAB prediction endpoint config from global variable to CmabConfig struct Changes: - Added go.mod replace directive to use local go-sdk for development - Implemented GetHoldoutList() and GetHoldoutsForFlag() in TestProjectConfig - Updated CMAB endpoint configuration to use CmabConfig.PredictionEndpointTemplate - Removed unused cmab package import Once go-sdk is released with holdouts, the go.mod replace can be removed and the version bumped to the new release.
Updated tests to reflect that CMAB prediction endpoint is now configured through CmabConfig.PredictionEndpointTemplate instead of a global variable. Removed assertions that checked cmab.CMABPredictionEndpoint since the endpoint is now encapsulated within the CMAB client configuration and cannot be easily verified from outside. The tests still verify that clients are created successfully with the configured endpoints.
Updated Agent to use go-sdk branch that removed the Holdouts field from OptimizelyConfig. This aligns with JavaScript and C# SDKs and fixes FSC test failures. Agent doesn't need holdouts exposed in OptimizelyConfig because: - The /v1/decide endpoint uses go-sdk's decision service which handles holdouts internally through GetHoldoutsForFlag() and GetHoldoutList() methods - OptimizelyConfig is for metadata, not decision-making - Holdout decision logic is already present in go-sdk v2.3.0 Changes: - Updated go.mod to use go-sdk@mpirnovar-fix-activate-endpoint-holdouts - Removed TestConfigIncludesHoldouts test - Removed test_config_with_holdouts and validate_holdout_structure tests - Removed session_override_sdk_key_holdouts fixture - Removed sdk_key_holdouts constant 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Updated go.mod to use released go-sdk v2.3.0 - Removed local replace directive for go-sdk development - Added comprehensive acceptance tests for holdouts functionality - test_decide_holdouts_simple.py: 3 basic tests verifying holdouts work through Agent - test_decide_holdouts.py: 7 comprehensive tests covering various holdout scenarios - User bucketed into holdout - User not in holdout audience - Multiple flags with holdouts (DecideAll) - Forced decisions overriding holdouts - Decision reasons - Impression event tracking - Multiple flags with different holdout configurations All tests verify that Agent correctly handles holdout decisions from go-sdk v2.3.0 without requiring any Agent code changes. Holdouts are evaluated internally by go-sdk's decision service and reflected in the ruleKey field. Co-Authored-By: Claude <[email protected]>
go-sdk v2.3.0 does not include holdouts field in OptimizelyConfig. The holdouts field was added in a later commit (e70a8f3) after v2.3.0 was tagged.
pvcraven
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment. Looks good.
|
|
||
| from tests.acceptance.helpers import ENDPOINT_CONFIG | ||
| from tests.acceptance.helpers import create_and_validate_request_and_response | ||
| from tests.acceptance.holdouts_datafile import holdouts_datafile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! It ws there when I was using local datafile for testing
Summary
Holdouts support for Agent. Most work is done in go-sdk under the hood.
pkg/optimizely/optimizelytest/config.go
Issues
https://optimizely-ext.atlassian.net/browse/FSSDK-11558