refactor: lift shared test settings up to common module by wgu-taylor-payne · Pull Request #37714 · openedx/openedx-platform

wgu-taylor-payne · 2025-12-02T19:16:36Z

Description

Extract common test setting values into a new shared module, openedx/envs/test.py. This prevents duplication and also allows us to remove the cms/envs/test.py dependency on lms/envs/test.py.

Supporting information

Part of effort to simplify settings. See ADR 0022 - Simplify Django Settings.

Testing instructions

Run the dump_settings management command on the different terminal settings modules, given different yml config files and compare the diff on the rendered settings between master and tpayne/add-common-test-module. To simplify this, I used the script diff_settings.py and also table_diff.py to simplify the diff output. I've added the output of these scripts in this gist.

There are two differences in the rendered cms/envs/test.py settings that I thought were tolerable:

PROCTORING_USER_OBFUSCATION_KEY - This was previously declared twice in cms/envs/test.py. It didn't seem that anything directly depends on this key having a specific value. This change aligns the value with lms/envs/test.py.

VERIFY_STUDENT - With the introduction of openedx/envs/common.py, when lms/envs/test.py would alter this value, it would also alter the value for cms/envs/test.py, since they are pointing to the same mutable object. Now, since we do not import, and thus not execute lms/envs/test.py, this value is no longer mutated.

Deadline

None

openedx-webhooks · 2025-12-02T19:16:41Z

Thanks for the pull request, @wgu-taylor-payne!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

openedx-webhooks · 2025-12-02T19:16:41Z

Thanks for the pull request, @wgu-taylor-payne!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

kdmccormick · 2025-12-02T19:19:33Z

Thanks @wgu-taylor-payne !

@feanil , I'm swamped with Ulmo stuff right now, do you have cycles to review this one?

feanil · 2025-12-02T19:30:42Z

Yes, happy to take a look at this, @wgu-taylor-payne I see it's still in draft, is it ready for review?

wgu-taylor-payne · 2025-12-02T20:16:22Z

@feanil Thanks for being willing to look this over. I'm still working to get the checks passing and then I'll mark it as ready.

wgu-taylor-payne · 2025-12-03T00:50:58Z

run-tests job failing on install system requirements step. Will retry tomorrow.

lms/envs/test.py

wgu-taylor-payne · 2025-12-03T16:16:10Z

cms/envs/test.py

+for theme_dir in COMPREHENSIVE_THEME_DIRS:  # pylint: disable=not-an-iterable
+    preview_template['DIRS'].insert(0, theme_dir)


This is to match the mutation to the LMS version of MAKO_TEMPLATE_DIRS_BASE that used to happen here.

As a side note, I'm not sure if mutating MAKO_TEMPLATE_DIRS_BASE in that function was intentional or an oversight.

Yea, I tried to see if it was being used anywhere directly and I didn't see MAKO_TEMPLATE_DIRS_BASE mentioned directly either. Given that the tasts are passing, I think it's fine to make the test modifications without updating that settings value.

Are you saying that keep this as is, since the tests are passing, or remove these lines and see if the tests pass without them?

Sorry for the lack of clarity, I think you can keep this as is since the tests are passing.

wgu-taylor-payne · 2025-12-03T16:21:49Z

@feanil This is ready to look over when you get the chance. Thanks!

feanil

Did a first pass, mostly minor improvements to reduce duplication further and some documentation improvements.

setup.cfg

cms/envs/test.py

feanil · 2025-12-03T19:55:00Z

cms/envs/test.py

+for theme_dir in COMPREHENSIVE_THEME_DIRS:  # pylint: disable=not-an-iterable
+    preview_template['DIRS'].insert(0, theme_dir)


Yea, I tried to see if it was being used anywhere directly and I didn't see MAKO_TEMPLATE_DIRS_BASE mentioned directly either. Given that the tasts are passing, I think it's fine to make the test modifications without updating that settings value.

lms/envs/test.py

wgu-taylor-payne · 2025-12-04T17:58:32Z

@feanil I've made some changes and responded to your comments. Ready for another look.

feanil

@wgu-taylor-payne I'm good with you merging this as is, or you can add one more comment, feel free to merge when you're ready, I don't need to review it again.

feanil · 2025-12-04T21:34:41Z

cms/envs/test.py

+for theme_dir in COMPREHENSIVE_THEME_DIRS:  # pylint: disable=not-an-iterable
+    preview_template['DIRS'].insert(0, theme_dir)


Sorry for the lack of clarity, I think you can keep this as is since the tests are passing.

feanil · 2025-12-04T21:43:32Z

cms/envs/test.py

+REGISTRATION_EXTRA_FIELDS.pop("marketing_emails_opt_in", None)
+
+# Course Live
+COURSE_LIVE_GLOBAL_CREDENTIALS["BIG_BLUE_BUTTON"] = big_blue_button_credentials


I think we can consider that in the future, but it feels too complicated and we can just leave this as is for now. It was initially hard for me to grok that the openedx/envs/test.py file doesn't have access to the two common.py files when it is being read, now that I understand that this makes a lot of sense.

Suggestion: add a comment to explain the above fact neard the definition of the big_blue_button_credentials variable in the test file?

wgu-taylor-payne requested a review from kdmccormick December 2, 2025 19:16

wgu-taylor-payne self-assigned this Dec 2, 2025

wgu-taylor-payne added the mao-onboarding Reviewing this will help onboard devs from an Axim mission-aligned organization (MAO). label Dec 2, 2025

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 2, 2025

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Dec 2, 2025

openedx-webhooks added this to Contributions Dec 2, 2025

github-project-automation bot moved this to Needs Triage in Contributions Dec 2, 2025

wgu-taylor-payne marked this pull request as draft December 2, 2025 19:21

wgu-taylor-payne force-pushed the tpayne/add-common-test-module branch from f1b21e2 to df2611b Compare December 2, 2025 19:25

wgu-taylor-payne force-pushed the tpayne/add-common-test-module branch from df2611b to cb1a1cf Compare December 3, 2025 00:36

wgu-taylor-payne force-pushed the tpayne/add-common-test-module branch from 1a4f8c8 to cb1a1cf Compare December 3, 2025 15:36

wgu-taylor-payne commented Dec 3, 2025

View reviewed changes

lms/envs/test.py Show resolved Hide resolved

wgu-taylor-payne commented Dec 3, 2025

View reviewed changes

wgu-taylor-payne marked this pull request as ready for review December 3, 2025 16:20

wgu-taylor-payne requested a review from feanil December 3, 2025 16:22

feanil reviewed Dec 3, 2025

View reviewed changes

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Dec 3, 2025

feanil approved these changes Dec 4, 2025

View reviewed changes

refactor: lift shared test settings up to common module

f31306e

wgu-taylor-payne force-pushed the tpayne/add-common-test-module branch from 8a3f39e to f31306e Compare December 4, 2025 23:10

wgu-taylor-payne merged commit d592784 into openedx:master Dec 4, 2025
49 checks passed

github-project-automation bot moved this from In Eng Review to Done in Contributions Dec 4, 2025

mraman-2U pushed a commit to mraman-2U/edx-platform that referenced this pull request Dec 24, 2025

refactor: lift shared test settings up to common module (openedx#37714)

85064da

		for theme_dir in COMPREHENSIVE_THEME_DIRS: # pylint: disable=not-an-iterable
		preview_template['DIRS'].insert(0, theme_dir)

Conversation

wgu-taylor-payne commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Uh oh!

openedx-webhooks commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Dec 2, 2025

Uh oh!

kdmccormick commented Dec 2, 2025

Uh oh!

feanil commented Dec 2, 2025

Uh oh!

wgu-taylor-payne commented Dec 2, 2025

Uh oh!

wgu-taylor-payne commented Dec 3, 2025

Uh oh!

Uh oh!

wgu-taylor-payne Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

feanil Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

wgu-taylor-payne Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

feanil Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

wgu-taylor-payne commented Dec 3, 2025

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feanil Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgu-taylor-payne commented Dec 4, 2025

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

Uh oh!

feanil Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

feanil Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wgu-taylor-payne commented Dec 2, 2025 •

edited

Loading

openedx-webhooks commented Dec 2, 2025 •

edited

Loading