Don't load django.test when not testing

[Collin Anderson]

I noticed that django/contrib/auth/hashers.py is loading the settings_changed signal from the test suite. This ends up also loading the whole test suite in addition.

All of the other code that handles the settings_changed signal is kept inside the test suite itself. I assume this code is kept out of the test suite because of the "core shouldn't be aware of contrib" rule. Seems to me it's more important to avoid unnecessarily loading the test suite on a production website.

Yes, I realize this is picky, but we're perfectionists, right?

[Collin Anderson]

​https://github.com/django/django/pull/1041

[Collin Anderson]

here's where the import was originally added: ​https://github.com/django/django/commit/2a09404792f9d369718143fdfbe9530fe2e60912

[Carl Meyer]

This pull request clearly can't work as written, since HASHERS and PREFERRED_HASHER don't exist in django.test.signals, so they can't be globaled there.

We need to decide if settings_changed is meant to be a generally-supported framework feature, or a test-only feature. If the former, then it should be moved out of django.test.signals. If the latter, then fixing this bug is complicated; we'd need a hook in the test runner to allow apps to contribute stuff (like settings_changed signal registrations) to setup_test_environment. Or an UNDER_TEST setting, so that code like this can make the import and signal registration conditional on UNDER_TEST. Or just live with the fact that django.test is imported in production.

[Aymeric Augustin]

setting_changed is only targeted at tests for the time being.

Maybe we can put this code under django/contrib/auth/tests? If that doesn't work, let's just live with the status quo.

[Claude Paroz]

The problem when moving the signal definition in django/contrib/auth/tests is that many test cases outside of auth tests are overriding the PASSWORD_HASHERS setting (see [8fad77da9597e0dd9fc]), and then the signal is not installed for those tests.

An argument for moving auth to core?

[Collin Anderson]

​https://github.com/django/django/pull/3332 I'm totally fine if this doens't make it in, but it's another attempt.

[Tim Graham]

Regarding the second option, here's the comment from django.test.signals:

# Most setting_changed receivers are supposed to be added below,
# except for cases where the receiver is related to a contrib app.

I don't really like the idea of django.test making special allowances for contrib apps (okay, that's not the case now, but let's not add more of it!).

I'm not sure there are any practical problems with moving the setting_changed signal into core (is it more a purity argument that users might construe that it can be used for more than testing)?

"Needs tests" is checked, but I am not really sure that is relevant (how can we check django.test is not loaded while we are testing?!).

[Collin Anderson]

Yeah, moving it to core seems like the cleanest way to go. Would we want to document that to? Or leave the documented version of it in test?

I also don't know how you would test it. I don't think it's a major problem if it regresses.

[Tim Graham]

For documentation, I think we could say "You can also import this signal from django.core.signals to avoid importing from django.test in non-test situations." After all, user code may have the same issue as Django's test suite.

[Collin Anderson]

Ok, patch updated.

[Collin Anderson]

We could put something like this alongside pep8, but again, it's really just an optimization.

git grep django.test $(git ls-files django | grep -v test) | grep import

[Tim Graham <timograham@…>]

In 5dddd79433ceb88ab67d9851b49a44ce5b8f509c:

Fixed #20349 -- Moved setting_changed signal to django.core.signals.

This removes the need to load django.test when not testing.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 890bfa36:

Refs #20349 -- Avoided loading testing libraries when not needed.