Opened 12 years ago

Closed 10 years ago

Last modified 3 years 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: 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 Collin Anderson, 12 years ago

Cc: cmawebsite@… added
Has patch: set

comment:2 by Collin Anderson, 12 years ago

comment:3 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Carl Meyer, 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 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 by Carl Meyer, 12 years ago

Needs tests: set
Patch needs improvement: set

comment:6 by Aymeric Augustin, 11 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 Claude Paroz, 11 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 Collin Anderson, 11 years ago

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

comment:9 by Collin Anderson, 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.

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

comment:10 by Tim Graham, 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 Collin Anderson, 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 Tim Graham, 10 years ago

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 by Collin Anderson, 10 years ago

Needs documentation: unset

Ok, patch updated.

comment:14 by Collin Anderson, 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 Tim Graham <timograham@…>, 10 years ago

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.

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 890bfa36:

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

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