Skip to content

refactor: migrate leftover FEATURES-as-dict test overrides off the dict #39005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
feanil merged 8 commits into from
Aug 20, 2026
Merged

refactor: migrate leftover FEATURES-as-dict test overrides off the dict #39005

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3971145
refactor: migrate EMBARGO test overrides off FEATURES-as-dict
feanil Aug 19, 2026
0fd354b
refactor: migrate ENABLE_CORS_HEADERS and ENABLE_CROSS_DOMAIN_CSRF_CO…
feanil Aug 19, 2026
7115717
refactor: migrate ENABLE_TEXTBOOK and ENABLE_EDXNOTES test overrides …
feanil Aug 19, 2026
d1f4484
refactor: migrate ENABLE_THIRD_PARTY_AUTH test usages off FEATURES-as…
feanil Aug 19, 2026
5761f8c
refactor: migrate ENABLE_EXPORT_GIT test override off FEATURES-as-dict
feanil Aug 19, 2026
1f82a10
refactor: migrate CERTS_HTML_VIEW_CONFIG_PATH test override off FEATU…
feanil Aug 19, 2026
d20ed5c
docs: drop stale FEATURES-dict references from comments and docstrings
feanil Aug 19, 2026
9466a86
refactor: replace third_party_auth skip helpers with skip_unless_lms
feanil Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cms/djangoapps/contentstore/management/commands/git_export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
any have taken place.

This functionality is also available as an export view in studio if the giturl
attribute is set and the FEATURE['ENABLE_EXPORT_GIT'] is set.
attribute is set and the ENABLE_EXPORT_GIT setting is enabled.
"""


Expand Down
5 changes: 1 addition & 4 deletions cms/djangoapps/contentstore/management/commands/tests/test_git_export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,11 @@
from cms.djangoapps.contentstore.git_export_utils import GitExportError
from cms.djangoapps.contentstore.tests.utils import CourseTestCase

FEATURES_WITH_EXPORT_GIT = settings.FEATURES.copy()
FEATURES_WITH_EXPORT_GIT['ENABLE_EXPORT_GIT'] = True
TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4().hex # noqa: UP031


@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE)
@override_settings(FEATURES=FEATURES_WITH_EXPORT_GIT)
@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, ENABLE_EXPORT_GIT=True)
class TestGitExport(CourseTestCase):
"""
Excercise the git_export django management command with various inputs.
Expand Down
2 changes: 1 addition & 1 deletion common/djangoapps/third_party_auth/tests/specs/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ def complete_url(self):


@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"
)

@kdmccormick kdmccormick Aug 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@feanil feanil Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@feanil feanil Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@kdmccormick kdmccormick Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@feanil feanil Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kdmccormick kdmccormick Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed!

@django_utils.override_settings() # For settings reversion on a method-by-method basis.
class IntegrationTest(testutil.TestCase, test.TestCase, HelperMixin):
Expand Down
6 changes: 1 addition & 5 deletions lms/djangoapps/certificates/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import ddt
import pytest
from django.conf import settings
from django.core.exceptions import ValidationError
from django.core.files.uploadedfile import SimpleUploadedFile
from django.test import TestCase
Expand Down Expand Up @@ -46,9 +45,6 @@
ENROLLMENT_METHOD = 'common.djangoapps.student.models.course_enrollment.CourseEnrollment.enrollment_mode_for_user'
PROFILE_METHOD = 'common.djangoapps.student.models_api.get_name'

FEATURES_INVALID_FILE_PATH = settings.FEATURES.copy()
FEATURES_INVALID_FILE_PATH['CERTS_HTML_VIEW_CONFIG_PATH'] = 'invalid/path/to/config.json'

TEST_DIR = path(__file__).dirname()
TEST_DATA_DIR = 'common/test/data/'
PLATFORM_ROOT = TEST_DIR.parent.parent.parent.parent
Expand Down Expand Up @@ -175,7 +171,7 @@ def test_get_not_enabled_returns_blank(self):
self.config.save()
assert len(self.config.get_config()) == 0

@override_settings(FEATURES=FEATURES_INVALID_FILE_PATH)
@override_settings(CERTS_HTML_VIEW_CONFIG_PATH='invalid/path/to/config.json')
def test_get_no_database_no_file(self):
"""
Tests get configuration that is not enabled.
Expand Down
10 changes: 5 additions & 5 deletions lms/djangoapps/courseware/tests/test_tabs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,11 +615,11 @@ def test_initialize_default_without_external_link(self):
assert not self.has_tab(self.course.tabs, 'external_discussion')
assert self.has_tab(self.course.tabs, 'discussion')

@override_settings(ENABLE_DISCUSSION_SERVICE=True)
@patch.dict("django.conf.settings.FEATURES", {
"ENABLE_TEXTBOOK": True,
"ENABLE_EDXNOTES": True,
})
@override_settings(
ENABLE_DISCUSSION_SERVICE=True,
ENABLE_TEXTBOOK=True,
ENABLE_EDXNOTES=True,
)
def test_iterate_displayable(self):
self.course.hide_progress_tab = False

Expand Down
12 changes: 4 additions & 8 deletions lms/djangoapps/experiments/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,22 +178,18 @@ def test_loads_valid_csrf_trusted_origins_list(self):

def cross_domain_config(func):
"""Decorator for configuring a cross-domain request. """
feature_flag_decorator = patch.dict(settings.FEATURES, {
'ENABLE_CORS_HEADERS': True,
'ENABLE_CROSS_DOMAIN_CSRF_COOKIE': True
})
settings_decorator = override_settings(
ENABLE_CORS_HEADERS=True,
ENABLE_CROSS_DOMAIN_CSRF_COOKIE=True,
CORS_ORIGIN_WHITELIST=['https://ecommerce.edx.org'],
CSRF_COOKIE_NAME="prod-edx-csrftoken",
CROSS_DOMAIN_CSRF_COOKIE_NAME="prod-edx-csrftoken",
CROSS_DOMAIN_CSRF_COOKIE_DOMAIN=".edx.org"
)
is_secure_decorator = patch.object(WSGIRequest, 'is_secure', return_value=True)

return feature_flag_decorator(
settings_decorator(
is_secure_decorator(func)
)
return settings_decorator(
is_secure_decorator(func)
)


Expand Down
4 changes: 2 additions & 2 deletions lms/djangoapps/verify_student/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ def test_course_mode_not_expired_verification_deadline_passed(self):
self.assertContains(response, "verification deadline")
self.assertContains(response, verification_deadline_in_past)

@patch.dict(settings.FEATURES, {'EMBARGO': True})
@override_settings(EMBARGO=True)
@ddt.data("verify_student_start_flow", "verify_student_begin_flow")
def test_embargo_restrict(self, payment_flow):
course = self._create_course("verified")
Expand All @@ -802,7 +802,7 @@ def test_embargo_restrict(self, payment_flow):
response = self._get_page(payment_flow, course.id, expected_status_code=302)
self.assertRedirects(response, redirect_url)

@patch.dict(settings.FEATURES, {'EMBARGO': True})
@override_settings(EMBARGO=True)
@ddt.data("verify_student_start_flow", "verify_student_begin_flow")
def test_embargo_allow(self, payment_flow):
course = self._create_course("verified")
Expand Down
8 changes: 2 additions & 6 deletions openedx/core/djangoapps/cors_csrf/tests/test_authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
"""Tests for the CORS CSRF version of Django Rest Framework's SessionAuthentication."""


from unittest.mock import patch

from django.conf import settings
from django.middleware.csrf import get_token
from django.test import TestCase
Expand Down Expand Up @@ -34,11 +32,9 @@ def test_perform_csrf_referer_check(self):
with self.assertRaisesRegex(PermissionDenied, 'CSRF'): # noqa: PT027
self.auth.enforce_csrf(request)

@patch.dict(settings.FEATURES, {
'ENABLE_CORS_HEADERS': True,
'ENABLE_CROSS_DOMAIN_CSRF_COOKIE': True
})
@override_settings(
ENABLE_CORS_HEADERS=True,
ENABLE_CROSS_DOMAIN_CSRF_COOKIE=True,
CORS_ORIGIN_WHITELIST=["https://www.edx.org"],
CROSS_DOMAIN_CSRF_COOKIE_NAME="prod-edx-csrftoken",
CROSS_DOMAIN_CSRF_COOKIE_DOMAIN=".edx.org"
Expand Down
2 changes: 1 addition & 1 deletion openedx/core/djangoapps/embargo/middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

Usage:

1) Enable embargo by setting `settings.FEATURES['EMBARGO']` to True.
1) Enable embargo by setting `settings.EMBARGO` to True.

2) In Django admin, create a new `IPFilter` model to block or whitelist
an IP address from accessing the site.
Expand Down
2 changes: 1 addition & 1 deletion openedx/core/djangoapps/user_authn/views/auto_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def auto_auth(request): # pylint: disable=too-many-statements
Create or configure a user account, then log in as that user.

Enabled only when
settings.FEATURES['AUTOMATIC_AUTH_FOR_TESTING'] is true.
settings.AUTOMATIC_AUTH_FOR_TESTING is true.

Accepts the following querystring parameters:
* `username`, `email`, and `password` for the user account
Expand Down
6 changes: 3 additions & 3 deletions openedx/core/djangoapps/user_authn/views/tests/test_auto_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class AutoAuthEnabledTestCase(AutoAuthTestCase, ModuleStoreTestCase):

@override_settings(AUTOMATIC_AUTH_FOR_TESTING=True)
def setUp(self):
# Patching the settings.FEATURES['AUTOMATIC_AUTH_FOR_TESTING']
# Patching the AUTOMATIC_AUTH_FOR_TESTING setting
# value affects the contents of urls.py,
# so we need to call super.setUp() which reloads urls.py (because
# of the UrlResetMixin)
Expand Down Expand Up @@ -305,7 +305,7 @@ class AutoAuthDisabledTestCase(AutoAuthTestCase):

@override_settings(AUTOMATIC_AUTH_FOR_TESTING=False)
def setUp(self):
# Patching the settings.FEATURES['AUTOMATIC_AUTH_FOR_TESTING']
# Patching the AUTOMATIC_AUTH_FOR_TESTING setting
# value affects the contents of urls.py,
# so we need to call super.setUp() which reloads urls.py (because
# of the UrlResetMixin)
Expand All @@ -329,7 +329,7 @@ class AutoAuthRestrictedTestCase(AutoAuthTestCase):

@override_settings(AUTOMATIC_AUTH_FOR_TESTING=True)
def setUp(self):
# Patching the settings.FEATURES['AUTOMATIC_AUTH_FOR_TESTING']
# Patching the AUTOMATIC_AUTH_FOR_TESTING setting
# value affects the contents of urls.py,
# so we need to call super.setUp() which reloads urls.py (because
# of the UrlResetMixin)
Expand Down
4 changes: 1 addition & 3 deletions openedx/core/djangoapps/user_authn/views/tests/test_login.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,7 @@ def test_login_failed_with_opt_in_flag_disabled(self):
mock_audit_log, 'warning', ['Login failed - Account not active for user.id: 1, resending activation']
)

@patch.dict(settings.FEATURES, {
"ENABLE_THIRD_PARTY_AUTH": True
})
@override_settings(ENABLE_THIRD_PARTY_AUTH=True)
@patch(
'openedx.core.djangoapps.user_authn.views.login.is_require_third_party_auth_enabled',
Mock(return_value=True)
Expand Down
4 changes: 1 addition & 3 deletions openedx/core/djangoapps/user_authn/views/tests/test_register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,7 @@ def setUp(self): # pylint: disable=arguments-differ
super().setUp()
self.url = reverse("user_api_registration")

@mock.patch.dict(settings.FEATURES, {
"ENABLE_THIRD_PARTY_AUTH": True,
})
@override_settings(ENABLE_THIRD_PARTY_AUTH=True)
@mock.patch(
'openedx.core.djangoapps.user_authn.views.register.is_require_third_party_auth_enabled',
mock.Mock(return_value=True)
Expand Down
2 changes: 1 addition & 1 deletion xmodule/modulestore/split_mongo/split.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1720,7 +1720,7 @@ def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fie

parent = new_structure['blocks'][block_id]

# Originally added to support entrance exams (settings.FEATURES.get('ENTRANCE_EXAMS'))
# Originally added to support entrance exams (the ENTRANCE_EXAMS feature)
if kwargs.get('position') is None:
parent.fields.setdefault('children', []).append(BlockKey.from_usage_key(xblock.location))
else:
Expand Down
Loading