[Django] #25415: Django 1.8 regression: tests no longer run checks
Django
-----------------------------------+--------------------
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25415>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by adamchainz):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Pull request: https://github.com/django/django/pull/5293
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:1>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by carljm):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:2>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:3>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by adamchainz):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:4>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:5>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by adamchainz):
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`?
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:6>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:7>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by adamchainz):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:8>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:9>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by adamchainz):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:10>
Django
Reporter: adamchainz | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Created a [https://groups.google.com/d/topic/django-developers/GyuVJ-
E6Or4/discussion 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."
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:11>
Django
-----------------------------------+------------------------------------
Reporter: adamchainz | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: 1.8 => master
* type: Bug => New feature
Comment:
As I wrote [https://groups.google.com/d/topic/django-developers/GyuVJ-
E6Or4/discussion 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:12>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by adamchainz):
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 :/
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:13>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"652bcc6f5fa9084768890488fec5208e082c2add" 652bcc6f]:
{{{
#!CommitTicketReference repository=""
revision="652bcc6f5fa9084768890488fec5208e082c2add"
Refs #25415 -- Fixed invalid models in the test suite.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:14>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:15>
Django
-----------------------------------+------------------------------------
Reporter: Adam Chainz | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"6d947e8c3218c7137cd3fffecf1ca5b9274a20c6" 6d947e8c]:
{{{
#!CommitTicketReference repository=""
revision="6d947e8c3218c7137cd3fffecf1ca5b9274a20c6"
Refs #25415 -- Fixed/silenced check errors in Django's test suite.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:15>
Django
-----------------------------------+------------------------------------
Reporter: Adam Chainz | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"391c450fbae8c3301954563288147578a1ae4a6d" 391c450]:
{{{
#!CommitTicketReference repository=""
revision="391c450fbae8c3301954563288147578a1ae4a6d"
Refs #25415 -- Made MySQL backend skip field validation of unsupported
models.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:16>
Django
-----------------------------------+------------------------------------
Reporter: Adam Chainz | Owner: nobody
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:"5eff8a77838540b27b6bef024dfccfd76008fd4c" 5eff8a77]:
{{{
#!CommitTicketReference repository=""
revision="5eff8a77838540b27b6bef024dfccfd76008fd4c"
Fixed #25415 -- Made DiscoverRunner run system checks.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25415#comment:17>