#20349 closed Cleanup/optimization (fixed)
Don't load django.test when not testing
Reported by: | Collin Anderson | Owned by: | nobody |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | cmawebsite@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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?
Change History (16)
comment:1 by , 12 years ago
Cc: | added |
---|---|
Has patch: | set |
comment:2 by , 12 years ago
here's where the import was originally added: https://github.com/django/django/commit/2a09404792f9d369718143fdfbe9530fe2e60912
comment:3 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 12 years ago
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 global
ed 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.
comment:5 by , 12 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
comment:6 by , 12 years ago
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.
comment:7 by , 12 years ago
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?
comment:8 by , 12 years ago
Summary: | Don't load test suite when not testing → Don't load django.test when not testing |
---|
comment:9 by , 10 years ago
Patch needs improvement: | unset |
---|
One option: move the signal into core.signals.
https://github.com/django/django/pull/3332
I realize another option could be to have a receiver in test.signals _call_ django.contrib.auth.hashers.reset_hashers(). That might be even better, but it's core testing importing contrib.
comment:10 by , 10 years ago
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?!).
comment:11 by , 10 years ago
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.
comment:12 by , 10 years ago
Needs documentation: | set |
---|---|
Needs tests: | unset |
Version: | 1.5 → master |
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.
comment:14 by , 10 years ago
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
comment:15 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
https://github.com/django/django/pull/1041