author | Michael Cooper <mcooper@mozilla.com> |
Wed, 28 Aug 2019 19:58:56 +0000 | |
changeset 490468 | 1c29b632b6eb962612767e7d2c330e46f96aeef9 |
parent 490467 | 6d157db92bc951194c5bd528d292c84e78bbb25d |
child 490469 | d986d84d5d29270c01355dbcaab13db91425d9cb |
push id | 36504 |
push user | ccoroiu@mozilla.com |
push date | Thu, 29 Aug 2019 04:08:39 +0000 |
treeherder | mozilla-central@7004b8779a36 [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | rdalal |
bugs | 1572479 |
milestone | 70.0a1 |
first release with | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
last release without | nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
|
--- a/toolkit/components/normandy/NormandyMigrations.jsm +++ b/toolkit/components/normandy/NormandyMigrations.jsm @@ -53,16 +53,17 @@ const NormandyMigrations = { migrations: [ migrateShieldPrefs, migrateStudiesEnabledWithoutHealthReporting, AddonStudies.migrateAddonStudyFieldsToSlugAndUserFacingFields, PreferenceExperiments.migrations.migration01MoveExperiments, PreferenceExperiments.migrations.migration02MultiPreference, PreferenceExperiments.migrations.migration03AddActionName, + PreferenceExperiments.migrations.migration04RenameNameToSlug, ], }; function migrateShieldPrefs() { const legacyBranch = Services.prefs.getBranch(LEGACY_PREF_PREFIX + "."); const newBranch = Services.prefs.getBranch(PREF_PREFIX + "."); for (const prefName of legacyBranch.getChildList("")) {
--- a/toolkit/components/normandy/actions/PreferenceExperimentAction.jsm +++ b/toolkit/components/normandy/actions/PreferenceExperimentAction.jsm @@ -37,30 +37,30 @@ var EXPORTED_SYMBOLS = ["PreferenceExper */ class PreferenceExperimentAction extends BaseStudyAction { get schema() { return ActionSchemas["multi-preference-experiment"]; } constructor() { super(); - this.seenExperimentNames = []; + this.seenExperimentSlugs = []; } async _run(recipe) { const { branches, isHighPopulation, isEnrollmentPaused, slug, userFacingName, userFacingDescription, } = recipe.arguments; - this.seenExperimentNames.push(slug); + this.seenExperimentSlugs.push(slug); // If we're not in the experiment, enroll! const hasSlug = await PreferenceExperiments.has(slug); if (!hasSlug) { // Check all preferences that could be used by this experiment. // If there's already an active experiment that has set that preference, abort. const activeExperiments = await PreferenceExperiments.getAllActive(); for (const branch of branches) { @@ -84,17 +84,17 @@ class PreferenceExperimentAction extends this.log.debug(`Enrollment is paused for experiment "${slug}"`); return; } // Otherwise, enroll! const branch = await this.chooseBranch(slug, branches); const experimentType = isHighPopulation ? "exp-highpop" : "exp"; await PreferenceExperiments.start({ - name: slug, + slug, actionName: this.name, branch: branch.slug, preferences: branch.preferences, experimentType, userFacingName, userFacingDescription, }); } else { @@ -135,22 +135,22 @@ class PreferenceExperimentAction extends return Promise.all( activeExperiments.map(experiment => { if (this.name != experiment.actionName) { // Another action is responsible for cleaning this one // up. Leave it alone. return null; } - if (this.seenExperimentNames.includes(experiment.name)) { + if (this.seenExperimentSlugs.includes(experiment.slug)) { return null; } - return PreferenceExperiments.stop(experiment.name, { + return PreferenceExperiments.stop(experiment.slug, { resetValue: true, reason: "recipe-not-seen", }).catch(e => { - this.log.warn(`Stopping experiment ${experiment.name} failed: ${e}`); + this.log.warn(`Stopping experiment ${experiment.slug} failed: ${e}`); }); }) ); } }
--- a/toolkit/components/normandy/content/about-studies/about-studies.js +++ b/toolkit/components/normandy/content/about-studies/about-studies.js @@ -185,31 +185,31 @@ class StudyList extends React.Component r("h2", {}, translations.activeStudiesList), r( "ul", { className: "study-list active-study-list" }, activeStudies.map(study => study.type === "addon" ? r(AddonStudyListItem, { key: study.slug, study, translations }) : r(PreferenceStudyListItem, { - key: study.name, + key: study.slug, study, translations, }) ) ), r("h2", {}, translations.completedStudiesList), r( "ul", { className: "study-list inactive-study-list" }, inactiveStudies.map(study => study.type === "addon" ? r(AddonStudyListItem, { key: study.slug, study, translations }) : r(PreferenceStudyListItem, { - key: study.name, + key: study.slug, study, translations, }) ) ) ); } } @@ -302,17 +302,17 @@ AddonStudyListItem.propTypes = { class PreferenceStudyListItem extends React.Component { constructor(props) { super(props); this.handleClickRemove = this.handleClickRemove.bind(this); } handleClickRemove() { sendPageEvent("RemovePreferenceStudy", { - experimentName: this.props.study.name, + experimentName: this.props.study.slug, reason: "individual-opt-out", }); } render() { const { study, translations } = this.props; let userFacingName = study.userFacingName; @@ -342,26 +342,30 @@ class PreferenceStudyListItem extends Re .replace(/%(?:1\$)?S/, sanitizedPreferenceName) .replace(/%(?:2\$)?S/, sanitizedPreferenceValue); } return r( "li", { className: classnames("study pref-study", { disabled: study.expired }), - "data-study-slug": study.name, // used to identify this row in tests + "data-study-slug": study.slug, // used to identify this row in tests }, r("div", { className: "study-icon" }, userFacingName), r( "div", { className: "study-details" }, r( "div", { className: "study-header" }, - r("span", { className: "study-name" }, study.name), + r( + "span", + { className: "study-name" }, + study.userFacingName || study.slug + ), r("span", {}, "\u2022"), // • r( "span", { className: "study-status" }, study.expired ? translations.completeStatus : translations.activeStatus ) @@ -381,17 +385,19 @@ class PreferenceStudyListItem extends Re r("div", { className: "button-box" }, translations.removeButton) ) ) ); } } PreferenceStudyListItem.propTypes = { study: PropTypes.shape({ - name: PropTypes.string.isRequired, + slug: PropTypes.string.isRequired, + userFacingName: PropTypes.string, + userFacingDescription: PropTypes.string, expired: PropTypes.bool.isRequired, preferenceName: PropTypes.string.isRequired, preferenceValue: PropTypes.oneOf( PropTypes.string, PropTypes.bool, PropTypes.number ).isRequired, }).isRequired,
--- a/toolkit/components/normandy/lib/ClientEnvironment.jsm +++ b/toolkit/components/normandy/lib/ClientEnvironment.jsm @@ -92,22 +92,22 @@ class ClientEnvironment extends ClientEn return request_time; })(); } static get experiments() { return (async () => { const names = { all: [], active: [], expired: [] }; - for (const experiment of await PreferenceExperiments.getAll()) { - names.all.push(experiment.name); - if (experiment.expired) { - names.expired.push(experiment.name); + for (const { slug, expired } of await PreferenceExperiments.getAll()) { + names.all.push(slug); + if (expired) { + names.expired.push(slug); } else { - names.active.push(experiment.name); + names.active.push(slug); } } return names; })(); } static get isFirstRun() {
--- a/toolkit/components/normandy/lib/PreferenceExperiments.jsm +++ b/toolkit/components/normandy/lib/PreferenceExperiments.jsm @@ -15,37 +15,37 @@ * server. They also expire if Firefox isn't able to contact the recipe server * after a period of time, as well as if the user modifies the preference during * an active experiment. */ /** * Experiments store info about an active or expired preference experiment. * @typedef {Object} Experiment - * @property {string} name - * Unique name of the experiment + * @property {string} slug + * A string uniquely identifying the experiment. Used for telemetry, and other + * machine-oriented use cases. Used as a display name if `userFacingName` is + * null. * @property {string|null} userFacingName - * A user-friendly name for the experiment. Null on old-style - * single-preference experiments, which do not have a - * userFacingName. + * A user-friendly name for the experiment. Null on old-style single-preference + * experiments, which do not have a userFacingName. * @property {string|null} userFacingDescription * A user-friendly description of the experiment. Null on old-style - * single-preference experiments, which do not have a - * userFacingDescription. + * single-preference experiments, which do not have a userFacingDescription. * @property {string} branch * Experiment branch that the user was matched to * @property {boolean} expired * If false, the experiment is active. * @property {string} lastSeen * ISO-formatted date string of when the experiment was last seen from the * recipe server. * @property {Object} preferences * An object consisting of all the preferences that are set by this experiment. - * Keys are the name of each preference affected by this experiment. - * Values are Preference Objects, about which see below. + * Keys are the name of each preference affected by this experiment. Values are + * Preference Objects, about which see below. * @property {string} experimentType * The type to report to Telemetry's experiment marker API. */ /** * Each Preference stores information about a preference that an * experiment sets. * @property {string|integer|boolean} preferenceValue @@ -241,39 +241,39 @@ var PreferenceExperiments = { experiment.preferences )) { if ( getPref(UserPreferences, prefName, prefInfo.preferenceType) !== prefInfo.preferenceValue ) { // if not, stop the experiment, and skip the remaining steps log.info( - `Stopping experiment "${experiment.name}" because its value changed` + `Stopping experiment "${experiment.slug}" because its value changed` ); - await this.stop(experiment.name, { + await this.stop(experiment.slug, { resetValue: false, reason: "user-preference-changed-sideload", }); stopped = true; break; } } if (stopped) { continue; } // Notify Telemetry of experiments we're running, since they don't persist between restarts TelemetryEnvironment.setExperimentActive( - experiment.name, + experiment.slug, experiment.branch, { type: EXPERIMENT_TYPE_PREFIX + experiment.experimentType } ); // Watch for changes to the experiment's preference - this.startObserver(experiment.name, experiment.preferences); + this.startObserver(experiment.slug, experiment.preferences); } }, /** * Save in-progress, default-branch preference experiments in a sub-branch of * the normandy preferences. On startup, we read these to set the * experimental values. * @@ -327,17 +327,23 @@ var PreferenceExperiments = { * data for testing. */ withMockExperiments(mockExperiments = []) { return function wrapper(testFunction) { return async function wrappedTestFunction(...args) { const experiments = {}; for (const exp of mockExperiments) { - experiments[exp.name] = exp; + if (exp.name) { + throw new Error( + "Preference experiments 'name' field has been replaced by 'slug' and 'userFacingName', please update." + ); + } + + experiments[exp.slug] = exp; } const data = { experiments }; const oldPromise = gStorePromise; gStorePromise = Promise.resolve({ data, saveSoon() {}, }); @@ -363,114 +369,121 @@ var PreferenceExperiments = { experiments: {}, }; store.saveSoon(); }, /** * Start a new preference experiment. * @param {Object} experiment - * @param {string} experiment.name + * @param {string} experiment.slug * @param {string} experiment.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. * @param {string} experiment.branch * @param {string} experiment.preferenceName * @param {string|integer|boolean} experiment.preferenceValue * @param {PreferenceBranchType} experiment.preferenceBranchType * @rejects {Error} * - If an experiment with the given name already exists * - if an experiment for the given preference is active * - If the given preferenceType does not match the existing stored preference */ async start({ - name, + name = null, // To check if old code is still using `name` instead of `slug`, and provide a nice error message + slug, actionName, branch, preferences, experimentType = "exp", userFacingName = null, userFacingDescription = null, }) { - log.debug(`PreferenceExperiments.start(${name}, ${branch})`); + if (name) { + throw new Error( + "Preference experiments 'name' field has been replaced by 'slug' and 'userFacingName', please update." + ); + } + + log.debug(`PreferenceExperiments.start(${slug}, ${branch})`); const store = await ensureStorage(); - if (name in store.data.experiments) { - TelemetryEvents.sendEvent("enrollFailed", "preference_study", name, { + if (slug in store.data.experiments) { + TelemetryEvents.sendEvent("enrollFailed", "preference_study", slug, { reason: "name-conflict", }); throw new Error( - `A preference experiment named "${name}" already exists.` + `A preference experiment with the slug "${slug}" already exists.` ); } const activeExperiments = Object.values(store.data.experiments).filter( e => !e.expired ); const preferencesWithConflicts = Object.keys(preferences).filter( preferenceName => { return activeExperiments.some(e => e.preferences.hasOwnProperty(preferenceName) ); } ); if (preferencesWithConflicts.length > 0) { - TelemetryEvents.sendEvent("enrollFailed", "preference_study", name, { + TelemetryEvents.sendEvent("enrollFailed", "preference_study", slug, { reason: "pref-conflict", }); throw new Error( `Another preference experiment for the pref "${ preferencesWithConflicts[0] }" is currently active.` ); } if (experimentType.length > MAX_EXPERIMENT_SUBTYPE_LENGTH) { - TelemetryEvents.sendEvent("enrollFailed", "preference_study", name, { + TelemetryEvents.sendEvent("enrollFailed", "preference_study", slug, { reason: "experiment-type-too-long", }); throw new Error( `experimentType must be less than ${MAX_EXPERIMENT_SUBTYPE_LENGTH} characters. ` + `"${experimentType}" is ${experimentType.length} long.` ); } // Sanity check each preference for (const [preferenceName, preferenceInfo] of Object.entries( preferences )) { const { preferenceBranchType, preferenceType } = preferenceInfo; const preferenceBranch = PreferenceBranchType[preferenceBranchType]; if (!preferenceBranch) { - TelemetryEvents.sendEvent("enrollFailed", "preference_study", name, { + TelemetryEvents.sendEvent("enrollFailed", "preference_study", slug, { reason: "invalid-branch", }); throw new Error( `Invalid value for preferenceBranchType: ${preferenceBranchType}` ); } const prevPrefType = Services.prefs.getPrefType(preferenceName); const givenPrefType = PREFERENCE_TYPE_MAP[preferenceType]; if (!preferenceType || !givenPrefType) { - TelemetryEvents.sendEvent("enrollFailed", "preference_study", name, { + TelemetryEvents.sendEvent("enrollFailed", "preference_study", slug, { reason: "invalid-type", }); throw new Error( `Invalid preferenceType provided (given "${preferenceType}")` ); } if ( prevPrefType !== Services.prefs.PREF_INVALID && prevPrefType !== givenPrefType ) { - TelemetryEvents.sendEvent("enrollFailed", "preference_study", name, { + TelemetryEvents.sendEvent("enrollFailed", "preference_study", slug, { reason: "invalid-type", }); throw new Error( `Previous preference value is of type "${prevPrefType}", but was given ` + `"${givenPrefType}" (${preferenceType})` ); } @@ -492,202 +505,202 @@ var PreferenceExperiments = { const preferenceBranch = PreferenceBranchType[preferenceBranchType]; setPref( preferenceBranch, preferenceName, preferenceType, preferenceValue ); } - PreferenceExperiments.startObserver(name, preferences); + PreferenceExperiments.startObserver(slug, preferences); /** @type {Experiment} */ const experiment = { - name, + slug, actionName, branch, expired: false, lastSeen: new Date().toJSON(), preferences, experimentType, userFacingName, userFacingDescription, }; - store.data.experiments[name] = experiment; + store.data.experiments[slug] = experiment; store.saveSoon(); - TelemetryEnvironment.setExperimentActive(name, branch, { + TelemetryEnvironment.setExperimentActive(slug, branch, { type: EXPERIMENT_TYPE_PREFIX + experimentType, }); - TelemetryEvents.sendEvent("enroll", "preference_study", name, { + TelemetryEvents.sendEvent("enroll", "preference_study", slug, { experimentType, branch, }); await this.saveStartupPrefs(); }, /** * Register a preference observer that stops an experiment when the user * modifies the preference. - * @param {string} experimentName + * @param {string} experimentSlug * @param {string} preferenceName * @param {string|integer|boolean} preferenceValue * @throws {Error} - * If an observer for the named experiment is already active. + * If an observer for the experiment is already active. */ - startObserver(experimentName, preferences) { - log.debug(`PreferenceExperiments.startObserver(${experimentName})`); + startObserver(experimentSlug, preferences) { + log.debug(`PreferenceExperiments.startObserver(${experimentSlug})`); - if (experimentObservers.has(experimentName)) { + if (experimentObservers.has(experimentSlug)) { throw new Error( - `An observer for the preference experiment ${experimentName} is already active.` + `An observer for the preference experiment ${experimentSlug} is already active.` ); } const observerInfo = { preferences, observe(aSubject, aTopic, preferenceName) { const { preferenceValue, preferenceType } = preferences[preferenceName]; const newValue = getPref( UserPreferences, preferenceName, preferenceType ); if (newValue !== preferenceValue) { - PreferenceExperiments.stop(experimentName, { + PreferenceExperiments.stop(experimentSlug, { resetValue: false, reason: "user-preference-changed", }).catch(Cu.reportError); } }, }; - experimentObservers.set(experimentName, observerInfo); + experimentObservers.set(experimentSlug, observerInfo); for (const preferenceName of Object.keys(preferences)) { Services.prefs.addObserver(preferenceName, observerInfo); } }, /** * Check if a preference observer is active for an experiment. - * @param {string} experimentName + * @param {string} experimentSlug * @return {Boolean} */ - hasObserver(experimentName) { - log.debug(`PreferenceExperiments.hasObserver(${experimentName})`); - return experimentObservers.has(experimentName); + hasObserver(experimentSlug) { + log.debug(`PreferenceExperiments.hasObserver(${experimentSlug})`); + return experimentObservers.has(experimentSlug); }, /** - * Disable a preference observer for the named experiment. - * @param {string} experimentName + * Disable a preference observer for an experiment. + * @param {string} experimentSlug * @throws {Error} - * If there is no active observer for the named experiment. + * If there is no active observer for the experiment. */ - stopObserver(experimentName) { - log.debug(`PreferenceExperiments.stopObserver(${experimentName})`); + stopObserver(experimentSlug) { + log.debug(`PreferenceExperiments.stopObserver(${experimentSlug})`); - if (!experimentObservers.has(experimentName)) { + if (!experimentObservers.has(experimentSlug)) { throw new Error( - `No observer for the preference experiment ${experimentName} found.` + `No observer for the preference experiment ${experimentSlug} found.` ); } - const observer = experimentObservers.get(experimentName); + const observer = experimentObservers.get(experimentSlug); for (const preferenceName of Object.keys(observer.preferences)) { Services.prefs.removeObserver(preferenceName, observer); } - experimentObservers.delete(experimentName); + experimentObservers.delete(experimentSlug); }, /** * Disable all currently-active preference observers for experiments. */ stopAllObservers() { log.debug("PreferenceExperiments.stopAllObservers()"); for (const observer of experimentObservers.values()) { for (const preferenceName of Object.keys(observer.preferences)) { Services.prefs.removeObserver(preferenceName, observer); } } experimentObservers.clear(); }, /** - * Update the timestamp storing when Normandy last sent a recipe for the named + * Update the timestamp storing when Normandy last sent a recipe for the * experiment. - * @param {string} experimentName + * @param {string} experimentSlug * @rejects {Error} - * If there is no stored experiment with the given name. + * If there is no stored experiment with the given slug. */ - async markLastSeen(experimentName) { - log.debug(`PreferenceExperiments.markLastSeen(${experimentName})`); + async markLastSeen(experimentSlug) { + log.debug(`PreferenceExperiments.markLastSeen(${experimentSlug})`); const store = await ensureStorage(); - if (!(experimentName in store.data.experiments)) { + if (!(experimentSlug in store.data.experiments)) { throw new Error( - `Could not find a preference experiment named "${experimentName}"` + `Could not find a preference experiment with the slug "${experimentSlug}"` ); } - store.data.experiments[experimentName].lastSeen = new Date().toJSON(); + store.data.experiments[experimentSlug].lastSeen = new Date().toJSON(); store.saveSoon(); }, /** * Stop an active experiment, deactivate preference watchers, and optionally * reset the associated preference to its previous value. - * @param {string} experimentName + * @param {string} experimentSlug * @param {Object} options * @param {boolean} [options.resetValue = true] * If true, reset the preference to its original value prior to * the experiment. Optional, defaults to true. * @param {String} [options.reason = "unknown"] * Reason that the experiment is ending. Optional, defaults to * "unknown". * @rejects {Error} - * If there is no stored experiment with the given name, or if the + * If there is no stored experiment with the given slug, or if the * experiment has already expired. */ - async stop(experimentName, { resetValue = true, reason = "unknown" } = {}) { + async stop(experimentSlug, { resetValue = true, reason = "unknown" } = {}) { log.debug( - `PreferenceExperiments.stop(${experimentName}, {resetValue: ${resetValue}, reason: ${reason}})` + `PreferenceExperiments.stop(${experimentSlug}, {resetValue: ${resetValue}, reason: ${reason}})` ); if (reason === "unknown") { - log.warn(`experiment ${experimentName} ending for unknown reason`); + log.warn(`experiment ${experimentSlug} ending for unknown reason`); } const store = await ensureStorage(); - if (!(experimentName in store.data.experiments)) { + if (!(experimentSlug in store.data.experiments)) { TelemetryEvents.sendEvent( "unenrollFailed", "preference_study", - experimentName, + experimentSlug, { reason: "does-not-exist" } ); throw new Error( - `Could not find a preference experiment named "${experimentName}"` + `Could not find a preference experiment with the slug "${experimentSlug}"` ); } - const experiment = store.data.experiments[experimentName]; + const experiment = store.data.experiments[experimentSlug]; if (experiment.expired) { TelemetryEvents.sendEvent( "unenrollFailed", "preference_study", - experimentName, + experimentSlug, { reason: "already-unenrolled" } ); throw new Error( - `Cannot stop preference experiment "${experimentName}" because it is already expired` + `Cannot stop preference experiment "${experimentSlug}" because it is already expired` ); } - if (PreferenceExperiments.hasObserver(experimentName)) { - PreferenceExperiments.stopObserver(experimentName); + if (PreferenceExperiments.hasObserver(experimentSlug)) { + PreferenceExperiments.stopObserver(experimentSlug); } if (resetValue) { for (const [preferenceName, prefInfo] of Object.entries( experiment.preferences )) { const { preferenceType, @@ -703,31 +716,31 @@ var PreferenceExperiments = { preferenceType, previousPreferenceValue ); } else if (preferenceBranchType === "user") { // Remove the "user set" value (which Shield set), but leave the default intact. preferences.clearUserPref(preferenceName); } else { log.warn( - `Can't revert pref ${preferenceName} for experiment ${experimentName} ` + + `Can't revert pref ${preferenceName} for experiment ${experimentSlug} ` + `because it had no default value. ` + `Preference will be reset at the next restart.` ); // It would seem that Services.prefs.deleteBranch() could be used for // this, but in Normandy's case it does not work. See bug 1502410. } } } experiment.expired = true; store.saveSoon(); - TelemetryEnvironment.setExperimentInactive(experimentName); - TelemetryEvents.sendEvent("unenroll", "preference_study", experimentName, { + TelemetryEnvironment.setExperimentInactive(experimentSlug); + TelemetryEvents.sendEvent("unenroll", "preference_study", experimentSlug, { didResetValue: resetValue ? "true" : "false", branch: experiment.branch, reason, }); await this.saveStartupPrefs(); }, /** @@ -742,32 +755,32 @@ var PreferenceExperiments = { ...experiment, preferences: { ...experiment.preferences, }, }; }, /** - * Get the experiment object for the named experiment. - * @param {string} experimentName + * Get the experiment object for the experiment. + * @param {string} experimentSlug * @resolves {Experiment} * @rejects {Error} - * If no preference experiment exists with the given name. + * If no preference experiment exists with the given slug. */ - async get(experimentName) { - log.debug(`PreferenceExperiments.get(${experimentName})`); + async get(experimentSlug) { + log.debug(`PreferenceExperiments.get(${experimentSlug})`); const store = await ensureStorage(); - if (!(experimentName in store.data.experiments)) { + if (!(experimentSlug in store.data.experiments)) { throw new Error( - `Could not find a preference experiment named "${experimentName}"` + `Could not find a preference experiment with the slug "${experimentSlug}"` ); } - return this._cloneExperiment(store.data.experiments[experimentName]); + return this._cloneExperiment(store.data.experiments[experimentSlug]); }, /** * Get a list of all stored experiment objects. * @resolves {Experiment[]} */ async getAll() { const store = await ensureStorage(); @@ -783,24 +796,24 @@ var PreferenceExperiments = { async getAllActive() { const store = await ensureStorage(); return Object.values(store.data.experiments) .filter(e => !e.expired) .map(e => this._cloneExperiment(e)); }, /** - * Check if an experiment exists with the given name. - * @param {string} experimentName + * Check if an experiment exists with the given slug. + * @param {string} experimentSlug * @resolves {boolean} True if the experiment exists, false if it doesn't. */ - async has(experimentName) { - log.debug(`PreferenceExperiments.has(${experimentName})`); + async has(experimentSlug) { + log.debug(`PreferenceExperiments.has(${experimentSlug})`); const store = await ensureStorage(); - return experimentName in store.data.experiments; + return experimentSlug in store.data.experiments; }, /** * These migrations should only be called from `NormandyMigrations.jsm` and tests. */ migrations: { /** Move experiments into a specific key. */ async migration01MoveExperiments(storage = null) { @@ -868,10 +881,24 @@ var PreferenceExperiments = { if (!experiment.actionName) { // Assume SinglePreferenceExperimentAction because as of this // writing, no multi-pref experiment recipe has launched. experiment.actionName = "SinglePreferenceExperimentAction"; } } storage.saveSoon(); }, + + async migration04RenameNameToSlug(storage = null) { + if (!storage) { + storage = await ensureStorage(); + } + // Rename "name" to "slug" to match the intended purpose of the field. + for (const experiment of Object.values(storage.data.experiments)) { + if (experiment.name && !experiment.slug) { + experiment.slug = experiment.name; + delete experiment.name; + } + } + storage.saveSoon(); + }, }, };
--- a/toolkit/components/normandy/lib/ShieldPreferences.jsm +++ b/toolkit/components/normandy/lib/ShieldPreferences.jsm @@ -56,17 +56,17 @@ var ShieldPreferences = { return action.unenroll(study.recipeId, "general-opt-out"); }); const experimentPromises = (await PreferenceExperiments.getAll()).map( experiment => { if (experiment.expired) { return null; } - return PreferenceExperiments.stop(experiment.name, { + return PreferenceExperiments.stop(experiment.slug, { reason: "general-opt-out", }); } ); const allPromises = studyPromises .concat(experimentPromises) .map(p => p && p.catch(err => Cu.reportError(err)));
--- a/toolkit/components/normandy/test/NormandyTestUtils.jsm +++ b/toolkit/components/normandy/test/NormandyTestUtils.jsm @@ -75,19 +75,30 @@ const NormandyTestUtils = { }; const preferences = {}; for (const [prefName, prefInfo] of Object.entries( attrs.preferences || defaultPref )) { preferences[prefName] = { ...defaultPrefInfo, ...prefInfo }; } + // Generate a slug from userFacingName + let { + userFacingName = `Test study ${_preferenceStudyFactoryId++}`, + slug, + } = attrs; + delete attrs.slug; + if (userFacingName && !slug) { + slug = userFacingName.replace(" ", "-").toLowerCase(); + } + return Object.assign( { - name: `Test study ${_preferenceStudyFactoryId++}`, + userFacingName, + slug, branch: "control", expired: false, lastSeen: new Date().toJSON(), experimentType: "exp", }, attrs, { preferences,
--- a/toolkit/components/normandy/test/browser/browser_ClientEnvironment.js +++ b/toolkit/components/normandy/test/browser/browser_ClientEnvironment.js @@ -128,18 +128,18 @@ add_task(async function testDoNotTrack() // doNotTrack is read from a preference await SpecialPowers.pushPrefEnv({ set: [["privacy.donottrackheader.enabled", true]], }); ok(ClientEnvironment.doNotTrack, "doNotTrack is read from preferences"); }); add_task(async function testExperiments() { - const active = { name: "active", expired: false }; - const expired = { name: "expired", expired: true }; + const active = { slug: "active", expired: false }; + const expired = { slug: "expired", expired: true }; const getAll = sinon .stub(PreferenceExperiments, "getAll") .callsFake(async () => [active, expired]); const experiments = await ClientEnvironment.experiments; Assert.deepEqual( experiments.all, ["active", "expired"],
--- a/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js +++ b/toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js @@ -25,17 +25,17 @@ function experimentFactory(attrs) { for (const [prefName, prefInfo] of Object.entries( attrs.preferences || defaultPref )) { preferences[prefName] = { ...defaultPrefInfo, ...prefInfo }; } return Object.assign( { - name: "fakename", + slug: "fakeslug", branch: "fakebranch", expired: false, lastSeen: NOW.toJSON(), experimentType: "exp", }, attrs, { preferences, @@ -168,16 +168,53 @@ const mockV4Data = { preferenceBranchType: "default", }, }, experimentType: "exp", }, }, }; +const mockV5Data = { + experiments: { + hypothetical_experiment: { + slug: "hypothetical_experiment", + branch: "hypo_1", + actionName: "SinglePreferenceExperimentAction", + expired: false, + lastSeen: NOW.toJSON(), + preferences: { + "some.pref": { + preferenceValue: 2, + preferenceType: "integer", + previousPreferenceValue: 1, + preferenceBranchType: "user", + }, + }, + experimentType: "exp", + }, + another_experiment: { + slug: "another_experiment", + branch: "another_4", + actionName: "SinglePreferenceExperimentAction", + expired: true, + lastSeen: NOW.toJSON(), + preferences: { + "another.pref": { + preferenceValue: true, + preferenceType: "boolean", + previousPreferenceValue: false, + preferenceBranchType: "default", + }, + }, + experimentType: "exp", + }, + }, +}; + /** * Make a mock `JsonFile` object with a no-op `saveSoon` method and a deep copy * of the data passed. * @param {Object} data the data in the store */ function makeMockJsonFile(data = {}) { return { // Deep clone the data in case migrations mutate it. @@ -198,16 +235,22 @@ add_task(async function test_migrations( 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 migration03KeepsActionName() { let mockData = JSON.parse(JSON.stringify(mockV3Data)); mockData.experiments.another_experiment.actionName = "SomeOldAction"; const mockJsonFile = makeMockJsonFile(mockData); // Output should be the same as mockV4Data, but preserving the action. const migratedData = JSON.parse(JSON.stringify(mockV4Data)); @@ -217,16 +260,17 @@ add_task(async function migration03Keeps 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); Assert.deepEqual( @@ -234,35 +278,35 @@ add_task(async function migrations_are_i mockJsonFileTwice.data, "migrating data twice should be idempotent for " + migration.name ); } }); // clearAllExperimentStorage decorate_task( - withMockExperiments([experimentFactory({ name: "test" })]), + withMockExperiments([experimentFactory({ slug: "test" })]), async function(experiments) { ok(await PreferenceExperiments.has("test"), "Mock experiment is detected."); await PreferenceExperiments.clearAllExperimentStorage(); ok( !(await PreferenceExperiments.has("test")), "clearAllExperimentStorage removed all stored experiments" ); } ); // start should throw if an experiment with the given name already exists decorate_task( - withMockExperiments([experimentFactory({ name: "test" })]), + withMockExperiments([experimentFactory({ slug: "test" })]), withSendEventStub, async function(experiments, sendEventStub) { await Assert.rejects( PreferenceExperiments.start({ - name: "test", + slug: "test", actionName: "SomeAction", branch: "branch", preferences: { "fake.preference": { preferenceValue: "value", preferenceType: "string", preferenceBranchType: "default", }, @@ -278,25 +322,25 @@ decorate_task( } ); // start should throw if an experiment for any of the given // preferences are active decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", preferences: { "fake.preferenceinteger": {} }, }), ]), withSendEventStub, async function(experiments, sendEventStub) { await Assert.rejects( PreferenceExperiments.start({ - name: "different", + slug: "different", actionName: "SomeAction", branch: "branch", preferences: { "fake.preference": { preferenceValue: "value", preferenceType: "string", preferenceBranchType: "default", }, @@ -324,17 +368,17 @@ decorate_task( // start should throw if an invalid preferenceBranchType is given decorate_task(withMockExperiments(), withSendEventStub, async function( experiments, sendEventStub ) { await Assert.rejects( PreferenceExperiments.start({ - name: "test", + slug: "test", actionName: "SomeAction", branch: "branch", preferences: { "fake.preference": { preferenceValue: "value", preferenceType: "string", preferenceBranchType: "invalid", }, @@ -363,17 +407,17 @@ decorate_task( sendEventStub ) { mockPreferences.set("fake.preference", "oldvalue", "default"); mockPreferences.set("fake.preference", "uservalue", "user"); mockPreferences.set("fake.preferenceinteger", 1, "default"); mockPreferences.set("fake.preferenceinteger", 101, "user"); const experiment = { - name: "test", + slug: "test", actionName: "SomeAction", branch: "branch", preferences: { "fake.preference": { preferenceValue: "newvalue", preferenceBranchType: "default", preferenceType: "string", }, @@ -387,17 +431,17 @@ decorate_task( await PreferenceExperiments.start(experiment); ok(await PreferenceExperiments.get("test"), "start saved the experiment"); ok( startObserverStub.calledWith("test", experiment.preferences), "start registered an observer" ); const expectedExperiment = { - name: "test", + slug: "test", branch: "branch", expired: false, preferences: { "fake.preference": { preferenceValue: "newvalue", preferenceType: "string", previousPreferenceValue: "oldvalue", preferenceBranchType: "default", @@ -459,17 +503,17 @@ decorate_task( withMockExperiments(), withMockPreferences, withStub(PreferenceExperiments, "startObserver"), async function(experiments, mockPreferences, startObserver) { mockPreferences.set("fake.preference", "olddefaultvalue", "default"); mockPreferences.set("fake.preference", "oldvalue", "user"); const experiment = { - name: "test", + slug: "test", actionName: "SomeAction", branch: "branch", preferences: { "fake.preference": { preferenceValue: "newvalue", preferenceType: "string", preferenceBranchType: "user", }, @@ -477,17 +521,17 @@ decorate_task( }; await PreferenceExperiments.start(experiment); ok( startObserver.calledWith("test", experiment.preferences), "start registered an observer" ); const expectedExperiment = { - name: "test", + slug: "test", branch: "branch", expired: false, preferences: { "fake.preference": { preferenceValue: "newvalue", preferenceType: "string", previousPreferenceValue: "oldvalue", preferenceBranchType: "user", @@ -523,17 +567,17 @@ decorate_task( decorate_task(withMockPreferences, withSendEventStub, async function( mockPreferences, sendEventStub ) { mockPreferences.set("fake.type_preference", "oldvalue"); await Assert.rejects( PreferenceExperiments.start({ - name: "test", + slug: "test", actionName: "SomeAction", branch: "branch", preferences: { "fake.type_preference": { preferenceBranchType: "user", preferenceValue: 12345, preferenceType: "integer", }, @@ -770,17 +814,17 @@ decorate_task(withMockExperiments(), asy /could not find/i, "markLastSeen threw because there was not a matching experiment" ); }); // markLastSeen should update the lastSeen date const oldDate = new Date(1988, 10, 1).toJSON(); decorate_task( - withMockExperiments([experimentFactory({ name: "test", lastSeen: oldDate })]), + withMockExperiments([experimentFactory({ slug: "test", lastSeen: oldDate })]), async function([experiment]) { await PreferenceExperiments.markLastSeen("test"); Assert.notEqual( experiment.lastSeen, oldDate, "markLastSeen updated the experiment lastSeen date" ); } @@ -804,17 +848,17 @@ decorate_task(withMockExperiments(), wit "test", { reason: "does-not-exist" }, ], ]); }); // stop should throw if the experiment is already expired decorate_task( - withMockExperiments([experimentFactory({ name: "test", expired: true })]), + withMockExperiments([experimentFactory({ slug: "test", expired: true })]), withSendEventStub, async function(experiments, sendEventStub) { await Assert.rejects( PreferenceExperiments.stop("test"), /already expired/, "stop threw an error because the experiment was already expired" ); @@ -829,17 +873,17 @@ decorate_task( } ); // stop should mark the experiment as expired, stop its observer, and revert the // preference value. decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", expired: false, branch: "fakebranch", preferences: { "fake.preference": { preferenceValue: "experimentvalue", preferenceType: "string", previousPreferenceValue: "oldvalue", preferenceBranchType: "default", @@ -907,17 +951,17 @@ decorate_task( PreferenceExperiments.stopAllObservers(); } ); // stop should also support user pref experiments decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", expired: false, preferences: { "fake.preference": { preferenceValue: "experimentvalue", preferenceType: "string", previousPreferenceValue: "oldvalue", preferenceBranchType: "user", }, @@ -956,17 +1000,17 @@ decorate_task( PreferenceExperiments.stopAllObservers(); } ); // stop should remove a preference that had no value prior to an experiment for user prefs decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", expired: false, preferences: { "fake.preference": { preferenceValue: "experimentvalue", preferenceType: "string", previousPreferenceValue: null, preferenceBranchType: "user", }, @@ -987,17 +1031,17 @@ decorate_task( stopObserver.restore(); } ); // stop should not modify a preference if resetValue is false decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", expired: false, branch: "fakebranch", preferences: { "fake.preference": { preferenceValue: "experimentvalue", preferenceType: "string", previousPreferenceValue: "oldvalue", preferenceBranchType: "default", @@ -1046,115 +1090,115 @@ decorate_task(withMockExperiments(), asy PreferenceExperiments.get("neverexisted"), /could not find/i, "get rejects if no experiment with the given name is found" ); }); // get decorate_task( - withMockExperiments([experimentFactory({ name: "test" })]), + withMockExperiments([experimentFactory({ slug: "test" })]), async function(experiments) { const experiment = await PreferenceExperiments.get("test"); - is(experiment.name, "test", "get fetches the correct experiment"); + is(experiment.slug, "test", "get fetches the correct experiment"); // Modifying the fetched experiment must not edit the data source. - experiment.name = "othername"; + experiment.slug = "othername"; const refetched = await PreferenceExperiments.get("test"); - is(refetched.name, "test", "get returns a copy of the experiment"); + is(refetched.slug, "test", "get returns a copy of the experiment"); } ); // get all decorate_task( withMockExperiments([ - experimentFactory({ name: "experiment1", disabled: false }), - experimentFactory({ name: "experiment2", disabled: true }), + experimentFactory({ slug: "experiment1", disabled: false }), + experimentFactory({ slug: "experiment2", disabled: true }), ]), async function testGetAll([experiment1, experiment2]) { const fetchedExperiments = await PreferenceExperiments.getAll(); is( fetchedExperiments.length, 2, "getAll returns a list of all stored experiments" ); Assert.deepEqual( - fetchedExperiments.find(e => e.name === "experiment1"), + fetchedExperiments.find(e => e.slug === "experiment1"), experiment1, "getAll returns a list with the correct experiments" ); const fetchedExperiment2 = fetchedExperiments.find( - e => e.name === "experiment2" + e => e.slug === "experiment2" ); Assert.deepEqual( fetchedExperiment2, experiment2, "getAll returns a list with the correct experiments, including disabled ones" ); - fetchedExperiment2.name = "othername"; + fetchedExperiment2.slug = "otherslug"; is( - experiment2.name, + experiment2.slug, "experiment2", "getAll returns copies of the experiments" ); } ); // get all active decorate_task( withMockExperiments([ experimentFactory({ - name: "active", + slug: "active", expired: false, }), experimentFactory({ - name: "inactive", + slug: "inactive", expired: true, }), ]), withMockPreferences, async function testGetAllActive([activeExperiment, inactiveExperiment]) { let allActiveExperiments = await PreferenceExperiments.getAllActive(); Assert.deepEqual( allActiveExperiments, [activeExperiment], "getAllActive only returns active experiments" ); - allActiveExperiments[0].name = "newfakename"; + allActiveExperiments[0].slug = "newfakename"; allActiveExperiments = await PreferenceExperiments.getAllActive(); Assert.notEqual( allActiveExperiments, "newfakename", "getAllActive returns copies of stored experiments" ); } ); // has decorate_task( - withMockExperiments([experimentFactory({ name: "test" })]), + withMockExperiments([experimentFactory({ slug: "test" })]), async function(experiments) { ok( await PreferenceExperiments.has("test"), "has returned true for a stored experiment" ); ok( !(await PreferenceExperiments.has("missing")), "has returned false for a missing experiment" ); } ); // init should register telemetry experiments decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", branch: "branch", preferences: { "fake.pref": { preferenceValue: "experiment value", expired: false, preferenceBranchType: "default", }, }, @@ -1177,17 +1221,17 @@ decorate_task( ); } ); // init should use the provided experiment type decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", branch: "branch", preferences: { "fake.pref": { preferenceValue: "experiment value", }, }, experimentType: "pref-test", }), @@ -1220,17 +1264,17 @@ decorate_task( withSendEventStub, async function testStartAndStopTelemetry( experiments, setActiveStub, setInactiveStub, sendEventStub ) { await PreferenceExperiments.start({ - name: "test", + slug: "test", actionName: "SomeAction", branch: "branch", preferences: { "fake.preference": { preferenceValue: "value", preferenceType: "string", preferenceBranchType: "default", }, @@ -1281,17 +1325,17 @@ decorate_task( withSendEventStub, async function testInitTelemetryExperimentType( experiments, setActiveStub, setInactiveStub, sendEventStub ) { await PreferenceExperiments.start({ - name: "test", + slug: "test", actionName: "SomeAction", branch: "branch", preferences: { "fake.preference": { preferenceValue: "value", preferenceType: "string", preferenceBranchType: "default", }, @@ -1321,30 +1365,30 @@ decorate_task( // Reset the preference so it doesn't interfere with other tests. Services.prefs.getDefaultBranch("fake.preference").deleteBranch(""); } ); // Experiments shouldn't be recorded by init() in telemetry if they are expired decorate_task( withMockExperiments([ - experimentFactory({ name: "expired", branch: "branch", expired: true }), + experimentFactory({ slug: "expired", branch: "branch", expired: true }), ]), withStub(TelemetryEnvironment, "setExperimentActive"), async function testInitTelemetryExpired(experiments, setActiveStub) { await PreferenceExperiments.init(); ok(!setActiveStub.called, "Expired experiment is not registered by init"); } ); // Experiments should end if the preference has been changed when init() is called decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", preferences: { "fake.preference": { preferenceValue: "experiment value", }, }, }), ]), withMockPreferences, @@ -1373,17 +1417,17 @@ decorate_task( ); } ); // init should register an observer for experiments decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", preferences: { "fake.preference": { preferenceValue: "experiment value", }, }, }), ]), withMockPreferences, @@ -1423,33 +1467,33 @@ decorate_task( ); } ); // saveStartupPrefs decorate_task( withMockExperiments([ experimentFactory({ - name: "char", + slug: "char", preferences: { "fake.char": { preferenceValue: "string", }, }, }), experimentFactory({ - name: "int", + slug: "int", preferences: { "fake.int": { preferenceValue: 2, }, }, }), experimentFactory({ - name: "bool", + slug: "bool", preferences: { "fake.bool": { preferenceValue: true, }, }, }), ]), async function testSaveStartupPrefs(experiments) { @@ -1477,17 +1521,17 @@ decorate_task( ); } ); // saveStartupPrefs errors for invalid pref type decorate_task( withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", preferences: { "fake.invalidValue": { preferenceValue: new Date(), }, }, }), ]), async function testSaveStartupPrefsError(experiments) { @@ -1498,26 +1542,26 @@ decorate_task( ); } ); // saveStartupPrefs should not store values for user-branch recipes decorate_task( withMockExperiments([ experimentFactory({ - name: "defaultBranchRecipe", + slug: "defaultBranchRecipe", preferences: { "fake.default": { preferenceValue: "experiment value", preferenceBranchType: "default", }, }, }), experimentFactory({ - name: "userBranchRecipe", + slug: "userBranchRecipe", preferences: { "fake.user": { preferenceValue: "experiment value", preferenceBranchType: "user", }, }, }), ]), @@ -1560,17 +1604,17 @@ decorate_task( withStub(PreferenceExperiments, "startObserver"), withStub(PreferenceExperiments, "stopObserver"), async function testDefaultBranchStop(mockExperiments, mockPreferences) { const prefName = "fake.preference"; mockPreferences.set(prefName, "old version's value", "default"); // start an experiment await PreferenceExperiments.start({ - name: "test", + slug: "test", actionName: "SomeAction", branch: "branch", preferences: { [prefName]: { preferenceValue: "experiment value", preferenceBranchType: "default", preferenceType: "string", }, @@ -1614,17 +1658,17 @@ decorate_task( withStub(PreferenceExperiments, "startObserver"), withStub(PreferenceExperiments, "stopObserver"), async function testDefaultBranchStop(mockExperiments, mockPreferences) { const prefName = "fake.preference"; mockPreferences.set(prefName, "old version's value", "default"); // start an experiment await PreferenceExperiments.start({ - name: "test", + slug: "test", actionName: "SomeAction", branch: "branch", preferences: { [prefName]: { preferenceValue: "experiment value", preferenceBranchType: "default", preferenceType: "string", }, @@ -1656,17 +1700,17 @@ decorate_task( "Preference should be absent" ); } ).skip(/* bug 1502410 and bug 1505941 */); // stop should pass "unknown" to telemetry event for `reason` if none is specified decorate_task( withMockExperiments([ - experimentFactory({ name: "test", preferences: { "fake.preference": {} } }), + experimentFactory({ slug: "test", preferences: { "fake.preference": {} } }), ]), withMockPreferences, withStub(PreferenceExperiments, "stopObserver"), withSendEventStub, async function testStopUnknownReason( experiments, mockPreferences, stopObserverStub, @@ -1681,21 +1725,21 @@ decorate_task( ); } ); // stop should pass along the value for resetValue to Telemetry Events as didResetValue decorate_task( withMockExperiments([ experimentFactory({ - name: "test1", + slug: "test1", preferences: { "fake.preference1": {} }, }), experimentFactory({ - name: "test2", + slug: "test2", preferences: { "fake.preference2": {} }, }), ]), withMockPreferences, withStub(PreferenceExperiments, "stopObserver"), withSendEventStub, async function testStopResetValue( experiments, @@ -1725,17 +1769,17 @@ decorate_task( // Should send the correct event telemetry when a study ends because // the user changed preferences during a browser run. decorate_task( withMockPreferences, withSendEventStub, withMockExperiments([ experimentFactory({ - name: "test", + slug: "test", expired: false, branch: "fakebranch", preferences: { "fake.preference": { preferenceValue: "experimentvalue", preferenceType: "string", previousPreferenceValue: "oldvalue", preferenceBranchType: "default",
--- a/toolkit/components/normandy/test/browser/browser_ShieldPreferences.js +++ b/toolkit/components/normandy/test/browser/browser_ShieldPreferences.js @@ -67,13 +67,13 @@ decorate_task( resolve(); } }); }); Services.prefs.setBoolPref(OPT_OUT_STUDIES_ENABLED_PREF, false); await stoppedBoth; Assert.deepEqual(stopArgs, [ - [study1.name, { reason: "general-opt-out" }], - [study2.name, { reason: "general-opt-out" }], + [study1.slug, { reason: "general-opt-out" }], + [study2.slug, { reason: "general-opt-out" }], ]); } );
--- a/toolkit/components/normandy/test/browser/browser_about_studies.js +++ b/toolkit/components/normandy/test/browser/browser_about_studies.js @@ -106,27 +106,30 @@ decorate_task( userFacingName: "C Fake Add-on Study", active: true, userFacingDescription: "C fake description", studyStartDate: new Date(2018, 0, 1), }), ]), PreferenceExperiments.withMockExperiments([ preferenceStudyFactory({ - name: "D Fake Preference Study", + slug: "fake-study-d", + userFacingName: "D Fake Preference Study", lastSeen: new Date(2018, 0, 3), expired: false, }), preferenceStudyFactory({ - name: "E Fake Preference Study", + slug: "fake-study-e", + userFacingName: "E Fake Preference Study", lastSeen: new Date(2018, 0, 5), expired: true, }), preferenceStudyFactory({ - name: "F Fake Preference Study", + slug: "fake-study-f", + userFacingName: "F Fake Preference Study", lastSeen: new Date(2018, 0, 6), expired: false, }), ]), withAboutStudies, async function testStudyListing(addonStudies, prefStudies, browser) { await ContentTask.spawn( browser, @@ -146,26 +149,26 @@ decorate_task( ).map(row => row.dataset.studySlug); const inactiveNames = Array.from( doc.querySelectorAll(".inactive-study-list .study") ).map(row => row.dataset.studySlug); Assert.deepEqual( activeNames, [ - prefStudies[2].name, + prefStudies[2].slug, addonStudies[0].slug, - prefStudies[0].name, + prefStudies[0].slug, addonStudies[2].slug, ], "Active studies are grouped by enabled status, and sorted by date" ); Assert.deepEqual( inactiveNames, - [prefStudies[1].name, addonStudies[1].slug], + [prefStudies[1].slug, addonStudies[1].slug], "Inactive studies are grouped by enabled status, and sorted by date" ); const activeAddonStudy = getStudyRow(doc, addonStudies[0].slug); ok( activeAddonStudy .querySelector(".study-description") .textContent.includes(addonStudies[0].userFacingDescription), @@ -194,42 +197,35 @@ decorate_task( "Complete", "Inactive studies are marked as complete." ); ok( !inactiveAddonStudy.querySelector(".remove-button"), "Inactive studies do not show a remove button" ); - const activePrefStudy = getStudyRow(doc, prefStudies[0].name); + const activePrefStudy = getStudyRow(doc, prefStudies[0].slug); const preferenceName = Object.keys(prefStudies[0].preferences)[0]; ok( activePrefStudy .querySelector(".study-description") .textContent.includes(preferenceName), "Preference studies show the preference they are changing" ); is( activePrefStudy.querySelector(".study-status").textContent, "Active", "Active studies show an 'Active' indicator." ); ok( activePrefStudy.querySelector(".remove-button"), "Active studies show a remove button" ); - is( - activePrefStudy - .querySelector(".study-icon") - .textContent.toLowerCase(), - "d", - "Study icons use the first letter of the study name." - ); - const inactivePrefStudy = getStudyRow(doc, prefStudies[1].name); + const inactivePrefStudy = getStudyRow(doc, prefStudies[1].slug); is( inactivePrefStudy.querySelector(".study-status").textContent, "Complete", "Inactive studies are marked as complete." ); ok( !inactivePrefStudy.querySelector(".remove-button"), "Inactive studies do not show a remove button" @@ -241,33 +237,33 @@ decorate_task( ); ok( getStudyRow(doc, addonStudies[0].slug).matches(".study.disabled"), "Clicking the remove button updates the UI to show that the study has been disabled." ); activePrefStudy.querySelector(".remove-button").click(); await ContentTaskUtils.waitForCondition(() => - getStudyRow(doc, prefStudies[0].name).matches(".study.disabled") + getStudyRow(doc, prefStudies[0].slug).matches(".study.disabled") ); ok( - getStudyRow(doc, prefStudies[0].name).matches(".study.disabled"), + getStudyRow(doc, prefStudies[0].slug).matches(".study.disabled"), "Clicking the remove button updates the UI to show that the study has been disabled." ); } ); const updatedAddonStudy = await AddonStudies.get(addonStudies[0].recipeId); ok( !updatedAddonStudy.active, "Clicking the remove button marks addon studies as inactive in storage." ); const updatedPrefStudy = await PreferenceExperiments.get( - prefStudies[0].name + prefStudies[0].slug ); ok( updatedPrefStudy.expired, "Clicking the remove button marks preference studies as expired in storage." ); } ); @@ -301,17 +297,18 @@ decorate_task( slug: "fake-addon-study", active: false, userFacingDescription: "A fake description", studyStartDate: new Date(2018, 0, 4), }), ]), PreferenceExperiments.withMockExperiments([ preferenceStudyFactory({ - name: "B Fake Preference Study", + slug: "fake-pref-study", + userFacingName: "B Fake Preference Study", lastSeen: new Date(2018, 0, 5), expired: true, }), ]), async function testStudyListingDisabled( browser, addonStudies, preferenceStudies @@ -382,27 +379,28 @@ decorate_task( userFacingName: "Fake Add-on Study", active: true, userFacingDescription: "A fake description", studyStartDate: new Date(2018, 0, 4), }), ]), PreferenceExperiments.withMockExperiments([ preferenceStudyFactory({ - name: "Fake Preference Study", + slug: "fake-pref-study", + userFacingName: "Fake Preference Study", lastSeen: new Date(2018, 0, 3), expired: false, }), ]), withAboutStudies, async function testStudyListing([addonStudy], [prefStudy], browser) { // The content page has already loaded. Disabling the studies here shouldn't // affect it, since it doesn't live-update. await AddonStudies.markAsEnded(addonStudy, "disabled-automatically-test"); - await PreferenceExperiments.stop(prefStudy.name, { + await PreferenceExperiments.stop(prefStudy.slug, { resetValue: false, reason: "disabled-automatically-test", }); await ContentTask.spawn( browser, { addonStudy, prefStudy }, async ({ addonStudy, prefStudy }) => { @@ -419,43 +417,43 @@ decorate_task( doc.querySelectorAll(".active-study-list .study") ).map(row => row.dataset.studySlug); let inactiveNames = Array.from( doc.querySelectorAll(".inactive-study-list .study") ).map(row => row.dataset.studySlug); Assert.deepEqual( activeNames, - [addonStudy.slug, prefStudy.name], + [addonStudy.slug, prefStudy.slug], "Both studies should be listed as active, even though they have been disabled outside of the page" ); Assert.deepEqual( inactiveNames, [], "No studies should be listed as inactive" ); const activeAddonStudy = getStudyRow(doc, addonStudy.slug); - const activePrefStudy = getStudyRow(doc, prefStudy.name); + const activePrefStudy = getStudyRow(doc, prefStudy.slug); activeAddonStudy.querySelector(".remove-button").click(); await ContentTaskUtils.waitForCondition(() => getStudyRow(doc, addonStudy.slug).matches(".study.disabled") ); ok( getStudyRow(doc, addonStudy.slug).matches(".study.disabled"), "Clicking the remove button updates the UI to show that the study has been disabled." ); activePrefStudy.querySelector(".remove-button").click(); await ContentTaskUtils.waitForCondition(() => - getStudyRow(doc, prefStudy.name).matches(".study.disabled") + getStudyRow(doc, prefStudy.slug).matches(".study.disabled") ); ok( - getStudyRow(doc, prefStudy.name).matches(".study.disabled"), + getStudyRow(doc, prefStudy.slug).matches(".study.disabled"), "Clicking the remove button updates the UI to show that the study has been disabled." ); activeNames = Array.from( doc.querySelectorAll(".active-study-list .study") ).map(row => row.dataset.studySlug); Assert.deepEqual(
--- a/toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js +++ b/toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js @@ -149,17 +149,17 @@ decorate_task( }); await action.runRecipe(recipe); await action.finalize(); Assert.deepEqual(startStub.args, [ [ { - name: "test", + slug: "test", actionName: "PreferenceExperimentAction", branch: "branch1", preferences: { "fake.preference": { preferenceValue: "branch1", preferenceBranchType: "user", preferenceType: "string", }, @@ -172,34 +172,34 @@ decorate_task( ], ]); } ); decorate_task( withStudiesEnabled, withStub(PreferenceExperiments, "markLastSeen"), - PreferenceExperiments.withMockExperiments([{ name: "test", expired: false }]), + PreferenceExperiments.withMockExperiments([{ slug: "test", expired: false }]), async function markSeen_if_experiment_active(markLastSeenStub) { const action = new PreferenceExperimentAction(); const recipe = preferenceExperimentFactory({ name: "test", }); await action.runRecipe(recipe); await action.finalize(); Assert.deepEqual(markLastSeenStub.args, [["test"]]); } ); decorate_task( withStudiesEnabled, withStub(PreferenceExperiments, "markLastSeen"), - PreferenceExperiments.withMockExperiments([{ name: "test", expired: true }]), + PreferenceExperiments.withMockExperiments([{ slug: "test", expired: true }]), async function dont_markSeen_if_experiment_expired(markLastSeenStub) { const action = new PreferenceExperimentAction(); const recipe = preferenceExperimentFactory({ name: "test", }); await action.runRecipe(recipe); await action.finalize(); @@ -223,19 +223,19 @@ decorate_task( Assert.deepEqual(startStub.args, [], "start was not called"); } ); decorate_task( withStudiesEnabled, withStub(PreferenceExperiments, "stop"), PreferenceExperiments.withMockExperiments([ - { name: "seen", expired: false, actionName: "PreferenceExperimentAction" }, + { slug: "seen", expired: false, actionName: "PreferenceExperimentAction" }, { - name: "unseen", + slug: "unseen", expired: false, actionName: "PreferenceExperimentAction", }, ]), async function stop_experiments_not_seen(stopStub) { const action = new PreferenceExperimentAction(); const recipe = preferenceExperimentFactory({ slug: "seen", @@ -250,22 +250,22 @@ decorate_task( } ); decorate_task( withStudiesEnabled, withStub(PreferenceExperiments, "stop"), PreferenceExperiments.withMockExperiments([ { - name: "seen", + slug: "seen", expired: false, actionName: "SinglePreferenceExperimentAction", }, { - name: "unseen", + slug: "unseen", expired: false, actionName: "SinglePreferenceExperimentAction", }, ]), async function dont_stop_experiments_for_other_action(stopStub) { const action = new PreferenceExperimentAction(); const recipe = preferenceExperimentFactory({ name: "seen", @@ -283,17 +283,17 @@ decorate_task( ); decorate_task( withStudiesEnabled, withStub(PreferenceExperiments, "start"), withStub(Uptake, "reportRecipe"), PreferenceExperiments.withMockExperiments([ { - name: "conflict", + slug: "conflict", preferences: { "conflict.pref": {}, }, expired: false, }, ]), async function do_nothing_if_preference_is_already_being_tested( startStub, @@ -442,17 +442,17 @@ decorate_task( }); await action.runRecipe(recipe); await action.finalize(); const activeExperiments = await PreferenceExperiments.getAllActive(); ok(activeExperiments.length > 0); Assert.deepEqual(activeExperiments, [ { - name: "integration test experiment", + slug: "integration test experiment", actionName: "PreferenceExperimentAction", branch: "branch1", preferences: { "fake.preference": { preferenceBranchType: "user", preferenceValue: "branch1", preferenceType: "string", previousPreferenceValue: "oldvalue",