diff --git a/evaluate_flag.js b/evaluate_flag.js index 37ce427..0717d2c 100644 --- a/evaluate_flag.js +++ b/evaluate_flag.js @@ -295,18 +295,30 @@ function variationForUser(r, user, flag) { if (r.variation != null) { // This represets a fixed variation; return it return r.variation; - } else if (r.rollout != null) { - // This represents a percentage rollout. Assume - // we're rolling out by key - const bucketBy = r.rollout.bucketBy != null ? r.rollout.bucketBy : 'key'; - const bucket = bucketUser(user, flag.key, bucketBy, flag.salt); - let sum = 0; - for (let i = 0; i < r.rollout.variations.length; i++) { - const variate = r.rollout.variations[i]; - sum += variate.weight / 100000.0; - if (bucket < sum) { - return variate.variation; + } + const rollout = r.rollout; + if (rollout) { + const variations = rollout.variations; + if (variations && variations.length > 0) { + // This represents a percentage rollout. Assume + // we're rolling out by key + const bucketBy = rollout.bucketBy || 'key'; + const bucket = bucketUser(user, flag.key, bucketBy, flag.salt); + let sum = 0; + for (let i = 0; i < variations.length; i++) { + const variate = variations[i]; + sum += variate.weight / 100000.0; + if (bucket < sum) { + return variate.variation; + } } + + // The user's bucket value was greater than or equal to the end of the last bucket. This could happen due + // to a rounding error, or due to the fact that we are scaling to 100000 rather than 99999, or the flag + // data could contain buckets that don't actually add up to 100000. Rather than returning an error in + // this case (or changing the scaling, which would potentially change the results for *all* users), we + // will simply put the user in the last bucket. + return variations[variations.length - 1].variation; } } diff --git a/test/evaluate_flag-test.js b/test/evaluate_flag-test.js index 796bba9..06220ab 100644 --- a/test/evaluate_flag-test.js +++ b/test/evaluate_flag-test.js @@ -696,6 +696,66 @@ describe('evaluate', () => { }); }); +describe('rollout', () => { + it('selects bucket', done => { + const user = { key: 'userkey' }; + const flagKey = 'flagkey'; + const salt = 'salt'; + + // First verify that with our test inputs, the bucket value will be greater than zero and less than 100000, + // so we can construct a rollout whose second bucket just barely contains that value + const bucketValue = Math.floor(evaluate.bucketUser(user, flagKey, 'key', salt) * 100000); + expect(bucketValue).toBeGreaterThan(0); + expect(bucketValue).toBeLessThan(100000); + + const badVariationA = 0, matchedVariation = 1, badVariationB = 2; + const rollout = { + variations: [ + { variation: badVariationA, weight: bucketValue }, // end of bucket range is not inclusive, so it will *not* match the target value + { variation: matchedVariation, weight: 1 }, // size of this bucket is 1, so it only matches that specific value + { variation: badVariationB, weight: 100000 - (bucketValue + 1) } + ] + }; + const flag = { + key: flagKey, + salt: salt, + on: true, + fallthrough: { rollout: rollout }, + variations: [ null, null, null ] + }; + evaluate.evaluate(flag, user, featureStore, eventFactory, (err, detail) => { + expect(err).toEqual(null); + expect(detail.variationIndex).toEqual(matchedVariation); + done(); + }); + }); + + it('uses last bucket if bucket value is equal to total weight', done => { + const user = { key: 'userkey' }; + const flagKey = 'flagkey'; + const salt = 'salt'; + + // We'll construct a list of variations that stops right at the target bucket value + const bucketValue = Math.floor(evaluate.bucketUser(user, flagKey, 'key', salt) * 100000); + + const rollout = { + variations: [ { variation: 0, weight: bucketValue }] + }; + const flag = { + key: flagKey, + salt: salt, + on: true, + fallthrough: { rollout: rollout }, + variations: [ null, null, null ] + }; + evaluate.evaluate(flag, user, featureStore, eventFactory, (err, detail) => { + expect(err).toEqual(null); + expect(detail.variationIndex).toEqual(0); + done(); + }); + }); +}); + describe('bucketUser', () => { it('gets expected bucket values for specific keys', () => { var user = { key: 'userKeyA' };