From f5f6fc49ba21bbf7689a30cc208aaf2b54c3e45b Mon Sep 17 00:00:00 2001 From: Adam Butterworth Date: Fri, 10 Apr 2020 14:59:46 -0400 Subject: [PATCH 1/5] Reenable Learning Sequence MFE redirect tests TNL-7157 These tests may were the cause of intermittent test failures a couple weeks ago. Here they are reenabled after changing the way ExperimentWaffleFlag is overridden. --- lms/djangoapps/courseware/tests/test_views.py | 23 +++++++++---------- lms/djangoapps/experiments/flags.py | 6 ++++- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 4dfbaac28b..8c44453bed 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -3256,7 +3256,7 @@ class TestShowCoursewareMFE(TestCase): ) for user, course_key, is_course_staff, preview_active, redirect_active in combos: with override_waffle_flag(COURSEWARE_MICROFRONTEND_COURSE_TEAM_PREVIEW, preview_active): - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, redirect_active): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=redirect_active): assert show_courseware_mfe_link(user, is_course_staff, course_key) is False @patch.dict(settings.FEATURES, {'ENABLE_COURSEWARE_MICROFRONTEND': True}) @@ -3276,14 +3276,14 @@ class TestShowCoursewareMFE(TestCase): ) for user, is_course_staff, preview_active, redirect_active in old_mongo_combos: with override_waffle_flag(COURSEWARE_MICROFRONTEND_COURSE_TEAM_PREVIEW, preview_active): - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, redirect_active): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=redirect_active): assert show_courseware_mfe_link(user, is_course_staff, old_course_key) is False # We've checked all old-style course keys now, so we can test only the # new ones going forward. Now we check combinations of waffle flags and # user permissions... with override_waffle_flag(COURSEWARE_MICROFRONTEND_COURSE_TEAM_PREVIEW, True): - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): # (preview=on, redirect=on) # Global and Course Staff can see the link. self.assertTrue(show_courseware_mfe_link(global_staff_user, True, new_course_key)) @@ -3292,7 +3292,7 @@ class TestShowCoursewareMFE(TestCase): # Regular users don't see the link. self.assertFalse(show_courseware_mfe_link(regular_user, False, new_course_key)) - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, False): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=False): # (preview=on, redirect=off) # Global and Course Staff can see the link. self.assertTrue(show_courseware_mfe_link(global_staff_user, True, new_course_key)) @@ -3303,7 +3303,7 @@ class TestShowCoursewareMFE(TestCase): self.assertFalse(show_courseware_mfe_link(regular_user, False, new_course_key)) with override_waffle_flag(COURSEWARE_MICROFRONTEND_COURSE_TEAM_PREVIEW, False): - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): # (preview=off, redirect=on) # Global staff see the link anyway self.assertTrue(show_courseware_mfe_link(global_staff_user, True, new_course_key)) @@ -3315,7 +3315,7 @@ class TestShowCoursewareMFE(TestCase): # Regular users don't see the link. self.assertFalse(show_courseware_mfe_link(regular_user, False, new_course_key)) - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, False): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=False): # (preview=off, redirect=off) # Global staff see the link anyway self.assertTrue(show_courseware_mfe_link(global_staff_user, True, new_course_key)) @@ -3348,8 +3348,7 @@ class TestShowCoursewareMFE(TestCase): '/block-v1:OpenEdX+MFE+2020+type@vertical+block@Getting_To_Know_You' ) -# TODO: TNL-7157 Re-enable these tests before Courseware MFE Canary -@unittest.skip + @patch.dict('django.conf.settings.FEATURES', {'ENABLE_COURSEWARE_MICROFRONTEND': True}) @ddt.ddt class MFERedirectTests(BaseViewsTestCase): @@ -3375,7 +3374,7 @@ class MFERedirectTests(BaseViewsTestCase): # learners will be redirected when the waffle flag is set lms_url, mfe_url = self._get_urls() - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): assert self.client.get(lms_url).url == mfe_url def test_staff_no_redirect(self): @@ -3387,14 +3386,14 @@ class MFERedirectTests(BaseViewsTestCase): self.client.login(username=course_staff.username, password='test') assert self.client.get(lms_url).status_code == 200 - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): assert self.client.get(lms_url).status_code == 200 # global staff will never be redirected self._create_global_staff_user() assert self.client.get(lms_url).status_code == 200 - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): assert self.client.get(lms_url).status_code == 200 def test_exam_no_redirect(self): @@ -3404,5 +3403,5 @@ class MFERedirectTests(BaseViewsTestCase): lms_url, mfe_url = self._get_urls() - with override_waffle_flag(REDIRECT_TO_COURSEWARE_MICROFRONTEND, True): + with REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True): assert self.client.get(lms_url).status_code == 200 diff --git a/lms/djangoapps/experiments/flags.py b/lms/djangoapps/experiments/flags.py index 3c274e3566..ca0ef80ea2 100644 --- a/lms/djangoapps/experiments/flags.py +++ b/lms/djangoapps/experiments/flags.py @@ -178,4 +178,8 @@ class ExperimentWaffleFlag(CourseWaffleFlag): if not active: bucket = 0 with patch.object(self, 'get_bucket', return_value=bucket): - yield + # TODO We can move this import to the top of the file once this code is + # not all contained within the __init__ module. + from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag + with override_waffle_flag(self, active): + yield From 2459382fe3decabf2296156e1e6aa1ef5878c872 Mon Sep 17 00:00:00 2001 From: Adam Butterworth Date: Mon, 13 Apr 2020 16:39:46 -0400 Subject: [PATCH 2/5] Update is_experiment_on and add ExperimentWaffleFlag override test --- lms/djangoapps/experiments/flags.py | 21 +++++++++++-------- .../experiments/tests/test_flags.py | 15 ++++++++++++- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/experiments/flags.py b/lms/djangoapps/experiments/flags.py index ca0ef80ea2..c7f8715775 100644 --- a/lms/djangoapps/experiments/flags.py +++ b/lms/djangoapps/experiments/flags.py @@ -170,16 +170,19 @@ class ExperimentWaffleFlag(CourseWaffleFlag): return self.is_enabled() def is_experiment_on(self, course_key=None): - return super().is_enabled(course_key) + # If no course_key is supplied check the global flag irrespective of courses + if course_key is None: + return super().is_enabled_without_course_context() + + return super().is_enabled(course_key=course_key) @contextmanager def override(self, active=True, bucket=1): # pylint: disable=arguments-differ - from mock import patch - if not active: - bucket = 0 - with patch.object(self, 'get_bucket', return_value=bucket): - # TODO We can move this import to the top of the file once this code is - # not all contained within the __init__ module. - from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag - with override_waffle_flag(self, active): + # Let CourseWaffleFlag override the base waffle flag value + with super().override(active=active): + # Now override the experiment bucket value + from mock import patch + if not active: + bucket = 0 + with patch.object(self, 'get_bucket', return_value=bucket): yield diff --git a/lms/djangoapps/experiments/tests/test_flags.py b/lms/djangoapps/experiments/tests/test_flags.py index c4fa568200..1d3c34eb04 100644 --- a/lms/djangoapps/experiments/tests/test_flags.py +++ b/lms/djangoapps/experiments/tests/test_flags.py @@ -105,7 +105,8 @@ class ExperimentWaffleFlagTests(SharedModuleStoreTestCase): def test_bucket_override(self, active, expected_bucket): bucket_flag = CourseWaffleFlag('experiments', 'test.0') with bucket_flag.override(active=active): - self.assertEqual(self.get_bucket(), expected_bucket) + self.assertEqual(self.flag.get_bucket(), expected_bucket) + self.assertEqual(self.flag.is_experiment_on(), active) def test_tracking(self): # Run twice, with same request @@ -143,3 +144,15 @@ class ExperimentWaffleFlagTests(SharedModuleStoreTestCase): self.assertEqual(self.flag.is_enabled_without_course_context(), False) self.assertEqual(self.flag.is_enabled(self.key), False) self.assertEqual(self.flag.is_enabled(), False) + + @ddt.data( + (True, 1, 1), + (True, 0, 0), + (False, 1, 0), # bucket is always 0 if the experiment is off + (False, 0, 0), + ) + @ddt.unpack + def test_override_method(self, active, bucket_override, expected_bucket): + with self.flag.override(active=active, bucket=bucket_override): + self.assertEqual(self.flag.get_bucket(), expected_bucket) + self.assertEqual(self.flag.is_experiment_on(), active) From e9e1715bd0d3f52ae672c627594b6089e703ab7e Mon Sep 17 00:00:00 2001 From: Adam Butterworth Date: Mon, 13 Apr 2020 16:42:18 -0400 Subject: [PATCH 3/5] Update test_flags.py --- lms/djangoapps/experiments/tests/test_flags.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/experiments/tests/test_flags.py b/lms/djangoapps/experiments/tests/test_flags.py index 1d3c34eb04..3f1328df87 100644 --- a/lms/djangoapps/experiments/tests/test_flags.py +++ b/lms/djangoapps/experiments/tests/test_flags.py @@ -102,11 +102,10 @@ class ExperimentWaffleFlagTests(SharedModuleStoreTestCase): (False, 1), ) @ddt.unpack - def test_bucket_override(self, active, expected_bucket): + def test_forcing_bucket(self, active, expected_bucket): bucket_flag = CourseWaffleFlag('experiments', 'test.0') with bucket_flag.override(active=active): - self.assertEqual(self.flag.get_bucket(), expected_bucket) - self.assertEqual(self.flag.is_experiment_on(), active) + self.assertEqual(self.get_bucket(), expected_bucket) def test_tracking(self): # Run twice, with same request @@ -152,6 +151,7 @@ class ExperimentWaffleFlagTests(SharedModuleStoreTestCase): (False, 0, 0), ) @ddt.unpack + # Test the override method def test_override_method(self, active, bucket_override, expected_bucket): with self.flag.override(active=active, bucket=bucket_override): self.assertEqual(self.flag.get_bucket(), expected_bucket) From b767d29561f07db2940984652f77a75ae21b194f Mon Sep 17 00:00:00 2001 From: Adam Butterworth Date: Mon, 13 Apr 2020 16:53:01 -0400 Subject: [PATCH 4/5] Add docstring about overrides --- lms/djangoapps/experiments/flags.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lms/djangoapps/experiments/flags.py b/lms/djangoapps/experiments/flags.py index c7f8715775..9e8429638b 100644 --- a/lms/djangoapps/experiments/flags.py +++ b/lms/djangoapps/experiments/flags.py @@ -40,6 +40,21 @@ class ExperimentWaffleFlag(CourseWaffleFlag): Bucket 0 is assumed to be the control bucket. See a HOWTO here: https://openedx.atlassian.net/wiki/spaces/AC/pages/1250623700/Bucketing+users+for+an+experiment + + When writing tests involving an ExperimentWaffleFlag you must not use the + override_waffle_flag utility. That will only turn the experiment on or off and won't + override bucketing. Instead use ExperimentWaffleFlag's override method which + will do both. Example: + + with MY_EXPERIMENT_WAFFLE_FLAG.override(active=True, bucket=1): + ... + + or as a decorator: + + @MY_EXPERIMENT_WAFFLE_FLAG.override(active=True, bucket=1) + def test_my_experiment(self): + ... + """ def __init__(self, waffle_namespace, flag_name, num_buckets=2, experiment_id=None, **kwargs): super().__init__(waffle_namespace, flag_name, **kwargs) From d066b43851e71d9c9a5f20936b99b29810e02484 Mon Sep 17 00:00:00 2001 From: Adam Butterworth Date: Tue, 14 Apr 2020 09:33:15 -0400 Subject: [PATCH 5/5] prefer is_experiment_on over _is_enabled --- lms/djangoapps/experiments/flags.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/experiments/flags.py b/lms/djangoapps/experiments/flags.py index 9e8429638b..254cb81bef 100644 --- a/lms/djangoapps/experiments/flags.py +++ b/lms/djangoapps/experiments/flags.py @@ -138,7 +138,7 @@ class ExperimentWaffleFlag(CourseWaffleFlag): return cache_response.value # Check if the main flag is even enabled for this user and course. - if not self._is_enabled(course_key): # grabs user from the current request, if any + if not self.is_experiment_on(course_key): # grabs user from the current request, if any return self._cache_bucket(experiment_name, 0) # Check if the enrollment should even be considered (if it started before the experiment wants, we ignore) @@ -189,7 +189,7 @@ class ExperimentWaffleFlag(CourseWaffleFlag): if course_key is None: return super().is_enabled_without_course_context() - return super().is_enabled(course_key=course_key) + return super().is_enabled(course_key) @contextmanager def override(self, active=True, bucket=1): # pylint: disable=arguments-differ