Bug 1443560: Convert checks to examine snapshotted events r=Gijs

Because the mock no longer checks whether the format of events is
correct, check the events that Telemetry actually records, in order to
make sure we conform to the event schema.

Some uses check that *no* calls were made to the stub. Although this
is technically still valid (because we don't care about the format of
events in this case), convert even these to check events for
consistency.

TelemetryTestUtils.assertEvents doesn't allow us to pass a message,
but most of these messages don't add anything useful and at least one
is just wrong so it's not a big loss.

Depends on D19540

Differential Revision: https://phabricator.services.mozilla.com/D19831

--HG--
extra : moz-landing-system : lando

toolkit/components/normandy/test/browser/browser_AddonStudies.js

@@ -107,8 +107,10 @@ decorate_task(
       newActiveStudy.studyEndDate,
       "init sets the study end date if a study's add-on is not installed."
     );
+    let events = Services.telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
+    events = (events.parent || []).filter(e => e[1] == "normandy");
     Assert.deepEqual(
-      sendEventStub.getCall(0).args,
+      events[0].slice(2), // strip timestamp and "normandy"
       ["unenroll", "addon_study", activeUninstalledStudy.name, {
         addonId: activeUninstalledStudy.addonId,
         addonVersion: activeUninstalledStudy.addonVersion,

toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js

@@ -57,10 +57,8 @@ decorate_task(
       "start threw an error due to a conflicting experiment name",
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enrollFailed", "preference_study", "test", {reason: "name-conflict"}]],
-      "event should be sent for failure",
+    sendEventStub.assertEvents(
+      [["enrollFailed", "preference_study", "test", {reason: "name-conflict"}]]
     );
   }
 );
@@ -83,10 +81,8 @@ decorate_task(
       "start threw an error due to an active experiment for the given preference",
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enrollFailed", "preference_study", "different", {reason: "pref-conflict"}]],
-      "event should be sent for failure",
+    sendEventStub.assertEvents(
+      [["enrollFailed", "preference_study", "different", {reason: "pref-conflict"}]]
     );
   }
 );
@@ -109,10 +105,8 @@ decorate_task(
       "start threw an error due to an invalid preference branch type",
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enrollFailed", "preference_study", "test", {reason: "invalid-branch"}]],
-      "event should be sent for failure",
+    sendEventStub.assertEvents(
+      [["enrollFailed", "preference_study", "test", {reason: "invalid-branch"}]]
     );
   }
 );
@@ -242,10 +236,8 @@ decorate_task(
       "start threw error for incompatible preference type"
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enrollFailed", "preference_study", "test", {reason: "invalid-type"}]],
-      "event should be sent for failure",
+    sendEventStub.assertEvents(
+      [["enrollFailed", "preference_study", "test", {reason: "invalid-type"}]]
     );
   }
 );
@@ -424,10 +416,8 @@ decorate_task(
       "stop threw an error because there are no experiments with the given name",
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["unenrollFailed", "preference_study", "test", {reason: "does-not-exist"}]],
-      "event should be sent for failure",
+    sendEventStub.assertEvents(
+      [["unenrollFailed", "preference_study", "test", {reason: "does-not-exist"}]]
     );
   }
 );
@@ -443,10 +433,8 @@ decorate_task(
       "stop threw an error because the experiment was already expired",
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["unenrollFailed", "preference_study", "test", {reason: "already-unenrolled"}]],
-      "event should be sent for failure",
+    sendEventStub.assertEvents(
+      [["unenrollFailed", "preference_study", "test", {reason: "already-unenrolled"}]]
     );
   }
 );
@@ -492,14 +480,12 @@ decorate_task(
       "stop cleared the startup preference for fake.preference.",
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
+    sendEventStub.assertEvents(
       [["unenroll", "preference_study", "test", {
         didResetValue: "true",
         reason: "test-reason",
         branch: "fakebranch",
-      }]],
-      "stop should send the correct telemetry event"
+      }]]
     );
 
     PreferenceExperiments.stopAllObservers();
@@ -590,14 +576,12 @@ decorate_task(
       "customvalue",
       "stop did not modify the preference",
     );
-    Assert.deepEqual(
-      sendEventStub.args,
+    sendEventStub.assertEvents(
       [["unenroll", "preference_study", "test", {
         didResetValue: "false",
         reason: "test-reason",
         branch: "fakebranch",
-      }]],
-      "stop should send the correct telemetry event"
+      }]]
     );
   }
 );
@@ -763,8 +747,7 @@ decorate_task(
     await PreferenceExperiments.stop("test", { reason: "test-reason" });
     Assert.deepEqual(setInactiveStub.args, [["test"]], "Experiment is unregistered by stop()");
 
-    Assert.deepEqual(
-      sendEventStub.args,
+    sendEventStub.assertEvents(
       [
         ["enroll", "preference_study", "test", {
           experimentType: "exp",
@@ -775,8 +758,7 @@ decorate_task(
           didResetValue: "true",
           branch: "branch",
         }],
-      ],
-      "PreferenceExperiments.start() and stop() should send the correct telemetry event"
+      ]
     );
   },
 );
@@ -804,13 +786,11 @@ decorate_task(
       "start() should register the experiment with the provided type",
     );
 
-    Assert.deepEqual(
-      sendEventStub.getCall(0).args,
-      ["enroll", "preference_study", "test", {
+    sendEventStub.assertEvents(
+      [["enroll", "preference_study", "test", {
         experimentType: "pref-test",
         branch: "branch",
-      }],
-      "start should include the passed reason in the telemetry event"
+      }]]
     );
 
     // start sets the passed preference in a way that is hard to mock.
@@ -1152,14 +1132,12 @@ decorate_task(
     // let the event loop tick to run the observer
     await Promise.resolve();
 
-    Assert.deepEqual(
-      sendEventStub.args,
+    sendEventStub.assertEvents(
       [["unenroll", "preference_study", "test", {
         didResetValue: "false",
         reason: "user-preference-changed",
         branch: "fakebranch",
-      }]],
-      "stop should send a telemetry event indicating the user unenrolled manually",
+      }]]
     );
   },
 );

toolkit/components/normandy/test/browser/browser_PreferenceRollouts.js

@@ -134,7 +134,7 @@ decorate_task(
       "rollouts should remain active when only one pref matches the built-in default",
     );
 
-    Assert.deepEqual(sendEventStub.args, [], "no events should be sent yet");
+    sendEventStub.assertEvents([]);
 
     // both prefs is enough
     await PreferenceRollouts.recordOriginalValues({"test.pref1": 2, "test.pref2": 2});
@@ -145,10 +145,8 @@ decorate_task(
       "rollouts should graduate when all prefs matches the built-in defaults",
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["graduate", "preference_rollout", "test-rollout", {}]],
-      "a graduation event should be sent",
+    sendEventStub.assertEvents(
+      [["graduate", "preference_rollout", "test-rollout"]]
     );
   },
 );

toolkit/components/normandy/test/browser/browser_actions_AddonStudyAction.js

@@ -58,7 +58,7 @@ decorate_task(
     await action.runRecipe(recipe);
     is(action.lastError, null, "lastError should be null");
     Assert.deepEqual(enrollSpy.args, [], "enroll should not be called");
-    Assert.deepEqual(sendEventStub.args, [], "no events should be sent");
+    sendEventStub.assertEvents([]);
   },
 );
 
@@ -76,10 +76,8 @@ decorate_task(
     const studies = await AddonStudies.getAll();
     Assert.deepEqual(studies, [], "the study should not be in the database");
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enrollFailed", "addon_study", recipe.arguments.name, {reason: "download-failure", detail: "ERROR_NETWORK_FAILURE"}]],
-      "An enrollFailed event should be sent",
+    sendEventStub.assertEvents(
+      [["enrollFailed", "addon_study", recipe.arguments.name, {reason: "download-failure", detail: "ERROR_NETWORK_FAILURE"}]]
     );
   }
 );
@@ -103,10 +101,8 @@ decorate_task(
 
     Assert.deepEqual(await AddonStudies.getAll(), [], "There should be no enrolled studies");
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enrollFailed", "addon_study", recipe.arguments.name, { reason: "conflicting-addon-id" }]],
-      "A enrollFailed event should be sent",
+    sendEventStub.assertEvents(
+      [["enrollFailed", "addon_study", recipe.arguments.name, { reason: "conflicting-addon-id" }]]
     );
   },
 );
@@ -153,10 +149,8 @@ decorate_task(
     ok(study.studyStartDate, "a start date should be assigned");
     is(study.studyEndDate, null, "an end date should not be assigned");
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enroll", "addon_study", recipe.arguments.name, { addonId: FIXTURE_ADDON_ID, addonVersion: "1.0" }]],
-      "an enrollment event should be sent",
+    sendEventStub.assertEvents(
+      [["enroll", "addon_study", recipe.arguments.name, { addonId: FIXTURE_ADDON_ID, addonVersion: "1.0" }]]
     );
 
     // cleanup
@@ -218,14 +212,12 @@ decorate_task(
     addon = await AddonManager.getAddonByID(addonId);
     is(addon, null, "the add-on should be uninstalled after unenrolling");
 
-    Assert.deepEqual(
-      sendEventStub.args,
+    sendEventStub.assertEvents(
       [["unenroll", "addon_study", study.name, {
         addonId,
         addonVersion: study.addonVersion,
         reason: "test-reason",
-      }]],
-      "an unenroll event should be sent",
+      }]]
     );
   },
 );
@@ -244,14 +236,12 @@ decorate_task(
     SimpleTest.monitorConsole(() => SimpleTest.finish(), [{message: /could not uninstall addon/i}]);
     await action.unenroll(study.recipeId);
 
-    Assert.deepEqual(
-      sendEventStub.args,
+    sendEventStub.assertEvents(
       [["unenroll", "addon_study", study.name, {
         addonId: study.addonId,
         addonVersion: study.addonVersion,
         reason: "unknown",
-      }]],
-      "an unenroll event should be sent",
+      }]]
     );
 
     SimpleTest.endMonitorConsole();
@@ -275,7 +265,7 @@ decorate_task(
     is(action.state, AddonStudyAction.STATE_FINALIZED, "the action should be finalized");
     is(action.lastError, null, "lastError should be null");
     Assert.deepEqual(enrollSpy.args, [], "enroll should not be called");
-    Assert.deepEqual(sendEventStub.args, [], "no events should be sent");
+    sendEventStub.assertEvents([]);
   },
 );
 
@@ -291,7 +281,7 @@ decorate_task(
     await action.runRecipe(recipe);
     await action.finalize();
     Assert.deepEqual(enrollSpy.args, [], "enroll should not be called");
-    Assert.deepEqual(sendEventStub.args, [], "no events should be sent");
+    sendEventStub.assertEvents([]);
   },
 );
 

toolkit/components/normandy/test/browser/browser_actions_PreferenceRollbackAction.js

@@ -61,10 +61,8 @@ decorate_task(
     );
 
     // Telemetry is updated
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["unenroll", "preference_rollback", recipe.arguments.rolloutSlug, {reason: "rollback"}]],
-      "an unenrollment event should be sent"
+    sendEventStub.assertEvents(
+      [["unenroll", "preference_rollback", recipe.arguments.rolloutSlug, {reason: "rollback"}]]
     );
     Assert.deepEqual(setExperimentInactiveStub.args, [["test-rollout"]], "the telemetry experiment should deactivated");
 
@@ -108,10 +106,8 @@ decorate_task(
       "Rollout should not change in db"
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["unenrollFailed", "preference_rollback", "graduated-rollout", {reason: "graduated"}]],
-      "correct event was sent"
+    sendEventStub.assertEvents(
+      [["unenrollFailed", "preference_rollback", "graduated-rollout", {reason: "graduated"}]]
     );
 
     // Cleanup
@@ -132,7 +128,7 @@ decorate_task(
     await action.finalize();
     is(action.lastError, null, "lastError should be null");
 
-    Assert.deepEqual(sendEventStub.args, [], "an unenrollFailure event should not be sent");
+    sendEventStub.assertEvents([]);
     Assert.deepEqual(
       reportRecipeStub.args,
       [[recipe.id, Uptake.RECIPE_SUCCESS]],
@@ -169,7 +165,7 @@ decorate_task(
     Assert.deepEqual(await PreferenceRollouts.getAll(), [rollout], "Rollout shouldn't change in db");
 
     // Telemetry is updated
-    Assert.deepEqual(sendEventStub.args, [], "no telemetry event should be sent");
+    sendEventStub.assertEvents([]);
     Assert.deepEqual(setExperimentInactiveStub.args, [], "telemetry experiments should not be updated");
 
     // Cleanup

toolkit/components/normandy/test/browser/browser_actions_PreferenceRolloutAction.js

@@ -55,10 +55,8 @@ decorate_task(
       "Rollout should be stored in db"
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enroll", "preference_rollout", recipe.arguments.slug, {}]],
-      "an enrollment event should be sent"
+    sendEventStub.assertEvents(
+      [["enroll", "preference_rollout", recipe.arguments.slug]]
     );
     Assert.deepEqual(
       setExperimentActiveStub.args,
@@ -149,13 +147,11 @@ decorate_task(
       "Rollout should be updated in db"
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
+    sendEventStub.assertEvents(
       [
-        ["enroll", "preference_rollout", "test-rollout", {}],
+        ["enroll", "preference_rollout", "test-rollout"],
         ["update", "preference_rollout", "test-rollout", {previousState: "active"}],
-      ],
-      "update event was sent"
+      ]
     );
 
     // Cleanup
@@ -204,12 +200,10 @@ decorate_task(
       "Rollout should be updated in db"
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
+    sendEventStub.assertEvents(
       [
         ["update", "preference_rollout", "test-rollout", {previousState: "graduated"}],
-      ],
-      "correct events was sent"
+      ]
     );
 
     // Cleanup
@@ -279,10 +273,9 @@ decorate_task(
       "Only recipe1's rollout should be stored in db",
     );
 
-    Assert.deepEqual(
-      sendEventStub.args,
+    sendEventStub.assertEvents(
       [
-        ["enroll", "preference_rollout", recipe1.arguments.slug, {}],
+        ["enroll", "preference_rollout", recipe1.arguments.slug],
         ["enrollFailed", "preference_rollout", recipe2.arguments.slug, {reason: "conflict", preference: "test.pref1"}],
         ["enrollFailed", "preference_rollout", recipe2.arguments.slug, {reason: "conflict", preference: "test.pref1"}],
       ]
@@ -318,10 +311,8 @@ decorate_task(
     is(Services.prefs.getPrefType("app.normandy.startupRolloutPrefs.test.pref"), Services.prefs.PREF_INVALID, "startup pref is not set");
 
     Assert.deepEqual(await PreferenceRollouts.getAll(), [], "no rollout is stored in the db");
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enrollFailed", "preference_rollout", recipe.arguments.slug, {reason: "invalid type", preference: "test.pref"}]],
-      "an enrollment failed event should be sent",
+    sendEventStub.assertEvents(
+      [["enrollFailed", "preference_rollout", recipe.arguments.slug, {reason: "invalid type", preference: "test.pref"}]]
     );
 
     // Cleanup
@@ -422,10 +413,8 @@ decorate_task(
     await action.finalize();
     is(action.lastError, null, "lastError should be null");
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enroll", "preference_rollout", "test-rollout", {}]],
-      "only an enrollment event should be generated",
+    sendEventStub.assertEvents(
+      [["enroll", "preference_rollout", "test-rollout"]]
     );
 
     Assert.deepEqual(
@@ -473,10 +462,8 @@ decorate_task(
     // rollout was not stored
     Assert.deepEqual(await PreferenceRollouts.getAll(), [], "Rollout should not be stored in db");
 
-    Assert.deepEqual(
-      sendEventStub.args,
-      [["enrollFailed", "preference_rollout", recipe.arguments.slug, {reason: "would-be-no-op"}]],
-      "an enrollment failure event should be sent"
+    sendEventStub.assertEvents(
+      [["enrollFailed", "preference_rollout", recipe.arguments.slug, {reason: "would-be-no-op"}]]
     );
     Assert.deepEqual(setExperimentActiveStub.args, [], "a telemetry experiment should not be activated");
 

toolkit/components/normandy/test/browser/head.js

@@ -6,6 +6,8 @@ ChromeUtils.import("resource://normandy/lib/SandboxManager.jsm", this);
 ChromeUtils.import("resource://normandy/lib/NormandyDriver.jsm", this);
 ChromeUtils.import("resource://normandy/lib/NormandyApi.jsm", this);
 ChromeUtils.import("resource://normandy/lib/TelemetryEvents.jsm", this);
+ChromeUtils.defineModuleGetter(this, "TelemetryTestUtils",
+                               "resource://testing-common/TelemetryTestUtils.jsm");
 
 // Load mocking/stubbing library, sinon
 // docs: http://sinonjs.org/docs/
@@ -348,6 +350,13 @@ this.studyEndObserved = function(recipeId) {
 this.withSendEventStub = function(testFunction) {
   return async function wrappedTestFunction(...args) {
     const stub = sinon.spy(TelemetryEvents, "sendEvent");
+    stub.assertEvents = (expected) => {
+      let events = Services.telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
+      events = (events.parent || []).filter(e => e[1] == "normandy");
+      expected = expected.map(event => ["normandy"].concat(event));
+      TelemetryTestUtils.assertEvents(events, expected);
+    };
+    Services.telemetry.clearEvents();
     try {
       await testFunction(...args, stub);
     } finally {