Bug 1233986 - Add test coverage. r=gfritzsche

MozReview-Commit-ID: 1Mi2ogNvy2o

browser/components/uitour/UITour.jsm

@@ -650,7 +650,9 @@ this.UITour = {
         string.data = value;
         Services.prefs.setComplexValue("browser.uitour.treatment." + name,
                                        Ci.nsISupportsString, string);
-        UITourHealthReport.recordTreatmentTag(name, value);
+        // The notification is only meant to be used in tests.
+        UITourHealthReport.recordTreatmentTag(name, value)
+                          .then(() => this.notify("TreatmentTag:TelemetrySent"));
         break;
       }
 
@@ -2163,15 +2165,15 @@ this.UITour.init();
  */
 const UITourHealthReport = {
   recordTreatmentTag: function(tag, value) {
-  TelemetryController.submitExternalPing("uitour-tag",
-    {
-      version: 1,
-      tagName: tag,
-      tagValue: value,
-    },
-    {
-      addClientId: true,
-      addEnvironment: true,
-    });
+    return TelemetryController.submitExternalPing("uitour-tag",
+      {
+        version: 1,
+        tagName: tag,
+        tagValue: value,
+      },
+      {
+        addClientId: true,
+        addEnvironment: true,
+      });
   }
 };

browser/components/uitour/test/browser_UITour.js

@@ -379,6 +379,9 @@ var tests = [
     let ac = new TelemetryArchiveTesting.Checker();
     yield ac.promiseInit();
     yield gContentAPI.setTreatmentTag("foobar", "baz");
+    // Wait until the treatment telemetry is sent before looking in the archive.
+    yield BrowserTestUtils.waitForContentEvent(gTestTab.linkedBrowser, "mozUITourNotification", false,
+                                               event => event.detail.event === "TreatmentTag:TelemetrySent");
     yield new Promise((resolve) => {
       gContentAPI.getTreatmentTag("foobar", (data) => {
         is(data.value, "baz", "set and retrieved treatmentTag");

toolkit/components/telemetry/tests/unit/head.js

@@ -77,7 +77,10 @@ const PingServer = {
 
   promiseNextRequest: function() {
     const deferred = this._defers[this._currentDeferred++];
-    return deferred.promise;
+    // Send the ping to the consumer on the next tick, so that the completion gets
+    // signaled to Telemetry.
+    return new Promise(r => Services.tm.currentThread.dispatch(() => r(deferred.promise),
+                                                               Ci.nsIThread.DISPATCH_NORMAL));
   },
 
   promiseNextPing: function() {

toolkit/components/telemetry/tests/unit/test_PingAPI.js

@@ -6,6 +6,7 @@
 
 "use strict";
 
+Cu.import("resource://gre/modules/ClientID.jsm", this);
 Cu.import("resource://gre/modules/TelemetryController.jsm", this);
 Cu.import("resource://gre/modules/TelemetryArchive.jsm", this);
 Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
@@ -402,7 +403,7 @@ add_task(function* test_clientId() {
   // Check that a ping submitted after the delayed telemetry initialization completed
   // should get a valid client id.
   yield TelemetryController.testReset();
-  const clientId = TelemetryController.clientID;
+  const clientId = yield ClientID.getClientID();
 
   let id = yield TelemetryController.submitExternalPing("test-type", {}, {addClientId: true});
   let ping = yield TelemetryArchive.promiseArchivedPingById(id);

toolkit/components/telemetry/tests/unit/test_TelemetryController.js

@@ -107,14 +107,6 @@ function run_test() {
 
 add_task(function* asyncSetup() {
   yield TelemetryController.testSetup();
-
-  gClientID = yield ClientID.getClientID();
-
-  // We should have cached the client id now. Lets confirm that by
-  // checking the client id before the async ping setup is finished.
-  let promisePingSetup = TelemetryController.testReset();
-  do_check_eq(TelemetryController.clientID, gClientID);
-  yield promisePingSetup;
 });
 
 // Ensure that not overwriting an existing file fails silently
@@ -128,6 +120,10 @@ add_task(function* test_overwritePing() {
 // Checks that a sent ping is correctly received by a dummy http server.
 add_task(function* test_simplePing() {
   PingServer.start();
+  // Update the Telemetry Server preference with the address of the local server.
+  // Otherwise we might end up sending stuff to a non-existing server after
+  // |TelemetryController.testReset| is called.
+  Preferences.set(TelemetryController.Constants.PREF_SERVER, "http://localhost:" + PingServer.port);
 
   yield sendPing(false, false);
   let request = yield PingServer.promiseNextRequest();
@@ -151,8 +147,6 @@ add_task(function* test_disableDataUpload() {
     return;
   }
 
-  const PREF_TELEMETRY_SERVER = "toolkit.telemetry.server";
-
   // Disable FHR upload: this should trigger a deletion ping.
   Preferences.set(PREF_FHR_UPLOAD_ENABLED, false);
 
@@ -190,7 +184,7 @@ add_task(function* test_disableDataUpload() {
   PingServer.start();
   // We set the new server using the pref, otherwise it would get reset with
   // |TelemetryController.testReset|.
-  Preferences.set(PREF_TELEMETRY_SERVER, "http://localhost:" + PingServer.port);
+  Preferences.set(TelemetryController.Constants.PREF_SERVER, "http://localhost:" + PingServer.port);
 
   // Reset the controller to spin the ping sending task.
   yield TelemetryController.testReset();
@@ -202,13 +196,65 @@ add_task(function* test_disableDataUpload() {
 });
 
 add_task(function* test_pingHasClientId() {
-  // Send a ping with a clientId.
+  const PREF_CACHED_CLIENTID = "toolkit.telemetry.cachedClientID";
+
+  // Make sure we have no cached client ID for this test: we'll try to send
+  // a ping with it while Telemetry is being initialized.
+  Preferences.reset(PREF_CACHED_CLIENTID);
+  yield TelemetryController.testShutdown();
+  yield ClientID._reset();
+  yield TelemetryStorage.testClearPendingPings();
+  // And also clear the counter histogram since we're here.
+  let h = Telemetry.getHistogramById("TELEMETRY_PING_SUBMISSION_WAITING_CLIENTID");
+  h.clear();
+
+  // Init telemetry and try to send a ping with a client ID.
+  let promisePingSetup = TelemetryController.testReset();
   yield sendPing(true, false);
+  Assert.equal(h.snapshot().sum, 1,
+               "We must have a ping waiting for the clientId early during startup.");
+  // Wait until we are fully initialized. Pings will be assembled but won't get
+  // sent before then.
+  yield promisePingSetup;
 
   let ping = yield PingServer.promiseNextPing();
-  checkPingFormat(ping, TEST_PING_TYPE, true, false);
+  // Fetch the client ID after initializing and fetching the the ping, so we
+  // don't unintentionally trigger its loading. We'll still need the client ID
+  // to see if the ping looks sane.
+  gClientID = yield ClientID.getClientID();
 
+  checkPingFormat(ping, TEST_PING_TYPE, true, false);
   Assert.equal(ping.clientId, gClientID, "The correct clientId must be reported.");
+
+  // Shutdown Telemetry so we can safely restart it.
+  yield TelemetryController.testShutdown();
+  yield TelemetryStorage.testClearPendingPings();
+
+  // We should have cached the client ID now. Lets confirm that by checking it before
+  // the async ping setup is finished.
+  h.clear();
+  promisePingSetup = TelemetryController.testReset();
+  yield sendPing(true, false);
+  yield promisePingSetup;
+
+  // Check that we received the cached client id.
+  Assert.equal(h.snapshot().sum, 0, "We must have used the cached clientId.");
+  ping = yield PingServer.promiseNextPing();
+  checkPingFormat(ping, TEST_PING_TYPE, true, false);
+  Assert.equal(ping.clientId, gClientID,
+               "Telemetry should report the correct cached clientId.");
+
+  // Check that sending a ping without relying on the cache, after the
+  // initialization, still works.
+  Preferences.reset(PREF_CACHED_CLIENTID);
+  yield TelemetryController.testShutdown();
+  yield TelemetryStorage.testClearPendingPings();
+  yield TelemetryController.testReset();
+  yield sendPing(true, false);
+  ping = yield PingServer.promiseNextPing();
+  checkPingFormat(ping, TEST_PING_TYPE, true, false);
+  Assert.equal(ping.clientId, gClientID, "The correct clientId must be reported.");
+  Assert.equal(h.snapshot().sum, 0, "No ping should have been waiting for a clientId.");
 });
 
 add_task(function* test_pingHasEnvironment() {