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.

Sign up for GitHub

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

Extended experiments URL parser to handle originalHash #4536

Merged
merged 8 commits into from
Aug 24, 2016
Merged

Extended experiments URL parser to handle originalHash #4536

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f0e64bd
Extended experiments URL parser to handle originalHash
Aug 16, 2016
8f092df
Intermediate state: Update in place, but working on tests. Do not me…
Aug 18, 2016
713d2a5
Update method of hash parsing for URL params.
Aug 22, 2016
21011c6
Remove a debugging-only log message.
Aug 22, 2016
a9d55b6
Lint fixes.
Aug 22, 2016
00689fa
Removed unnecessary viewerFor call.
Aug 23, 2016
e06f9c5
Updated expectations for rand.called.
Aug 23, 2016
59ecd5d
Updated expectations for rand.called.
Aug 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 133 additions & 19 deletions ads/google/a4a/test/test-google-ads-a4a-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,29 @@ import {
isInManualExperiment,
} from '../traffic-experiments';
import {resetExperimentToggles_} from '../../../../src/experiments';
import {installViewerService} from '../../../../src/service/viewer-impl';
import {resetServiceForTesting} from '../../../../src/service';
import {documentStateFor} from '../../../../src/document-state';
import {platformFor} from '../../../../src/platform';
import * as sinon from 'sinon';

const EXP_ID = 'EXP_ID';
/** @type {!Branches} */
const EXTERNAL_BRANCHES = {
control: 'EXT_CONTROL',
experiment: 'EXT_EXPERIMENT',
};
/** @type {!Branches} */
const INTERNAL_BRANCHES = {
control: 'INT_CONTROL',
experiment: 'INT_EXPERIMENT',
};

describe('a4a_config', () => {
let sandbox;
let win;
let rand;
let events;

beforeEach(() => {
sandbox = sinon.sandbox.create();
Expand All @@ -38,32 +55,34 @@ describe('a4a_config', () => {
href: 'https://cdn.ampproject.org/fnord',
pathname: '/fnord',
origin: 'https://cdn.ampproject.org',
hash: '',
},
document: {
hidden: false,
cookie: null,
visibilityState: 'visible',
addEventListener: function(type, listener) {
events[type] = listener;
},
},
crypto: {
subtle: true,
webkitSubtle: true,
},
navigator: window.navigator,
};
events = {};
platformFor(win);
documentStateFor(win);
installViewerService(win);
});

afterEach(() => {
resetExperimentToggles_(); // Clear saved, page-level experiment state.
resetServiceForTesting(win, 'viewer');
sandbox.restore();
});

const EXP_ID = 'EXP_ID';
const EXTERNAL_BRANCHES = {
control: 'EXT_CONTROL',
experiment: 'EXT_EXPERIMENT',
};
const INTERNAL_BRANCHES = {
control: 'INT_CONTROL',
experiment: 'INT_EXPERIMENT',
};

it('should attach expt ID and return true when expt is on', () => {
rand.onFirstCall().returns(-1); // Force experiment on.
rand.onSecondCall().returns(0.75); // Select second branch.
Expand Down Expand Up @@ -107,7 +126,7 @@ describe('a4a_config', () => {
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.false;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called ever').to.be.false;
expect(rand, 'rand called ever').to.not.be.called;
expect(element.getAttribute('data-experiment-id')).to.not.be.ok;
});

Expand All @@ -119,7 +138,7 @@ describe('a4a_config', () => {
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.false;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called ever').to.be.false;
expect(rand, 'rand called ever').to.not.be.called;
expect(element.getAttribute('data-experiment-id')).to.not.be.ok;
});

Expand Down Expand Up @@ -155,7 +174,7 @@ describe('a4a_config', () => {
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.false;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called at least once').to.be.true;
expect(rand, 'rand called at least once').to.be.called;
expect(element.getAttribute('data-experiment-id')).to.not.be.ok;
});

Expand All @@ -167,7 +186,7 @@ describe('a4a_config', () => {
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.false;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called at least once').to.be.true;
expect(rand, 'rand called at least once').to.be.called;
expect(element.getAttribute('data-experiment-id')).to.not.be.ok;
});

Expand All @@ -181,7 +200,7 @@ describe('a4a_config', () => {
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.true;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called at least once').to.be.true;
expect(rand, 'rand called at least once').to.be.called;
expect(element.getAttribute('data-experiment-id')).to.equal(
INTERNAL_BRANCHES.experiment);
});
Expand All @@ -196,7 +215,7 @@ describe('a4a_config', () => {
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.true;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called at least once').to.be.true;
expect(rand, 'rand called at least once').to.be.called;
expect(element.getAttribute('data-experiment-id')).to.equal(
INTERNAL_BRANCHES.experiment);
});
Expand All @@ -211,7 +230,7 @@ describe('a4a_config', () => {
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.true;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called at least once').to.be.false;
expect(rand, 'rand never called').to.not.be.called;
expect(element.getAttribute('data-experiment-id')).to.equal(
EXTERNAL_BRANCHES.experiment);
});
Expand All @@ -227,7 +246,7 @@ describe('a4a_config', () => {
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.false;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called at least once').to.be.false;
expect(rand, 'rand never called').to.not.be.called;
expect(element.getAttribute('data-experiment-id')).to.equal(
EXTERNAL_BRANCHES.control);
});
Expand Down Expand Up @@ -258,7 +277,7 @@ describe('a4a_config', () => {
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.true;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called at least once').to.be.false;
expect(rand, 'rand never called').to.not.be.called;
expect(isInManualExperiment(element), 'element in manual experiment')
.to.be.true;
// And it shouldn't be in any *other* experiments.
Expand All @@ -272,5 +291,100 @@ describe('a4a_config', () => {
}
});
});
});

// These tests are separated because they need to invoke installViewerService
// within the test, rather than in the beforeEach().
describe('a4a_config hash param parsing', () => {
let sandbox;
let win;
let rand;
let events;

beforeEach(() => {
sandbox = sinon.sandbox.create();
rand = sandbox.stub(Math, 'random');
win = {
AMP_MODE: {
localDev: true,
},
location: {
href: 'https://cdn.ampproject.org/fnord',
pathname: '/fnord',
origin: 'https://cdn.ampproject.org',
hash: '',
search: 'somewhere=over&the=rainbow',
},
document: {
hidden: false,
cookie: null,
visibilityState: 'visible',
addEventListener: function(type, listener) {
events[type] = listener;
},
},
crypto: {
subtle: true,
webkitSubtle: true,
},
navigator: window.navigator,
};
events = {};
platformFor(win);
documentStateFor(win);
});
afterEach(() => {
resetExperimentToggles_(); // Clear saved, page-level experiment state.
resetServiceForTesting(win, 'viewer');
sandbox.restore();
});

const hashBaseConditions = ['#exp=PARAM',
'#p=blarg&exp=PARAM',
'#p=blarg&exp=PARAM&s=987',
'#p=blarg&exp=zort:123,PARAM,spaz:987&s=987'];

hashBaseConditions.forEach(hashBase => {
it(`should find viewer param when pattern is ${hashBase}`, () => {
win.location.hash = hashBase.replace('PARAM', 'a4a:-1');
installViewerService(win);
// Ensure that internal branches aren't attached, even if the PRNG
// would normally trigger them.
rand.onFirstCall().returns(-1);
const element = document.createElement('div');
// Should not register as 'A4A enabled', but should still attach the
// control experiment ID.
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm.. googleAdsIsA4AEnabled has a side effect to element. And all the following test actually depend on this side effect.

Any way to improve the code? Or maybe just rename it to a better name?

Copy link
Author

Choose a reason for hiding this comment

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

The side effect is deliberate. The attribute that's set on the element is to communicate the experiment ID to future processing, regardless of whether it's other A4A code or 3p iframe code that ends up processing the ad.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understand, I saw it clearly documented in the JS doc. Still, the naming confused me again (I recall that I was confused when reviewing that function in the previous PR).

registerElementIfA4AEnabled() how does this name sound to you?

I'm totally fine to get this PR in and have a renaming afterwards, if you think it makes sense.

INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.true;
expect(win.document.cookie).to.be.null;
expect(rand, 'rand never called').to.not.be.called;
expect(isInManualExperiment(element), 'element in manual experiment')
.to.be.true;
// And it shouldn't be in any *other* experiments.
for (const branch in EXTERNAL_BRANCHES) {
expect(isInExperiment(element, EXTERNAL_BRANCHES[branch]),
'element in ', EXTERNAL_BRANCHES[branch]).to.be.false;
}
for (const branch in EXTERNAL_BRANCHES) {
expect(isInExperiment(element, INTERNAL_BRANCHES[branch]),
'element in ', EXTERNAL_BRANCHES[branch]).to.be.false;
}
});

it(`hash should trump search; pattern=${hashBase}`, () => {
win.location.search = hashBase.replace('PARAM', 'a4a:-1');
win.location.hash = hashBase.replace('PARAM', 'a4a:2');
installViewerService(win);
// Ensure that internal branches aren't attached, even if the PRNG
// would normally trigger them.
rand.onFirstCall().returns(-1);
const element = document.createElement('div');
expect(googleAdsIsA4AEnabled(win, element, EXP_ID, EXTERNAL_BRANCHES,
INTERNAL_BRANCHES), 'googleAdsIsA4AEnabled').to.be.true;
expect(win.document.cookie).to.be.null;
expect(rand.called, 'rand called at least once').to.be.false;
Copy link
Contributor

Choose a reason for hiding this comment

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

one more left.

I think you can also remove the message, as it will say something similar for you.

Copy link
Author

Choose a reason for hiding this comment

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

D'oh! Ok, this time I did a global search for rand.called and for to.not.be.called. I think (hope?) I got them all. Also removed messages. PTAL.

expect(element.getAttribute('data-experiment-id')).to.equal(
EXTERNAL_BRANCHES.experiment);
});
});
});
4 changes: 3 additions & 1 deletion ads/google/a4a/traffic-experiments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {isGoogleAdsA4AValidEnvironment} from './utils';
import {isExperimentOn, toggleExperiment} from '../../../src/experiments';
import {dev} from '../../../src/log';
import {getMode} from '../../../src/mode';
import {viewerFor} from '../../../src/viewer';
import {parseQueryString} from '../../../src/url';

/** @typedef {{string: {branches: !Branches}}} */
Expand Down Expand Up @@ -121,7 +122,8 @@ export function googleAdsIsA4AEnabled(win, element, experimentName,
*/
function maybeSetExperimentFromUrl(win, experimentName,
controlBranchId, treatmentBranchId, manualId) {
const expParam = parseQueryString(win.location.search)['exp'];
const expParam = viewerFor(win).getParam('exp') ||
parseQueryString(win.location.search)['exp'];
if (!expParam) {
return;
}
Expand Down