Opened 3 years ago

Closed 20 months ago

#20349 closed Cleanup/optimization (fixed)

Don't load django.test when not testing

Reported by: CollinAnderson 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 CollinAnderson

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

comment:2 Changed 3 years ago by CollinAnderson

comment:3 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 3 years ago by carljm

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 carljm

  • Needs tests set
  • Patch needs improvement set

comment:6 Changed 3 years ago by aaugustin

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 claudep

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 CollinAnderson

  • Summary changed from Don't load test suite when not testing to Don't load django.test when not testing

comment:9 Changed 23 months ago by collinanderson

  • 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 21 months ago by collinanderson (previous) (diff)

comment:10 Changed 21 months ago by timgraham

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 20 months ago by collinanderson

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 20 months ago by timgraham

  • Needs documentation set
  • Needs tests unset
  • Version changed from 1.5 to 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:13 Changed 20 months ago by collinanderson

  • Needs documentation unset

Ok, patch updated.

comment:14 Changed 20 months ago by collinanderson

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 20 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

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