Bug 1773583 - remove stub of enrollmentPromise from nimbus' Experimen…

…tFakes r=barret

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

browser/components/newtab/test/browser/browser_asrouter_remoteimages.js

@@ -380,12 +380,11 @@ add_task(async function test_remoteImages_prefetch() {
     PREFETCH_FINISHED_TOPIC
   );
 
-  const {
-    enrollmentPromise,
-    doExperimentCleanup,
-  } = ExperimentFakes.enrollmentHelper(recipe);
+  const { doExperimentCleanup } = await ExperimentFakes.enrollmentHelper(
+    recipe
+  );
 
-  await Promise.all([enrollmentPromise, prefetchFinished]);
+  await prefetchFinished;
 
   try {
     await RemoteImages.withDb(async db => {

browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_migrate_v2.js

@@ -1333,7 +1333,7 @@ async function withOnlineExperiment(callback) {
   let {
     enrollmentPromise,
     doExperimentCleanup,
-  } = ExperimentFakes.enrollmentHelper(
+  } = await ExperimentFakes.enrollmentHelper(
     ExperimentFakes.recipe("firefox-suggest-offline-vs-online", {
       active: true,
       branches: [

toolkit/components/nimbus/test/NimbusTestUtils.jsm

@@ -25,7 +25,6 @@ XPCOMUtils.defineLazyModuleGetters(lazy, {
   NormandyUtils: "resource://normandy/lib/NormandyUtils.jsm",
   _RemoteSettingsExperimentLoader:
     "resource://nimbus/lib/RemoteSettingsExperimentLoader.jsm",
-  sinon: "resource://testing-common/Sinon.jsm",
   FeatureManifest: "resource://nimbus/FeatureManifest.js",
   JsonSchema: "resource://gre/modules/JsonSchema.jsm",
 });
@@ -170,17 +169,7 @@ const ExperimentTestUtils = {
 
 const ExperimentFakes = {
   manager(store) {
-    let sandbox = lazy.sinon.createSandbox();
-    let manager = new lazy._ExperimentManager({ store: store || this.store() });
-    // We want calls to `store.addEnrollment` to implicitly validate the
-    // enrollment before saving to store
-    let origAddExperiment = manager.store.addEnrollment.bind(manager.store);
-    sandbox.stub(manager.store, "addEnrollment").callsFake(async enrollment => {
-      await ExperimentTestUtils.validateEnrollment(enrollment);
-      return origAddExperiment(enrollment);
-    });
-
-    return manager;
+    return new lazy._ExperimentManager({ store: store || this.store() });
   },
   store() {
     return new ExperimentStore("FakeStore", {
@@ -260,24 +249,16 @@ const ExperimentFakes = {
         },
       ],
     });
-    let {
-      enrollmentPromise,
-      doExperimentCleanup,
-    } = this.enrollmentHelper(recipe, { manager });
-
-    await enrollmentPromise;
+    let { doExperimentCleanup } = await this.enrollmentHelper(recipe, {
+      manager,
+    });
 
     return doExperimentCleanup;
   },
-  enrollmentHelper(recipe = {}, { manager = lazy.ExperimentManager } = {}) {
-    let enrollmentPromise = new Promise(resolve =>
-      manager.store.on(`update:${recipe.slug}`, (event, experiment) => {
-        if (experiment.active) {
-          manager.store._syncToChildren({ flush: true });
-          resolve(experiment);
-        }
-      })
-    );
+  async enrollmentHelper(
+    recipe = {},
+    { manager = lazy.ExperimentManager } = {}
+  ) {
     let unenrollCompleted = slug =>
       new Promise(resolve =>
         manager.store.on(`update:${slug}`, (event, experiment) => {
@@ -304,10 +285,10 @@ const ExperimentFakes = {
       if (!manager.store._isReady) {
         throw new Error("Manager store not ready, call `manager.onStartup`");
       }
-      manager.enroll(recipe, "enrollmentHelper");
+      await manager.enroll(recipe, "enrollmentHelper");
     }
 
-    return { enrollmentPromise, doExperimentCleanup };
+    return { doExperimentCleanup };
   },
   // Experiment store caches in prefs Enrollments for fast sync access
   cleanupStorePrefCache() {

toolkit/components/nimbus/test/browser/browser_experiment_evaluate_jexl.js

@@ -84,12 +84,9 @@ add_task(async function test_evaluate_active_experiments_activeExperiments() {
   recipe.branches[0].slug = "mochitest-active-foo";
   delete recipe.branches[1];
 
-  let {
-    enrollmentPromise,
-    doExperimentCleanup,
-  } = ExperimentFakes.enrollmentHelper(recipe);
-
-  await enrollmentPromise;
+  const { doExperimentCleanup } = await ExperimentFakes.enrollmentHelper(
+    recipe
+  );
 
   Assert.equal(
     await RemoteSettingsExperimentLoader.evaluateJexl(

toolkit/components/nimbus/test/browser/browser_experiment_single_feature_enrollment.js

@@ -76,14 +76,12 @@ const SINGLE_FEATURE_RECIPE = {
 const SYNC_DATA_PREF_BRANCH = "nimbus.syncdatastore.";
 
 add_task(async function test_TODO() {
-  let {
-    enrollmentPromise,
-    doExperimentCleanup,
-  } = ExperimentFakes.enrollmentHelper(SINGLE_FEATURE_RECIPE);
-  let sandbox = sinon.createSandbox();
-  let stub = sandbox.stub(ExperimentAPI, "recordExposureEvent");
+  const sandbox = sinon.createSandbox();
+  const stub = sandbox.stub(ExperimentAPI, "recordExposureEvent");
 
-  await enrollmentPromise;
+  const { doExperimentCleanup } = await ExperimentFakes.enrollmentHelper(
+    SINGLE_FEATURE_RECIPE
+  );
 
   Assert.ok(
     ExperimentAPI.getExperiment({ featureId: "urlbar" }),

toolkit/components/nimbus/test/browser/browser_nimbus_telemetry.js

@@ -21,10 +21,11 @@ const EXPERIMENT_TYPE = "nimbus";
 const EVENT_FILTER = { category: TELEMETRY_CATEGORY };
 
 add_setup(async function() {
-  let sandbox = sinon.createSandbox();
+  const sandbox = sinon.createSandbox();
   // stub the `observe` method to make sure the Experiment Manager
   // pref listener doesn't trigger and cause side effects
   sandbox.stub(ExperimentManager, "observe");
+
   await SpecialPowers.pushPrefEnv({
     set: [["app.shield.optoutstudies.enabled", true]],
   });

toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_init.js

@@ -32,7 +32,7 @@ function getRecipe(slug) {
 
 add_task(async function test_double_feature_enrollment() {
   let sandbox = sinon.createSandbox();
-  let { doExperimentCleanup } = ExperimentFakes.enrollmentHelper();
+  let { doExperimentCleanup } = await ExperimentFakes.enrollmentHelper();
   let sendFailureTelemetryStub = sandbox.stub(
     ExperimentManager,
     "sendFailureTelemetry"

toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_remote_defaults.js

@@ -117,6 +117,7 @@ async function setup(configuration) {
 }
 
 add_task(async function test_remote_fetch_and_ready() {
+  const sandbox = sinon.createSandbox();
   const fooInstance = new ExperimentFeature("foo", FOO_FAKE_FEATURE_MANIFEST);
   const barInstance = new ExperimentFeature("bar", BAR_FAKE_FEATURE_MANIFEST);
 
@@ -125,7 +126,6 @@ add_task(async function test_remote_fetch_and_ready() {
     barInstance
   );
 
-  const sandbox = sinon.createSandbox();
   const setExperimentActiveStub = sandbox.stub(
     TelemetryEnvironment,
     "setExperimentActive"
@@ -267,10 +267,12 @@ add_task(async function test_remote_fetch_and_ready() {
 
   cleanupTestFeatures();
   await cleanup();
+  sandbox.restore();
 });
 
 add_task(async function test_remote_fetch_on_updateRecipes() {
-  let sandbox = sinon.createSandbox();
+  const sandbox = sinon.createSandbox();
+
   let updateRecipesStub = sandbox.stub(
     RemoteSettingsExperimentLoader,
     "updateRecipes"
@@ -305,6 +307,7 @@ add_task(async function test_remote_fetch_on_updateRecipes() {
   Services.prefs.clearUserPref(
     "app.update.lastUpdateTime.rs-experiment-loader-timer"
   );
+  sandbox.restore();
 });
 
 add_task(async function test_finalizeRemoteConfigs_cleanup() {
@@ -346,6 +349,7 @@ add_task(async function test_finalizeRemoteConfigs_cleanup() {
       source: "rs-loader",
     }
   );
+
   let stubFoo = sinon.stub();
   let stubBar = sinon.stub();
   featureFoo.onUpdate(stubFoo);
@@ -411,7 +415,8 @@ add_task(async function test_finalizeRemoteConfigs_cleanup() {
 // If the remote config data returned from the store is not modified
 // this test should not throw
 add_task(async function remote_defaults_no_mutation() {
-  let sandbox = sinon.createSandbox();
+  const sandbox = sinon.createSandbox();
+
   sandbox.stub(ExperimentAPI._store, "getRolloutForFeature").returns(
     Cu.cloneInto(
       {
@@ -434,6 +439,8 @@ add_task(async function remote_defaults_no_mutation() {
 });
 
 add_task(async function remote_defaults_active_remote_defaults() {
+  const sandbox = sinon.createSandbox();
+
   ExperimentAPI._store._deleteForTests("foo");
   ExperimentAPI._store._deleteForTests("bar");
   let barFeature = new ExperimentFeature("bar", {
@@ -486,27 +493,28 @@ add_task(async function remote_defaults_active_remote_defaults() {
     targeting: "'bar' in activeRollouts",
   });
 
-  // Order is important, rollout2 won't match at first
+  // Order is important, rollout2 won't match at first.
+  // We cannot use setup() here becuase RS returns records in a
+  // non-deterministic order.
+  sandbox
+    .stub(RemoteSettingsExperimentLoader.remoteSettingsClient, "get")
+    .resolves([rollout2, rollout1]);
   const { cleanup } = await setup([rollout2, rollout1]);
-  let updatePromise = new Promise(resolve => barFeature.onUpdate(resolve));
   RemoteSettingsExperimentLoader._initialized = true;
   await RemoteSettingsExperimentLoader.updateRecipes("mochitest");
 
-  await updatePromise;
-
   Assert.ok(barFeature.getVariable("enabled"), "Enabled on first sync");
   Assert.ok(!fooFeature.getVariable("enabled"), "Targeting doesn't match");
 
-  let featureUpdate = new Promise(resolve => fooFeature.onUpdate(resolve));
   await RemoteSettingsExperimentLoader.updateRecipes("mochitest");
-  await featureUpdate;
 
   Assert.ok(fooFeature.getVariable("enabled"), "Targeting should match");
   ExperimentAPI._store._deleteForTests("foo");
   ExperimentAPI._store._deleteForTests("bar");
 
   cleanup();
   cleanupTestFeatures();
+  sandbox.restore();
 });
 
 add_task(async function remote_defaults_variables_storage() {

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

@@ -13,26 +13,3 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   ExperimentTestUtils: "resource://testing-common/NimbusTestUtils.jsm",
   ExperimentFakes: "resource://testing-common/NimbusTestUtils.jsm",
 });
-
-add_setup(function() {
-  let sandbox = sinon.createSandbox();
-
-  /* We stub the functions that operate with enrollments and remote rollouts
-   * so that any access to store something is implicitly validated against
-   * the schema and no records have missing (or extra) properties while in tests
-   */
-
-  let origAddExperiment = ExperimentManager.store.addEnrollment.bind(
-    ExperimentManager.store
-  );
-  sandbox
-    .stub(ExperimentManager.store, "addEnrollment")
-    .callsFake(async enrollment => {
-      await ExperimentTestUtils.validateEnrollment(enrollment);
-      return origAddExperiment(enrollment);
-    });
-
-  registerCleanupFunction(() => {
-    sandbox.restore();
-  });
-});

toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature_getAllVariables.js

@@ -67,7 +67,7 @@ add_task(
 add_task(
   async function test_ExperimentFeature_getAllVariables_prefsOverExperiment() {
     const { sandbox, manager } = await setupForExperimentFeature();
-    const { doExperimentCleanup } = ExperimentFakes.enrollmentHelper(
+    const { doExperimentCleanup } = await ExperimentFakes.enrollmentHelper(
       undefined,
       {
         manager,
@@ -123,7 +123,7 @@ add_task(
   async function test_ExperimentFeature_getAllVariables_experimentOverRemote() {
     Services.prefs.clearUserPref(TEST_FALLBACK_PREF);
     const { manager } = await setupForExperimentFeature();
-    const { doExperimentCleanup } = ExperimentFakes.enrollmentHelper(
+    const { doExperimentCleanup } = await ExperimentFakes.enrollmentHelper(
       undefined,
       {
         manager,

toolkit/components/nimbus/test/unit/test_ExperimentManager_enroll.js

@@ -754,15 +754,17 @@ add_task(async function test_featureIds_is_stored() {
   recipe.bucketConfig.count = recipe.bucketConfig.total;
   const store = ExperimentFakes.store();
   const manager = ExperimentFakes.manager(store);
+  const sandbox = sinon.createSandbox();
+  sandbox.spy(store, "addEnrollment");
 
   await manager.onStartup();
 
-  const {
-    enrollmentPromise,
-    doExperimentCleanup,
-  } = ExperimentFakes.enrollmentHelper(recipe, { manager });
-
-  await enrollmentPromise;
+  const { doExperimentCleanup } = await ExperimentFakes.enrollmentHelper(
+    recipe,
+    {
+      manager,
+    }
+  );
 
   Assert.ok(manager.store.addEnrollment.calledOnce, "experiment is stored");
   let [enrollment] = manager.store.addEnrollment.firstCall.args;
@@ -774,6 +776,7 @@ add_task(async function test_featureIds_is_stored() {
   );
 
   await doExperimentCleanup();
+  sandbox.restore();
 });
 
 add_task(async function experiment_and_rollout_enroll_and_cleanup() {

toolkit/components/nimbus/test/unit/test_ExperimentManager_unenroll.js

@@ -200,7 +200,7 @@ add_task(async function test_unenroll_uploadPref() {
   const recipe = ExperimentFakes.recipe("foo");
 
   await manager.onStartup();
-  await ExperimentFakes.enrollmentHelper(recipe, { manager }).enrollmentPromise;
+  await ExperimentFakes.enrollmentHelper(recipe, { manager });
 
   Assert.equal(
     manager.store.get(recipe.slug).active,

toolkit/components/nimbus/test/unit/test_NimbusTestUtils.js

@@ -32,14 +32,11 @@ add_task(async function test_enrollmentHelper() {
 
   await manager.onStartup();
 
-  let {
-    enrollmentPromise,
-    doExperimentCleanup,
-  } = ExperimentFakes.enrollmentHelper(recipe, { manager });
-
-  await enrollmentPromise;
+  let { doExperimentCleanup } = await ExperimentFakes.enrollmentHelper(recipe, {
+    manager,
+  });
 
-  Assert.ok(manager.store.getAllActive().length === 1, "Enrolled");
+  Assert.equal(manager.store.getAllActive().length, 1, "Enrolled");
   Assert.equal(
     manager.store.getAllActive()[0].slug,
     recipe.slug,