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
136 changes: 136 additions & 0 deletions extensions/amp-experiment/0.1/test/test-variant.js
@@ -0,0 +1,136 @@
/**
* 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: {}});
}).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: {
'non_number_percentage': '50',
},
});
}).to.throw(/Invalid percentage/);
});

it('should work around float rounding error', () => {
expect(() => {
allocateVariant(fakeWin, {
variants: {
'a': 50.1,
'b': 40.3,
'c': 9.2,
'd': 0.4,
// They add up to 100.00000000000001​​​ in JS
},
});
}).to.not.throw();
});

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('can have no variant allocated if variants don\'t add up to 100', () => {
return expect(allocateVariant(fakeWin, {
cidScope: null,
variants: {
'-Variant_1': 2.1,
'-Variant_2': 23.3,
'-Variant_3': 20.123,
},
})).to.eventually.equal(null);
});
});
95 changes: 95 additions & 0 deletions extensions/amp-experiment/0.1/variant.js
@@ -0,0 +1,95 @@
/**
* 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';
import {user} from '../../../src/log';

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>}
*/
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 specific order since the default object key
// enumeration is implementation (browser) dependent.
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++) {
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;
user.assert(isObject(variants) && Object.keys(variants).length > 0,
'Missing experiment variants config.');

let totalPercentage = 0;
for (const variantName in variants) {
if (variants.hasOwnProperty(variantName)) {
user.assert(nameValidator.test(variantName),
'Invalid variant name: %s. Allowed chars are [a-zA-Z0-9-_].',
variantName);

const percentage = variants[variantName];
user.assert(
typeof percentage === 'number' && percentage > 0 && percentage < 100,
'Invalid percentage %s:%s. Has to be in range of (0,100)',
variantName, percentage);
totalPercentage += percentage;
}
}
user.assert(totalPercentage./*avoid float precision error*/toFixed(6) <= 100,
'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 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
* @return {!Promise<!number>} a float number in the range of [0, 100)
*/
function getBucketTicket(win, opt_cidScope) {
if (opt_cidScope) {
// TODO(@lannka, #1411): implement hashing with CID
return Promise.resolve(1);
} else {
return Promise.resolve(win.Math.random() * 100);
}
}