Bug 1630420 - Check last build from appinfo instead of tracking it manually in Normandy r=mossop

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

testing/modules/AppInfo.jsm

@@ -24,35 +24,32 @@ let origRuntime = Cc["@mozilla.org/xre/app-info;1"].getService(
  *   version:         nsIXULAppInfo.version
  *   platformVersion: nsIXULAppInfo.platformVersion
  *   OS:              nsIXULRuntime.OS
+ *   appBuildID:      nsIXULRuntime.appBuildID
+ *   lastAppBuildID:  nsIXULRuntime.lastAppBuildID
+ *   lastAppVersion:  nsIXULRuntime.lastAppVersion
  *
  *   crashReporter:   nsICrashReporter interface is implemented if true
  *   extraProps:      extra properties added to XULAppInfo
  */
 var newAppInfo = function(options = {}) {
-  let ID = "ID" in options ? options.ID : "xpcshell@tests.mozilla.org";
-  let name = "name" in options ? options.name : "xpcshell";
-  let version = "version" in options ? options.version : "1";
-  let platformVersion =
-    "platformVersion" in options ? options.platformVersion : "p-ver";
-  let OS = "OS" in options ? options.OS : "XPCShell";
   let extraProps = "extraProps" in options ? options.extraProps : {};
 
   let appInfo = {
     // nsIXULAppInfo
     vendor: "Mozilla",
-    name,
-    ID,
-    version,
-    appBuildID: "20160315",
+    name: options.name ?? "xpcshell",
+    ID: options.ID ?? "xpcshell@tests.mozilla.org",
+    version: options.version ?? "1",
+    appBuildID: options.appBuildID ?? "20160315",
 
     // nsIPlatformInfo
-    platformVersion,
+    platformVersion: options.platformVersion ?? "p-ver",
     platformBuildID: origPlatformInfo.platformBuildID,
 
     // nsIXULRuntime
     inSafeMode: false,
     logConsoleErrors: true,
-    OS,
+    OS: options.OS ?? "XPCShell",
     XPCOMABI: "noarch-spidermonkey",
     invalidateCachesOnRestart() {},
     shouldBlockIncompatJaws: false,
@@ -65,12 +62,15 @@ var newAppInfo = function(options = {}) {
     },
   };
 
+  appInfo.lastAppBuildID = options.lastAppBuildID ?? appInfo.appBuildID;
+  appInfo.lastAppVersion = options.lastAppVersion ?? appInfo.version;
+
   let interfaces = [Ci.nsIXULAppInfo, Ci.nsIPlatformInfo, Ci.nsIXULRuntime];
   if ("nsIWinAppHelper" in Ci) {
     interfaces.push(Ci.nsIWinAppHelper);
   }
 
-  if ("crashReporter" in options && options.crashReporter) {
+  if (options.crashReporter) {
     // nsICrashReporter
     appInfo.annotations = {};
     appInfo.annotateCrashReport = function(key, data) {

toolkit/components/normandy/NormandyMigrations.jsm

@@ -68,6 +68,7 @@ const NormandyMigrations = {
     PreferenceExperiments.migrations.migration04RenameNameToSlug,
     RecipeRunner.migrations.migration01RemoveOldRecipesCollection,
     AddonStudies.migrations.migration02RemoveOldAddonStudyAction,
+    migrateRemoveLastBuildIdPref,
   ],
 };
 
@@ -143,3 +144,11 @@ function migrateStudiesEnabledWithoutHealthReporting() {
     optOutStudiesEnabled && healthReportUploadEnabled
   );
 }
+
+/**
+ * Tracking last build ID is now done by comparing Services.appinfo.appBuildID
+ * and Services.appinfo.lastAppBuildID. Remove the manual tracking.
+ */
+function migrateRemoveLastBuildIdPref() {
+  Services.prefs.clearUserPref("app.normandy.last_seen_buildid");
+}

toolkit/components/normandy/lib/RecipeRunner.jsm

@@ -48,7 +48,6 @@ const SHIELD_ENABLED_PREF = "app.normandy.enabled";
 const DEV_MODE_PREF = "app.normandy.dev_mode";
 const API_URL_PREF = "app.normandy.api_url";
 const LAZY_CLASSIFY_PREF = "app.normandy.experiments.lazy_classify";
-const LAST_BUILDID_PREF = "app.normandy.last_seen_buildid";
 const ONSYNC_SKEW_SEC_PREF = "app.normandy.onsync_skew_sec";
 
 // Timer last update preference.
@@ -102,16 +101,9 @@ var RecipeRunner = {
     // If we've seen a build ID from a previous run that doesn't match the
     // current build ID, run immediately. This is probably an upgrade or
     // downgrade, which may cause recipe eligibility to change.
-    let lastSeenBuildID = Services.prefs.getCharPref(LAST_BUILDID_PREF, "");
     let hasNewBuildID =
-      lastSeenBuildID && Services.appinfo.appBuildID != lastSeenBuildID;
-
-    if (hasNewBuildID || !lastSeenBuildID) {
-      Services.prefs.setCharPref(
-        LAST_BUILDID_PREF,
-        Services.appinfo.appBuildID
-      );
-    }
+      Services.appinfo.lastAppBuildID != null &&
+      Services.appinfo.lastAppBuildID != Services.appinfo.appBuildID;
 
     // Dev mode is a mode used for development and QA that bypasses the normal
     // timer function of Normandy, to make testing more convenient.

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

@@ -441,7 +441,7 @@ decorate_task(
     await RecipeRunner.init();
     ok(
       !runStub.called,
-      "RecipeRunner.run is called immediately when not in dev mode or first run"
+      "RecipeRunner.run should not be called immediately when not in dev mode or first run"
     );
     ok(registerTimerStub.called, "RecipeRunner.init registers a timer");
   }
@@ -509,31 +509,6 @@ decorate_task(
   }
 );
 
-// Test that new build IDs trigger immediate recipe runs
-decorate_task(
-  withPrefEnv({
-    set: [
-      ["datareporting.healthreport.uploadEnabled", true], // telemetry enabled
-      ["app.normandy.last_seen_buildid", "not-the-current-buildid"],
-    ],
-  }),
-  withStub(RecipeRunner, "run"),
-  withStub(RecipeRunner, "registerTimer"),
-  withStub(RecipeRunner, "watchPrefs"),
-  async function testInitFirstRun(runStub, registerTimerStub, watchPrefsStub) {
-    await RecipeRunner.init();
-    Assert.deepEqual(
-      runStub.args,
-      [[{ trigger: "newBuildID" }]],
-      "RecipeRunner.run is called immediately on a new build ID"
-    );
-    ok(
-      registerTimerStub.called,
-      "RecipeRunner.registerTimer registers a timer"
-    );
-  }
-);
-
 // Test that prefs are watched correctly
 decorate_task(
   withPrefEnv({

toolkit/components/normandy/test/unit/test_RecipeRunner.js

@@ -0,0 +1,34 @@
+/**
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+const { updateAppInfo } = ChromeUtils.import(
+  "resource://testing-common/AppInfo.jsm"
+);
+const { RecipeRunner } = ChromeUtils.import(
+  "resource://normandy/lib/RecipeRunner.jsm"
+);
+
+// Test that new build IDs trigger immediate recipe runs
+add_task(async () => {
+  updateAppInfo({
+    appBuildID: "new-build-id",
+    lastAppBuildID: "old-build-id",
+  });
+  const runStub = sinon.stub(RecipeRunner, "run");
+  const registerTimerStub = sinon.stub(RecipeRunner, "registerTimer");
+  sinon.stub(RecipeRunner, "watchPrefs");
+
+  Services.prefs.setBoolPref("app.normandy.first_run", false);
+
+  await RecipeRunner.init();
+  Assert.deepEqual(
+    runStub.args,
+    [[{ trigger: "newBuildID" }]],
+    "RecipeRunner.run is called immediately on a new build ID"
+  );
+  ok(registerTimerStub.called, "RecipeRunner.registerTimer registers a timer");
+
+  sinon.restore();
+});

toolkit/components/normandy/test/unit/xpcshell.ini

@@ -12,3 +12,4 @@ tags = normandy
 
 [test_addon_unenroll.js]
 [test_NormandyApi.js]
+[test_RecipeRunner.js]

toolkit/modules/tests/xpcshell/test_firstStartup.js

@@ -6,11 +6,15 @@ const { AppConstants } = ChromeUtils.import(
 const { FirstStartup } = ChromeUtils.import(
   "resource://gre/modules/FirstStartup.jsm"
 );
+const { updateAppInfo } = ChromeUtils.import(
+  "resource://testing-common/AppInfo.jsm"
+);
 
 const PREF_TIMEOUT = "first-startup.timeout";
 const PROBE_NAME = "firstStartup.statusCode";
 
 add_task(async function test_success() {
+  updateAppInfo();
   FirstStartup.init();
   if (AppConstants.MOZ_NORMANDY) {
     equal(FirstStartup.state, FirstStartup.SUCCESS);
@@ -30,6 +34,7 @@ add_task(async function test_success() {
 });
 
 add_task(async function test_timeout() {
+  updateAppInfo();
   Services.prefs.setIntPref(PREF_TIMEOUT, 0);
   FirstStartup.init();