From 106667a2f7d854b2c43c6b697cca8f717522ae2d Mon Sep 17 00:00:00 2001 From: tlong2 Date: Thu, 11 May 2017 18:54:46 +0100 Subject: [PATCH] holdout experiment for AdSense amp-auto-ads (#9261) --- .../a4a/test/test-traffic-experiments.js | 16 ---- ads/google/a4a/test/test-utils.js | 48 ++++++++++- ads/google/a4a/traffic-experiments.js | 30 ++----- ads/google/a4a/utils.js | 34 +++++++- ads/google/adsense-amp-auto-ads.js | 62 ++++++++++++++ ads/google/test/test-adsense-amp-auto-ads.js | 83 +++++++++++++++++++ build-system/dep-check-config.js | 1 + .../0.1/amp-ad-network-adsense-impl.js | 11 ++- .../test/test-amp-ad-network-adsense-impl.js | 39 ++++++++- .../amp-auto-ads/0.1/ad-network-config.js | 19 +++++ extensions/amp-auto-ads/0.1/amp-auto-ads.js | 4 + .../0.1/test/test-ad-network-config.js | 33 ++++++++ .../0.1/test/test-amp-auto-ads.js | 54 +++++++++++- tools/experiments/experiments.js | 6 ++ 14 files changed, 389 insertions(+), 51 deletions(-) create mode 100644 ads/google/adsense-amp-auto-ads.js create mode 100644 ads/google/test/test-adsense-amp-auto-ads.js diff --git a/ads/google/a4a/test/test-traffic-experiments.js b/ads/google/a4a/test/test-traffic-experiments.js index 0611a993914b..28608607f362 100644 --- a/ads/google/a4a/test/test-traffic-experiments.js +++ b/ads/google/a4a/test/test-traffic-experiments.js @@ -18,7 +18,6 @@ import {ampdocServiceFor} from '../../../../src/ampdoc'; import {installDocService} from '../../../../src/service/ampdoc-impl'; import { addExperimentIdToElement, - mergeExperimentIds, isInExperiment, isExternallyTriggeredExperiment, isInternallyTriggeredExperiment, @@ -214,21 +213,6 @@ describe('all-traffic-experiments-tests', () => { }); }); - describe('#mergeExperimentIds', () => { - it('should merge a single id to itself', () => { - expect(mergeExperimentIds('12345')).to.equal('12345'); - }); - it('should merge a single ID to a list', () => { - expect(mergeExperimentIds('12345', '3,4,5,6')).to.equal('3,4,5,6,12345'); - }); - it('should discard invalid ID', () => { - expect(mergeExperimentIds('frob', '3,4,5,6')).to.equal('3,4,5,6'); - }); - it('should return empty string for invalid input', () => { - expect(mergeExperimentIds('frob')).to.equal(''); - }); - }); - describe('#isInExperiment', () => { it('should return false for empty element and any query', () => { const element = document.createElement('div'); diff --git a/ads/google/a4a/test/test-utils.js b/ads/google/a4a/test/test-utils.js index 938ac4dc4e70..80bc0b4d468e 100644 --- a/ads/google/a4a/test/test-utils.js +++ b/ads/google/a4a/test/test-utils.js @@ -19,6 +19,7 @@ import { extractAmpAnalyticsConfig, extractGoogleAdCreativeAndSignature, googleAdUrl, + mergeExperimentIds, } from '../utils'; import {createElementWithAttributes} from '../../../../src/dom'; import {base64UrlDecodeToBytes} from '../../../../src/utils/base64'; @@ -270,11 +271,11 @@ describe('Google A4A utils', () => { const impl = new MockA4AImpl(elem); noopMethods(impl, doc, sandbox); return fixture.addElement(elem).then(() => { - return googleAdUrl(impl, '', 0, [], []).then(url1 => { + return googleAdUrl(impl, '', 0, [], [], []).then(url1 => { expect(url1).to.match(/ifi=1/); - return googleAdUrl(impl, '', 0, [], []).then(url2 => { + return googleAdUrl(impl, '', 0, [], [], []).then(url2 => { expect(url2).to.match(/ifi=2/); - return googleAdUrl(impl, '', 0, [], []).then(url3 => { + return googleAdUrl(impl, '', 0, [], [], []).then(url3 => { expect(url3).to.match(/ifi=3/); }); }); @@ -346,5 +347,46 @@ describe('Google A4A utils', () => { }); }); + it('should include all experiment ids', function() { + // When ran locally, this test tends to exceed 2000ms timeout. + this.timeout(5000); + return createIframePromise().then(fixture => { + setupForAdTesting(fixture); + const doc = fixture.doc; + const elem = createElementWithAttributes(doc, 'amp-a4a', { + 'type': 'adsense', + 'width': '320', + 'height': '50', + 'data-experiment-id': '123,456', + }); + const impl = new MockA4AImpl(elem); + noopMethods(impl, doc, sandbox); + return fixture.addElement(elem).then(() => { + return googleAdUrl(impl, '', 0, [], [], ['789', '098']).then(url1 => { + expect(url1).to.match(/eid=123%2C456%2C789%2C098/); + }); + }); + }); + }); + }); + + describe('#mergeExperimentIds', () => { + it('should merge a single id to itself', () => { + expect(mergeExperimentIds(['12345'])).to.equal('12345'); + }); + it('should merge a single ID to a list', () => { + expect(mergeExperimentIds(['12345'], '3,4,5,6')) + .to.equal('3,4,5,6,12345'); + }); + it('should merge multiple IDs into a list', () => { + expect(mergeExperimentIds(['12345','6789'], '3,4,5,6')) + .to.equal('3,4,5,6,12345,6789'); + }); + it('should discard invalid ID', () => { + expect(mergeExperimentIds(['frob'], '3,4,5,6')).to.equal('3,4,5,6'); + }); + it('should return empty string for invalid input', () => { + expect(mergeExperimentIds(['frob'])).to.equal(''); + }); }); }); diff --git a/ads/google/a4a/traffic-experiments.js b/ads/google/a4a/traffic-experiments.js index a41452a9e9ad..4501d54c3e07 100644 --- a/ads/google/a4a/traffic-experiments.js +++ b/ads/google/a4a/traffic-experiments.js @@ -22,7 +22,11 @@ * impacts on click-throughs. */ -import {isGoogleAdsA4AValidEnvironment, EXPERIMENT_ATTRIBUTE} from './utils'; +import { + isGoogleAdsA4AValidEnvironment, + mergeExperimentIds, + EXPERIMENT_ATTRIBUTE, +} from './utils'; import { isExperimentOn, forceExperimentBranch, @@ -309,28 +313,6 @@ export function validateExperimentIds(idList) { return idList.every(id => { return !isNaN(parseInt(id, 10)); }); } -/** - * Add a new experiment ID to a (possibly empty) existing set of experiment IDs. - * The {@code currentIdString} may be {@code null} or {@code ''}, but if it is - * populated, it must contain a comma-separated list of integer experiment IDs - * (per {@code parseExperimentIds()}). Returns the new set of IDs, encoded - * as a comma-separated list. Does not de-duplicate ID entries. - * - * @param {!string} newId ID to merge in. Must be a stringified integer - * (base 10). - * @param {?string} currentIdString If present, a string containing a - * comma-separated list of integer experiment IDs. - * @returns {string} New experiment list string, including newId iff it is - * a valid (integer) experiment ID. - * @see parseExperimentIds, validateExperimentIds - */ -export function mergeExperimentIds(newId, currentIdString) { - if (newId && !isNaN(parseInt(newId, 10))) { - return currentIdString ? (currentIdString + ',' + newId) : newId; - } - return currentIdString || ''; -} - /** * Adds a single experimentID to an element iff it's a valid experiment ID. * @@ -341,7 +323,7 @@ export function addExperimentIdToElement(experimentId, element) { const currentEids = element.getAttribute(EXPERIMENT_ATTRIBUTE); if (currentEids && validateExperimentIds(parseExperimentIds(currentEids))) { element.setAttribute(EXPERIMENT_ATTRIBUTE, - mergeExperimentIds(experimentId, currentEids)); + mergeExperimentIds([experimentId], currentEids)); } else { element.setAttribute(EXPERIMENT_ATTRIBUTE, experimentId); } diff --git a/ads/google/a4a/utils.js b/ads/google/a4a/utils.js index fbc4f9cd6e16..44d5ac47ffc9 100644 --- a/ads/google/a4a/utils.js +++ b/ads/google/a4a/utils.js @@ -132,10 +132,14 @@ export function isReportingEnabled(ampElement) { * Parameters that will be put at the end of the URL, where they may be * elided for length reasons. Intended for parameters with potentially * long values, like URLs. + * @param {!Array=} opt_experimentIds Any experiments IDs (in addition + * to those specified on the ad element) that should be included in the + * request. * @return {!Promise} */ export function googleAdUrl( - a4a, baseUrl, startTime, queryParams, unboundedQueryParams) { + a4a, baseUrl, startTime, queryParams, unboundedQueryParams, + opt_experimentIds) { // TODO: Maybe add checks in case these promises fail. /** @const {!Promise} */ const referrerPromise = viewerForDoc(a4a.getAmpDoc()).getReferrerUrl(); @@ -172,6 +176,10 @@ export function googleAdUrl( if (isCanary(win)) { queryParams.push({name: 'isc', value: '1'}); } + let eids = adElement.getAttribute('data-experiment-id'); + if (opt_experimentIds) { + eids = mergeExperimentIds(opt_experimentIds, eids); + } const allQueryParams = queryParams.concat( [ { @@ -187,7 +195,7 @@ export function googleAdUrl( {name: 'output', value: 'html'}, {name: 'nhd', value: iframeDepth}, {name: 'iu', value: adElement.getAttribute('data-ad-slot')}, - {name: 'eid', value: adElement.getAttribute('data-experiment-id')}, + {name: 'eid', value: eids}, {name: 'biw', value: viewportRect.width}, {name: 'bih', value: viewportRect.height}, {name: 'adx', value: slotRect.left}, @@ -480,3 +488,25 @@ export function extractAmpAnalyticsConfig( } return null; } + +/** + * Add new experiment IDs to a (possibly empty) existing set of experiment IDs. + * The {@code currentIdString} may be {@code null} or {@code ''}, but if it is + * populated, it must contain a comma-separated list of integer experiment IDs + * (per {@code parseExperimentIds()}). Returns the new set of IDs, encoded + * as a comma-separated list. Does not de-duplicate ID entries. + * + * @param {!Array} newIds IDs to merge in. Should contain stringified + * integer (base 10) experiment IDs. + * @param {?string} currentIdString If present, a string containing a + * comma-separated list of integer experiment IDs. + * @returns {string} New experiment list string, including newId iff it is + * a valid (integer) experiment ID. + * @see parseExperimentIds, validateExperimentIds + */ +export function mergeExperimentIds(newIds, currentIdString) { + const newIdString = newIds.filter(newId => Number(newId)).join(','); + currentIdString = currentIdString || ''; + return currentIdString + (currentIdString && newIdString ? ',' : '') + + newIdString; +} diff --git a/ads/google/adsense-amp-auto-ads.js b/ads/google/adsense-amp-auto-ads.js new file mode 100644 index 000000000000..ad622517cf7d --- /dev/null +++ b/ads/google/adsense-amp-auto-ads.js @@ -0,0 +1,62 @@ +/** + * Copyright 2017 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 { + randomlySelectUnsetExperiments, + getExperimentBranch, +} from '../../src/experiments'; + + +/** @const {string} */ +export const ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME = + 'amp-auto-ads-adsense-holdout'; + + +/** + * @enum {string} + */ +export const AdSenseAmpAutoAdsHoldoutBranches = { + CONTROL: '3782001', // don't run amp-auto-ads + EXPERIMENT: '3782002', // do run amp-auto-ads +}; + + +/** @const {!../../src/experiments.ExperimentInfo} */ +const ADSENSE_AMP_AUTO_ADS_EXPERIMENT_INFO = { + isTrafficEligible: win => !!win.document.querySelector('AMP-AUTO-ADS'), + branches: [ + AdSenseAmpAutoAdsHoldoutBranches.CONTROL, + AdSenseAmpAutoAdsHoldoutBranches.EXPERIMENT, + ], +}; + + +/** + * This has the side-effect of selecting the page into a branch of the + * experiment, which becomes sticky for the entire pageview. + * + * @param {!Window} win + * @return {?string} + */ +export function getAdSenseAmpAutoAdsExpBranch(win) { + const experiments = {}; + experiments[ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME] = + ADSENSE_AMP_AUTO_ADS_EXPERIMENT_INFO; + randomlySelectUnsetExperiments(win, experiments); + return getExperimentBranch(win, ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME) + || null; +}; diff --git a/ads/google/test/test-adsense-amp-auto-ads.js b/ads/google/test/test-adsense-amp-auto-ads.js new file mode 100644 index 000000000000..a03f25ae5f71 --- /dev/null +++ b/ads/google/test/test-adsense-amp-auto-ads.js @@ -0,0 +1,83 @@ +/** + * Copyright 2017 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 { + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches, + getAdSenseAmpAutoAdsExpBranch, +} from '../adsense-amp-auto-ads'; +import { + RANDOM_NUMBER_GENERATORS, + toggleExperiment, +} from '../../../src/experiments'; + +describes.realWin('adsense-amp-auto-ads', {}, env => { + let win; + let sandbox; + let accurateRandomStub; + let cachedAccuratePrng; + + beforeEach(() => { + win = env.win; + sandbox = env.sandbox; + + accurateRandomStub = sandbox.stub().returns(-1); + cachedAccuratePrng = RANDOM_NUMBER_GENERATORS.accuratePrng; + RANDOM_NUMBER_GENERATORS.accuratePrng = accurateRandomStub; + }); + + afterEach(() => { + sandbox.restore(); + RANDOM_NUMBER_GENERATORS.accuratePrng = cachedAccuratePrng; + }); + + it('should pick the control branch when experiment on and amp-auto-ads ' + + 'tag present.', () => { + const ampAutoAdsEl = win.document.createElement('amp-auto-ads'); + win.document.body.appendChild(ampAutoAdsEl); + toggleExperiment(win, ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, true); + RANDOM_NUMBER_GENERATORS.accuratePrng.onFirstCall().returns(0.4); + expect(getAdSenseAmpAutoAdsExpBranch(win)) + .to.equal(AdSenseAmpAutoAdsHoldoutBranches.CONTROL); + }); + + it('should pick the experiment branch when experiment on and amp-auto-ads ' + + 'tag present.', () => { + const ampAutoAdsEl = win.document.createElement('amp-auto-ads'); + win.document.body.appendChild(ampAutoAdsEl); + toggleExperiment(win, ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, true); + RANDOM_NUMBER_GENERATORS.accuratePrng.onFirstCall().returns(0.6); + expect(getAdSenseAmpAutoAdsExpBranch(win)) + .to.equal(AdSenseAmpAutoAdsHoldoutBranches.EXPERIMENT); + }); + + it('should not pick a branch when experiment off.', () => { + const ampAutoAdsEl = win.document.createElement('amp-auto-ads'); + win.document.body.appendChild(ampAutoAdsEl); + toggleExperiment(win, ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, false); + RANDOM_NUMBER_GENERATORS.accuratePrng.onFirstCall().returns(0.4); + expect(getAdSenseAmpAutoAdsExpBranch(win)).to.be.null; + }); + + it('should not pick a branch when experiment on but no and amp-auto-ads ' + + 'tag present.', () => { + toggleExperiment(win, ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, true); + RANDOM_NUMBER_GENERATORS.accuratePrng.onFirstCall().returns(0.4); + expect(getAdSenseAmpAutoAdsExpBranch(win)).to.be.null; + }); +}); + + diff --git a/build-system/dep-check-config.js b/build-system/dep-check-config.js index c8c425f4f8bb..a567e15e59c5 100644 --- a/build-system/dep-check-config.js +++ b/build-system/dep-check-config.js @@ -100,6 +100,7 @@ exports.rules = [ 'ads/**->src/types.js', 'ads/**->src/string.js', 'ads/**->src/style.js', + 'ads/google/adsense-amp-auto-ads.js->src/experiments.js', // ads/google/a4a doesn't contain 3P ad code and should probably move // somewhere else at some point 'ads/google/a4a/**->src/ad-cid.js', diff --git a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js index 12ecce5a8212..0d91920994a0 100644 --- a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js +++ b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js @@ -48,6 +48,9 @@ import { import {viewerForDoc} from '../../../src/services'; import {AdsenseSharedState} from './adsense-shared-state'; import {insertAnalyticsElement} from '../../../src/analytics'; +import { + getAdSenseAmpAutoAdsExpBranch, +} from '../../../ads/google/adsense-amp-auto-ads'; /** @const {string} */ const ADSENSE_BASE_URL = 'https://googleads.g.doubleclick.net/pagead/ads'; @@ -183,8 +186,14 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { paramList.push({name: 'prev_fmts', value: sharedStateParams.prevFmts}); } + const experimentIds = []; + const ampAutoAdsBranch = getAdSenseAmpAutoAdsExpBranch(this.win); + if (ampAutoAdsBranch) { + experimentIds.push(ampAutoAdsBranch); + } + return googleAdUrl( - this, ADSENSE_BASE_URL, startTime, paramList, []); + this, ADSENSE_BASE_URL, startTime, paramList, [], experimentIds); } /** @override */ diff --git a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js index eb986c3fd7b4..5e48130beb1a 100644 --- a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js +++ b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js @@ -36,7 +36,14 @@ import { addAttributesToElement, } from '../../../../src/dom'; import {installDocService} from '../../../../src/service/ampdoc-impl'; -import {toggleExperiment} from '../../../../src/experiments'; +import { + toggleExperiment, + forceExperimentBranch, +} from '../../../../src/experiments'; +import { + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches, +} from '../../../../ads/google/adsense-amp-auto-ads'; function createAdsenseImplElement(attributes, opt_doc, opt_tag) { const doc = opt_doc || document; @@ -494,9 +501,11 @@ describes.sandboxed('amp-ad-network-adsense-impl', {}, () => { resetSharedState(); }); - afterEach(() => - toggleExperiment(window, 'as-use-attr-for-format', false)); - + afterEach(() => { + toggleExperiment(window, 'as-use-attr-for-format', false); + toggleExperiment( + window, 'ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME', false); + }); it('formats client properly', () => { element.setAttribute('data-ad-client', 'SoMeClient'); @@ -560,6 +569,28 @@ describes.sandboxed('amp-ad-network-adsense-impl', {}, () => { // Ensure that "auto" doesn't appear anywhere here: expect(url).to.match(/format=[0-9]+x[0-9]+&w=[0-9]+&h=[0-9]+/)); }); + it('includes eid when in amp-auto-ads holdout control', () => { + forceExperimentBranch(window, + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches.CONTROL); + new AmpAd(element).upgradeCallback(); + impl.onLayoutMeasure(); + return impl.getAdUrl().then(url => { + expect(url).to.match(new RegExp( + `eid=[^&]*${AdSenseAmpAutoAdsHoldoutBranches.CONTROL}`)); + }); + }); + it('includes eid when in amp-auto-ads holdout experiment', () => { + forceExperimentBranch(window, + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches.EXPERIMENT); + new AmpAd(element).upgradeCallback(); + impl.onLayoutMeasure(); + return impl.getAdUrl().then(url => { + expect(url).to.match(new RegExp( + `eid=[^&]*${AdSenseAmpAutoAdsHoldoutBranches.EXPERIMENT}`)); + }); + }); }); describe('#unlayoutCallback', () => { diff --git a/extensions/amp-auto-ads/0.1/ad-network-config.js b/extensions/amp-auto-ads/0.1/ad-network-config.js index 67b500824953..c16794cb77c1 100644 --- a/extensions/amp-auto-ads/0.1/ad-network-config.js +++ b/extensions/amp-auto-ads/0.1/ad-network-config.js @@ -14,6 +14,10 @@ * limitations under the License. */ +import { + AdSenseAmpAutoAdsHoldoutBranches, + getAdSenseAmpAutoAdsExpBranch, +} from '../../../ads/google/adsense-amp-auto-ads'; import {buildUrl} from '../../../ads/google/a4a/url-builder'; import {documentInfoForDoc} from '../../../src/services'; import {parseUrl} from '../../../src/url'; @@ -27,6 +31,13 @@ import {viewportForDoc} from '../../../src/services'; */ class AdNetworkConfigDef { + /** + * Indicates whether amp-auto-ads should be enabled on this pageview. + * @param {!Window} unusedWin + * @return {boolean} true if amp-auto-ads should be enabled on this pageview. + */ + isEnabled(unusedWin) {} + /** * @return {string} */ @@ -70,6 +81,14 @@ class AdSenseNetworkConfig { this.autoAmpAdsElement_ = autoAmpAdsElement; } + /** + * @param {!Window} win + */ + isEnabled(win) { + const branch = getAdSenseAmpAutoAdsExpBranch(win); + return branch != AdSenseAmpAutoAdsHoldoutBranches.CONTROL; + } + /** @override */ getConfigUrl() { const docInfo = documentInfoForDoc(this.autoAmpAdsElement_); diff --git a/extensions/amp-auto-ads/0.1/amp-auto-ads.js b/extensions/amp-auto-ads/0.1/amp-auto-ads.js index bd2242c92daf..c47b8f6c9d55 100644 --- a/extensions/amp-auto-ads/0.1/amp-auto-ads.js +++ b/extensions/amp-auto-ads/0.1/amp-auto-ads.js @@ -39,6 +39,10 @@ export class AmpAutoAds extends AMP.BaseElement { const adNetwork = getAdNetworkConfig(type, this.element); user().assert(adNetwork, 'No AdNetworkConfig for type: ' + type); + if (!adNetwork.isEnabled(this.win)) { + return; + } + this.getConfig_(adNetwork.getConfigUrl()).then(configObj => { if (!configObj) { return; diff --git a/extensions/amp-auto-ads/0.1/test/test-ad-network-config.js b/extensions/amp-auto-ads/0.1/test/test-ad-network-config.js index 79ddc4aaac62..5ca2b169e442 100644 --- a/extensions/amp-auto-ads/0.1/test/test-ad-network-config.js +++ b/extensions/amp-auto-ads/0.1/test/test-ad-network-config.js @@ -15,7 +15,15 @@ */ import {getAdNetworkConfig} from '../ad-network-config'; +import { + toggleExperiment, + forceExperimentBranch, +} from '../../../../src/experiments'; import {viewportForDoc} from '../../../../src/services'; +import { + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches, +} from '../../../../ads/google/adsense-amp-auto-ads'; describes.realWin('ad-network-config', { amp: { @@ -46,6 +54,31 @@ describes.realWin('ad-network-config', { ampAutoAdsElem.setAttribute('data-ad-client', AD_CLIENT); }); + it('should report enabled when holdout experiment not on', () => { + toggleExperiment( + env.win, ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, false); + const adNetwork = getAdNetworkConfig('adsense', ampAutoAdsElem); + expect(adNetwork.isEnabled(env.win)).to.equal(true); + }); + + it('should report enabled when holdout experiment on and experiment ' + + 'branch picked', () => { + forceExperimentBranch(env.win, + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches.EXPERIMENT); + const adNetwork = getAdNetworkConfig('adsense', ampAutoAdsElem); + expect(adNetwork.isEnabled(env.win)).to.equal(true); + }); + + it('should report disabled when holdout experiment on and control ' + + 'branch picked', () => { + forceExperimentBranch(env.win, + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches.CONTROL); + const adNetwork = getAdNetworkConfig('adsense', ampAutoAdsElem); + expect(adNetwork.isEnabled(env.win)).to.equal(false); + }); + it('should generate the config fetch URL', () => { const adNetwork = getAdNetworkConfig('adsense', ampAutoAdsElem); expect(adNetwork.getConfigUrl()).to.equal( diff --git a/extensions/amp-auto-ads/0.1/test/test-amp-auto-ads.js b/extensions/amp-auto-ads/0.1/test/test-amp-auto-ads.js index 3aa61410e3f8..13098dd082ec 100644 --- a/extensions/amp-auto-ads/0.1/test/test-amp-auto-ads.js +++ b/extensions/amp-auto-ads/0.1/test/test-amp-auto-ads.js @@ -15,10 +15,17 @@ */ import {AmpAutoAds} from '../amp-auto-ads'; -import {toggleExperiment} from '../../../../src/experiments'; +import { + toggleExperiment, + forceExperimentBranch, +} from '../../../../src/experiments'; import {xhrFor} from '../../../../src/services'; import {waitForChild} from '../../../../src/dom'; import {viewportForDoc} from '../../../../src/services'; +import { + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches, +} from '../../../../ads/google/adsense-amp-auto-ads'; describes.realWin('amp-auto-ads', { amp: { @@ -162,6 +169,51 @@ describes.realWin('amp-auto-ads', { }); }); + it('should insert ads on the page when in holdout experiment branch', () => { + forceExperimentBranch(env.win, + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches.EXPERIMENT); + + ampAutoAdsElem.setAttribute('data-ad-client', AD_CLIENT); + ampAutoAdsElem.setAttribute('type', 'adsense'); + ampAutoAds.buildCallback(); + + return new Promise(resolve => { + waitForChild(anchor4, parent => { + return parent.childNodes.length > 0; + }, () => { + expect(anchor1.childNodes).to.have.lengthOf(1); + expect(anchor2.childNodes).to.have.lengthOf(1); + expect(anchor3.childNodes).to.have.lengthOf(0); + expect(anchor4.childNodes).to.have.lengthOf(1); + verifyAdElement(anchor1.childNodes[0]); + verifyAdElement(anchor2.childNodes[0]); + verifyAdElement(anchor4.childNodes[0]); + resolve(); + }); + }); + }); + + it('should not insert ads on the page when in holdout control branch', () => { + forceExperimentBranch(env.win, + ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, + AdSenseAmpAutoAdsHoldoutBranches.CONTROL); + + ampAutoAdsElem.setAttribute('data-ad-client', AD_CLIENT); + ampAutoAdsElem.setAttribute('type', 'adsense'); + ampAutoAds.buildCallback(); + + return new Promise(resolve => { + setTimeout(() => { + expect(anchor1.childNodes).to.have.lengthOf(0); + expect(anchor2.childNodes).to.have.lengthOf(0); + expect(anchor3.childNodes).to.have.lengthOf(0); + expect(anchor4.childNodes).to.have.lengthOf(0); + resolve(); + }, 500); + }); + }); + it('should insert ads with the config provided attributes', () => { configObj.attributes = { 'bad-name': 'should be filtered', diff --git a/tools/experiments/experiments.js b/tools/experiments/experiments.js index eff5a1412084..7d6db0ad5f0e 100644 --- a/tools/experiments/experiments.js +++ b/tools/experiments/experiments.js @@ -92,6 +92,12 @@ const EXPERIMENTS = [ spec: 'https://github.com/ampproject/amphtml/issues/6196', cleanupIssue: 'https://github.com/ampproject/amphtml/issues/6217', }, + { + id: 'amp-auto-ads-adsense-holdout', + name: 'AMP Auto Ads AdSense Holdout', + spec: 'https://github.com/ampproject/amphtml/issues/6196', + cleanupIssue: 'https://github.com/ampproject/amphtml/issues/9247', + }, { id: 'amp-google-vrview-image', name: 'AMP VR Viewer for images via Google VRView',