Run admin checks if DEBUG is False

[Adam Johnson]

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

[Adam Johnson]

​https://github.com/django/django/pull/6980

[Tim Graham]

A couple tests need to be fixed and a mention in the release notes could be useful.

[Adam Johnson]

Should I change the 1.10 release notes?

[Tim Graham]

1.10 is at release candidate stage and only receiving fixes for release blockers. Please target 1.11.

[Adam Johnson]

Thanks, wasn't entirely sure if small changes like this were still accepted.

[Adam Johnson]

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 AdminSites 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?

[Tim Graham]

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.

[Adam Johnson]

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 :/

[Adam Johnson]

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.

[Tim Graham]

I'm not sure it's acceptable to remove regressions tests such as the one added in 93deb1691eb27dc89135511fb0c10e077c8baca7 without offering some replacement.

[Adam Johnson]

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...

[Tim Graham <timograham@…>]

In cd86f03:

Refs #26961 -- Fixed invalid ModelAdmins in tests.

[Tim Graham]

The final bit of this work is on the ​PR for #27673.

[Tim Graham <timograham@…>]

In 9daf8c43:

Fixed #26961 -- Made admin checks run when DEBUG=False.