[Django] #20349: Don't load test suite when not testing
Django
--------------------------------------+--------------------
Reporter: CollinAnderson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20349>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: CollinAnderson | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.5
Component: Testing framework | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by CollinAnderson):
* cc: cmawebsite@… (added)
* needs_docs: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_better_patch: => 0
Comment:
https://github.com/django/django/pull/1041
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:1>
Django
Reporter: CollinAnderson | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.5
Component: Testing framework | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by CollinAnderson):
here's where the import was originally added:
https://github.com/django/django/commit/2a09404792f9d369718143fdfbe9530fe2e60912
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:2>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:3>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:4>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:5>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:6>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: CollinAnderson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:8>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* needs_better_patch: 1 => 0
Comment:
https://github.com/django/django/pull/3332
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:9>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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?!).
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:10>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:11>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 0 => 1
* version: 1.5 => master
* needs_tests: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:12>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* needs_docs: 1 => 0
Comment:
Ok, patch updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:13>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:14>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"5dddd79433ceb88ab67d9851b49a44ce5b8f509c"]:
{{{
#!CommitTicketReference repository=""
revision="5dddd79433ceb88ab67d9851b49a44ce5b8f509c"
Fixed #20349 -- Moved setting_changed signal to django.core.signals.
This removes the need to load django.test when not testing.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:15>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"890bfa368c33d6ae19fe45cf1eed7e2e8d63160e" 890bfa36]:
{{{
#!CommitTicketReference repository=""
revision="890bfa368c33d6ae19fe45cf1eed7e2e8d63160e"
Refs #20349 -- Avoided loading testing libraries when not needed.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20349#comment:16>