[Django] #26961: Admin check does not run unless DEBUG = True
Django
-------------------------------+--------------------
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Keywords: checks
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
In our CI system we run `call_command('check')` during tests and
`manage.py check` before deployment. Two bugs crept through recently
thanks to broken admin checks.
This is because the contrib.admin checks don't run unless `DEBUG = True`,
which is not recommended for tests or production, so we never saw these
checks break.
The commit that moved the checks from custom validation to the check
framework hints at why this it was historically only when `DEBUG = True`
at
https://github.com/django/django/commit/4ad1eb1c14b629cf5bcfd253ed40e875f1bddd47
#diff-57866af2aff590165ffe4967107424fdL69 :
> Don't import the humongous validation code unless required
This isn't valid reason with the check framework since all the code is
imported anyway when assigning `checks_class` on `BaseModelAdmin`, the
only memory now saved is that the list of failing checks isn't
instantiated.
I'd argue the `DEBUG` requirement should be removed
--
Ticket URL: <https://code.djangoproject.com/ticket/26961>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.9
Keywords: checks | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by adamchainz):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
https://github.com/django/django/pull/6980
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:1>
Django
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
A couple tests need to be fixed and a mention in the release notes could
be useful.
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: adamchainz | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:3>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adamchainz):
Should I change the 1.10 release notes?
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:4>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
1.10 is at release candidate stage and only receiving fixes for release
blockers. Please target 1.11.
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:5>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adamchainz):
Thanks, wasn't entirely sure if small changes like this were still
accepted.
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:6>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adamchainz):
One of the failures is interesting:
{{{
File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label
/trusty-pr/python/python2.7/tests/model_validation/tests.py", line 23, in
test_models_validate
management.call_command("check", stdout=six.StringIO())
}}}
It looks like this test calls `check` which runs against everything in
memory. Needs refactoring?
Also this is partly caused by the way the admin checks run - they get
executed during `site.register` and append all the errors to a global list
of failures. This backfires in the wake of multiple `AdminSite`s which get
created during the tests, with `ModelAdmin` classes registered and
deregistered, leaving the errors they created at each `register` in the
list. Any idea why the list isn't just calculated dynamically when `check`
is called?
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:7>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
Yes, I guess that first test isn't ideal (see its origin in
a19e9d80ffa10f8da43addcaa4ddd440beee8a4d). I guess it could be removed if
#25415 is completed.
I can't advise on the design of the admin checks but from what I remember,
the implementation seemed a bit odd when I was looking through it, so if
you want to try to refactor it, that would be welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:8>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adamchainz):
I've re-factored the admin checks to be dynamically created, by keeping
track of all the `AdminSite` references that exist, and calling `check` on
each individually when the admin app `check` function is called. That in
itself didn't seem to cause any errors.
I've also fixed a number of checks that were failing for the model and
admin classes defined in tests, if only I'd finished #25415 and made tests
run checks again!
Another thing I had to do was make `CheckMessage` succeed in comparisons
with non-`CheckMessage` classes because some tests return plain strings
from their temporary `check` functions and then these don't compare with
real check messages that come out, and the test crashes unhelpfully.
I'm still working a couple of failures that seem broken due to the lack of
integration between checks and the app registry. Some tests fail with
checks from apps that have been unloaded through
`override_settings(INSTALLED_APPS=`. Unfortunately the app registry
doesn't reset the check registry, and it can't really because the
interface of checks allows not only to be registered during
`AppConfig.ready` but just at import time, so clearing the check registry
on app registry reload would unload those checks and then not re-register
them...
The exact failures are:
{{{
======================================================================
ERROR: test_migrate_with_system_checks
(migrations.test_commands.MigrateTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py",
line 329, in run
testMethod()
File "/Users/adamj/Documents/Projects/django/django/test/utils.py", line
209, in inner
return func(*args, **kwargs)
File
"/Users/adamj/Documents/Projects/django/tests/migrations/test_commands.py",
line 65, in test_migrate_with_system_checks
call_command('migrate', skip_checks=False, no_color=True, stdout=out)
File
"/Users/adamj/Documents/Projects/django/django/core/management/__init__.py",
line 130, in call_command
return command.execute(*args, **defaults)
File
"/Users/adamj/Documents/Projects/django/django/core/management/base.py",
line 353, in execute
self.check()
File
"/Users/adamj/Documents/Projects/django/django/core/management/base.py",
line 431, in check
raise SystemCheckError(msg)
SystemCheckError: SystemCheckError: System check identified some issues:
ERRORS:
<class 'admin_views.admin.ChapterXtra1Admin'>: (admin.E116) The value of
'list_filter[4]' refers to 'chap__book__promo', which does not refer to a
Field.
<class 'admin_views.admin.ChapterXtra1Admin'>: (admin.E116) The value of
'list_filter[4]' refers to 'chap__book__promo', which does not refer to a
Field.
<class 'admin_views.admin.ChapterXtra1Admin'>: (admin.E116) The value of
'list_filter[5]' refers to 'chap__book__promo__name', which does not refer
to a Field.
<class 'admin_views.admin.ChapterXtra1Admin'>: (admin.E116) The value of
'list_filter[5]' refers to 'chap__book__promo__name', which does not refer
to a Field.
======================================================================
FAIL: test_system_exit (user_commands.tests.CommandTests)
Exception raised in a command should raise CommandError with
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py",
line 329, in run
testMethod()
File
"/Users/adamj/Documents/Projects/django/tests/user_commands/tests.py",
line 60, in test_system_exit
self.assertIn("CommandError", stderr.getvalue())
File
"/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py",
line 803, in assertIn
self.fail(self._formatMessage(msg, standardMsg))
File
"/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/case.py",
line 410, in fail
raise self.failureException(msg)
AssertionError: 'CommandError' not found in "\x1b[31;1mSystemCheckError:
System check identified some issues:\n\x1b[0m\nERRORS:\n\x1b[31;1m<class
'admin_views.admin.ChapterXtra1Admin'>: (admin.E116) The value of
'list_filter[4]' refers to 'chap__book__promo', which does not refer to a
Field.\x1b[0m\n\x1b[31;1m<class 'admin_views.admin.ChapterXtra1Admin'>:
(admin.E116) The value of 'list_filter[4]' refers to 'chap__book__promo',
which does not refer to a Field.\x1b[0m\n\x1b[31;1m<class
'admin_views.admin.ChapterXtra1Admin'>: (admin.E116) The value of
'list_filter[5]' refers to 'chap__book__promo__name', which does not refer
to a Field.\x1b[0m\n\x1b[31;1m<class
'admin_views.admin.ChapterXtra1Admin'>: (admin.E116) The value of
'list_filter[5]' refers to 'chap__book__promo__name', which does not refer
to a Field.\x1b[0m\n"
}}}
On its own, running the `admin_views` tests with a `call_command('check')`
inserted, the checks pass. It seems that something is affecting these
`admin_views` models before `test_migrate_with_system_checks` then re-runs
the checks and finds them failing. I can't figure it out :/
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:9>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adamchainz):
OK I couldn't figure out an easy way around the above confusion. I thing
it's straight up wrong to run 'check' with `INSTALLED_APPS` overridden. As
such I've deleted or modified the few tests that did so.
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:10>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
I'm not sure it's acceptable to remove regressions tests such as the one
added in 93deb1691eb27dc89135511fb0c10e077c8baca7 without offering some
replacement.
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:11>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adamchainz):
Fair, though the bug corrected in 93deb1691eb27dc89135511fb0c10e077c8baca7
is a pretty rare one though.
Afaict the main reason tests are overriding `INSTALLED_APPS` before doing
`call_command('check')` is because there are so many check errors in other
test apps. Which leads me back to fixing all those as part of #25415...
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:12>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"cd86f03591c4147b7c39e226a0d4bc4dc295d2ca" cd86f03]:
{{{
#!CommitTicketReference repository=""
revision="cd86f03591c4147b7c39e226a0d4bc4dc295d2ca"
Refs #26961 -- Fixed invalid ModelAdmins in tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:13>
Django
Reporter: Adam Chainz | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution:
Keywords: checks | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
The final bit of this work is on the
[https://github.com/django/django/pull/7780 PR] for #27673.
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Chainz | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: 1.9
Severity: Normal | Resolution: fixed
Keywords: checks | Triage Stage: Ready for
| checkin
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:"9daf8c43bdfa9f6418f6f47d202055b97244debe" 9daf8c43]:
{{{
#!CommitTicketReference repository=""
revision="9daf8c43bdfa9f6418f6f47d202055b97244debe"
Fixed #26961 -- Made admin checks run when DEBUG=False.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26961#comment:15>