Skip to content

Commit

Permalink
Cleanup adsense amp-auto-ads holdout experiment. Close #9247. (#20774)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
nbeloglazov authored and keithwrightbos committed Feb 13, 2019
1 parent 4f3cd0d commit 6831b63
Show file tree
Hide file tree
Showing 8 changed files with 3 additions and 222 deletions.
63 changes: 0 additions & 63 deletions ads/google/adsense-amp-auto-ads.js

This file was deleted.

83 changes: 0 additions & 83 deletions ads/google/test/test-adsense-amp-auto-ads.js

This file was deleted.

1 change: 0 additions & 1 deletion build-system/dep-check-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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');
Expand Down
13 changes: 3 additions & 10 deletions extensions/amp-auto-ads/0.1/ad-network-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@
* limitations under the License.
*/

import {
AdSenseAmpAutoAdsHoldoutBranches,
getAdSenseAmpAutoAdsExpBranch,
} from '../../../ads/google/adsense-amp-auto-ads';
import {
AdSenseAmpAutoAdsResponsiveBranches,
getAdSenseAmpAutoAdsResponsiveExperimentBranch,
Expand Down Expand Up @@ -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;
}

/**
Expand Down
29 changes: 0 additions & 29 deletions extensions/amp-auto-ads/0.1/test/test-ad-network-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down
6 changes: 0 additions & 6 deletions tools/experiments/experiments.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down

0 comments on commit 6831b63

Please sign in to comment.