From dc94e359c19e24c7b6ec54ee2075659a6280ac88 Mon Sep 17 00:00:00 2001 From: Charlie Date: Tue, 21 Nov 2023 10:20:48 -0600 Subject: [PATCH] add published_date field to experiment schema --- components/nimbus/src/enrollment.rs | 7 +++ components/nimbus/src/schema.rs | 14 +++++ components/nimbus/src/tests/helpers.rs | 52 +++++++++++++++++ .../nimbus/src/tests/test_enrollment.rs | 57 ++++++++++++++++++- 4 files changed, 129 insertions(+), 1 deletion(-) diff --git a/components/nimbus/src/enrollment.rs b/components/nimbus/src/enrollment.rs index 0a4552600f..a4d52631f6 100644 --- a/components/nimbus/src/enrollment.rs +++ b/components/nimbus/src/enrollment.rs @@ -681,6 +681,7 @@ impl<'a> EnrollmentsEvolver<'a> { // Step 3. Evolve the remaining enrollments with the previous and // next data. + let next_experiments = sort_experiments_by_published_date(next_experiments); for next_experiment in next_experiments { let slug = &next_experiment.slug; @@ -946,6 +947,12 @@ where .collect() } +pub(crate) fn sort_experiments_by_published_date(experiments: &[Experiment]) -> Vec<&Experiment> { + let mut experiments: Vec<_> = experiments.iter().collect(); + experiments.sort_by(|a, b| a.published_date.cmp(&b.published_date)); + experiments +} + /// Take a list of enrollments and a map of experiments, and generate mapping of `feature_id` to /// `EnrolledFeatureConfig` structs. fn map_features( diff --git a/components/nimbus/src/schema.rs b/components/nimbus/src/schema.rs index 3f1a22b3b2..e2faea9771 100644 --- a/components/nimbus/src/schema.rs +++ b/components/nimbus/src/schema.rs @@ -46,6 +46,7 @@ pub struct Experiment { pub reference_branch: Option, #[serde(default)] pub is_rollout: bool, + pub published_date: Option>, // N.B. records in RemoteSettings will have `id` and `filter_expression` fields, // but we ignore them because they're for internal use by RemoteSettings. } @@ -76,6 +77,19 @@ impl Experiment { feature_ids.into_iter().collect() } + + #[cfg(test)] + pub(crate) fn patch(&self, patch: Value) -> Self { + let mut experiment = serde_json::to_value(self).unwrap(); + if let (Some(e), Some(w)) = (experiment.as_object(), patch.as_object()) { + let mut e = e.clone(); + for (key, value) in w { + e.insert(key.clone(), value.clone()); + } + experiment = serde_json::to_value(e).unwrap(); + } + serde_json::from_value(experiment).unwrap() + } } impl ExperimentMetadata for Experiment { diff --git a/components/nimbus/src/tests/helpers.rs b/components/nimbus/src/tests/helpers.rs index 2020b29aec..2c9159d37f 100644 --- a/components/nimbus/src/tests/helpers.rs +++ b/components/nimbus/src/tests/helpers.rs @@ -552,3 +552,55 @@ pub fn get_targeted_experiment(slug: &str, targeting: &str) -> serde_json::Value "last_modified": 1_602_197_324_372i64, }) } + +pub(crate) fn get_experiment_with_published_date( + slug: &str, + published_date: Option, +) -> Experiment { + serde_json::from_value(json!({ + "schemaVersion": "1.0.0", + "slug": slug, + "endDate": null, + "branches":[ + { + "slug": "control", + "ratio": 1, + "feature": { + "featureId": "about_welcome", + "enabled": true, + } + }, + { + "slug": "treatment", + "ratio": 1, + "feature": { + "featureId": "about_welcome", + "enabled": false, + } + }, + ], + "featureIds": ["about_welcome"], + "channel": "nightly", + "probeSets":[], + "startDate":null, + "appName":"fenix", + "appId":"org.mozilla.fenix", + "bucketConfig":{ + // Also enroll everyone. + "count":10_000, + "start":0, + "total":10_000, + "namespace":"secure-silver", + "randomizationUnit":"nimbus_id" + }, + "userFacingName":"2nd test experiment", + "referenceBranch":"control", + "isEnrollmentPaused":false, + "proposedEnrollment":7, + "userFacingDescription":"2nd test experiment.", + "id":"secure-silver", + "last_modified":1_602_197_324_372i64, + "publishedDate": published_date + })) + .unwrap() +} diff --git a/components/nimbus/src/tests/test_enrollment.rs b/components/nimbus/src/tests/test_enrollment.rs index e9e826ea34..df11afa031 100644 --- a/components/nimbus/src/tests/test_enrollment.rs +++ b/components/nimbus/src/tests/test_enrollment.rs @@ -4,7 +4,7 @@ // Testing enrollment.rs -use crate::tests::helpers::get_bucketed_rollout; +use crate::tests::helpers::{get_bucketed_rollout, get_experiment_with_published_date}; use crate::{ defaults::Defaults, enrollment::*, @@ -3136,3 +3136,58 @@ fn test_populate_feature_maps() -> Result<()> { Ok(()) } + +#[test] +fn test_sort_experiments_by_published_date() -> Result<()> { + let slug_1 = "slug-1"; + let slug_2 = "slug-2"; + let slug_3 = "slug-3"; + let slug_4 = "slug-4"; + let experiments = vec![ + get_experiment_with_published_date(slug_1, None), + get_experiment_with_published_date(slug_2, Some("2023-11-21T18:00:00Z".into())), + get_experiment_with_published_date(slug_3, None), + get_experiment_with_published_date(slug_4, Some("2023-11-21T15:00:00Z".into())), + ]; + let result = sort_experiments_by_published_date(&experiments); + + assert_eq!(slug_1, result[0].slug); + assert_eq!(slug_3, result[1].slug); + assert_eq!(slug_4, result[2].slug); + assert_eq!(slug_2, result[3].slug); + Ok(()) +} + +#[test] +fn test_evolve_enrollments_ordering() -> Result<()> { + let _ = env_logger::try_init(); + let (_, app_ctx, aru) = local_ctx(); + let th = app_ctx.into(); + let ids = HashSet::new(); + let evolver = EnrollmentsEvolver::new(&aru, &th, &ids); + + let exp1 = get_single_feature_experiment("slug-1", "colliding-feature", json!({"x": 1 })) + .patch(json!({"publishedDate": "2023-11-21T18:00:00Z"})); + let exp2 = get_single_feature_experiment("slug-2", "colliding-feature", json!({"x": 2 })) + .patch(json!({"publishedDate": "2023-11-21T15:00:00Z"})); + + let all_experiments = [exp1, exp2]; + let no_experiments: [Experiment; 0] = []; + + let (enrollments, _) = + evolver.evolve_enrollment_recipes(true, &no_experiments, &all_experiments, &[])?; + + let observed = map_features_by_feature_id(&enrollments, &all_experiments, &ids); + let expected = HashMap::from([( + "colliding-feature".to_string(), + EnrolledFeatureConfig::new( + "colliding-feature", + json!({"x": 2 }), + "slug-2", + Some("control"), + ), + )]); + assert_eq!(observed, expected); + + Ok(()) +}