Refactor redirect tests to use reload_redirects_with_settings instead of override_settings

[localjo]

This update introduces a new decorator, skip_if_ffcom_redirects_disabled, to conditionally skip tests related to Firefox.com redirects when the corresponding setting is disabled. The decorator replaces the previous use of @override_settings(ENABLE_FIREFOX_COM_REDIRECTS=True) in multiple test cases, enhancing test flexibility and maintainability.

This update introduces a new decorator, reload_redirects_with_settings, to properly handle redirect testing when settings need to be overridden. The decorator replaces the previous use of @override_settings(ENABLE_FIREFOX_COM_REDIRECTS=True) in multiple test cases, addressing the core issue where Django loads redirects once at startup and doesn't reload the URL configuration when @override_settings is used.

One-line summary

Replace override_settings decorator with new reload_redirects_with_settings decorator to reload redirects.

Significant changes and points to review

Verify whether the new decorator is appropriately abstracted to be useful for all types of redirects.

Issue / Bugzilla link

Fixes #16410

Testing

Set ENABLE_FIREFOX_COM_REDIRECTS=False or ENABLE_FIREFOX_COM_REDIRECTS=True

Then run pytest lib bedrock

All tests should run and pass.

[localjo]

@janbrasna GitHub won't let me tag you to review this, but it's my proposed solution to #16410

[janbrasna]

I'm not a reviewer of these as it's assigned based on codeowners — so I'm only leaving notes here for the reviewer in relation to the ticket I opened:

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.91%. Comparing base (b0478b6) to head (07ad6b6).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #16414   +/-   ##
=======================================
  Coverage   79.91%   79.91%           
=======================================
  Files         158      158           
  Lines        8538     8538           
=======================================
  Hits         6823     6823           
  Misses       1715     1715           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[localjo]

@janbrasna @stephaniehobson I've updated this PR (including the title and original description) to add an abstract decorator instead of a Firefox.com specific one. It's essentially an override_settings decorator that also forces the reload of redirects during tests so that they're tested with the appropriate settings overrides applied.

[stephaniehobson]

@janbrasna how does this look to you?

[janbrasna]

TBH I got stuck a bit as this reached a sort of pmac-fu level.

I'm not the right one to review this, but one thing I've noticed to double check:

[janbrasna]

… and here it reloads the module with the overridden settings.

And then saves the state as "original". It feels like it's saving the test-specific patterns at this point, not what was mapped at the time of the original import.

But this is way out of my wheelhouse at this point (more like I'm standing at the shore just waving the boats passing by;)…). I could probably prove myself wrong easily with some debug prints, but didn't want to start messing with this instead of a real reviewer, so I guess Steve might want to play with this when he gets the chance, or maybe pair over it a bit.

[localjo]

This is a very good point. I decided to rework this a bit, so now instead of trying to store/restore the redirect patterns, it just stores the original environment variable setting, reloads the redirects for the test, then restores the environment variable setting and reloads the redirects again after the test is finished. I also made this a little more generic, and added a convenience decorator @enable_fxc_redirects specifically for the Firefox.com redirects.

[stevejalim]

Just to chip in here - I had the same thoughts as @janbrasna commented, so thanks for refactoring @localjo

Looking at it from a high level, I think this would be useful to have as a decorator for tests, for sure, but last night @stephaniehobson and I were lining up some pieces of work and one of them is to just make the redirects to www.firefox.com the default behaviour, with no option to turn them off now we've turned them on.

As such, the ground may move beneath this PR pretty soon, so I'm thinking we park this for a little longer, make those redirects un-toggleable, and then rebase this to add in extra tooling for future tests, even though we won't need it for the www.firefox.com redirects. We can drop the convenience func, as it won't be needed.

I'm sorry if that's frustrating, Jo! I think it's a handy tool and will be good to fold in soon

[janbrasna]

I think we might want to make use of the reloading for #16443 redux — using settings as part of expected test results wholesale makes me a bit nervous, as I'd much rather have the inputs for the test overridden to some expected results, and then test the expected results statically (unless the actual goal of a test is to confirm the setting being applied for different values), without any hypothetical variability creeping in having the possibility to skew the tests (and from my understanding that would have been @maribedran's preferred strategy too anyways).

So once the other redirect removals land, I believe you can sync on the approach to repurpose this to achieve the same, just for settings.FXC_BASE_URL override to stabilize the test URLs to expected values, hard-coded.