Fixed #25415 -- Made DiscoverRunner run system checks. #6294

adamchainz · 2016-03-14T23:01:11Z

Revival of #5293.

adamchainz · 2016-03-15T10:41:53Z

@timgraham I've fixed all but the "CommaSeparatedIntegerField has been deprecated." warnings. Is there a recommended way of fixing them? We could just ignore the warning in the tests, or we could subclass it to ignore the warning and use that subclass throughout the test suite

timgraham · 2016-03-15T14:12:38Z

Can we use SILENCED_SYSTEM_CHECKS? I don't think we should change all instances of ForeignKey(unique=True) to OneToOneField, for example, as it's a valid scenario that should continue to be tested.

adamchainz · 2016-03-15T15:17:30Z

Although some instances are testing behaviour that might be dfiferent for OneToOneField, most of the ForeignKey(unique=True) instances seem to be more valid as OneToOneFields though.

Silencing across the entire test suite is potentially dangerous as it might prevent problems in future tests. Ideally we'd be able to silence for specific instances of warnings, though that sounds like a big yak shave...

timgraham · 2016-03-15T15:53:16Z

The idea of silencing specific warnings has come up before. It would be a nice to have, but as you said, not quite sure of the best implementation and how much work it would be.

I don't see foresee any problems with silencing the warnings for CommaSeparatedIntegerField and ForeignKey(unique=True) but if you come up with some other solution, I'll be happy to look at it.

adamchainz · 2016-03-16T23:11:39Z

Updated to use SILENCED_SYSTEM_CHECKS - setting it in runtests.py seems to work, afaict this is the 'base' of the settings used in tests.

adamchainz · 2016-03-18T14:31:05Z

Added a release note, turned into two commits - one to fix discoverrunner, one to fix the errors 👍

timgraham · 2016-03-25T23:40:08Z

There are some problems with the MySQL and MySQL GIS builds.

adamchainz · 2016-03-31T14:49:52Z

Both seem unrelated

MySQL:

  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python2.7/django/contrib/gis/db/models/fields.py", line 120, in db_type
    return connection.ops.geo_db_type(self)
AttributeError: 'DatabaseOperations' object has no attribute 'geo_db_type'

MySQL GIS:

  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql_gis/label/trusty-pr/python/python2.7/django/contrib/gis/db/models/fields.py", line 420, in _check_connection
    raise ImproperlyConfigured('Raster fields require backends with raster support.')
django.core.exceptions.ImproperlyConfigured: Raster fields require backends with raster support.

Just rebased on master to retrigger builds, GIS error seems to have happened again.

timgraham · 2016-03-31T14:58:05Z

In the first case, I think the problem is that the checks are run on GIS models when a GIS backend isn't in use.

adamchainz · 2016-03-31T15:01:55Z

Ah, that would do it

charettes · 2016-03-31T15:52:47Z

Looks like you'll need to skip these checks if not all(getattr(connection.features, feature, False) for feature in field.model._meta.required_db_features)

timgraham · 2016-08-16T20:02:07Z

tests/sites_framework/models.py

@@ -9,7 +9,6 @@ class AbstractArticle(models.Model):
    title = models.CharField(max_length=50)


Does this actually identify a regression in 3a47d42? The check error CustomArticle.on_site: (sites.E001) CurrentSiteManager could not find a field named 'sites'. seems to suggest that model's override of the parent manager isn't working. \cc @loic

I thought it might be but wasn't sure

Created ticket: https://code.djangoproject.com/ticket/27073#ticket

Just tested it and I can confirm that this is no longer needed after applying #7115.

Thanks @loic

timgraham · 2016-09-09T20:28:05Z

tests/test_runner/tests.py

@@ -203,8 +203,6 @@ def test_ticket_17477(self):

 class Sqlite3InMemoryTestDbs(TestCase):

-    available_apps = []


I don't think this is related to any check errors, is it?

I see the exception if it's removed:

====================================================================== ERROR: test_transaction_support (test_runner.tests.Sqlite3InMemoryTestDbs) Ticket #16329: sqlite3 in-memory test databases ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/tim/code/django/django/apps/registry.py", line 148, in get_app_config return self.app_configs[app_label] KeyError: 'auth' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/tim/code/django/tests/test_runner/tests.py", line 227, in test_transaction_support DiscoverRunner(verbosity=0).setup_databases() File "/home/tim/code/django/django/test/runner.py", line 537, in setup_databases self.parallel, **kwargs File "/home/tim/code/django/django/test/utils.py", line 205, in setup_databases call_command('check') File "/home/tim/code/django/django/core/management/__init__.py", line 130, in call_command return command.execute(*args, **defaults) File "/home/tim/code/django/django/core/management/base.py", line 345, in execute output = self.handle(*args, **options) File "/home/tim/code/django/django/core/management/commands/check.py", line 68, in handle fail_level=getattr(checks, options['fail_level']), File "/home/tim/code/django/django/core/management/base.py", line 374, in check include_deployment_checks=include_deployment_checks, File "/home/tim/code/django/django/core/management/base.py", line 361, in _run_checks return checks.run_checks(**kwargs) File "/home/tim/code/django/django/core/checks/registry.py", line 81, in run_checks new_errors = check(app_configs=app_configs) File "/home/tim/code/django/django/contrib/auth/checks.py", line 16, in check_user_model cls = apps.get_model(settings.AUTH_USER_MODEL) File "/home/tim/code/django/django/apps/registry.py", line 195, in get_model return self.get_app_config(app_label).get_model(model_name.lower()) File "/home/tim/code/django/django/apps/registry.py", line 155, in get_app_config raise LookupError(message) LookupError: No installed app with label 'auth'.

We should check if this is the proper fix or if this reveals some other problem, I'm not sure offhand.

I tried running the tests without it but they fail (only on Windows for some reason)

Afaict it was added just as an optimization. Starting a new DiscoverRunner inside the test suite re-runs the checks, one of which has been registered from auth and fails because auth is no longer in INSTALLED_APPS. So I think just removing the available_apps line is legit.

@adamchainz, did you investigate this?

timgraham · 2016-09-09T20:35:12Z

To address the new clashing table errors, I think we should modify that check to ignore unmanaged models. The patched I attached to https://code.djangoproject.com/ticket/27204 fixes the check errors that popped up in model_options and unmanaged_models.

timgraham · 2016-09-09T20:41:17Z

Some additional output capturing might be needed. I see several "System check identified no issues (21 silenced)." in the test output.

charettes · 2016-09-09T20:57:05Z

@timgraham , Model (and fields) checks must not be run if all not all required db features are available on all connections. We should make Model.check() abort immediately if not all(getattr(connection.features, feature, False) for feature in self._meta.required_db_features).

charettes · 2016-09-10T22:45:44Z

@timgraham, that's what I had in mind: #7227.

timgraham · 2016-09-17T00:13:35Z

django/test/utils.py

@@ -200,6 +201,9 @@ def setup_databases(verbosity, interactive, keepdb=False, debug_sql=False, paral
        for alias in connections:
            connections[alias].force_debug_cursor = True

+    # Now run the system checks. They aren't run until now since some checks require database access.
+    call_command('check')


Pass the verbosity argument here.

adamchainz · 2016-11-05T13:03:50Z

I've merged in something similar to #7227 but only for the failing models. In the process I also found and removed some import hacks from the GIS tests which were missed when making GDAL a required dependency. Tests are green, ready for re-review 🎉

timgraham · 2016-11-25T15:10:14Z

tests/gis_tests/rasterapp/models.py

+from django.db import connections
+
+
+class SkipChecksIfNotDBFeaturesMixin(object):


It seems like this could be useful in other tests at some point. Rather than burying it here, it might be useful to put it somewhere more reusable/discoverable (even if it's not public API at this point). Maybe django.core.checks.tests.py? Any opinion @charettes?

I feel like it's a bit specific?

adamchainz

Here's a comment I wrote during DUTH github didn't post because the new review UI is unclear

timgraham

MySQL fails with:

django/contrib/gis/db/models/fields.py", line 123, in db_type
    return connection.ops.geo_db_type(self)
AttributeError: 'DatabaseOperations' object has no attribute 'geo_db_type'

timgraham · 2016-11-30T16:43:14Z

tests/gis_tests/rasterapp/models.py

+
+class SkipChecksIfNotDBFeaturesMixin(object):
+    """
+    Required because we make the models but then skip the tests for them


I'd say, "Skip system checks on models that don't have all the required_db_features."

timgraham · 2016-11-30T16:48:01Z

django/test/utils.py

@@ -200,6 +201,9 @@ def setup_databases(verbosity, interactive, keepdb=False, debug_sql=False, paral
        for alias in connections:
            connections[alias].force_debug_cursor = True

+    # Now run the system checks. They aren't run until now since some checks require database access.
+    call_command('check', verbosity=verbosity)


I'm not sure if this is the best place for this -- doesn't seem like "running system checks" is related to "setting up databases". Anyway, this function is documented in topics/testing/advanced.txt and the docs should be updated if this change happens.

The main reason I put it here is because this is where it was implicitly being run in 1.7, as part of migrate. Will try put it later in the flow.

I remember - another reason I did this was so that nose, my test runner at the time, would automatically run them, because it calls setup_databases. I now use pytest which also has this behaviour, but I'm making pytest-django run the checks itself in pytest-dev/pytest-django#414 .

timgraham · 2016-11-30T16:48:07Z

docs/releases/1.11.txt

@@ -397,6 +397,8 @@ Tests
 * Added support for :meth:`python:unittest.TestCase.subTest`’s when using the
  :option:`test --parallel` option.

+* Tests now run checks at the start of the test run, as they did in Django 1.7.


I think "as they did in Django 1.7" needn't be mentioned at this point. For third-party test runners, it's probably also useful to mention exactly where the check are run from.

timgraham · 2016-11-30T16:49:04Z

django/test/utils.py

@@ -200,6 +201,9 @@ def setup_databases(verbosity, interactive, keepdb=False, debug_sql=False, paral
        for alias in connections:
            connections[alias].force_debug_cursor = True

+    # Now run the system checks. They aren't run until now since some checks require database access.


Please wrap comments at 79 characters.

timgraham · 2016-11-30T16:49:13Z

django/core/management/commands/test.py

@@ -8,6 +8,7 @@
 class Command(BaseCommand):
    help = 'Discover and run tests in the specified modules or the current directory.'

+    # Tests do run the checks, but only after databases have been set up


use periods

adamchainz · 2016-11-30T17:24:10Z

Will fix MySQL failure. That's annoying, tests were passing before my rebase yesterday.

adamchainz · 2016-11-30T21:18:54Z

I found that there were more problem models in other parts of gis_tests that have started failing checks, idk why exactly though. Since it was only the MySQL check_field that was actually failing when it called db_type, I've moved the check for 'model enabled' there. It's not the most logical place but it 'works for now'. In other news its check_field needed updating anyway because it was using django.db.connection instead of self.connection, meaning it'd start using the wrong connection if you had e.g. postgres as your default DB and mysql as a secondary.

adamchainz · 2016-11-30T22:11:02Z

More check failures following 7cddd8a 😄 :

ERRORS:
max_lengths.PersonWithCustomMaxLengths.vcard: (fields.E202) FileField's 'upload_to' argument must be a relative path, not an absolute path.
	HINT: Remove the leading slash.
max_lengths.PersonWithDefaultMaxLengths.vcard: (fields.E202) FileField's 'upload_to' argument must be a relative path, not an absolute path.
	HINT: Remove the leading slash.
serializers.FileData.data: (fields.E202) FileField's 'upload_to' argument must be a relative path, not an absolute path.
	HINT: Remove the leading slash.

charettes · 2016-11-30T22:12:53Z

@adamchainz hopefully we can ship this before introducing yet another failing check 😄

timgraham · 2016-11-30T22:18:18Z

docs/releases/1.11.txt

@@ -402,6 +402,8 @@ Tests
 * Added support for :meth:`python:unittest.TestCase.subTest`’s when using the
  :option:`test --parallel` option.

+* Tests now run checks at the start of the test run.


Could you explain that it's DiscoverRunner that does this. Perhaps the docs for DiscoverRunner should also be updated.

timgraham · 2016-11-30T22:19:04Z

django/core/management/commands/test.py

@@ -8,6 +8,7 @@
 class Command(BaseCommand):
    help = 'Discover and run tests in the specified modules or the current directory.'

+    # Tests do run the checks, but only after databases have been set up.


DiscoverRunner runs checks after databases are set up.

…isn't supported

timgraham · 2016-12-29T17:38:20Z

merged in 6d947e8, 391c450, 5eff8a7, thanks!

adamchainz force-pushed the ticket_25415 branch from 5e3a4d2 to 778b584 Compare March 16, 2016 23:11

adamchainz force-pushed the ticket_25415 branch 3 times, most recently from 1eab279 to 5db2f8b Compare March 18, 2016 14:25

adamchainz force-pushed the ticket_25415 branch from 5db2f8b to 781367d Compare March 31, 2016 14:43

adamchainz mentioned this pull request May 23, 2016

Automatically run checks at the start of tests pytest-dev/pytest-django#337

Open

adamchainz force-pushed the ticket_25415 branch from 781367d to cf70c22 Compare August 15, 2016 21:40

timgraham reviewed Aug 16, 2016
View reviewed changes

adamchainz force-pushed the ticket_25415 branch from beae876 to b6365e3 Compare August 19, 2016 15:51

timgraham reviewed Sep 9, 2016
View reviewed changes

timgraham reviewed Sep 17, 2016

View reviewed changes

adamchainz force-pushed the ticket_25415 branch 2 times, most recently from 62fb973 to 986da7a Compare September 23, 2016 07:09

adamchainz force-pushed the ticket_25415 branch 2 times, most recently from 41d2189 to b70375e Compare November 5, 2016 10:57

This was referenced Nov 5, 2016

Refs #25415 -- Stopped running checks on models with missing features. #7227

Closed

Run checks at the start of tests pytest-dev/pytest-django#414

Open

adamchainz force-pushed the ticket_25415 branch from 482d0dd to b4fb194 Compare November 7, 2016 09:33

timgraham reviewed Nov 25, 2016

View reviewed changes

adamchainz commented Nov 25, 2016

View reviewed changes

adamchainz changed the title ~~Fixed #25415 -- made test runner perform system checks~~ Fixed #25415 -- Made test runner perform system checks Nov 26, 2016

adamchainz changed the title ~~Fixed #25415 -- Made test runner perform system checks~~ Fixed #25415 -- Made DisoverRunner run system checks Nov 26, 2016

adamchainz mentioned this pull request Nov 29, 2016

Fixed #26961 -- Made Admin checks run when DEBUG = False #6980

Closed

adamchainz force-pushed the ticket_25415 branch from b4fb194 to fc65b7c Compare November 29, 2016 19:54

timgraham changed the title ~~Fixed #25415 -- Made DisoverRunner run system checks~~ Fixed #25415 -- Made DiscoverRunner run system checks. Nov 29, 2016

timgraham reviewed Nov 30, 2016

View reviewed changes

adamchainz force-pushed the ticket_25415 branch 2 times, most recently from 8e53c42 to 5bf7ece Compare November 30, 2016 21:16

adamchainz force-pushed the ticket_25415 branch from 5bf7ece to 84d1125 Compare November 30, 2016 21:19

adamchainz force-pushed the ticket_25415 branch from 84d1125 to 4ef20b2 Compare November 30, 2016 22:15

timgraham reviewed Nov 30, 2016

View reviewed changes

adamchainz force-pushed the ticket_25415 branch from 4ef20b2 to c3f3199 Compare November 30, 2016 23:01

Adam Chainz added 3 commits December 28, 2016 21:24

Fixed #25415 -- Made DiscoverRunner run system checks

c7ad20f

Refs #25415 -- Fixed all check errors in Django's test suite

b31f465

Refs #25415 -- Made MySQL backend skip field validation if the model …

594458b

…isn't supported

adamchainz force-pushed the ticket_25415 branch from c3f3199 to 594458b Compare December 28, 2016 21:25

timgraham closed this Dec 29, 2016

adamchainz mentioned this pull request Jan 4, 2017

Fixed #27673 -- Prevented system checks for contrib.admin during registration. #7780

Merged

		@@ -9,7 +9,6 @@ class AbstractArticle(models.Model):
		title = models.CharField(max_length=50)

		@@ -203,8 +203,6 @@ def test_ticket_17477(self):

		class Sqlite3InMemoryTestDbs(TestCase):

		available_apps = []

		from django.db import connections


		class SkipChecksIfNotDBFeaturesMixin(object):

Fixed #25415 -- Made DiscoverRunner run system checks. #6294

Fixed #25415 -- Made DiscoverRunner run system checks. #6294

Conversation

adamchainz commented Mar 14, 2016

adamchainz commented Mar 15, 2016

timgraham commented Mar 15, 2016

adamchainz commented Mar 15, 2016

timgraham commented Mar 15, 2016

adamchainz commented Mar 16, 2016

adamchainz commented Mar 18, 2016

timgraham commented Mar 25, 2016

adamchainz commented Mar 31, 2016

timgraham commented Mar 31, 2016

adamchainz commented Mar 31, 2016

charettes commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgraham commented Sep 9, 2016

timgraham commented Sep 9, 2016

charettes commented Sep 9, 2016

charettes commented Sep 10, 2016

Choose a reason for hiding this comment

adamchainz commented Nov 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamchainz left a comment

Choose a reason for hiding this comment

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamchainz commented Nov 30, 2016

adamchainz commented Nov 30, 2016

adamchainz commented Nov 30, 2016

charettes commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgraham commented Dec 29, 2016