From 8e29cbfa100ec372484f959cadc470141082ff43 Mon Sep 17 00:00:00 2001 From: Michael Cooper Date: Mon, 4 May 2020 18:27:40 +0000 Subject: [PATCH] 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 | 26 +++++++------- .../normandy/NormandyMigrations.jsm | 9 +++++ .../components/normandy/lib/RecipeRunner.jsm | 12 ++----- .../test/browser/browser_RecipeRunner.js | 27 +-------------- .../normandy/test/unit/test_RecipeRunner.js | 34 +++++++++++++++++++ .../normandy/test/unit/xpcshell.ini | 1 + .../tests/xpcshell/test_firstStartup.js | 5 +++ 7 files changed, 65 insertions(+), 49 deletions(-) create mode 100644 toolkit/components/normandy/test/unit/test_RecipeRunner.js diff --git a/testing/modules/AppInfo.jsm b/testing/modules/AppInfo.jsm index d0e78b7afdc0..138e5c3fbaca 100644 --- a/testing/modules/AppInfo.jsm +++ b/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) { diff --git a/toolkit/components/normandy/NormandyMigrations.jsm b/toolkit/components/normandy/NormandyMigrations.jsm index baea747775a6..0be18939b796 100644 --- a/toolkit/components/normandy/NormandyMigrations.jsm +++ b/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"); +} diff --git a/toolkit/components/normandy/lib/RecipeRunner.jsm b/toolkit/components/normandy/lib/RecipeRunner.jsm index f93875f9bcb7..ceea037abf6d 100644 --- a/toolkit/components/normandy/lib/RecipeRunner.jsm +++ b/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. diff --git a/toolkit/components/normandy/test/browser/browser_RecipeRunner.js b/toolkit/components/normandy/test/browser/browser_RecipeRunner.js index 5a8cc0f63832..c60b4b1ac692 100644 --- a/toolkit/components/normandy/test/browser/browser_RecipeRunner.js +++ b/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({ diff --git a/toolkit/components/normandy/test/unit/test_RecipeRunner.js b/toolkit/components/normandy/test/unit/test_RecipeRunner.js new file mode 100644 index 000000000000..38b8cce8ce32 --- /dev/null +++ b/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(); +}); diff --git a/toolkit/components/normandy/test/unit/xpcshell.ini b/toolkit/components/normandy/test/unit/xpcshell.ini index 2634f539629d..ca2c9c65d7c0 100644 --- a/toolkit/components/normandy/test/unit/xpcshell.ini +++ b/toolkit/components/normandy/test/unit/xpcshell.ini @@ -12,3 +12,4 @@ tags = normandy [test_addon_unenroll.js] [test_NormandyApi.js] +[test_RecipeRunner.js] diff --git a/toolkit/modules/tests/xpcshell/test_firstStartup.js b/toolkit/modules/tests/xpcshell/test_firstStartup.js index aa82d5d5d4a8..a5fce5b563bb 100644 --- a/toolkit/modules/tests/xpcshell/test_firstStartup.js +++ b/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();