Make DiscoverRunner run system checks

[Adam Johnson]

In Django 1.7, 'manage.py test' would run the system checks as part of 'migrate'. This was reliable since at that point the test database[s] exists. Since #23685 , the call_command('migrate') in create_test_db has been skipping the checks and hence no test run calls the checks.

I think the checks should run *somewhere* in the test framework; perhaps not as part of migrate, since there may be multiple databases to set up. I am manually adding call_command('check') to the start of the test runner's run_suite in my project and this works fine.

I found this when upgrading our application, which uses some custom checks we rely on for developer synchronization on e.g. installed pip packages, and the checks stopped running.

[Adam Johnson]

Pull request: ​https://github.com/django/django/pull/5293

[Carl Meyer]

Wouldn't a much simpler solution to this be to flip the requires_system_checks class attribute of django.core.management.commands.test.Command from False to True?

I'm not sure why it is currently False (it was set that way without comment in the original commit that added the checks framework). Maybe that was just a workaround to avoid the checks being run twice?

[Adam Johnson]

No, as I tried that on my project and it failed CI because those system checks run with the unmutated database settings from setup_test_databases - and the non- test_ prefixed database does not exist in our CI environment. Probably the same for everyone else.

[Carl Meyer]

I see. I doubt that's "the same for everyone else," as until fairly recently Django required the "main" database to exist before tests could be run at all, because it connected to the main database in order to create the test one. But nonetheless you're right that failing to support it would still be a regression from 1.7, even if it's not a common need.

In that case your PR looks pretty reasonable, though I think it would be good to add comments at two locations -- at requires_system_checks = False and at the location where you call_command('check') -- explaining why this is done instead of a simple requires_system_checks = True.

[Adam Johnson]

Oh yes, the _nodb_connection change. I forgot I'd actually backported that on to our 1.7 version :)

Comments added! Is the test okay, or should the check_framework tests be a good app and register the check as part of an AppConfig.ready?

[Carl Meyer]

I don't know how much it matters if the check is registered in an AppConfig.ready, but have you checked to make sure that test fails currently without the code changes? It seems like a bit of a fragile test - if anything in the test suite prior to this point causes checks to run, the test will pass regardless of whether the test runner actually runs checks.

Personally I'd lean towards a more reliable lower-level unit test of the runner, but that would be more work to write (and might require some mocking).

[Adam Johnson]

Yes I checked it without the patch, runtests.py check_framework failed before and passed afterwards.

It does look like a lot of work to create a standalone runner, as I would at least need to override the settings so setup_databases finds no databases to setup. Will have a look around the test suite for examples of runner instantiation to gauge feasibility.

[Tim Graham]

I'm of the opinion that running the system checks as part of the manage.py test command should be opt-in (for example, by writing a test that asserts the call_command('check') output is empty. For example, when debugging a single test, it doesn't seem necessary to have the overhead of running check. As more options are added to check (e.g. #25500), a default implementation as currently proposed could become increasing inflexible. I'd like to document the change in the 1.8 release notes and suggest the alternative.

[Adam Johnson]

I don't think they should be optional, or if they are, they should be opt-out. The checks are a brilliant guard against error, but not running them as part of test invites them not being run at all in a TDD workflow, as often code can be developed with nothing but running the tests. It is also surprising that *only* test doesn't run them, since every other manage command does.

At YPlan we couldn't do without them as part of tests. Our aforementioned 'installed packages' check saves a lot of time that would otherwise be wasted understanding confusing error messages about imports not working, and our other custom checks do verification similar to Django's, for issues that without resolution it does not make sense to even attempt do any tests. Also we don't notice any real overhead, we can still get a single test to run in 1 second (with --keepdb :) ) despite all our extra messing around with pip freeze etc.

[Tim Graham]

Created a ​django-developers discussion. Maybe it would be acceptable to use the approach suggested by Shai, "it would seem that the best default is to run checks unless specific tests are selected."

[Tim Graham]

As I wrote ​to the mailing list , I'm fine to proceed with this. We need a separate commit to fix the existing errors/warnings in Django's test suite, as well as documentation for the change and a mention in the release notes.

[Adam Johnson]

I've corrected all the check errors exception from some E340 errors (check added June 3 in #12810):

django.core.management.base.SystemCheckError: SystemCheckError: System check identified some issues:

ERRORS:
model_options.Article.authors: (fields.E340) The field's intermediary table 'model_options_articleref_authors' clashes with the table name of 'model_options.ArticleRef.authors'.
model_options.Article.reviewers: (fields.E340) The field's intermediary table 'model_options_articleref_reviewers' clashes with the table name of 'model_options.ArticleRef.reviewers'.
model_options.ArticleRef.authors: (fields.E340) The field's intermediary table 'model_options_articleref_authors' clashes with the table name of 'model_options.Article.authors'.
model_options.ArticleRef.reviewers: (fields.E340) The field's intermediary table 'model_options_articleref_reviewers' clashes with the table name of 'model_options.Article.reviewers'.
unmanaged_models.C01.mm_a: (fields.E340) The field's intermediary table 'd01' clashes with the table name of 'unmanaged_models.Intermediate'.
unmanaged_models.C02.mm_a: (fields.E340) The field's intermediary table 'd01' clashes with the table name of 'unmanaged_models.C01.mm_a'.

In the first case the test models seem to hack their M2M fields to use the same tables on purpose (??). In the second case the tests are re-using the tables for unmanaged models on purpose.

For the latter case, I can imagine that E340 should ignore unmanaged models, but I'm not 100% sure. For the former though, whether it's the model_options tests that are 'wrong', or the check, I don't think I'm qualified to know :/

[Tim Graham <timograham@…>]

In 652bcc6f:

Refs #25415 -- Fixed invalid models in the test suite.

[Tim Graham <timograham@…>]

In 6d947e8c:

Refs #25415 -- Fixed/silenced check errors in Django's test suite.

[Tim Graham <timograham@…>]

In 391c450:

Refs #25415 -- Made MySQL backend skip field validation of unsupported models.

[Tim Graham <timograham@…>]

In 5eff8a77:

Fixed #25415 -- Made DiscoverRunner run system checks.