From cbcf8a3fecc889064b0034e6a4d058649183eb2f Mon Sep 17 00:00:00 2001 From: Csoregi Natalia Date: Wed, 3 Mar 2021 13:36:59 +0200 Subject: [PATCH] Backed out changeset 2ee3856bd2ae (bug 1693301) for failures on browser_remotesettingsexperimentloader_init.js. CLOSED TREE --- .../browser_aboutwelcome_multistage.js | 80 +++++------ toolkit/components/nimbus/docs/index.rst | 1 - toolkit/components/nimbus/docs/testing.md | 61 --------- .../components/nimbus/lib/ExperimentStore.jsm | 2 +- .../nimbus/test/NimbusTestUtils.jsm | 40 ------ ...ser_remotesettingsexperimentloader_init.js | 126 +++++++++--------- .../nimbus/test/unit/test_NimbusTestUtils.js | 37 ----- 7 files changed, 98 insertions(+), 249 deletions(-) delete mode 100644 toolkit/components/nimbus/docs/testing.md diff --git a/browser/components/newtab/test/browser/browser_aboutwelcome_multistage.js b/browser/components/newtab/test/browser/browser_aboutwelcome_multistage.js index 333064a9d248..5a7960f76745 100644 --- a/browser/components/newtab/test/browser/browser_aboutwelcome_multistage.js +++ b/browser/components/newtab/test/browser/browser_aboutwelcome_multistage.js @@ -232,34 +232,30 @@ add_task(async function setup() { */ add_task(async function test_multistage_zeroOnboarding_experimentAPI() { await setAboutWelcomePref(true); + let updatePromise = ExperimentFakes.waitForExperimentUpdate(ExperimentAPI, { + slug: "mochitest-1-aboutwelcome", + }); + ExperimentAPI._store.addExperiment({ + slug: "mochitest-1-aboutwelcome", + branch: { + slug: "mochitest-1-aboutwelcome", + feature: { + enabled: false, + featureId: "aboutwelcome", + value: null, + }, + }, + active: true, + }); - let { - enrollmentPromise, - doExperimentCleanup, - } = ExperimentFakes.enrollmentHelper( - ExperimentFakes.recipe("mochitest-1-aboutwelcome", { - branches: [ - { - slug: "mochitest-1-aboutwelcome", - feature: { - enabled: false, - featureId: "aboutwelcome", - value: null, - }, - }, - ], - active: true, - }) - ); - - await enrollmentPromise; + await updatePromise; + ExperimentAPI._store._syncToChildren({ flush: true }); let tab = await BrowserTestUtils.openNewForegroundTab( gBrowser, "about:welcome", true ); - registerCleanupFunction(() => { BrowserTestUtils.removeTab(tab); }); @@ -275,7 +271,7 @@ add_task(async function test_multistage_zeroOnboarding_experimentAPI() { ["div.onboardingContainer", "main.AW_STEP1"] ); - await doExperimentCleanup(); + ExperimentAPI._store._deleteForTests("mochitest-1-aboutwelcome"); Assert.equal(ExperimentAPI._store.getAll().length, 0, "Cleanup done"); }); @@ -284,27 +280,25 @@ add_task(async function test_multistage_zeroOnboarding_experimentAPI() { */ add_task(async function test_multistage_aboutwelcome_experimentAPI() { await setAboutWelcomePref(true); + await setAboutWelcomeMultiStage({}); + let updatePromise = ExperimentFakes.waitForExperimentUpdate(ExperimentAPI, { + slug: "mochitest-aboutwelcome", + }); + ExperimentAPI._store.addExperiment({ + slug: "mochitest-aboutwelcome", + branch: { + slug: "mochitest-aboutwelcome", + feature: { + enabled: true, + featureId: "aboutwelcome", + value: TEST_MULTISTAGE_CONTENT, + }, + }, + active: true, + }); - let { - enrollmentPromise, - doExperimentCleanup, - } = ExperimentFakes.enrollmentHelper( - ExperimentFakes.recipe("mochitest-aboutwelcome", { - branches: [ - { - slug: "mochitest-aboutwelcome-branch", - feature: { - enabled: true, - featureId: "aboutwelcome", - value: TEST_MULTISTAGE_CONTENT, - }, - }, - ], - active: true, - }) - ); - - await enrollmentPromise; + await updatePromise; + ExperimentAPI._store._syncToChildren({ flush: true }); let tab = await BrowserTestUtils.openNewForegroundTab( gBrowser, @@ -383,7 +377,7 @@ add_task(async function test_multistage_aboutwelcome_experimentAPI() { ["div.onboardingContainer"] ); - await doExperimentCleanup(); + ExperimentAPI._store._deleteForTests("mochitest-aboutwelcome"); Assert.equal(ExperimentAPI._store.getAll().length, 0, "Cleanup done"); }); diff --git a/toolkit/components/nimbus/docs/index.rst b/toolkit/components/nimbus/docs/index.rst index c1fc63fdba7f..75ab9076f0c6 100644 --- a/toolkit/components/nimbus/docs/index.rst +++ b/toolkit/components/nimbus/docs/index.rst @@ -13,4 +13,3 @@ Learn more :maxdepth: 2 integration.md - testing.md \ No newline at end of file diff --git a/toolkit/components/nimbus/docs/testing.md b/toolkit/components/nimbus/docs/testing.md deleted file mode 100644 index b61c95f4671a..000000000000 --- a/toolkit/components/nimbus/docs/testing.md +++ /dev/null @@ -1,61 +0,0 @@ -# Nimbus Testing Helpers - -In order to make testing easier we created some helpers that can be accessed by including - -```js -const { ExperimentFakes } = ChromeUtils.import( - "resource://testing-common/NimbusTestUtils.jsm" -); -``` - -## Testing your feature integrating with Nimbus - -1. You need to create a recipe - -```js -let recipe = ExperimentFakes.recipe("my-cool-experiment", { - branches: [ - { - slug: "treatment-branch", - ratio: 1, - feature: { - featureId: "", - // The feature is on - enabled: true, - // If you defined `variables` in the MANIFEST - // the `value` should match that schema - value: null, - }, - }, - ], - bucketConfig: { - start: 0, - // Ensure 100% enrollment - count: 10000, - total: 10000, - namespace: "my-mochitest", - randomizationUnit: "normandy_id", - }, -}); -``` - -2. Now with the newly created recipe you want the test to enroll in the experiment - -```js -let { - enrollmentPromise, - doExperimentCleanup, -} = ExperimentFakes.enrollmentHelper(recipe); - -// Await for enrollment to complete - -await enrollmentPromise; - -// Now you can assume the feature is enabled so you can -// test and that it's doing the right thing - -// Assert.ok(It works!) - -// Finishing up -await doExperimentCleanup(); -``` diff --git a/toolkit/components/nimbus/lib/ExperimentStore.jsm b/toolkit/components/nimbus/lib/ExperimentStore.jsm index 5920c26f5f59..198af18b6e2b 100644 --- a/toolkit/components/nimbus/lib/ExperimentStore.jsm +++ b/toolkit/components/nimbus/lib/ExperimentStore.jsm @@ -179,8 +179,8 @@ class ExperimentStore extends SharedDataMap { ); } this.set(experiment.slug, experiment); - this._updateSyncStore(experiment); this._emitExperimentUpdates(experiment); + this._updateSyncStore(experiment); } /** diff --git a/toolkit/components/nimbus/test/NimbusTestUtils.jsm b/toolkit/components/nimbus/test/NimbusTestUtils.jsm index a097f0d492c0..43bc2704d072 100644 --- a/toolkit/components/nimbus/test/NimbusTestUtils.jsm +++ b/toolkit/components/nimbus/test/NimbusTestUtils.jsm @@ -11,7 +11,6 @@ const { XPCOMUtils } = ChromeUtils.import( XPCOMUtils.defineLazyModuleGetters(this, { _ExperimentManager: "resource://nimbus/lib/ExperimentManager.jsm", - ExperimentManager: "resource://nimbus/lib/ExperimentManager.jsm", ExperimentStore: "resource://nimbus/lib/ExperimentStore.jsm", NormandyUtils: "resource://normandy/lib/NormandyUtils.jsm", FileTestUtils: "resource://testing-common/FileTestUtils.jsm", @@ -68,45 +67,6 @@ const ExperimentFakes = { return new Promise(resolve => ExperimentAPI.on("update", options, resolve)); }, - enrollmentHelper(recipe = {}, { manager = ExperimentManager } = {}) { - let enrollmentPromise = new Promise(resolve => - manager.store.on(`update:${recipe.slug}`, (event, experiment) => { - if (experiment.active) { - resolve(experiment); - } - }) - ); - let unenrollCompleted = slug => - new Promise(resolve => - manager.store.on(`update:${slug}`, (event, experiment) => { - if (!experiment.active) { - // Removes recipe from file storage which - // (normally the users archive of past experiments) - manager.store._deleteForTests(recipe.slug); - resolve(); - } - }) - ); - let doExperimentCleanup = async () => { - for (let experiment of manager.store.getAllActive()) { - let promise = unenrollCompleted(experiment.slug); - manager.unenroll(experiment.slug, "cleanup"); - await promise; - } - if (manager.store.getAllActive().length) { - throw new Error("Cleanup failed"); - } - }; - - if (recipe.slug) { - if (!manager.store._isReady) { - throw new Error("Manager store not ready, call `manager.onStartup`"); - } - manager.enroll(recipe); - } - - return { enrollmentPromise, doExperimentCleanup }; - }, childStore() { return new ExperimentStore("FakeStore", { isParent: false }); }, diff --git a/toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_init.js b/toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_init.js index bd35ad2f4a68..10a6b8edbde6 100644 --- a/toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_init.js +++ b/toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_init.js @@ -18,16 +18,47 @@ const { ExperimentFakes } = ChromeUtils.import( const { ExperimentManager } = ChromeUtils.import( "resource://nimbus/lib/ExperimentManager.jsm" ); -const { ExperimentAPI } = ChromeUtils.import( - "resource://nimbus/ExperimentAPI.jsm" -); const { BrowserTestUtils } = ChromeUtils.import( "resource://testing-common/BrowserTestUtils.jsm" ); const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm"); -function getRecipe(slug) { - return ExperimentFakes.recipe(slug, { +let rsClient; + +add_task(async function setup() { + await SpecialPowers.pushPrefEnv({ + set: [ + ["messaging-system.log", "all"], + ["app.shield.optoutstudies.enabled", true], + ], + }); + rsClient = RemoteSettings("nimbus-desktop-experiments"); + + registerCleanupFunction(async () => { + await SpecialPowers.popPrefEnv(); + await rsClient.db.clear(); + }); +}); + +add_task(async function test_double_feature_enrollment() { + let sandbox = sinon.createSandbox(); + // We want to prevent this because it would start a recipe + // update outside of our asserts + sandbox.stub(RemoteSettingsExperimentLoader, "setTimer"); + let sendFailureTelemetryStub = sandbox.stub( + ExperimentManager, + "sendFailureTelemetry" + ); + + for (let experiment of ExperimentManager.store.getAllActive()) { + ExperimentManager.unenroll(experiment.slug, "cleanup"); + ExperimentManager.store._deleteForTests(experiment.slug); + } + await BrowserTestUtils.waitForCondition( + () => ExperimentManager.store.getAllActive().length === 0 + ); + + const recipe1 = ExperimentFakes.recipe("foo" + Date.now(), { bucketConfig: { start: 0, // Make sure the experiment enrolls @@ -36,84 +67,47 @@ function getRecipe(slug) { namespace: "mochitest", randomizationUnit: "normandy_id", }, - targeting: "true", }); -} - -add_task(async function setup() { - let rsClient = RemoteSettings("nimbus-desktop-experiments"); - - await rsClient.db.importChanges( - {}, - 42, - [getRecipe("foo" + Math.random()), getRecipe("bar" + Math.random())], - { - clear: true, - } - ); - - registerCleanupFunction(async () => { - await rsClient.db.clear(); - }); -}); - -async function setup() { - let sandbox = sinon.createSandbox(); - // We want to prevent this because it would start a recipe - // update as soon as we turn on the optoutstudies pref - sandbox.stub(RemoteSettingsExperimentLoader, "setTimer"); - sandbox.stub(RemoteSettingsExperimentLoader, "onEnabledPrefChange"); - RemoteSettingsExperimentLoader._updating = true; - - await SpecialPowers.pushPrefEnv({ - set: [ - ["messaging-system.log", "all"], - ["app.shield.optoutstudies.enabled", true], - ], + const recipe2 = ExperimentFakes.recipe("foo" + Date.now(), { + bucketConfig: { + start: 0, + // Make sure the experiment enrolls + count: 10000, + total: 10000, + namespace: "mochitest", + randomizationUnit: "normandy_id", + }, }); - registerCleanupFunction(async () => { - await SpecialPowers.popPrefEnv(); - sandbox.restore(); + await rsClient.db.importChanges({}, 42, [recipe1, recipe2], { + clear: true, }); -} -add_task(async function test_double_feature_enrollment() { - let { doExperimentCleanup } = ExperimentFakes.enrollmentHelper(); - await doExperimentCleanup(); - await setup(); RemoteSettingsExperimentLoader.uninit(); - let sandbox = sinon.createSandbox(); - let sendFailureTelemetryStub = sandbox.stub( - ExperimentManager, - "sendFailureTelemetry" + let enrolledPromise = new Promise(resolve => + ExperimentManager.store.on("update:test-feature", resolve) ); - - Assert.ok(ExperimentManager.store.getAllActive().length === 0, "Clean state"); - await RemoteSettingsExperimentLoader.init(); - - await ExperimentFakes.waitForExperimentUpdate(ExperimentAPI, { - featureId: "test-feature", - }); + await enrolledPromise; Assert.ok( RemoteSettingsExperimentLoader._initialized, "It should initialize and process the recipes" ); - Assert.equal( - ExperimentManager.store.getAllActive().length, - 1, - "1 active experiment" - ); - - await BrowserTestUtils.waitForCondition( - () => sendFailureTelemetryStub.callCount, + BrowserTestUtils.waitForCondition( + () => sendFailureTelemetryStub.calledOnce, "Expected to fail one of the recipes" ); - await doExperimentCleanup(); + for (let experiment of ExperimentManager.store.getAllActive()) { + ExperimentManager.unenroll(experiment.slug, "cleanup"); + ExperimentManager.store._deleteForTests(experiment.slug); + } + await BrowserTestUtils.waitForCondition( + () => ExperimentManager.store.getAllActive().length === 0 + ); await SpecialPowers.popPrefEnv(); + await rsClient.db.clear(); sandbox.restore(); }); diff --git a/toolkit/components/nimbus/test/unit/test_NimbusTestUtils.js b/toolkit/components/nimbus/test/unit/test_NimbusTestUtils.js index 1fbc4ab3b57a..c507eba4c9e2 100644 --- a/toolkit/components/nimbus/test/unit/test_NimbusTestUtils.js +++ b/toolkit/components/nimbus/test/unit/test_NimbusTestUtils.js @@ -11,40 +11,3 @@ add_task(async function test_recipe_fake_validates() { "should produce a valid experiment recipe" ); }); - -add_task(async function test_enrollmentHelper() { - let recipe = ExperimentFakes.recipe("bar"); - recipe.branches.forEach(branch => { - // Use a feature that will set the sync pref cache - branch.feature.featureId = "aboutwelcome"; - }); - let manager = ExperimentFakes.manager(); - - await manager.onStartup(); - - let { - enrollmentPromise, - doExperimentCleanup, - } = ExperimentFakes.enrollmentHelper(recipe, { manager }); - - await enrollmentPromise; - - Assert.ok(manager.store.getAllActive().length === 1, "Enrolled"); - Assert.equal( - manager.store.getAllActive()[0].slug, - recipe.slug, - "Has expected slug" - ); - Assert.ok( - Services.prefs.prefHasUserValue("nimbus.syncdatastore.aboutwelcome"), - "Sync pref cache set" - ); - - await doExperimentCleanup(); - - Assert.ok(manager.store.getAll().length === 0, "Cleanup done"); - Assert.ok( - !Services.prefs.prefHasUserValue("nimbus.syncdatastore.aboutwelcome"), - "Sync pref cache is cleared" - ); -});