refactor: migrate leftover FEATURES-as-dict test overrides off the dict - #39005
Conversation
The EMBARGO production readers (lms/urls.py, openedx/core/djangoapps/embargo/api.py)
already read flat settings.EMBARGO; two verify_student view tests still set the flag via
@patch.dict(settings.FEATURES, {'EMBARGO': True}). Convert them to
@override_settings(EMBARGO=True) (matching the already-migrated decorator elsewhere in
the file) so no settings.FEATURES usage remains here.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…OKIE test overrides off FEATURES-as-dict
Both flags' production readers (openedx/core/djangoapps/cors_csrf/middleware.py) already
read flat settings.ENABLE_CORS_HEADERS / settings.ENABLE_CROSS_DOMAIN_CSRF_COOKIE. Two
tests still enabled them via @patch.dict(settings.FEATURES, {...}) sitting next to an
override_settings for the companion CORS_* settings. Fold the two flags into the adjacent
override_settings so no settings.FEATURES usage remains, and drop the now-unused patch
import in the cors_csrf test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…off FEATURES-as-dict
Both flags' production readers already read flat settings (courseware/plugins.py,
courseware/tabs.py, edxnotes/decorators.py, cms course_metadata.py). One tabs test still
enabled them via @patch.dict(settings.FEATURES, {...}) alongside an
@override_settings(ENABLE_DISCUSSION_SERVICE=True); fold both flags into that
override_settings so no settings.FEATURES usage remains.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-dict
The ENABLE_THIRD_PARTY_AUTH production readers already read flat
settings.ENABLE_THIRD_PARTY_AUTH (lms/urls.py, oauth_dispatch/urls.py). Remaining test
usages:
- test_login.py / test_register.py enabled it via patch.dict(settings.FEATURES, {...}),
now override_settings(ENABLE_THIRD_PARTY_AUTH=True).
- third_party_auth/tests/specs/base.py gated IntegrationTest with
"AUTH_FEATURES_KEY in django_settings.FEATURES"; switched to the flat-based
testutil.AUTH_FEATURE_ENABLED (hasattr(settings, 'ENABLE_THIRD_PARTY_AUTH')), matching
the pattern already used in third_party_auth/tests/test_views.py.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The production reader is a SettingToggle('ENABLE_EXPORT_GIT') that reads the flat
settings.ENABLE_EXPORT_GIT. The git-export command test still enabled it by building a
FEATURES_WITH_EXPORT_GIT = settings.FEATURES.copy() and applying
@override_settings(FEATURES=...). Drop the dict copy and fold ENABLE_EXPORT_GIT=True into
the class-level override_settings so no settings.FEATURES usage remains; the SettingToggle
picks up the flat override.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…RES-as-dict CERTS_HTML_VIEW_CONFIG_PATH is not read anywhere in the platform anymore, so the CertificateHtmlViewConfiguration test's FEATURES override is a no-op. It was applied via a FEATURES_INVALID_FILE_PATH = settings.FEATURES.copy() + @override_settings(FEATURES=...). Convert to the equivalent flat @override_settings(CERTS_HTML_VIEW_CONFIG_PATH=...) (still a no-op, but off the dict), drop the dict copy, and remove the now-unused settings import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
These flags are now flat Django settings, but several comments/docstrings still described them as FEATURES-dict keys, and each kept a 'settings.FEATURES' reference alive in a repo-wide grep. Update them to the flat setting they now describe: - embargo/middleware.py: settings.FEATURES['EMBARGO'] -> settings.EMBARGO - git_export.py: FEATURE['ENABLE_EXPORT_GIT'] -> ENABLE_EXPORT_GIT setting - auto_auth.py: settings.FEATURES['AUTOMATIC_AUTH_FOR_TESTING'] -> settings.AUTOMATIC_AUTH_FOR_TESTING - split_mongo/split.py: reword the historical entrance-exams comment off the dict syntax - test_auto_auth.py: update the setUp comments to name the flat setting No code/behavior change; comment/docstring text only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kdmccormick
left a comment
kdmccormick
left a comment
There was a problem hiding this comment.
One request, but if you'd rather not do it, then I'll approve.
| @unittest.skipUnless( | ||
| testutil.AUTH_FEATURES_KEY in django_settings.FEATURES, testutil.AUTH_FEATURES_KEY + " not in settings.FEATURES" | ||
| testutil.AUTH_FEATURE_ENABLED, testutil.AUTH_FEATURES_KEY + " not enabled" | ||
| ) |
There was a problem hiding this comment.
Seems to me like testutil.AUTH_FEATURE_ENABLED is flawed.
It only ever checked if 'ENABLE_THIRD_PARTY_AUTH' was a key of FEATURES, which is always true for LMS (even though its value was False). After this change, it's the same: it's just checking if hasattr(settings, 'ENABLE_THIRD_PARTY_AUTH'), which is always true in LMS (with value False) and false in CMS. In other words, we're always running tests for third_party_auth in LMS, whether or not the feature is enabled. Which is fine, I think.
I know you are avoiding scope-creep on these PRs, but in this case I think the status quo is particularly silly+confusing and the fix is straightforward. Would you mind killing AUTH_FEATURES_KEY/AUTH_FEATURE_ENABLED and replacing all instances of @unittest.skipUnless(AUTH_FEATURE_ENABLED, AUTH_FEATURES_KEY + ' not enabled') and @skip_unless_third_party_auth with simply @skip_unless_lms?
There was a problem hiding this comment.
Seems like a reasonable thing to cleanup. It's always hard to decide where to draw that line but I'll add it as a commit on this PR shortly.
There was a problem hiding this comment.
So TPA is currently enabled in test but not enabled lms/env/common.py so just thinking out loud here that we probably want it to be enabled by default in common. Do you see any concerns with that @kdmccormick ?
There was a problem hiding this comment.
I agree with that in theory but I really don't know what the downstream effects of that would be, so I'd recommend ticketing that but not making it part of this PR.
There was a problem hiding this comment.
Totally, just thinking through it here, but I think that will be part of a series of PRs to update the default settings in the platform to make more sense.
testutil.AUTH_FEATURE_ENABLED was hasattr(settings, 'ENABLE_THIRD_PARTY_AUTH'), which is true under LMS settings (the flag is defined in lms/envs/common.py, absent from cms/envs) and false under CMS, regardless of the flag's value. So both @unittest.skipUnless(AUTH_FEATURE_ENABLED, ...) and the skip_unless_thirdpartyauth() helper (whose docstring literally says "skip ... tests in CMS") were roundabout ways of saying "run only in the LMS test suite" -- but they read as if they gated on the feature being enabled, which was misleading. Replace every usage with the existing @skip_unless_lms decorator and delete AUTH_FEATURES_KEY, AUTH_FEATURE_ENABLED, and skip_unless_thirdpartyauth(). Behavior is unchanged: these tests run in the LMS suite and skip in the CMS suite (verified locally: LMS runs, CMS skips). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Clears the remaining
settings.FEATUREStest overrides — flags whose production readers were migrated in earlier batches, but whose test decorators were left onpatch.dict(settings.FEATURES, …)/override_settings(FEATURES=…)/settings.FEATURES.copy(). These are currently harmless (the flat reader ignores the dict mutation) but each still referencessettings.FEATURES, so they'd break the moment the dict/FeaturesProxybridge is removed.Found via a repo-wide
settings.FEATURESsweep (not the reader-only picker).Flags (one commit each; co-located pairs together)
override_settings.test_login.py/test_register.py(@override_settings), andthird_party_auth/tests/specs/base.pyswitched from'ENABLE_THIRD_PARTY_AUTH' in settings.FEATURESto the flat-basedtestutil.AUTH_FEATURE_ENABLED(already the pattern intest_views.py).SettingToggle.Plus a
docs:commit updating stalesettings.FEATURES['X']references in comments/docstrings (EMBARGO, ENABLE_EXPORT_GIT, AUTOMATIC_AUTH_FOR_TESTING, ENTRANCE_EXAMS) to the flat settings they now describe.Each migrated flag's production reader already reads flat settings; the affected tests were run locally and pass.