refactor: migrate leftover FEATURES-as-dict test overrides off the dict - #39005
Open
feanil wants to merge 7 commits into
Open
refactor: migrate leftover FEATURES-as-dict test overrides off the dict#39005feanil wants to merge 7 commits into
feanil wants to merge 7 commits into
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>
feanil
marked this pull request as ready for review
August 19, 2026 15:37
feanil
requested review from
a team,
farhan,
irtazaakram and
salman2013
as code owners
August 19, 2026 15:37
kdmccormick
reviewed
Aug 19, 2026
kdmccormick
left a comment
kdmccormick
left a comment
Member
There was a problem hiding this comment.
One request, but if you'd rather not do it, then I'll approve.
Comment on lines
578
to
580
| @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" | ||
| ) |
Member
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?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.