Bug 1633883 - TelemetryTestUtils.assertScalar assumes Number (so behaves odd for boolean, strings) r=chutten

Split assertScalar to assertScalarUnset to allow the former to plain check the value including strings, false and 0.

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

accessible/tests/browser/telemetry/browser_HCM_telemetry.js

@@ -59,10 +59,9 @@ async function closeColorsDialog(dialogWin) {
 }
 
 function verifyBackplate(expectedValue) {
-  const snapshot = TelemetryTestUtils.getProcessScalars("parent", false, true);
-  ok("a11y.backplate" in snapshot, "Backplate scalar must be present.");
-  is(
-    snapshot["a11y.backplate"],
+  TelemetryTestUtils.assertScalar(
+    TelemetryTestUtils.getProcessScalars("parent", false, true),
+    "a11y.backplate",
     expectedValue,
     "Backplate scalar is logged as " + expectedValue
   );
@@ -289,9 +288,6 @@ add_task(async function testSetNever() {
   gBrowser.removeCurrentTab();
 });
 
-// XXX We're uable to assert scalars that are false using assertScalar util,
-// so we directly query the scalar in the scalar list below. (bug 1633883)
-
 add_task(async function testBackplate() {
   is(
     Services.prefs.getBoolPref("browser.display.permit_backplate"),

browser/components/sessionstore/test/browser_background_tab_crash.js

@@ -123,8 +123,15 @@ async function crashBackgroundTabs(tabs) {
  * @param desc test description to output to the log
  */
 function checkTelemetry(expectedCount, desc) {
+  const scalars = TelemetryTestUtils.getProcessScalars("parent");
+  // This telemetry adds up from 0, so expected 0 results in unset scalar.
+  if (expectedCount === 0) {
+    TelemetryTestUtils.assertScalarUnset(scalars, TABUI_PRESENTED_KEY);
+    return;
+  }
+
   TelemetryTestUtils.assertScalar(
-    TelemetryTestUtils.getProcessScalars("parent"),
+    scalars,
     TABUI_PRESENTED_KEY,
     expectedCount,
     desc + " telemetry"

browser/components/tests/browser/browser_browserGlue_telemetry.js

@@ -6,21 +6,42 @@ http://creativecommons.org/publicdomain/zero/1.0/ */
 // Check that telemetry reports Firefox is not pinned on any OS at startup.
 add_task(function check_startup_pinned_telemetry() {
   const scalars = TelemetryTestUtils.getProcessScalars("parent");
-  const check = (key, val, msg) => Assert.strictEqual(scalars[key], val, msg);
 
   // Check the appropriate telemetry is set or not reported by platform.
   switch (AppConstants.platform) {
     case "win":
-      check("os.environment.is_taskbar_pinned", false, "Pin set on win");
-      check("os.environment.is_kept_in_dock", undefined, "Dock unset on win");
+      TelemetryTestUtils.assertScalar(
+        scalars,
+        "os.environment.is_taskbar_pinned",
+        false,
+        "Pin set on win"
+      );
+      TelemetryTestUtils.assertScalarUnset(
+        scalars,
+        "os.environment.is_kept_in_dock"
+      );
       break;
     case "macosx":
-      check("os.environment.is_taskbar_pinned", undefined, "Pin unset on mac");
-      check("os.environment.is_kept_in_dock", false, "Dock set on mac");
+      TelemetryTestUtils.assertScalarUnset(
+        scalars,
+        "os.environment.is_taskbar_pinned"
+      );
+      TelemetryTestUtils.assertScalar(
+        scalars,
+        "os.environment.is_kept_in_dock",
+        false,
+        "Dock set on mac"
+      );
       break;
     default:
-      check("os.environment.is_taskbar_pinned", undefined, "Pin unset on lin");
-      check("os.environment.is_kept_in_dock", undefined, "Dock unset on lin");
+      TelemetryTestUtils.assertScalarUnset(
+        scalars,
+        "os.environment.is_taskbar_pinned"
+      );
+      TelemetryTestUtils.assertScalarUnset(
+        scalars,
+        "os.environment.is_kept_in_dock"
+      );
       break;
   }
 });

browser/extensions/formautofill/test/browser/creditCard/browser_creditCard_telemetry.js

@@ -160,11 +160,9 @@ add_task(async function test_popup_opened() {
     1,
     "There should be 1 section detected."
   );
-  TelemetryTestUtils.assertScalar(
+  TelemetryTestUtils.assertScalarUnset(
     TelemetryTestUtils.getProcessScalars("content"),
-    "formautofill.creditCards.submitted_sections_count",
-    0,
-    "There should be no sections submitted."
+    "formautofill.creditCards.submitted_sections_count"
   );
 
   SpecialPowers.clearUserPref(AUTOFILL_CREDITCARDS_AVAILABLE_PREF);

browser/modules/test/browser/browser_UsageTelemetry.js

@@ -36,62 +36,56 @@ let checkScalars = countsObject => {
   const scalars = TelemetryTestUtils.getProcessScalars("parent");
 
   // Check the expected values. Scalars that are never set must not be reported.
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  const checkScalar = (key, val, msg) =>
+    val > 0
+      ? TelemetryTestUtils.assertScalar(scalars, key, val, msg)
+      : TelemetryTestUtils.assertScalarUnset(scalars, key);
+  checkScalar(
     MAX_CONCURRENT_TABS,
     countsObject.maxTabs,
     "The maximum tab count must match the expected value."
   );
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  checkScalar(
     TAB_EVENT_COUNT,
     countsObject.tabOpenCount,
     "The number of open tab event count must match the expected value."
   );
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  checkScalar(
     MAX_TAB_PINNED,
     countsObject.maxTabsPinned,
     "The maximum tabs pinned count must match the expected value."
   );
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  checkScalar(
     TAB_PINNED_EVENT,
     countsObject.tabPinnedCount,
     "The number of tab pinned event count must match the expected value."
   );
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  checkScalar(
     MAX_CONCURRENT_WINDOWS,
     countsObject.maxWindows,
     "The maximum window count must match the expected value."
   );
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  checkScalar(
     WINDOW_OPEN_COUNT,
     countsObject.windowsOpenCount,
     "The number of window open event count must match the expected value."
   );
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  checkScalar(
     TOTAL_URI_COUNT,
     countsObject.totalURIs,
     "The total URI count must match the expected value."
   );
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  checkScalar(
     UNIQUE_DOMAINS_COUNT,
     countsObject.domainCount,
     "The unique domains count must match the expected value."
   );
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  checkScalar(
     UNFILTERED_URI_COUNT,
     countsObject.totalUnfilteredURIs,
     "The unfiltered URI count must match the expected value."
   );
-  TelemetryTestUtils.assertScalar(
-    scalars,
+  checkScalar(
     TOTAL_URI_COUNT_NORMAL_AND_PRIVATE_MODE,
     countsObject.totalURIsNormalAndPrivateMode,
     "The total URI count for both normal and private mode must match the expected value."

browser/modules/test/browser/browser_UsageTelemetry_domains.js

@@ -80,7 +80,18 @@ add_task(async function test_URIAndDomainCounts() {
     gBrowser,
     "about:blank"
   );
-  checkCounts({ totalURIs: 0, domainCount: 0, totalUnfilteredURIs: 0 });
+  TelemetryTestUtils.assertScalarUnset(
+    TelemetryTestUtils.getProcessScalars("parent"),
+    TOTAL_URI_COUNT
+  );
+  TelemetryTestUtils.assertScalarUnset(
+    TelemetryTestUtils.getProcessScalars("parent"),
+    UNIQUE_DOMAINS_COUNT
+  );
+  TelemetryTestUtils.assertScalarUnset(
+    TelemetryTestUtils.getProcessScalars("parent"),
+    UNFILTERED_URI_COUNT
+  );
 
   // Open a different page and check the counts.
   BrowserTestUtils.loadURI(firstTab.linkedBrowser, "http://example.com/");

browser/modules/test/unit/test_ProfileCounter.js

@@ -161,14 +161,10 @@ function checkSuccess(profilesReported, rawCount = profilesReported) {
 
 function checkError() {
   const scalars = TelemetryTestUtils.getProcessScalars("parent");
-  // Bug 1633883 - SCALAR_ERROR_VALUE is 0, but currently, assertScalar, if
-  //               given an expected value of 0, will check that the scalar is
-  //               unset rather than checking that its value is 0.
-  //               If this is changed, the check below can be replaced with a
-  //               TelemetryTestUtils.assertScalar call.
-  Assert.ok(
-    PROFILE_COUNT_SCALAR in scalars &&
-      scalars[PROFILE_COUNT_SCALAR] === SCALAR_ERROR_VALUE,
+  TelemetryTestUtils.assertScalar(
+    scalars,
+    PROFILE_COUNT_SCALAR,
+    SCALAR_ERROR_VALUE,
     "The value reported to telemetry should be the error value"
   );
 }

toolkit/components/extensions/test/xpcshell/test_ext_telemetry.js

@@ -151,14 +151,14 @@ if (AppConstants.MOZ_BUILD_APP === "browser") {
     );
   });
 
-  add_task(async function test_telemetry_scalar_set() {
+  add_task(async function test_telemetry_scalar_set_bool_true() {
     Services.telemetry.clearScalars();
     await run({
       backgroundScript: async () => {
         await browser.telemetry.scalarSet("telemetry.test.boolean_kind", true);
-        browser.test.notifyPass("scalar_set");
+        browser.test.notifyPass("scalar_set_bool_true");
       },
-      doneSignal: "scalar_set",
+      doneSignal: "scalar_set_bool_true",
     });
     TelemetryTestUtils.assertScalar(
       TelemetryTestUtils.getProcessScalars("parent", false, true),
@@ -167,6 +167,30 @@ if (AppConstants.MOZ_BUILD_APP === "browser") {
     );
   });
 
+  add_task(async function test_telemetry_scalar_set_bool_false() {
+    Services.telemetry.clearScalars();
+    await run({
+      backgroundScript: async () => {
+        await browser.telemetry.scalarSet("telemetry.test.boolean_kind", false);
+        browser.test.notifyPass("scalar_set_bool_false");
+      },
+      doneSignal: "scalar_set_bool_false",
+    });
+    TelemetryTestUtils.assertScalar(
+      TelemetryTestUtils.getProcessScalars("parent", false, true),
+      "telemetry.test.boolean_kind",
+      false
+    );
+  });
+
+  add_task(async function test_telemetry_scalar_unset_bool() {
+    Services.telemetry.clearScalars();
+    TelemetryTestUtils.assertScalarUnset(
+      TelemetryTestUtils.getProcessScalars("parent", false, true),
+      "telemetry.test.boolean_kind"
+    );
+  });
+
   add_task(async function test_telemetry_scalar_set_unknown_name() {
     let { messages } = await promiseConsoleOutput(async function() {
       await run({
@@ -186,6 +210,25 @@ if (AppConstants.MOZ_BUILD_APP === "browser") {
     );
   });
 
+  add_task(async function test_telemetry_scalar_set_zero() {
+    Services.telemetry.clearScalars();
+    await run({
+      backgroundScript: async () => {
+        await browser.telemetry.scalarSet(
+          "telemetry.test.unsigned_int_kind",
+          0
+        );
+        browser.test.notifyPass("scalar_set_zero");
+      },
+      doneSignal: "scalar_set_zero",
+    });
+    TelemetryTestUtils.assertScalar(
+      TelemetryTestUtils.getProcessScalars("parent", false, true),
+      "telemetry.test.unsigned_int_kind",
+      0
+    );
+  });
+
   add_task(async function test_telemetry_scalar_set_maximum() {
     Services.telemetry.clearScalars();
     await run({
@@ -590,7 +633,7 @@ if (AppConstants.MOZ_BUILD_APP === "browser") {
       doneSignal: "register_scalars_string",
     });
     TelemetryTestUtils.assertScalar(
-      TelemetryTestUtils.getProcessScalars("parent", false, true),
+      TelemetryTestUtils.getProcessScalars("dynamic", false, true),
       "telemetry.test.dynamic.webext_string",
       "hello"
     );
@@ -624,7 +667,11 @@ if (AppConstants.MOZ_BUILD_APP === "browser") {
       },
       doneSignal: "register_scalars_multiple",
     });
-    const scalars = TelemetryTestUtils.getProcessScalars("parent", false, true);
+    const scalars = TelemetryTestUtils.getProcessScalars(
+      "dynamic",
+      false,
+      true
+    );
     TelemetryTestUtils.assertScalar(
       scalars,
       "telemetry.test.dynamic.webext_string",

toolkit/components/telemetry/tests/utils/TelemetryTestUtils.jsm

@@ -12,19 +12,24 @@ var TelemetryTestUtils = {
   /* Scalars */
 
   /**
-   * An helper that asserts the value of a scalar if it's expected to be > 0,
-   * otherwise makes sure that the scalar has not been reported.
+   * A helper that asserts the value of a scalar.
    *
    * @param {Object} scalars The snapshot of the scalars.
    * @param {String} scalarName The name of the scalar to check.
-   * @param {Number} value The expected value for the provided scalar.
+   * @param {Boolean|Number|String} value The expected value for the scalar.
    * @param {String} msg The message to print when checking the value.
    */
   assertScalar(scalars, scalarName, value, msg) {
-    if (value > 0) {
-      Assert.equal(scalars[scalarName], value, msg);
-      return;
-    }
+    Assert.equal(scalars[scalarName], value, msg);
+  },
+
+  /**
+   * A helper that asserts a scalar is not set.
+   *
+   * @param {Object} scalars The snapshot of the scalars.
+   * @param {String} scalarName The name of the scalar to check.
+   */
+  assertScalarUnset(scalars, scalarName) {
     Assert.ok(!(scalarName in scalars), scalarName + " must not be reported.");
   },
 

toolkit/modules/tests/xpcshell/test_AllowedAppSources.js

@@ -83,12 +83,10 @@ function runTest(
 
   OsEnvironment.reportAllowedAppSources();
 
-  const scalars = TelemetryTestUtils.getProcessScalars("parent");
-  // Can't use TelemetryTestUtils.assertScalar here. It does not currently work
-  // for strings. See Bug 1633883.
-  const scalarName = "os.environment.allowed_app_sources";
-  Assert.ok(
-    scalarName in scalars && scalars[scalarName] === expectedScalarValue,
+  TelemetryTestUtils.assertScalar(
+    TelemetryTestUtils.getProcessScalars("parent"),
+    "os.environment.allowed_app_sources",
+    expectedScalarValue,
     "The allowed app sources reported should match the expected sources"
   );
 }