From 2936185e6079f2ce63435700e11be53e7454fe8b Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Thu, 3 Aug 2023 10:52:14 -0700 Subject: [PATCH] fix: Ensure that test data user targets are handled correctly. --- .../__tests__/LDClient.evaluation.test.ts | 23 +++++ .../integrations/test_data/TestData.test.ts | 91 ++++++++++++++++--- .../test_data/TestDataFlagBuilder.ts | 10 +- 3 files changed, 110 insertions(+), 14 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts b/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts index ad4ea1b07a..a5665de724 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts @@ -137,6 +137,29 @@ describe('given an LDClient with test data', () => { reason: { kind: 'ERROR', errorKind: 'FLAG_NOT_FOUND' }, }); }); + + it('evaluates and updates user targets from test data', async () => { + // This is a specific test case for a customer issue. + // It tests the combination of evaluation and test data functionality. + const userUuid = '1234'; + const userContextObject = { + kind: 'user', + key: userUuid, + }; + + await td.update( + td.flag('my-feature-flag-1').variationForUser(userUuid, false).fallthroughVariation(false), + ); + const valueA = await client.variation('my-feature-flag-1', userContextObject, 'default'); + expect(valueA).toEqual(false); + + await td.update( + td.flag('my-feature-flag-1').variationForUser(userUuid, true).fallthroughVariation(false), + ); + + const valueB = await client.variation('my-feature-flag-1', userContextObject, 'default'); + expect(valueB).toEqual(true); + }); }); describe('given an offline client', () => { diff --git a/packages/shared/sdk-server/__tests__/integrations/test_data/TestData.test.ts b/packages/shared/sdk-server/__tests__/integrations/test_data/TestData.test.ts index 4ba82b6999..81690750bc 100644 --- a/packages/shared/sdk-server/__tests__/integrations/test_data/TestData.test.ts +++ b/packages/shared/sdk-server/__tests__/integrations/test_data/TestData.test.ts @@ -207,9 +207,8 @@ describe('given a TestData instance', () => { expect(builtFlag.fallthrough).toEqual({ variation: 1 }); expect(builtFlag.offVariation).toEqual(0); expect(builtFlag.variations).toEqual([true, false]); - expect(builtFlag.contextTargets).toEqual([ - { contextKind: 'user', values: ['billy'], variation: 0 }, - ]); + expect(builtFlag.contextTargets).toEqual([{ contextKind: 'user', values: [], variation: 0 }]); + expect(builtFlag.targets).toEqual([{ values: ['billy'], variation: 0 }]); expect(builtFlag.rules).toEqual(flagRules); }); @@ -249,16 +248,32 @@ describe('given a TestData instance', () => { it('can set boolean values for a specific user target', () => { const flag = td.flag('test-flag').variationForContext('user', 'potato', false); const flag2 = td.flag('test-flag').variationForUser('potato', true); - expect(flag.build(0).contextTargets).toEqual([ + const builtFlag1 = flag.build(0); + const builtFlag2 = flag2.build(0); + // User targets order by the context targets, but use values from + // the legacy targets. + expect(builtFlag1.contextTargets).toEqual([ { contextKind: 'user', + variation: 1, + values: [], + }, + ]); + expect(builtFlag1.targets).toEqual([ + { variation: 1, values: ['potato'], }, ]); - expect(flag2.build(0).contextTargets).toEqual([ + expect(builtFlag2.contextTargets).toEqual([ { contextKind: 'user', + variation: 0, + values: [], + }, + ]); + expect(builtFlag2.targets).toEqual([ + { variation: 0, values: ['potato'], }, @@ -347,13 +362,32 @@ describe('given a TestData instance', () => { it('can move a targeted context from one variation to another', () => { const flag = td .flag('test-flag') - .variationForContext('user', 'ben', false) - .variationForContext('user', 'ben', true); + .variationForContext('org', 'ben', false) + .variationForContext('org', 'ben', true); + // Because there was only one target in the first variation there will be only + // a single variation after that target is removed. + expect(flag.build(1).contextTargets).toEqual([ + { + contextKind: 'org', + variation: 0, + values: ['ben'], + }, + ]); + }); + + it('can move a targeted user from one variation to another', () => { + const flag = td.flag('test-flag').variationForUser('ben', false).variationForUser('ben', true); // Because there was only one target in the first variation there will be only // a single variation after that target is removed. expect(flag.build(1).contextTargets).toEqual([ { contextKind: 'user', + variation: 0, + values: [], + }, + ]); + expect(flag.build(1).targets).toEqual([ + { variation: 0, values: ['ben'], }, @@ -363,19 +397,51 @@ describe('given a TestData instance', () => { it('if a targeted context is moved from one variation to another, then other targets remain for that variation', () => { const flag = td .flag('test-flag') - .variationForContext('user', 'ben', false) - .variationForContext('user', 'joe', false) - .variationForContext('user', 'ben', true); + .variationForUser('ben', false) + .variationForUser('joe', false) + .variationForUser('ben', true); expect(flag.build(1).contextTargets).toEqual([ { contextKind: 'user', variation: 0, - values: ['ben'], + values: [], }, { contextKind: 'user', variation: 1, + values: [], + }, + ]); + + expect(flag.build(1).targets).toEqual([ + { + variation: 0, + values: ['ben'], + }, + { + variation: 1, + values: ['joe'], + }, + ]); + }); + + it('if a targeted user is moved from one variation to another, then other targets remain for that variation', () => { + const flag = td + .flag('test-flag') + .variationForContext('org', 'ben', false) + .variationForContext('org', 'joe', false) + .variationForContext('org', 'ben', true); + + expect(flag.build(1).contextTargets).toEqual([ + { + contextKind: 'org', + variation: 0, + values: ['ben'], + }, + { + contextKind: 'org', + variation: 1, values: ['joe'], }, ]); @@ -393,7 +459,7 @@ describe('given a TestData instance', () => { { contextKind: 'user', variation: 1, - values: ['ben'], + values: [], }, { contextKind: 'potato', @@ -401,5 +467,6 @@ describe('given a TestData instance', () => { values: ['russet', 'yukon'], }, ]); + expect(flag.build(0).targets).toEqual([{ variation: 1, values: ['ben'] }]); }); }); diff --git a/packages/shared/sdk-server/src/integrations/test_data/TestDataFlagBuilder.ts b/packages/shared/sdk-server/src/integrations/test_data/TestDataFlagBuilder.ts index 8485cc7847..168480f43f 100644 --- a/packages/shared/sdk-server/src/integrations/test_data/TestDataFlagBuilder.ts +++ b/packages/shared/sdk-server/src/integrations/test_data/TestDataFlagBuilder.ts @@ -394,18 +394,24 @@ export default class TestDataFlagBuilder { if (this.data.targetsByVariation) { const contextTargets: Target[] = []; + const userTargets: Omit[] = []; Object.entries(this.data.targetsByVariation).forEach( ([variation, contextTargetsForVariation]) => { Object.entries(contextTargetsForVariation).forEach(([contextKind, values]) => { + const numberVariation = parseInt(variation, 10); contextTargets.push({ contextKind, - values, + values: contextKind === 'user' ? [] : values, // Iterating the object it will be a string. - variation: parseInt(variation, 10), + variation: numberVariation, }); + if (contextKind === 'user') { + userTargets.push({ values, variation: numberVariation }); + } }); }, ); + baseFlagObject.targets = userTargets; baseFlagObject.contextTargets = contextTargets; }