Skip to content
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

Implement function that allocate variant based on an experiment config. #3762

Merged
merged 9 commits into from Jun 28, 2016
118 changes: 118 additions & 0 deletions extensions/amp-experiment/0.1/test/test-variant.js
@@ -0,0 +1,118 @@
/**
* Copyright 2016 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import {allocateVariant} from '../variant';

describe('allocateVariant', () => {

let fakeWin;

beforeEach(() => {
fakeWin = {
Math: {
random: () => {
return 0.567;
},
},
};
});

it('should throw for invalid config', () => {
expect(() => {
allocateVariant(fakeWin, null);
}).to.throw();

expect(() => {
allocateVariant(fakeWin, undefined);
}).to.throw();

expect(() => {
allocateVariant(fakeWin, {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need a test for allocateVariant(fakeWin, { variants: {} }); as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

}).to.throw(/Missing experiment variants config/);

expect(() => {
allocateVariant(fakeWin, {
variants: {
'invalid_char_%_in_name': 1,
},
});
}).to.throw(/Invalid variant name/);

expect(() => {
allocateVariant(fakeWin, {
variants: {
'variant_1': 50,
'variant_2': 51,
},
});
}).to.throw(/Total percentage is bigger than 100/);

expect(() => {
allocateVariant(fakeWin, {
variants: {
'negative_percentage': -1,
},
});
}).to.throw(/Invalid percentage/);

expect(() => {
allocateVariant(fakeWin, {
variants: {
'too_big_percentage': 101,
},
});
}).to.throw(/Invalid percentage/);

expect(() => {
allocateVariant(fakeWin, {
variants: {
'negative_percentage': '50',
},
});
}).to.throw(/Invalid percentage/);
});

it('without CID scope, succeed with a variant allocated', () => {
return expect(allocateVariant(fakeWin, {
cidScope: null,
variants: {
'-Variant_1': 56.1,
'-Variant_2': 23.3,
},
})).to.eventually.equal('-Variant_2');
});

it('should allocate variant in name order', () => {
return expect(allocateVariant(fakeWin, {
cidScope: null,
variants: {
'-Variant_2': 50,
'-Variant_1': 50,
},
})).to.eventually.equal('-Variant_2');
});

it('without CID scope, succeed with no variant allocated', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the case when all the variants don't add up to 100% percent traffic. So there'll be visits excluded from the experiment, hence no variant is allocated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the name then? This doesn't make it obvious why null is expected.

return expect(allocateVariant(fakeWin, {
cidScope: null,
variants: {
'-Variant_1': 2.1,
'-Variant_2': 23.3,
'-Variant_3': 20.123,
},
})).to.eventually.equal(null);
});
});
97 changes: 97 additions & 0 deletions extensions/amp-experiment/0.1/variant.js
@@ -0,0 +1,97 @@
/**
* Copyright 2016 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import {isObject} from '../../../src/types';

const nameValidator = /^[\w-]+$/;

/**
* Allocates the current page view to an experiment variant based on the given
* experiment config.
* @param {!Window} win
* @param {!Object} config
* @return {Promise<?string>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add either ? or ! - we require it for all types except for primitives.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meaning to Promise, e.g. !Promise

*/
export function allocateVariant(win, config) {
validateConfig(config);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How often will allocateVariant be called? If many times, we probably may want to validate only once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be called once per config. So no need to keep the validation state.


const cidScope =
config.cidScope === undefined ? 'amp-experiment' : config.cidScope;

return getBucketTicket(win, cidScope).then(bucketTicket => {
let upperBound = 0;

// Loop through keys in a stable order.
const variantNames = Object.keys(config.variants).sort();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is sorting necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also vote to use the user-specified order in the config - that'd make things clearer to the publisher why any option is selected.

Copy link
Contributor Author

@lannka lannka Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"for..in" doesn't guarantee the same iterating order across browser.

"The mechanics of enumerating the properties ... is implementation dependent."

See some discussion here.

This allocation algorithm depends on a consistent order, no matter what order. Ordering by name seems to be the easiest way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Ok. I thought this was put to rest years ago with all non-numeric keys iterated in the defined order. But if there's a risk - let's sort.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's defined in ES6, and all current browsers follow it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know it's in ES6. But AMP targets browsers that are non-ES6 right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but all current browsers follow the spec. It's in ES6 because the browsers standardized it a long time ago.

for (let i = 0; i < variantNames.length; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i++

upperBound += config.variants[variantNames[i]];
if (bucketTicket < upperBound) {
return variantNames[i];
}
}
return null;
});
}

/**
* Validates an experiment config.
* @param {!Object} config
* @throws {!Error}
*/
function validateConfig(config) {
const variants = config.variants;
if (!isObject(variants) || Object.keys(variants).length == 0) {
throw new Error('Missing experiment variants config.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of error is this? Dev or user error? If user - you should use user from log.js to assert/throw.

}

let totalPercentage = 0;
for (const variantName in variants) {
if (variants.hasOwnProperty(variantName)) {
if (!variantName.match(nameValidator)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RegExp#test will be faster here to get a boolean.

throw new Error('Invalid variant name: '
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for all errors here.

+ variantName + '. Allowed chars are [a-zA-Z0-9-_].');
}

const percentage = variants[variantName];
if (typeof percentage !== 'number'
|| percentage <= 0 || percentage >= 100) {
throw new Error('Invalid percentage ' + variantName + ': ' + percentage
+ '. Has to be in range of [0,100]');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ and ] are usually used to mean inclusive, but you're test is for exclusive. I'd actually rather it be inclusive.

}
totalPercentage += percentage;
}
}
if (totalPercentage > 100) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are almost guaranteed to run into a floating point precision error. I'd much rather require ints for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ability to have a variant with less than 1% traffic is a reasonable use case, so float is inevitable.

I had a quick fix here using number.toFixed(6). PTAL

throw new Error('Total percentage is bigger than 100: ' + totalPercentage);
}
}

/**
* Returns a float number (bucket ticket) in the range of [0, 100]. The number
* is hashed from the current CID of the given scope (opt_cidScope). If the
* scope is not provided, a random number is used.
* @param {!Window} win
* @param {?string} opt_cidScope
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's optional, the type is {string=}

* @return {Promise<!number>} a number in the range of [0, 100]
*/
function getBucketTicket(win, opt_cidScope) {
if (opt_cidScope) {
// TODO(lannka): implement CID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For implementation TODO's (especially major ones) we require this format:
TODO(@name, #bug): description

This way we can create/close github issues for better tracking and visibility.

return Promise.resolve(1);
} else {
return Promise.resolve(win.Math.random() * 100);
}
}