-
Notifications
You must be signed in to change notification settings - Fork 86
[AI-FSSDK] [FSSDK-12369] Add local holdouts support to JavaScript SDK #1151
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2879,4 +2879,227 @@ describe('DecisionService', () => { | |
| expect(variation).toBe(null); | ||
| }); | ||
| }); | ||
|
|
||
| // Level 2 decision service tests for local holdouts (FSSDK-12369) | ||
| // One test per branch of the pseudocode in Step 3 of the ticket. | ||
| describe('local holdouts (FSSDK-12369)', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please remove ticket number from the title |
||
| // Helper: build a datafile that has a local holdout targeting a specific experiment or delivery rule. | ||
| const makeLocalHoldoutDatafile = (targetRuleId: string, ruleIds: string[] = [targetRuleId]) => { | ||
| const datafile = getDecisionTestDatafile(); | ||
| (datafile as any).holdouts = [ | ||
| { | ||
| id: 'local_holdout_id', | ||
| key: 'local_holdout', | ||
| status: 'Running', | ||
| includedFlags: [], | ||
| excludedFlags: [], | ||
| includedRules: ruleIds, | ||
| audienceIds: [], | ||
| audienceConditions: [], | ||
| variations: [ | ||
| { | ||
| id: 'local_holdout_variation_id', | ||
| key: 'local_holdout_variation', | ||
| variables: [] | ||
| } | ||
| ], | ||
| trafficAllocation: [ | ||
| { entityId: 'local_holdout_variation_id', endOfRange: 10000 } | ||
| ] | ||
| } | ||
| ]; | ||
| return datafile; | ||
| }; | ||
|
|
||
| beforeEach(() => { | ||
| mockBucket.mockReset(); | ||
| }); | ||
|
|
||
| it('global holdout branch: global holdout is evaluated before per-rule logic', async () => { | ||
| const datafile = getDecisionTestDatafile(); | ||
| (datafile as any).holdouts = [ | ||
| { | ||
| id: 'global_holdout_id', | ||
| key: 'global_holdout', | ||
| status: 'Running', | ||
| includedFlags: [], | ||
| excludedFlags: [], | ||
| // No includedRules → global holdout | ||
| audienceIds: [], | ||
| audienceConditions: [], | ||
| variations: [ | ||
| { id: 'global_holdout_var_id', key: 'global_holdout_var', variables: [] } | ||
| ], | ||
| trafficAllocation: [{ entityId: 'global_holdout_var_id', endOfRange: 10000 }] | ||
| } | ||
| ]; | ||
| const config = createProjectConfig(datafile); | ||
| const { decisionService } = getDecisionService(); | ||
|
|
||
| // bucket returns the global holdout variation for the holdout, nothing for experiments | ||
| mockBucket.mockImplementation((params: BucketerParams) => { | ||
| if (params.experimentId === 'global_holdout_id') { | ||
| return { result: 'global_holdout_var_id', reasons: [] }; | ||
| } | ||
| return { result: null, reasons: [] }; | ||
| }); | ||
|
|
||
| const user = new OptimizelyUserContext({ optimizely: {} as any, userId: 'user1' }); | ||
| const feature = config.featureKeyMap['flag_1']; | ||
| const value = await decisionService.resolveVariationsForFeatureList('async', config, [feature], user, {}).get(); | ||
|
|
||
| // Decision should be from the global holdout, not from any experiment | ||
| expect(value[0].result.decisionSource).toBe(DECISION_SOURCES.HOLDOUT); | ||
| expect(value[0].result.experiment?.id).toBe('global_holdout_id'); | ||
| }); | ||
|
|
||
| it('local holdout hit branch: user bucketed into local holdout for experiment rule returns holdout variation; audience and traffic not evaluated for that rule', async () => { | ||
| // exp_1 has id '2001' | ||
| const config = createProjectConfig(makeLocalHoldoutDatafile('2001')); | ||
| const { decisionService } = getDecisionService(); | ||
|
|
||
| // bucket returns holdout variation when evaluating the local holdout | ||
| mockBucket.mockImplementation((params: BucketerParams) => { | ||
| if (params.experimentId === 'local_holdout_id') { | ||
| return { result: 'local_holdout_variation_id', reasons: [] }; | ||
| } | ||
| return { result: null, reasons: [] }; | ||
| }); | ||
|
|
||
| const user = new OptimizelyUserContext({ optimizely: {} as any, userId: 'user1' }); | ||
| const feature = config.featureKeyMap['flag_1']; | ||
| const value = await decisionService.resolveVariationsForFeatureList('async', config, [feature], user, {}).get(); | ||
|
|
||
| // Should return holdout decision for the local holdout | ||
| expect(value[0].result.decisionSource).toBe(DECISION_SOURCES.HOLDOUT); | ||
| expect(value[0].result.experiment?.id).toBe('local_holdout_id'); | ||
| expect(value[0].result.variation?.id).toBe('local_holdout_variation_id'); | ||
| }); | ||
|
|
||
| it('local holdout miss branch: user not bucketed into local holdout falls through to regular rule evaluation', async () => { | ||
| // exp_1 has id '2001' and audience 4001 (age <= 22) | ||
| const config = createProjectConfig(makeLocalHoldoutDatafile('2001')); | ||
| const { decisionService } = getDecisionService(); | ||
|
|
||
| // bucket returns null for the local holdout, then succeeds for the experiment | ||
| mockBucket.mockImplementation((params: BucketerParams) => { | ||
| if (params.experimentId === 'local_holdout_id') { | ||
| return { result: null, reasons: [] }; | ||
| } | ||
| if (params.experimentId === '2001') { | ||
| return { result: '5001', reasons: [] }; // variation_1 in exp_1 | ||
| } | ||
| return { result: null, reasons: [] }; | ||
| }); | ||
|
|
||
| const user = new OptimizelyUserContext({ | ||
| optimizely: {} as any, | ||
| userId: 'user1', | ||
| attributes: { age: 15 }, // satisfies 4001 audience (age <= 22) | ||
| }); | ||
| const feature = config.featureKeyMap['flag_1']; | ||
| const value = await decisionService.resolveVariationsForFeatureList('async', config, [feature], user, {}).get(); | ||
|
|
||
| // Should fall through to experiment evaluation (not holdout) | ||
| expect(value[0].result.decisionSource).toBe(DECISION_SOURCES.FEATURE_TEST); | ||
| expect(value[0].result.variation?.id).toBe('5001'); | ||
| }); | ||
|
|
||
| it('rule specificity: local holdout targeting experiment rule X does not affect experiment rule Y', async () => { | ||
| // exp_1 = '2001', exp_2 = '2002'. Local holdout targets only '2002' (exp_2). | ||
| // Audience for exp_1: 4001 (age <= 22). User satisfies exp_1 audience but not exp_2. | ||
| const config = createProjectConfig(makeLocalHoldoutDatafile('2002')); | ||
| const { decisionService } = getDecisionService(); | ||
|
|
||
| // bucket returns holdout variation only for the local holdout when evaluating for '2002', | ||
| // and returns experiment variation for '2001' | ||
| mockBucket.mockImplementation((params: BucketerParams) => { | ||
| if (params.experimentId === 'local_holdout_id') { | ||
| return { result: 'local_holdout_variation_id', reasons: [] }; | ||
| } | ||
| if (params.experimentId === '2001') { | ||
| return { result: '5001', reasons: [] }; | ||
| } | ||
| return { result: null, reasons: [] }; | ||
| }); | ||
|
|
||
| // User satisfies exp_1 audience (age <= 22) | ||
| const user = new OptimizelyUserContext({ | ||
| optimizely: {} as any, | ||
| userId: 'user1', | ||
| attributes: { age: 15 }, | ||
| }); | ||
| const feature = config.featureKeyMap['flag_1']; | ||
| const value = await decisionService.resolveVariationsForFeatureList('async', config, [feature], user, {}).get(); | ||
|
|
||
| // exp_1 is evaluated first; local holdout targets '2002' not '2001', so exp_1 is evaluated normally | ||
| expect(value[0].result.decisionSource).toBe(DECISION_SOURCES.FEATURE_TEST); | ||
| expect(value[0].result.experiment?.id).toBe('2001'); | ||
| }); | ||
|
|
||
| it('local holdout applies to delivery rules (rollouts) as well as experiment rules', async () => { | ||
| // delivery_1 has id '3001' | ||
| const config = createProjectConfig(makeLocalHoldoutDatafile('3001')); | ||
| const { decisionService } = getDecisionService(); | ||
|
|
||
| // bucket returns null for all experiments and the local holdout variation for delivery rule | ||
| mockBucket.mockImplementation((params: BucketerParams) => { | ||
| if (params.experimentId === 'local_holdout_id') { | ||
| return { result: 'local_holdout_variation_id', reasons: [] }; | ||
| } | ||
| return { result: null, reasons: [] }; | ||
| }); | ||
|
|
||
| // No audience attributes → experiments won't match, falls through to rollout | ||
| const user = new OptimizelyUserContext({ | ||
| optimizely: {} as any, | ||
| userId: 'user1', | ||
| attributes: { age: 15 }, // satisfies 4001 used by delivery_1 | ||
| }); | ||
| const feature = config.featureKeyMap['flag_1']; | ||
| const value = await decisionService.resolveVariationsForFeatureList('async', config, [feature], user, {}).get(); | ||
|
|
||
| // Should be a holdout decision from the local holdout targeting the delivery rule | ||
| expect(value[0].result.decisionSource).toBe(DECISION_SOURCES.HOLDOUT); | ||
| expect(value[0].result.experiment?.id).toBe('local_holdout_id'); | ||
| }); | ||
|
|
||
| it('forced decision beats a 100% traffic local holdout: forced decision takes precedence over local holdout', async () => { | ||
| // exp_1 has id '2001', key 'exp_1', variation key 'variation_1' (id '5001') | ||
| // Local holdout targets '2001' with 100% traffic allocation. | ||
| // User also has a forced decision set for exp_1. | ||
| // Expected: forced decision wins; decisionSource is FEATURE_TEST, not HOLDOUT. | ||
| const config = createProjectConfig(makeLocalHoldoutDatafile('2001')); | ||
| const { decisionService } = getDecisionService(); | ||
|
|
||
| // bucket should NOT be called for local_holdout_id because forced decision short-circuits first | ||
| mockBucket.mockImplementation((params: BucketerParams) => { | ||
| if (params.experimentId === 'local_holdout_id') { | ||
| // returning holdout variation here to prove the test fails if ordering is wrong | ||
| return { result: 'local_holdout_variation_id', reasons: [] }; | ||
| } | ||
| return { result: null, reasons: [] }; | ||
| }); | ||
|
|
||
| const user = new OptimizelyUserContext({ | ||
| optimizely: {} as any, | ||
| userId: 'user1', | ||
| attributes: { age: 15 }, | ||
| }); | ||
|
|
||
| // Set forced decision for exp_1 → variation_1 | ||
| user.setForcedDecision( | ||
| { flagKey: 'flag_1', ruleKey: 'exp_1' }, | ||
| { variationKey: 'variation_1' } | ||
| ); | ||
|
|
||
| const feature = config.featureKeyMap['flag_1']; | ||
| const value = await decisionService.resolveVariationsForFeatureList('async', config, [feature], user, {}).get(); | ||
|
|
||
| // Forced decision must win — source must be FEATURE_TEST, not HOLDOUT | ||
| expect(value[0].result.decisionSource).toBe(DECISION_SOURCES.FEATURE_TEST); | ||
| expect(value[0].result.variation?.key).toBe('variation_1'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,8 @@ import { | |
| getVariationIdFromExperimentAndVariationKey, | ||
| getVariationFromId, | ||
| getVariationKeyFromId, | ||
| getGlobalHoldouts, | ||
| getHoldoutsForRule, | ||
| isActive, | ||
| ProjectConfig, | ||
| } from '../../project_config/project_config'; | ||
|
|
@@ -135,6 +137,8 @@ interface DecisionServiceOptions { | |
|
|
||
| interface DeliveryRuleResponse<T, K> extends DecisionResponse<T> { | ||
| skipToEveryoneElse: K; | ||
| /** Set when a local holdout was hit for this delivery rule (FSSDK-12369). */ | ||
| localHoldoutDecision?: DecisionObj; | ||
| } | ||
|
|
||
| interface UserProfileTracker { | ||
|
|
@@ -145,6 +149,12 @@ interface UserProfileTracker { | |
| type VarationKeyWithCmabParams = { | ||
| variationKey?: string; | ||
| cmabUuid?: string; | ||
| /** | ||
| * Set when the variation comes from a local holdout (FSSDK-12369). | ||
| * When present, the caller should use this as the full decision result | ||
| * instead of constructing one from the experiment. | ||
| */ | ||
| localHoldoutDecision?: DecisionObj; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This field does not make sense inside this type. This will work functionally now, but I would like to refactor it later |
||
| }; | ||
| export type DecisionReason = [string, ...any[]]; | ||
| export type VariationResult = DecisionResponse<VarationKeyWithCmabParams>; | ||
|
|
@@ -943,11 +953,11 @@ export class DecisionService { | |
| }); | ||
| } | ||
|
|
||
| // all global holouts should be evaluated for all flags | ||
| // global holdouts are available in configObj.holdouts | ||
| const { holdouts } = configObj; | ||
| // Global holdouts are evaluated at flag level, before any per-rule logic. | ||
| // getGlobalHoldouts() returns holdouts with includedRules == null/undefined. | ||
| const globalHoldouts = getGlobalHoldouts(configObj); | ||
|
|
||
| for (const holdout of holdouts) { | ||
| for (const holdout of globalHoldouts) { | ||
| const holdoutDecision = this.getVariationForHoldout(configObj, holdout, user); | ||
| decideReasons.push(...holdoutDecision.reasons); | ||
|
|
||
|
|
@@ -1092,11 +1102,19 @@ export class DecisionService { | |
| }); | ||
| } | ||
|
|
||
| // If a local holdout was hit, return the holdout decision directly (preserves holdout as experiment). | ||
| if (decisionVariation.result.localHoldoutDecision) { | ||
| return Value.of(op, { | ||
| result: decisionVariation.result.localHoldoutDecision, | ||
| reasons: decideReasons, | ||
| }); | ||
| } | ||
|
|
||
| if(!decisionVariation.result.variationKey) { | ||
| return this.traverseFeatureExperimentList( | ||
| op, configObj, feature, fromIndex + 1, user, decideReasons, decideOptions, userProfileTracker); | ||
| } | ||
|
|
||
| const variationKey = decisionVariation.result.variationKey; | ||
| let variation: Variation | null = experiment.variationKeyMap[variationKey]; | ||
| if (!variation) { | ||
|
|
@@ -1184,6 +1202,13 @@ export class DecisionService { | |
| variation = decisionVariation.result; | ||
| skipToEveryoneElse = decisionVariation.skipToEveryoneElse; | ||
| if (variation) { | ||
| // If a local holdout was hit for this delivery rule, use its decision object directly. | ||
| if (decisionVariation.localHoldoutDecision) { | ||
| return { | ||
| result: decisionVariation.localHoldoutDecision, | ||
| reasons: decideReasons, | ||
| }; | ||
| } | ||
| rolloutRule = configObj.experimentIdMap[rolloutRules[index].id]; | ||
| decisionObj = { | ||
| experiment: rolloutRule, | ||
|
|
@@ -1562,6 +1587,25 @@ export class DecisionService { | |
| reasons: decideReasons, | ||
| }); | ||
| } | ||
|
|
||
| // Check local holdouts targeting this specific experiment rule (FSSDK-12369). | ||
| // Inserted immediately after the forced-decision block, before regular rule evaluation. | ||
| const localHoldoutsForExperiment = getHoldoutsForRule(configObj, rule.id); | ||
| for (const holdout of localHoldoutsForExperiment) { | ||
| const holdoutDecision = this.getVariationForHoldout(configObj, holdout, user); | ||
| decideReasons.push(...holdoutDecision.reasons); | ||
| if (holdoutDecision.result.variation) { | ||
| // Signal the caller to use the holdout decision directly, preserving the holdout as experiment. | ||
| return Value.of(op, { | ||
| result: { | ||
| variationKey: holdoutDecision.result.variation.key, | ||
| localHoldoutDecision: holdoutDecision.result, | ||
| }, | ||
| reasons: decideReasons, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| const decisionVariationValue = this.resolveVariation(op, configObj, rule, user, decideOptions, userProfileTracker); | ||
|
|
||
| return decisionVariationValue.then((variationResult) => { | ||
|
|
@@ -1608,6 +1652,22 @@ export class DecisionService { | |
| }; | ||
| } | ||
|
|
||
| // Check local holdouts targeting this specific delivery rule (FSSDK-12369). | ||
| // Inserted immediately after the forced-decision block, before audience and traffic allocation checks. | ||
| const localHoldoutsForDelivery = getHoldoutsForRule(configObj, rule.id); | ||
| for (const holdout of localHoldoutsForDelivery) { | ||
| const holdoutDecision = this.getVariationForHoldout(configObj, holdout, user); | ||
| decideReasons.push(...holdoutDecision.reasons); | ||
| if (holdoutDecision.result.variation) { | ||
| return { | ||
| result: holdoutDecision.result.variation, | ||
| reasons: decideReasons, | ||
| skipToEveryoneElse, | ||
| localHoldoutDecision: holdoutDecision.result, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| const userId = user.getUserId(); | ||
| const attributes = user.getAttributes(); | ||
| const bucketingId = this.getBucketingId(userId, attributes); | ||
|
|
||
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.
please remove the ticket number