From 860ab205503f532d8f0f5e4af1fa4a09a09c3a48 Mon Sep 17 00:00:00 2001 From: Michael Cooper Date: Fri, 6 Mar 2020 19:35:02 +0000 Subject: [PATCH] Bug 1604365 - Remove old preference-experiment action r=Gijs Differential Revision: https://phabricator.services.mozilla.com/D64498 --HG-- extra : moz-landing-system : lando --- .../SinglePreferenceExperimentAction.jsm | 95 ------------- .../normandy/lib/ActionsManager.jsm | 7 +- .../normandy/lib/PreferenceExperiments.jsm | 23 ++++ .../normandy/test/NormandyTestUtils.jsm | 1 + .../normandy/test/browser/browser.ini | 1 - .../browser/browser_PreferenceExperiments.js | 126 +++++++++++++----- ...ctions_SinglePreferenceExperimentAction.js | 116 ---------------- 7 files changed, 114 insertions(+), 255 deletions(-) delete mode 100644 toolkit/components/normandy/actions/SinglePreferenceExperimentAction.jsm delete mode 100644 toolkit/components/normandy/test/browser/browser_actions_SinglePreferenceExperimentAction.js diff --git a/toolkit/components/normandy/actions/SinglePreferenceExperimentAction.jsm b/toolkit/components/normandy/actions/SinglePreferenceExperimentAction.jsm deleted file mode 100644 index 1a95dfc89928..000000000000 --- a/toolkit/components/normandy/actions/SinglePreferenceExperimentAction.jsm +++ /dev/null @@ -1,95 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -"use strict"; - -const { PreferenceExperimentAction } = ChromeUtils.import( - "resource://normandy/actions/PreferenceExperimentAction.jsm" -); -ChromeUtils.defineModuleGetter( - this, - "ActionSchemas", - "resource://normandy/actions/schemas/index.js" -); -ChromeUtils.defineModuleGetter( - this, - "JsonSchemaValidator", - "resource://gre/modules/components-utils/JsonSchemaValidator.jsm" -); - -var EXPORTED_SYMBOLS = ["SinglePreferenceExperimentAction"]; - -/** - * The backwards-compatible version of the preference experiment that - * can only accept a single preference. - */ -class SinglePreferenceExperimentAction extends PreferenceExperimentAction { - get schema() { - return ActionSchemas["single-preference-experiment"]; - } - - async _run(recipe) { - const { - preferenceBranchType, - preferenceName, - preferenceType, - branches, - ...remainingArguments - } = recipe.arguments; - - const newArguments = { - // The multi-preference-experiment schema requires a string - // name/description, which are necessary in the wire format, but - // experiment objects can have null for these fields. Add some - // filler fields here and remove them after validation. - userFacingName: "temp-name", - userFacingDescription: "temp-description", - ...remainingArguments, - branches: branches.map(branch => { - const { value, ...branchProps } = branch; - return { - ...branchProps, - preferences: { - [preferenceName]: { - preferenceBranchType, - preferenceType, - preferenceValue: value, - }, - }, - }; - }), - }; - - const multiprefSchema = ActionSchemas["multi-preference-experiment"]; - - let [ - valid, - validatedArguments, - ] = JsonSchemaValidator.validateAndParseParameters( - newArguments, - multiprefSchema - ); - if (!valid) { - throw new Error( - `Transformed arguments do not match schema. Original arguments: ${JSON.stringify( - recipe.arguments - )}, new arguments: ${JSON.stringify( - newArguments - )}, schema: ${JSON.stringify(multiprefSchema)}` - ); - } - - validatedArguments.userFacingName = null; - validatedArguments.userFacingDescription = null; - - recipe.arguments = validatedArguments; - - const newRecipe = { - ...recipe, - arguments: validatedArguments, - }; - - return super._run(newRecipe); - } -} diff --git a/toolkit/components/normandy/lib/ActionsManager.jsm b/toolkit/components/normandy/lib/ActionsManager.jsm index 2a52bee967dc..dc4200f5ef5c 100644 --- a/toolkit/components/normandy/lib/ActionsManager.jsm +++ b/toolkit/components/normandy/lib/ActionsManager.jsm @@ -23,8 +23,6 @@ XPCOMUtils.defineLazyModuleGetters(this, { PreferenceRolloutAction: "resource://normandy/actions/PreferenceRolloutAction.jsm", ShowHeartbeatAction: "resource://normandy/actions/ShowHeartbeatAction.jsm", - SinglePreferenceExperimentAction: - "resource://normandy/actions/SinglePreferenceExperimentAction.jsm", Uptake: "resource://normandy/lib/Uptake.jsm", }); @@ -41,13 +39,10 @@ const actionConstructors = { "preference-rollback": PreferenceRollbackAction, "preference-rollout": PreferenceRolloutAction, "show-heartbeat": ShowHeartbeatAction, - "single-preference-experiment": SinglePreferenceExperimentAction, }; // Legacy names used by the server and older clients for actions. -const actionAliases = { - "preference-experiment": "single-preference-experiment", -}; +const actionAliases = {}; /** * A class to manage the actions that recipes can use in Normandy. diff --git a/toolkit/components/normandy/lib/PreferenceExperiments.jsm b/toolkit/components/normandy/lib/PreferenceExperiments.jsm index 1c615e2c7efa..72c3675a5bfc 100644 --- a/toolkit/components/normandy/lib/PreferenceExperiments.jsm +++ b/toolkit/components/normandy/lib/PreferenceExperiments.jsm @@ -47,6 +47,9 @@ * A random ID generated at time of enrollment. It should be included on all * telemetry related to this experiment. It should not be re-used by other * studies, or any other purpose. May be null on old experiments. + * @property {string} actionName + * The action who knows about this experiment and is responsible for cleaning + * it up. This should correspond to the `name` of some BaseAction subclass. */ /** @@ -941,5 +944,25 @@ var PreferenceExperiments = { } storage.saveSoon(); }, + + async migration05RemoveOldAction() { + const experiments = await PreferenceExperiments.getAllActive(); + for (const experiment of experiments) { + if (experiment.actionName == "SinglePreferenceExperimentAction") { + try { + await PreferenceExperiments.stop(experiment.slug, { + resetValue: true, + reason: "migration-removing-single-pref-action", + }); + } catch (e) { + log.error( + `Stopping preference experiment ${ + experiment.slug + } during migration failed: ${e}` + ); + } + } + } + }, }, }; diff --git a/toolkit/components/normandy/test/NormandyTestUtils.jsm b/toolkit/components/normandy/test/NormandyTestUtils.jsm index 1d5c23055475..304426ff4294 100644 --- a/toolkit/components/normandy/test/NormandyTestUtils.jsm +++ b/toolkit/components/normandy/test/NormandyTestUtils.jsm @@ -104,6 +104,7 @@ const NormandyTestUtils = { lastSeen: new Date().toJSON(), experimentType: "exp", enrollmentId: NormandyUtils.generateUuid(), + actionName: "PreferenceExperimentAction", }, attrs, { diff --git a/toolkit/components/normandy/test/browser/browser.ini b/toolkit/components/normandy/test/browser/browser.ini index 5cbd9d2dac08..d25defe05338 100644 --- a/toolkit/components/normandy/test/browser/browser.ini +++ b/toolkit/components/normandy/test/browser/browser.ini @@ -20,7 +20,6 @@ head = head.js [browser_actions_PreferenceRolloutAction.js] [browser_actions_PreferenceRollbackAction.js] [browser_actions_ShowHeartbeatAction.js] -[browser_actions_SinglePreferenceExperimentAction.js] [browser_ActionsManager.js] [browser_AddonRollouts.js] [browser_AddonStudies.js] diff --git a/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js b/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js index 9bac8f24f9a6..1e368f190666 100644 --- a/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js +++ b/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js @@ -181,6 +181,30 @@ const mockV5Data = { }, }; +const migrationsInfo = [ + { + migration: PreferenceExperiments.migrations.migration01MoveExperiments, + dataBefore: mockV1Data, + dataAfter: mockV2Data, + }, + { + migration: PreferenceExperiments.migrations.migration02MultiPreference, + dataBefore: mockV2Data, + dataAfter: mockV3Data, + }, + { + migration: PreferenceExperiments.migrations.migration03AddActionName, + dataBefore: mockV3Data, + dataAfter: mockV4Data, + }, + { + migration: PreferenceExperiments.migrations.migration04RenameNameToSlug, + dataBefore: mockV4Data, + dataAfter: mockV5Data, + }, + // Migration 5 is not a simple data migration. This style of tests does not apply to it. +]; + /** * Make a mock `JsonFile` object with a no-op `saveSoon` method and a deep copy * of the data passed. @@ -196,27 +220,30 @@ function makeMockJsonFile(data = {}) { /** Test that each migration results in the expected data */ add_task(async function test_migrations() { - let mockJsonFile = makeMockJsonFile(mockV1Data); - await PreferenceExperiments.migrations.migration01MoveExperiments( - mockJsonFile - ); - Assert.deepEqual(mockJsonFile.data, mockV2Data); + for (const { migration, dataAfter, dataBefore } of migrationsInfo) { + let mockJsonFile = makeMockJsonFile(dataBefore); + await migration(mockJsonFile); + Assert.deepEqual( + mockJsonFile.data, + dataAfter, + `Migration ${migration.name} should result in the expected data` + ); + } +}); - mockJsonFile = makeMockJsonFile(mockV2Data); - await PreferenceExperiments.migrations.migration02MultiPreference( - mockJsonFile - ); - Assert.deepEqual(mockJsonFile.data, mockV3Data); - - mockJsonFile = makeMockJsonFile(mockV3Data); - await PreferenceExperiments.migrations.migration03AddActionName(mockJsonFile); - Assert.deepEqual(mockJsonFile.data, mockV4Data); - - mockJsonFile = makeMockJsonFile(mockV4Data); - await PreferenceExperiments.migrations.migration04RenameNameToSlug( - mockJsonFile - ); - Assert.deepEqual(mockJsonFile.data, mockV5Data); +add_task(async function migrations_are_idempotent() { + for (const { migration, dataBefore } of migrationsInfo) { + const mockJsonFileOnce = makeMockJsonFile(dataBefore); + const mockJsonFileTwice = makeMockJsonFile(dataBefore); + await migration(mockJsonFileOnce); + await migration(mockJsonFileTwice); + await migration(mockJsonFileTwice); + Assert.deepEqual( + mockJsonFileOnce.data, + mockJsonFileTwice.data, + "migrating data twice should be idempotent for " + migration.name + ); + } }); add_task(async function migration03KeepsActionName() { @@ -231,26 +258,51 @@ add_task(async function migration03KeepsActionName() { Assert.deepEqual(mockJsonFile.data, migratedData); }); -add_task(async function migrations_are_idempotent() { - let dataVersions = [ - [PreferenceExperiments.migrations.migration01MoveExperiments, mockV1Data], - [PreferenceExperiments.migrations.migration02MultiPreference, mockV2Data], - [PreferenceExperiments.migrations.migration03AddActionName, mockV3Data], - [PreferenceExperiments.migrations.migration04RenameNameToSlug, mockV4Data], - ]; - for (const [migration, mockOldData] of dataVersions) { - const mockJsonFileOnce = makeMockJsonFile(mockOldData); - const mockJsonFileTwice = makeMockJsonFile(mockOldData); - await migration(mockJsonFileOnce); - await migration(mockJsonFileTwice); - await migration(mockJsonFileTwice); +// Test that migration 5 works as expected +decorate_task( + PreferenceExperiments.withMockExperiments([ + NormandyTestUtils.factories.preferenceStudyFactory({ + actionName: "PreferenceExperimentAction", + expired: false, + }), + NormandyTestUtils.factories.preferenceStudyFactory({ + actionName: "SinglePreferenceExperimentAction", + expired: false, + }), + ]), + async function migration05Works([expKeep, expExpire]) { + // pre check + const activeSlugsBefore = (await PreferenceExperiments.getAllActive()).map( + e => e.slug + ); Assert.deepEqual( - mockJsonFileOnce.data, - mockJsonFileTwice.data, - "migrating data twice should be idempotent for " + migration.name + activeSlugsBefore, + [expKeep.slug, expExpire.slug], + "Both experiments should be present and active before the migration" + ); + + // run the migration + await PreferenceExperiments.migrations.migration05RemoveOldAction(); + + // verify behavior + const activeSlugsAfter = (await PreferenceExperiments.getAllActive()).map( + e => e.slug + ); + Assert.deepEqual( + activeSlugsAfter, + [expKeep.slug], + "The single pref experiment should be ended by the migration" + ); + const allSlugsAfter = (await PreferenceExperiments.getAll()).map( + e => e.slug + ); + Assert.deepEqual( + allSlugsAfter, + [expKeep.slug, expExpire.slug], + "Both experiments should still exist after the migration" ); } -}); +); // clearAllExperimentStorage decorate_task( diff --git a/toolkit/components/normandy/test/browser/browser_actions_SinglePreferenceExperimentAction.js b/toolkit/components/normandy/test/browser/browser_actions_SinglePreferenceExperimentAction.js deleted file mode 100644 index 378f458698d0..000000000000 --- a/toolkit/components/normandy/test/browser/browser_actions_SinglePreferenceExperimentAction.js +++ /dev/null @@ -1,116 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ -"use strict"; - -ChromeUtils.import( - "resource://gre/modules/components-utils/Sampling.jsm", - this -); -ChromeUtils.import("resource://gre/modules/Services.jsm", this); -ChromeUtils.import("resource://gre/modules/Preferences.jsm", this); -ChromeUtils.import("resource://gre/modules/TelemetryEnvironment.jsm", this); -ChromeUtils.import("resource://normandy/actions/BaseAction.jsm", this); -ChromeUtils.import("resource://normandy/lib/ClientEnvironment.jsm", this); -ChromeUtils.import("resource://normandy/lib/PreferenceExperiments.jsm", this); -ChromeUtils.import("resource://normandy/lib/TelemetryEvents.jsm", this); -ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this); -ChromeUtils.import( - "resource://normandy/actions/PreferenceExperimentAction.jsm", - this -); -ChromeUtils.import( - "resource://normandy/actions/SinglePreferenceExperimentAction.jsm", - this -); - -function argumentsFactory(args) { - return { - slug: "test", - preferenceName: "fake.preference", - preferenceType: "string", - preferenceBranchType: "default", - branches: [{ slug: "test", value: "foo", ratio: 1 }], - isHighPopulation: false, - ...args, - }; -} - -function preferenceExperimentFactory(args) { - return recipeFactory({ - name: "preference-experiment", - arguments: argumentsFactory(args), - }); -} - -decorate_task( - withStudiesEnabled, - withStub(PreferenceExperimentAction.prototype, "_run"), - PreferenceExperiments.withMockExperiments([]), - async function enroll_user_if_never_been_in_experiment(runStub) { - const action = new SinglePreferenceExperimentAction(); - const recipe = preferenceExperimentFactory({ - slug: "test", - preferenceName: "fake.preference", - preferenceBranchType: "user", - branches: [ - { - slug: "branch1", - value: "branch1", - ratio: 1, - }, - { - slug: "branch2", - value: "branch2", - ratio: 1, - }, - ], - }); - sinon - .stub(action, "chooseBranch") - .callsFake(async function(slug, branches) { - return branches[0]; - }); - - await action.processRecipe(recipe, BaseAction.suitability.FILTER_MATCH); - await action.finalize(); - - Assert.deepEqual(runStub.args, [ - [ - { - id: recipe.id, - name: "preference-experiment", - arguments: { - slug: "test", - userFacingName: null, - userFacingDescription: null, - isHighPopulation: false, - branches: [ - { - slug: "branch1", - ratio: 1, - preferences: { - "fake.preference": { - preferenceValue: "branch1", - preferenceType: "string", - preferenceBranchType: "user", - }, - }, - }, - { - slug: "branch2", - ratio: 1, - preferences: { - "fake.preference": { - preferenceValue: "branch2", - preferenceType: "string", - preferenceBranchType: "user", - }, - }, - }, - ], - }, - }, - ], - ]); - } -);