address comment

ampproject · Jun 19, 2019 · 2451d65 · 2451d65
1 parent 3718e59
commit 2451d65
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 8 deletions.
diff --git a/build-system/amp4test.js b/build-system/amp4test.js
@@ -75,6 +75,7 @@ app.use('/compose-doc', function(req, res) {
     head,
     spec,
   });
+  res.cookie('test-cookie', 'test');
   res.send(doc);
 });
 
@@ -186,11 +187,7 @@ app.use('/request-bank/:bid/teardown/', (req, res) => {
  */
 app.get('/a4a/:bid', (req, res) => {
   const {bid} = req.params;
-  const preSet = `
-  <script>{
-    document.cookie = 'test-cookie=test';
-  }</script>`;
-  const body = preSet + `
+  const body = `
   <a href=https://ampbyexample.com target=_blank>
     <amp-img alt="AMP Ad" height=250 src=//localhost:9876/amp4test/request-bank/${bid}/deposit/image width=300></amp-img>
   </a>
@@ -216,7 +213,7 @@ app.get('/a4a/:bid', (req, res) => {
             "navType": "\${navType}",
             "navRedirectCount": "\${navRedirectCount}",
             "sourceUrl": "\${sourceUrl}",
-            "cookie": "COOKIE(test-cookie)"
+            "cookie": "\${cookie(test-cookie)}"
           }
         }
       }
@@ -230,6 +227,7 @@ app.get('/a4a/:bid', (req, res) => {
     css: 'body { background-color: #f4f4f4; }',
     extensions: ['amp-analytics'],
   });
+  res.cookie('test-cookie', 'test');
   res.send(doc);
 });
 

diff --git a/extensions/amp-analytics/0.1/vendors.js b/extensions/amp-analytics/0.1/vendors.js
@@ -130,6 +130,7 @@ export const ANALYTICS_CONFIG = /** @type {!JsonObject} */ ({
       'clientId': 'CLIENT_ID',
       'consentState': 'CONSENT_STATE',
       'contentLoadTime': 'CONTENT_LOAD_TIME',
+      'cookie': 'COOKIE',
       'counter': 'COUNTER',
       'documentCharset': 'DOCUMENT_CHARSET',
       'documentReferrer': 'DOCUMENT_REFERRER',

diff --git a/test/integration/test-amp-analytics.js b/test/integration/test-amp-analytics.js
@@ -47,7 +47,8 @@ describe('amp-analytics', function() {
             "a": 1,
             "b": "\${title}",
             "cid": "\${clientId(_cid)}",
-            "loadend": "\${navTiming(loadEventEnd)}"
+            "loadend": "\${navTiming(loadEventEnd)}",
+            "cookie": "\${cookie(test-cookie)}"
           }
         }
         </script>
@@ -67,6 +68,7 @@ describe('amp-analytics', function() {
           expect(q['b']).to.equal('AMP TEST');
           expect(q['cid']).to.equal('amp-12345');
           expect(q['loadend']).to.not.equal('0');
+          expect(q['cookie']).to.equal('test');
           expect(
             req.headers.referer,
             'should keep referrer if no referrerpolicy specified'

diff --git a/test/integration/test-amp-inabox.js b/test/integration/test-amp-inabox.js
@@ -90,6 +90,9 @@ describe
         const queries = parseQueryString(req.url.substr('/bar'.length));
         expect(queries['cid']).to.equal('');
         expect(queries['sourceUrl']).be.ok;
+        // Cookie is sent via http response header when requesting
+        // localhost:9876/amp4test/a4a/
+        // COOKIE macro is not allowed in inabox and resolves to empty
         expect(queries['cookie']).to.equal('');
       });
       return Promise.all([imgPromise, pixelPromise, analyticsPromise]);
@@ -144,7 +147,6 @@ describe
         env => {
           it('should layout amp-img, amp-pixel, amp-analytics', () => {
             // See amp4test.js for creative content
-            debugger;
             return testAmpComponents();
           });