From 6831b637c92fe5666a7226576fbf9237ea0700b8 Mon Sep 17 00:00:00 2001 From: Nikita Beloglazov Date: Wed, 13 Feb 2019 12:37:10 -0800 Subject: [PATCH] Cleanup adsense amp-auto-ads holdout experiment. Close #9247. (#20774) It has been running for a while and we no longer need it. If someone decides to look at results in future - they can rollback this commit and restart experiment. Tested: presubmit --- ads/google/adsense-amp-auto-ads.js | 63 -------------- 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 | 7 -- .../test/test-amp-ad-network-adsense-impl.js | 23 ----- .../amp-auto-ads/0.1/ad-network-config.js | 13 +-- .../0.1/test/test-ad-network-config.js | 29 ------- tools/experiments/experiments.js | 6 -- 8 files changed, 3 insertions(+), 222 deletions(-) delete mode 100644 ads/google/adsense-amp-auto-ads.js delete mode 100644 ads/google/test/test-adsense-amp-auto-ads.js diff --git a/ads/google/adsense-amp-auto-ads.js b/ads/google/adsense-amp-auto-ads.js deleted file mode 100644 index 30fb59389e72..000000000000 --- a/ads/google/adsense-amp-auto-ads.js +++ /dev/null @@ -1,63 +0,0 @@ -/** - * 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 { - ExperimentInfo, // eslint-disable-line no-unused-vars - getExperimentBranch, - randomlySelectUnsetExperiments, -} 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 = /** @type {!Object} */ ({}); - 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 deleted file mode 100644 index a03f25ae5f71..000000000000 --- a/ads/google/test/test-adsense-amp-auto-ads.js +++ /dev/null @@ -1,83 +0,0 @@ -/** - * 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 665f0f52a4b5..80d5c8153582 100644 --- a/build-system/dep-check-config.js +++ b/build-system/dep-check-config.js @@ -156,7 +156,6 @@ exports.rules = [ 'ads/**->src/string.js', 'ads/**->src/style.js', 'ads/**->src/consent-state.js', - 'ads/google/adsense-amp-auto-ads.js->src/experiments.js', 'ads/google/adsense-amp-auto-ads-responsive.js->src/experiments.js', 'ads/google/doubleclick.js->src/experiments.js', // ads/google/a4a doesn't contain 3P ad code and should probably move 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 cfe636eb6055..fcc5519e2771 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 @@ -59,9 +59,6 @@ import { } from '../../../src/style'; import {dev, devAssert, user} from '../../../src/log'; import {domFingerprintPlain} from '../../../src/utils/dom-fingerprint'; -import { - getAdSenseAmpAutoAdsExpBranch, -} from '../../../ads/google/adsense-amp-auto-ads'; import { getAdSenseAmpAutoAdsResponsiveExperimentBranch, } from '../../../ads/google/adsense-amp-auto-ads-responsive'; @@ -374,12 +371,8 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A { }; const experimentIds = []; - const ampAutoAdsBranch = getAdSenseAmpAutoAdsExpBranch(this.win); const ampAutoAdsResponsiveBranch = getAdSenseAmpAutoAdsResponsiveExperimentBranch(this.win); - if (ampAutoAdsBranch) { - experimentIds.push(ampAutoAdsBranch); - } if (ampAutoAdsResponsiveBranch) { experimentIds.push(ampAutoAdsResponsiveBranch); } 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 0cdc4593bc82..cf7519306b6a 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 @@ -19,10 +19,6 @@ // always available for them. However, when we test an impl in isolation, // AmpAd is not loaded already, so we need to load it separately. import '../../../amp-ad/0.1/amp-ad'; -import { - ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, - AdSenseAmpAutoAdsHoldoutBranches, -} from '../../../../ads/google/adsense-amp-auto-ads'; import {AmpA4A} from '../../../amp-a4a/0.1/amp-a4a'; import {AmpAd} from '../../../amp-ad/0.1/amp-ad'; import { @@ -535,25 +531,6 @@ describes.realWin('amp-ad-network-adsense-impl', { return impl.getAdUrl().then( url => expect(url).to.match(/eid=[^&]*21062003/)); }); - it('includes eid when in amp-auto-ads holdout control', () => { - forceExperimentBranch(impl.win, - ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, - AdSenseAmpAutoAdsHoldoutBranches.CONTROL); - impl.divertExperiments(); - 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(impl.win, - ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, - AdSenseAmpAutoAdsHoldoutBranches.EXPERIMENT); - return impl.getAdUrl().then(url => { - expect(url).to.match(new RegExp( - `eid=[^&]*${AdSenseAmpAutoAdsHoldoutBranches.EXPERIMENT}`)); - }); - }); it('returns the right URL', () => { element.setAttribute('data-ad-slot', 'some_slot'); element.setAttribute('data-language', 'lxz'); 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 27f7ffd12e88..6dea755e9dc6 100644 --- a/extensions/amp-auto-ads/0.1/ad-network-config.js +++ b/extensions/amp-auto-ads/0.1/ad-network-config.js @@ -14,10 +14,6 @@ * limitations under the License. */ -import { - AdSenseAmpAutoAdsHoldoutBranches, - getAdSenseAmpAutoAdsExpBranch, -} from '../../../ads/google/adsense-amp-auto-ads'; import { AdSenseAmpAutoAdsResponsiveBranches, getAdSenseAmpAutoAdsResponsiveExperimentBranch, @@ -169,12 +165,9 @@ class AdSenseNetworkConfig { this.autoAmpAdsElement_ = autoAmpAdsElement; } - /** - * @param {!Window} win - */ - isEnabled(win) { - const branch = getAdSenseAmpAutoAdsExpBranch(win); - return branch != AdSenseAmpAutoAdsHoldoutBranches.CONTROL; + /** @override */ + isEnabled() { + return true; } /** 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 b748bf8dd656..ebb728aef372 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 @@ -14,10 +14,6 @@ * limitations under the License. */ -import { - ADSENSE_AMP_AUTO_ADS_HOLDOUT_EXPERIMENT_NAME, - AdSenseAmpAutoAdsHoldoutBranches, -} from '../../../../ads/google/adsense-amp-auto-ads'; import { ADSENSE_AMP_AUTO_ADS_RESPONSIVE_EXPERIMENT_NAME, AdSenseAmpAutoAdsResponsiveBranches, @@ -58,31 +54,6 @@ 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/tools/experiments/experiments.js b/tools/experiments/experiments.js index 5c15577cf10a..17b683d6000a 100644 --- a/tools/experiments/experiments.js +++ b/tools/experiments/experiments.js @@ -104,12 +104,6 @@ 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-auto-ads-adsense-responsive', name: 'AMP Auto Ads AdSense Responsive',