Opened 3 years ago

Closed 22 months ago

#20349 closed Cleanup/optimization (fixed)

Don't load django.test when not testing

Reported by: Collin Anderson Owned by: nobody
Component: Testing framework Version: master
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


I noticed that django/contrib/auth/ 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 (15)

comment:1 Changed 3 years ago by Collin Anderson

Cc: cmawebsite@… added
Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 3 years ago by Collin Anderson

comment:3 Changed 3 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

comment:4 Changed 3 years ago by 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.

comment:5 Changed 3 years ago by Carl Meyer

Needs tests: set
Patch needs improvement: set

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by 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?

comment:8 Changed 3 years ago by Collin Anderson

Summary: Don't load test suite when not testingDon't load django.test when not testing

comment:9 Changed 2 years ago by Collin Anderson

Patch needs improvement: unset

One option: move the signal into core.signals.

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.

Last edited 2 years ago by Collin Anderson (previous) (diff)

comment:10 Changed 2 years ago by 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?!).

comment:11 Changed 22 months ago by 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.

comment:12 Changed 22 months ago by Tim Graham

Needs documentation: set
Needs tests: unset
Version: 1.5master

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:13 Changed 22 months ago by Collin Anderson

Needs documentation: unset

Ok, patch updated.

comment:14 Changed 22 months ago by 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

comment:15 Changed 22 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 5dddd79433ceb88ab67d9851b49a44ce5b8f509c:

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

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

Note: See TracTickets for help on using tickets.
Back to Top