From 9d15c94522e98aab98cdd9b268bc120d62165521 Mon Sep 17 00:00:00 2001 From: Micajuine Ho Date: Thu, 23 Apr 2020 10:13:37 -0700 Subject: [PATCH] Adding warning --- .../amp-analytics/0.1/analytics-root.js | 52 +++++++++-------- .../0.1/test/test-analytics-root.js | 28 +++++++--- .../amp-analytics/0.1/test/test-events.js | 56 +------------------ 3 files changed, 51 insertions(+), 85 deletions(-) diff --git a/extensions/amp-analytics/0.1/analytics-root.js b/extensions/amp-analytics/0.1/analytics-root.js index e12e66bb614e..47cb63bcab6d 100644 --- a/extensions/amp-analytics/0.1/analytics-root.js +++ b/extensions/amp-analytics/0.1/analytics-root.js @@ -301,11 +301,8 @@ export class AnalyticsRoot { userAssert(false, `Invalid query selector ${selector}`); } for (let j = 0; j < nodeList.length; j++) { - if ( - this.contains(nodeList[j]) && - this.elementContainsDataVarsAttribute_(nodeList[j]) - ) { - elementArray.push(nodeList[j]); + if (this.contains(nodeList[j])) { + this.checkElementDataVars(nodeList[j], elementArray, selector); } } userAssert(elementArray.length, `Element "${selector}" not found`); @@ -319,6 +316,33 @@ export class AnalyticsRoot { }); } + /** + * Check if the given element has a data-vars attribute. + * Warn if not the case. + * @param {!Element} element + * @param {!Array} elementArray + * @param {string} selector + */ + checkElementDataVars(element, elementArray, selector) { + const dataVarKeys = Object.keys( + getDataParamsFromAttributes( + element, + /* computeParamNameFunc */ undefined, + VARIABLE_DATA_ATTRIBUTE_KEY + ) + ); + if (dataVarKeys.length) { + elementArray.push(element); + } else { + user().warn( + TAG, + 'An element was ommited from selector "%s"' + + ' because no data-vars-* attribute was found.', + selector + ); + } + } + /** * Searches the AMP element that matches the selector within the scope of the * analytics root in relationship to the specified context node. @@ -384,24 +408,6 @@ export class AnalyticsRoot { } } - /** - * Ensure that the element has a data-vars attribute for - * unique requests. - * @param {!Element} element - * @return {boolean} - */ - elementContainsDataVarsAttribute_(element) { - return ( - Object.keys( - getDataParamsFromAttributes( - element, - /* computeParamNameFunc */ undefined, - VARIABLE_DATA_ATTRIBUTE_KEY - ) - ).length !== 0 - ); - } - /** * Creates listener-filter for DOM events to check against the specified * selector. If the node (or its ancestors) match the selector the listener diff --git a/extensions/amp-analytics/0.1/test/test-analytics-root.js b/extensions/amp-analytics/0.1/test/test-analytics-root.js index 954c3b6ab549..4eadbb62b312 100644 --- a/extensions/amp-analytics/0.1/test/test-analytics-root.js +++ b/extensions/amp-analytics/0.1/test/test-analytics-root.js @@ -26,6 +26,7 @@ import { } from '../visibility-manager'; import {VisibilityManagerForMApp} from '../visibility-manager-for-mapp'; import {toggleExperiment} from '../../../../src/experiments'; +import {user} from '../../../../src/log'; describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => { let win; @@ -391,16 +392,21 @@ describes.realWin('AmpdocAnalyticsRoot', {amp: 1}, (env) => { }); it('should only find elements with data-vars-*', async () => { + const spy = env.sandbox.spy(user(), 'warn'); + child.classList.add('myClass'); child2.classList.add('myClass'); child3.classList.add('myClass'); child3.removeAttribute('data-vars-id'); - - expect(await root.getAmpElements(body, ['.myClass'])).to.deep.equal([ - child, - child2, - ]); + const children = await root.getAmpElements(body, ['.myClass']); + expect(spy).callCount(1); + expect(spy).to.have.been.calledWith( + 'amp-analytics/analytics-root', + 'An element was ommited from selector "%s" because no data-vars-* attribute was found.', + '.myClass' + ); + expect(children).to.deep.equal([child, child2]); }); it('should remove duplicate elements found', async () => { @@ -756,6 +762,7 @@ describes.realWin( }); it('should find all elements by selector', async () => { + const spy = env.sandbox.spy(user(), 'warn'); const child2 = win.document.createElement('child'); const child3 = win.document.createElement('child'); const child4 = win.document.createElement('child'); @@ -778,9 +785,14 @@ describes.realWin( child2.setAttribute('data-vars-id', '456'); child3.setAttribute('data-vars-id', '789'); parentChild.setAttribute('data-vars-id', 'abc'); - expect( - await root.getAmpElements(body, ['.myClass'], null) - ).to.deep.equals([child, child2]); + const elements = await root.getAmpElements(body, ['.myClass'], null); + expect(spy).callCount(1); + expect(spy).to.have.been.calledWith( + 'amp-analytics/analytics-root', + 'An element was ommited from selector "%s" because no data-vars-* attribute was found.', + '.myClass' + ); + expect(elements).to.deep.equals([child, child2]); // Check that non-experiment version works toggleExperiment(win, 'visibility-trigger-improvements', false); expect( diff --git a/extensions/amp-analytics/0.1/test/test-events.js b/extensions/amp-analytics/0.1/test/test-events.js index f23b739a74c2..a95c8de2c6cf 100644 --- a/extensions/amp-analytics/0.1/test/test-events.js +++ b/extensions/amp-analytics/0.1/test/test-events.js @@ -1944,14 +1944,10 @@ describes.realWin('Events', {amp: 1}, (env) => { describe('multi selector visibility trigger', () => { let unlisten; let unlisten2; - let unlisten3; - let target3; let config; let readyPromise; let targetSignals2; - let targetSignals3; let saveCallback2; - let saveCallback3; let eventsSpy; let res; let error; @@ -1961,27 +1957,14 @@ describes.realWin('Events', {amp: 1}, (env) => { readyPromise = Promise.resolve(); unlisten = env.sandbox.spy(); unlisten2 = env.sandbox.spy(); - unlisten3 = env.sandbox.spy(); config = {}; eventsSpy = env.sandbox.spy(tracker, 'onEvent_'); - target3 = win.document.createElement('div'); - target3.classList.add('target2'); - win.document.body.appendChild(target3); - target2.classList.add('i-amphtml-element'); targetSignals2 = new Signals(); target2.signals = () => targetSignals2; - target3.classList.add('i-amphtml-element'); - targetSignals3 = new Signals(); - target3.signals = () => targetSignals3; - - target.setAttribute('data-vars-id', '123'); - target2.setAttribute('data-vars-id', '456'); - target3.setAttribute('data-vars-id', '789'); - saveCallback2 = env.sandbox.match((arg) => { if (typeof arg == 'function') { saveCallback2.callback = arg; @@ -1989,25 +1972,18 @@ describes.realWin('Events', {amp: 1}, (env) => { } return false; }); - saveCallback3 = env.sandbox.match((arg) => { - if (typeof arg == 'function') { - saveCallback3.callback = arg; - return true; - } - return false; - }); }); afterEach(async () => { if (!error) { - [unlisten, unlisten2, unlisten3].forEach((value) => { + [unlisten, unlisten2].forEach((value) => { if (value) { expect(value).to.not.be.called; } }); expect(res).to.be.a('function'); await res(); - [unlisten, unlisten2, unlisten3].forEach((value) => { + [unlisten, unlisten2].forEach((value) => { if (value) { expect(value).to.be.calledOnce; } @@ -2032,11 +2008,6 @@ describes.realWin('Events', {amp: 1}, (env) => { .withExactArgs('ini-load', target2) .returns(readyPromise) .once(); - iniLoadTrackerMock - .expects('getElementSignal') - .withExactArgs('ini-load', target3) - .returns(readyPromise) - .once(); visibilityManagerMock .expects('listenElement') .withExactArgs( @@ -2059,32 +2030,18 @@ describes.realWin('Events', {amp: 1}, (env) => { ) .returns(unlisten2) .once(); - visibilityManagerMock - .expects('listenElement') - .withExactArgs( - target3, - config.visibilitySpec, - readyPromise, - /* createReportReadyPromiseFunc */ null, - saveCallback3 - ) - .returns(unlisten3) - .once(); // Dispose function res = tracker.add(analyticsElement, 'visible', config, eventResolver); const unlistenReady = getAmpElementSpy.returnValues[0]; const unlistenReady2 = getAmpElementSpy.returnValues[1]; - const unlistenReady3 = getAmpElementSpy.returnValues[2]; // #getAmpElement Promise await unlistenReady; await unlistenReady2; - await unlistenReady3; // #assertMeasurable_ Promise await macroTask(); await macroTask(); saveCallback.callback({totalVisibleTime: 10}); saveCallback2.callback({totalVisibleTime: 15}); - saveCallback3.callback({totalVisibleTime: 20}); // Testing that visibilty manager mock sends state to onEvent_ expect(eventsSpy.getCall(0).args[0]).to.equal('visible'); @@ -2092,21 +2049,12 @@ describes.realWin('Events', {amp: 1}, (env) => { expect(eventsSpy.getCall(0).args[2]).to.equal(target); expect(eventsSpy.getCall(0).args[3]).to.deep.equal({ totalVisibleTime: 10, - id: '123', }); expect(eventsSpy.getCall(1).args[0]).to.equal('visible'); expect(eventsSpy.getCall(1).args[1]).to.equal(eventResolver); expect(eventsSpy.getCall(1).args[2]).to.equal(target2); expect(eventsSpy.getCall(1).args[3]).to.deep.equal({ totalVisibleTime: 15, - id: '456', - }); - expect(eventsSpy.getCall(2).args[0]).to.equal('visible'); - expect(eventsSpy.getCall(2).args[1]).to.equal(eventResolver); - expect(eventsSpy.getCall(2).args[2]).to.equal(target3); - expect(eventsSpy.getCall(2).args[3]).to.deep.equal({ - totalVisibleTime: 20, - id: '789', }); });