Opened 9 years ago

Closed 8 years ago

#25415 closed New feature (fixed)

Make DiscoverRunner run system checks

Reported by: Adam Johnson Owned by: nobody
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Change History (17)

comment:2 by Carl Meyer, 9 years ago

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?

comment:3 by Carl Meyer, 9 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:4 by Adam Johnson, 9 years ago

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.

comment:5 by Carl Meyer, 9 years ago

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.

comment:6 by Adam Johnson, 9 years ago

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?

comment:7 by Carl Meyer, 9 years ago

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

comment:8 by Adam Johnson, 9 years ago

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.

comment:9 by Tim Graham, 9 years ago

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.

comment:10 by Adam Johnson, 9 years ago

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.

comment:11 by Tim Graham, 9 years ago

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

comment:12 by Tim Graham, 9 years ago

Summary: Django 1.8 regression: tests no longer run checksMake DiscoverRunner run system checks
Type: BugNew feature
Version: 1.8master

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.

comment:13 by Adam Johnson, 8 years ago

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

comment:14 by Tim Graham <timograham@…>, 8 years ago

In 652bcc6f:

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

comment:15 by Tim Graham <timograham@…>, 8 years ago

In 6d947e8c:

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

comment:16 by Tim Graham <timograham@…>, 8 years ago

In 391c450:

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

comment:17 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 5eff8a77:

Fixed #25415 -- Made DiscoverRunner run system checks.

Note: See TracTickets for help on using tickets.
Back to Top