From 441e01c56800bd96d5243388b40eec06f0f3ade5 Mon Sep 17 00:00:00 2001
From: Peter Bacon Darwin <pete@bacondarwin.com>
Date: Thu, 19 Oct 2017 13:58:27 +0100
Subject: [PATCH] fix(aio): simplify GaService to avoid e2e test failures

The GaService and the E2E specs were unnecessarily complicated and had
arbitrary async timeouts to ensure that the interplay between the GA
library code and the rest of the app worked correctly. This resulted
in potential flaky tests if the timeouts were not adequate; this was
experienced when Travis upgraded to Chrome 62.

The new approach is to block loading of the Analytics library altogether
if there is a `__e2e__` flag set in the `SessionStorage` of the browser.
It doesn't appear to be enough just to set the flag directly on the
window. I think this is because the window gets cleaned when navigation
occurs (but I am not certain).

The downside of this is that we had to add a small piece of extra logic
to the GA snippet in index.html; and we also had to switch from using
`<script async ...>` to a programmatic approach to loading the GA library
which prevents the browser from eagerly fetching the library. This may
be mitigated by adding it to the HTTP/2 push configuration of the Firebase
hosting.

Re-enables the test that was disabled in https://github.com/angular/angular/pull/19784

Closes #19785
---
 aio/e2e/app.e2e-spec.ts               |  31 +++--
 aio/e2e/app.po.ts                     |  35 +----
 aio/src/app/app.module.ts             |   4 +-
 aio/src/app/shared/ga.service.spec.ts | 187 ++++++++++----------------
 aio/src/app/shared/ga.service.ts      |  35 +----
 aio/src/app/shared/window.ts          |   4 +
 aio/src/index.html                    |   8 +-
 7 files changed, 112 insertions(+), 192 deletions(-)
 create mode 100644 aio/src/app/shared/window.ts

diff --git a/aio/e2e/app.e2e-spec.ts b/aio/e2e/app.e2e-spec.ts
index c60c6022b9..15e4ad9f71 100644
--- a/aio/e2e/app.e2e-spec.ts
+++ b/aio/e2e/app.e2e-spec.ts
@@ -68,30 +68,33 @@ describe('site App', function() {
 
   // TODO(https://github.com/angular/angular/issues/19785): Activate this again
   // once it is no more flaky.
-  xdescribe('google analytics', () => {
-    beforeEach(done => page.gaReady.then(done));
-
-    it('should call ga', done => {
-      page.ga()
-        .then(calls => {
-          expect(calls.length).toBeGreaterThan(2, 'ga calls');
-          done();
-        });
-    });
+  describe('google analytics', () => {
 
     it('should call ga with initial URL', done => {
       let path: string;
-
+      page.navigateTo('api');
       page.locationPath()
         .then(p => path = p)
         .then(() => page.ga().then(calls => {
-          expect(calls.length).toBeGreaterThan(2, 'ga calls');
-          expect(calls[1]).toEqual(['set', 'page', path]);
+          // The last call (length-1) will be the `send` command
+          // The second to last call (length-2) will be the command to `set` the page url
+          expect(calls[calls.length - 2]).toEqual(['set', 'page', path]);
           done();
         }));
     });
 
-    // Todo: add test to confirm tracking URL when navigate.
+    it('should call ga with new URL on navigation', done => {
+      let path: string;
+      page.getLink('features').click();
+      page.locationPath()
+        .then(p => path = p)
+        .then(() => page.ga().then(calls => {
+          // The last call (length-1) will be the `send` command
+          // The second to last call (length-2) will be the command to `set` the page url
+          expect(calls[calls.length - 2]).toEqual(['set', 'page', path]);
+          done();
+        }));
+    });
   });
 
   describe('search', () => {
diff --git a/aio/e2e/app.po.ts b/aio/e2e/app.po.ts
index cf71d51d61..8e1d906e1b 100644
--- a/aio/e2e/app.po.ts
+++ b/aio/e2e/app.po.ts
@@ -12,7 +12,6 @@ export class SitePage {
     .all(by.css('a'))
     .filter((a: ElementFinder) => a.getAttribute('href').then(href => githubRegex.test(href)))
     .first();
-  gaReady: promise.Promise<any>;
 
   static setWindowWidth(newWidth: number) {
     const win = browser.driver.manage().window();
@@ -25,11 +24,14 @@ export class SitePage {
                   .first();
   }
   getLink(path) { return element(by.css(`a[href="${path}"]`)); }
-  ga() { return browser.executeScript('return window["gaCalls"]') as promise.Promise<any[][]>; }
+  ga() { return browser.executeScript('return window["ga"].q') as promise.Promise<any[][]>; }
   locationPath() { return browser.executeScript('return document.location.pathname') as promise.Promise<string>; }
 
   navigateTo(pageUrl = '') {
-    return browser.get('/' + pageUrl).then(_ => this.replaceGa(_));
+    return browser.get('/' + pageUrl)
+      // We need to tell the index.html not to load the real analytics library
+      // See the GA snippet in index.html
+      .then(() => browser.driver.executeScript('sessionStorage.setItem("__e2e__", true);'));
   }
 
   getDocViewerText() {
@@ -61,31 +63,4 @@ export class SitePage {
     browser.wait(ExpectedConditions.presenceOf(results.first()), 8000);
     return results;
   }
-
-  /**
-   * Replace the ambient Google Analytics tracker with homebrew spy
-   * don't send commands to GA during e2e testing!
-   * @param _ - forward's anything passed in
-   */
-  private replaceGa(_: any) {
-
-    this.gaReady = browser.driver.executeScript(() => {
-      // Give ga() a "ready" callback:
-      // https://developers.google.com/analytics/devguides/collection/analyticsjs/command-queue-reference
-      window['ga'](() => {
-        window['gaCalls'] = [];
-        window['ga'] = function() { window['gaCalls'].push(arguments); };
-      });
-
-    })
-    .then(() => {
-      // wait for GaService to start using window.ga after analytics lib loads.
-      const d =  promise.defer();
-      setTimeout(() => d.fulfill(), 1000); // GaService.initializeDelay
-      return d.promise;
-    });
-
-    return _;
-  }
 }
-
diff --git a/aio/src/app/app.module.ts b/aio/src/app/app.module.ts
index 52815c6511..671ef5f10c 100644
--- a/aio/src/app/app.module.ts
+++ b/aio/src/app/app.module.ts
@@ -45,6 +45,7 @@ import { ScrollService } from 'app/shared/scroll.service';
 import { ScrollSpyService } from 'app/shared/scroll-spy.service';
 import { SearchBoxComponent } from './search/search-box/search-box.component';
 import { TocService } from 'app/shared/toc.service';
+import { WindowToken, windowProvider } from 'app/shared/window';
 
 import { SharedModule } from 'app/shared/shared.module';
 
@@ -113,7 +114,8 @@ export const svgIconProviders = [
     ScrollSpyService,
     SearchService,
     svgIconProviders,
-    TocService
+    TocService,
+    { provide: WindowToken, useFactory: windowProvider },
   ],
   bootstrap: [AppComponent]
 })
diff --git a/aio/src/app/shared/ga.service.spec.ts b/aio/src/app/shared/ga.service.spec.ts
index 5281142ced..be3c80cc59 100644
--- a/aio/src/app/shared/ga.service.spec.ts
+++ b/aio/src/app/shared/ga.service.spec.ts
@@ -1,130 +1,87 @@
 import { ReflectiveInjector } from '@angular/core';
-import { fakeAsync, tick } from '@angular/core/testing';
 
 import { GaService } from 'app/shared/ga.service';
-import { Logger } from 'app/shared/logger.service';
+import { WindowToken } from 'app/shared/window';
 
 describe('GaService', () => {
-  let gaSpy: jasmine.Spy;
+  let gaService: GaService;
   let injector: ReflectiveInjector;
-
-  // filter for 'send' which communicates with server
-  // returns the url of the 'send pageview'
-  function gaSpySendCalls() {
-    let lastUrl: string;
-    return gaSpy.calls.all()
-      .reduce((acc, c) =>  {
-        const args = c.args;
-        if (args[0] === 'set') {
-          lastUrl = args[2];
-        } else if (args[0] === 'send') {
-          acc.push(lastUrl);
-        }
-        return acc;
-      }, []);
-  }
+  let gaSpy: jasmine.Spy;
+  let mockWindow: any;
 
   beforeEach(() => {
-    injector = ReflectiveInjector.resolveAndCreate([
-        GaService,
-        { provide: Logger, useClass: TestLogger }
-    ]);
+    gaSpy = jasmine.createSpy('ga');
+    mockWindow = { ga: gaSpy };
+    injector = ReflectiveInjector.resolveAndCreate([GaService, { provide: WindowToken, useFactory: () => mockWindow }]);
+    gaService = injector.get(GaService);
   });
 
-  describe('with ambient GA', () => {
-    let gaService: GaService;
-
-    beforeEach(fakeAsync(() => {
-      this.winGa = window['ga']; // remember current GA tracker just in case
-
-      // Replace Google Analytics tracker with spy after calling "ga ready" callback
-      window['ga'] = (fn: Function) => {
-        window['ga'] = gaSpy = jasmine.createSpy('ga');
-        fn();
-        tick(GaService.initializeDelay); // see GaService#initializeGa
-      };
-      gaService = injector.get(GaService);
-    }));
-
-    afterEach(() => {
-      window['ga'] = this.winGa;
-    });
-
-    it('should initialize ga with "create" when constructed', () => {
-      const first = gaSpy.calls.first().args;
-      expect(first[0]).toBe('create');
-    });
-
-    describe('#locationChanged(url)', () => {
-      it('should send page to url w/ leading slash', () => {
-        gaService.locationChanged('testUrl');
-        let args = gaSpy.calls.all()[1].args;
-        expect(args).toEqual(['set', 'page', '/testUrl']);
-        args = gaSpy.calls.all()[2].args;
-        expect(args).toEqual(['send', 'pageview']);
-      });
-    });
-
-    describe('#sendPage(url)', () => {
-      it('should set page to url w/ leading slash', () => {
-        gaService.sendPage('testUrl');
-        const args = gaSpy.calls.all()[1].args;
-        expect(args).toEqual(['set', 'page', '/testUrl']);
-      });
-
-      it('should send "pageview" ', () => {
-        gaService.sendPage('testUrl');
-        const args = gaSpy.calls.all()[2].args;
-        expect(args).toEqual(['send', 'pageview']);
-      });
-
-      it('should not send twice with same URL, back-to-back', () => {
-        gaService.sendPage('testUrl');
-        gaService.sendPage('testUrl');
-        expect(gaSpySendCalls()).toEqual(['/testUrl']);
-      });
-
-      it('should send twice with same URL, back-to-back, even when the hash changes', () => {
-        // Therefore it is up to caller NOT to call it when hash changes if this is unwanted.
-        // See LocationService and its specs
-        gaService.sendPage('testUrl#one');
-        gaService.sendPage('testUrl#two');
-        expect(gaSpySendCalls()).toEqual([
-          '/testUrl#one',
-          '/testUrl#two'
-        ]);
-
-      });
-
-      it('should send same URL twice when other intervening URL', () => {
-        gaService.sendPage('testUrl');
-        gaService.sendPage('testUrl2');
-        gaService.sendPage('testUrl');
-        expect(gaSpySendCalls()).toEqual([
-          '/testUrl',
-          '/testUrl2',
-          '/testUrl'
-        ]);
-      });
-    });
-
+  it('should initialize ga with "create" when constructed', () => {
+    const first = gaSpy.calls.first().args;
+    expect(first[0]).toBe('create');
   });
 
-  describe('when no ambient GA', () => {
-    let gaService: GaService;
-    let logger: TestLogger;
-
-    it('should log with "create" when constructed', () => {
-      gaService = injector.get(GaService);
-      logger = injector.get(Logger);
-      expect(logger.log.calls.count()).toBe(1, 'logger.log should be called');
-      const first = logger.log.calls.first().args;
-      expect(first[0]).toBe('ga:');
-      expect(first[1][0]).toBe('create'); // first[1] is the array of args to ga()
+  describe('#locationChanged(url)', () => {
+    it('should send page to url w/ leading slash', () => {
+      gaService.locationChanged('testUrl');
+      expect(gaSpy).toHaveBeenCalledWith('set', 'page', '/testUrl');
+      expect(gaSpy).toHaveBeenCalledWith('send', 'pageview');
     });
   });
+
+  describe('#sendPage(url)', () => {
+    it('should set page to url w/ leading slash', () => {
+      gaService.sendPage('testUrl');
+      expect(gaSpy).toHaveBeenCalledWith('set', 'page', '/testUrl');
+    });
+
+    it('should send "pageview" ', () => {
+      gaService.sendPage('testUrl');
+      expect(gaSpy).toHaveBeenCalledWith('send', 'pageview');
+    });
+
+    it('should not send twice with same URL, back-to-back', () => {
+      gaService.sendPage('testUrl');
+      gaSpy.calls.reset();
+      gaService.sendPage('testUrl');
+      expect(gaSpy).not.toHaveBeenCalled();
+    });
+
+    it('should send again even if only the hash changes', () => {
+      // Therefore it is up to caller NOT to call it when hash changes if this is unwanted.
+      // See LocationService and its specs
+      gaService.sendPage('testUrl#one');
+      expect(gaSpy).toHaveBeenCalledWith('set', 'page', '/testUrl#one');
+      expect(gaSpy).toHaveBeenCalledWith('send', 'pageview');
+      gaSpy.calls.reset();
+      gaService.sendPage('testUrl#two');
+      expect(gaSpy).toHaveBeenCalledWith('set', 'page', '/testUrl#two');
+      expect(gaSpy).toHaveBeenCalledWith('send', 'pageview');
+    });
+
+    it('should send same URL twice when other intervening URL', () => {
+      gaService.sendPage('testUrl');
+      expect(gaSpy).toHaveBeenCalledWith('set', 'page', '/testUrl');
+      expect(gaSpy).toHaveBeenCalledWith('send', 'pageview');
+      gaSpy.calls.reset();
+      gaService.sendPage('testUrl2');
+      expect(gaSpy).toHaveBeenCalledWith('set', 'page', '/testUrl2');
+      expect(gaSpy).toHaveBeenCalledWith('send', 'pageview');
+      gaSpy.calls.reset();
+      gaService.sendPage('testUrl');
+      expect(gaSpy).toHaveBeenCalledWith('set', 'page', '/testUrl');
+      expect(gaSpy).toHaveBeenCalledWith('send', 'pageview');
+    });
+  });
+
+  it('should support replacing the `window.ga` function', () => {
+    const gaSpy2 = jasmine.createSpy('new ga');
+    mockWindow.ga = gaSpy2;
+    gaSpy.calls.reset();
+
+    gaService.sendPage('testUrl');
+    expect(gaSpy).not.toHaveBeenCalled();
+    expect(gaSpy2).toHaveBeenCalledWith('set', 'page', '/testUrl');
+    expect(gaSpy2).toHaveBeenCalledWith('send', 'pageview');
+  });
 });
-
-class TestLogger {
-  log = jasmine.createSpy('log');
-}
diff --git a/aio/src/app/shared/ga.service.ts b/aio/src/app/shared/ga.service.ts
index c9563f9cf8..747e8285b4 100644
--- a/aio/src/app/shared/ga.service.ts
+++ b/aio/src/app/shared/ga.service.ts
@@ -1,7 +1,7 @@
-import { Injectable } from '@angular/core';
+import { Inject, Injectable } from '@angular/core';
 
 import { environment } from '../../environments/environment';
-import { Logger } from 'app/shared/logger.service';
+import { WindowToken } from 'app/shared/window';
 
 @Injectable()
 /**
@@ -10,15 +10,10 @@ import { Logger } from 'app/shared/logger.service';
  * Associates data with a GA "property" from the environment (`gaId`).
  */
 export class GaService {
-  // ms to wait before acquiring window.ga after analytics library loads
-  // empirically determined to allow time for e2e test setup
-  static initializeDelay = 1000;
 
   private previousUrl: string;
-  private ga: (...rest: any[]) => void;
 
-  constructor(private logger: Logger) {
-    this.initializeGa();
+  constructor(@Inject(WindowToken) private window: Window) {
     this.ga('create', environment['gaId'] , 'auto');
   }
 
@@ -34,27 +29,7 @@ export class GaService {
     this.ga('send', 'pageview');
   }
 
-  // These gyrations are necessary to make the service e2e testable
-  // and to disable ga tracking during e2e tests.
-  private initializeGa() {
-    const ga = window['ga'];
-    if (ga) {
-      // Queue commands until GA analytics script has loaded.
-      const gaQueue: any[][] = [];
-      this.ga = (...rest: any[]) => { gaQueue.push(rest); };
-
-      // Then send queued commands to either real or e2e test ga();
-      // after waiting to allow possible e2e test to replace global ga function
-      ga(() => setTimeout(() => {
-        // this.logger.log('GA fn:', window['ga'].toString());
-        this.ga = window['ga'];
-        gaQueue.forEach((command) => this.ga.apply(null, command));
-      }, GaService.initializeDelay));
-
-    } else {
-      // delegate `ga` calls to the logger if no ga installed
-      this.ga = (...rest: any[]) => { this.logger.log('ga:', rest); };
-    }
+  ga(...args) {
+    this.window['ga'](...args);
   }
-
 }
diff --git a/aio/src/app/shared/window.ts b/aio/src/app/shared/window.ts
new file mode 100644
index 0000000000..c1eba5a7e8
--- /dev/null
+++ b/aio/src/app/shared/window.ts
@@ -0,0 +1,4 @@
+import { InjectionToken } from '@angular/core';
+
+export const WindowToken = new InjectionToken('Window');
+export function windowProvider() { return window; }
diff --git a/aio/src/index.html b/aio/src/index.html
index fca6077a28..9318d4af05 100644
--- a/aio/src/index.html
+++ b/aio/src/index.html
@@ -34,9 +34,13 @@
 
   <!-- Google Analytics -->
   <script>
-  window.ga=window.ga||function(){(ga.q=ga.q||[]).push(arguments)};ga.l=+new Date;
+  // Note this is a customised version of the GA tracking snippet to aid e2e testing
+  // See the bit between /**/.../**/
+  (function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){
+  (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
+  m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;/**/i.sessionStorage.__e2e__||/**/m.parentNode.insertBefore(a,m)
+  })(window,document,'script','https://www.google-analytics.com/analytics.js','ga');
   </script>
-  <script async src='https://www.google-analytics.com/analytics.js'></script>
   <!-- End Google Analytics -->
 
   <script>