From a1721fd18709015e080b7ee2275e82dc757b1faa Mon Sep 17 00:00:00 2001 From: Barret Rennie Date: Mon, 26 Jun 2023 21:56:12 +0000 Subject: [PATCH] Bug 1840436 - Add targeting for all enrollments, past and present r=emcminn Differential Revision: https://phabricator.services.mozilla.com/D182096 --- .../nimbus/lib/ExperimentManager.sys.mjs | 6 + .../unit/test_ExperimentManager_lifecycle.js | 9 + ...eSettingsExperimentLoader_updateRecipes.js | 186 ++++++++++++++++++ 3 files changed, 201 insertions(+) diff --git a/toolkit/components/nimbus/lib/ExperimentManager.sys.mjs b/toolkit/components/nimbus/lib/ExperimentManager.sys.mjs index b43b528681a1..4be1b3dbe557 100644 --- a/toolkit/components/nimbus/lib/ExperimentManager.sys.mjs +++ b/toolkit/components/nimbus/lib/ExperimentManager.sys.mjs @@ -128,6 +128,12 @@ export class _ExperimentManager { .map(rollout => rollout.slug); }, }); + Object.defineProperty(context, "enrollments", { + get: async () => { + await this.store.ready(); + return this.store.getAll().map(enrollment => enrollment.slug); + }, + }); return context; } diff --git a/toolkit/components/nimbus/test/unit/test_ExperimentManager_lifecycle.js b/toolkit/components/nimbus/test/unit/test_ExperimentManager_lifecycle.js index 2b1df9981441..09f8bbfcece5 100644 --- a/toolkit/components/nimbus/test/unit/test_ExperimentManager_lifecycle.js +++ b/toolkit/components/nimbus/test/unit/test_ExperimentManager_lifecycle.js @@ -487,6 +487,7 @@ add_task(async function test_context_paramters() { Assert.deepEqual(await targetingCtx.activeRollouts, []); Assert.deepEqual(await targetingCtx.previousExperiments, []); Assert.deepEqual(await targetingCtx.previousRollouts, []); + Assert.deepEqual(await targetingCtx.enrollments, []); await manager.enroll(experiment, "test"); await manager.enroll(rollout, "test"); @@ -496,6 +497,10 @@ add_task(async function test_context_paramters() { Assert.deepEqual(await targetingCtx.activeRollouts, ["rollout"]); Assert.deepEqual(await targetingCtx.previousExperiments, []); Assert.deepEqual(await targetingCtx.previousRollouts, []); + Assert.deepEqual([...(await targetingCtx.enrollments)].sort(), [ + "experiment", + "rollout", + ]); manager.unenroll(experiment.slug); manager.unenroll(rollout.slug); @@ -505,4 +510,8 @@ add_task(async function test_context_paramters() { Assert.deepEqual(await targetingCtx.activeRollouts, []); Assert.deepEqual(await targetingCtx.previousExperiments, ["experiment"]); Assert.deepEqual(await targetingCtx.previousRollouts, ["rollout"]); + Assert.deepEqual([...(await targetingCtx.enrollments)].sort(), [ + "experiment", + "rollout", + ]); }); diff --git a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js index ba241ae1b5b4..f41f35f9cb4d 100644 --- a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js +++ b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js @@ -1449,3 +1449,189 @@ add_task(async function test_active_and_past_experiment_targeting() { await assertEmptyStore(manager.store, { cleanup: true }); cleanupFeatures(); }); + +add_task(async function test_enrollment_targeting() { + const loader = ExperimentFakes.rsLoader(); + const manager = loader.manager; + + await loader.init(); + await manager.onStartup(); + await manager.store.ready(); + + const cleanupFeatures = ExperimentTestUtils.addTestFeatures( + new ExperimentFeature("feature-a", { + isEarlyStartup: false, + variables: {}, + }), + new ExperimentFeature("feature-b", { + isEarlyStartup: false, + variables: {}, + }), + new ExperimentFeature("feature-c", { + isEarlyStartup: false, + variables: {}, + }), + new ExperimentFeature("feature-d", { + isEarlyStartup: false, + variables: {}, + }) + ); + + function recipe( + name, + featureId, + { targeting = "true", isRollout = false } = {} + ) { + return ExperimentFakes.recipe(name, { + branches: [ + { + ...ExperimentFakes.recipe.branches[0], + features: [{ featureId, value: {} }], + }, + ], + bucketConfig: { + ...ExperimentFakes.recipe.bucketConfig, + count: 1000, + }, + targeting, + isRollout, + }); + } + + const experimentA = recipe("experiment-a", "feature-a", { + targeting: "!('rollout-c' in enrollments)", + }); + const experimentB = recipe("experiment-b", "feature-b", { + targeting: "'rollout-a' in enrollments", + }); + const experimentC = recipe("experiment-c", "feature-c"); + + const rolloutA = recipe("rollout-a", "feature-a", { + targeting: "!('experiment-c' in enrollments)", + isRollout: true, + }); + const rolloutB = recipe("rollout-b", "feature-b", { + targeting: "'experiment-a' in enrollments", + isRollout: true, + }); + const rolloutC = recipe("rollout-c", "feature-c", { isRollout: true }); + + async function check(current, past, unenrolled) { + await loader.updateRecipes(); + + for (const slug of current) { + const enrollment = manager.store.get(slug); + Assert.equal( + enrollment?.active, + true, + `Enrollment exists for ${slug} and is active` + ); + } + + for (const slug of past) { + const enrollment = manager.store.get(slug); + Assert.equal( + enrollment?.active, + false, + `Enrollment exists for ${slug} and is inactive` + ); + } + + for (const slug of unenrolled) { + Assert.ok( + !manager.store.get(slug), + `Enrollment does not exist for ${slug}` + ); + } + } + + sinon + .stub(loader.remoteSettingsClient, "get") + .resolves([experimentB, rolloutB]); + await check( + [], + [], + [ + "experiment-a", + "experiment-b", + "experiment-c", + "rollout-a", + "rollout-b", + "rollout-c", + ] + ); + + // Order matters -- B will be checked before A. + loader.remoteSettingsClient.get.resolves([ + experimentB, + rolloutB, + experimentA, + rolloutA, + ]); + await check( + ["experiment-a", "rollout-a"], + [], + ["experiment-b", "experiment-c", "rollout-b", "rollout-c"] + ); + + // B will see A enrolled. + loader.remoteSettingsClient.get.resolves([ + experimentB, + rolloutB, + experimentA, + rolloutA, + ]); + await check( + ["experiment-a", "experiment-b", "rollout-a", "rollout-b"], + [], + ["experiment-c", "rollout-c"] + ); + + // Order matters -- A will be checked before C. + loader.remoteSettingsClient.get.resolves([ + experimentB, + rolloutB, + experimentA, + rolloutA, + experimentC, + rolloutC, + ]); + await check( + [ + "experiment-a", + "experiment-b", + "experiment-c", + "rollout-a", + "rollout-b", + "rollout-c", + ], + [], + [] + ); + + // A will see C has enrolled and unenroll. B will stay enrolled. + await check( + ["experiment-b", "experiment-c", "rollout-b", "rollout-c"], + ["experiment-a", "rollout-a"], + [] + ); + + // A being unenrolled does not affect B. Rollout A will not re-enroll due to targeting. + await check( + ["experiment-b", "experiment-c", "rollout-b", "rollout-c"], + ["experiment-a", "rollout-a"], + [] + ); + + for (const slug of [ + "experiment-b", + "experiment-c", + "rollout-b", + "rollout-c", + ]) { + manager.unenroll(slug); + } + + await assertEmptyStore(manager.store, { cleanup: true }); + cleanupFeatures(); +});