We should use test-skipping, not conditional discovery in runtests.py

[Carl Meyer]

Currently the ​https://github.com/django/django/blob/master/tests/runtests.py#L62 get_test_modules function in runtests.py has hardcoded skips in place for GIS and Postgres-specific tests. We should remove this logic from runtests.py and instead ensure that the relevant tests use test-skipping appropriately. Reasons:

1. Regardless of the discovery logic in get_test_modules, you can always run any of these tests explicitly by providing the test label, and if you do so with the wrong database, the result should be skipped tests (with a reason for the skip which the test runner can output at the appropriate verbosity level), not failing tests (which could be quite confusing).

2. The overall number of tests in Django's test suite (when no test labels are provided) should not vary depending on your settings, some tests should just be marked as skips. This makes it explicit to someone running the tests with a non-Postgres database (for instance) that there are a number of Postgres-specific tests which are being skipped for them, rather than silently hiding those tests entirely.

[Claude Paroz]

In fact I think that django.contrib.gis tests already satisfy your demand. If someone has a GIS-lib-free system at hand, it would be nice to test and report any failure/traceback.

[Carl Meyer]

Yes, that's right. I fixed one issue in 4932a7b8e0a25c7447d7ad7ae8ae53a35d2541ac yesterday. I have a system here that doesn't have GEOS installed, and I can run runtests.py django.contrib.gis and get a bunch of skipped tests, some passed tests, no failures or errors.

[Carl Meyer]

Hmm, I'll need to look into this a bit more. The issue is not with the tests themselves, but with the test models/migrations; in at least the GIS case, those fail immediately when creating the test database if you don't have a GIS-enabled database backend.

I know we had this working when the test-discovery branch was merged; I'll need to look back and see how it worked then and what's changed since.

[Preston Timmons]

Carl,

I think the discovery result is still the same as it was when the discover runner was merged. Claude's changes simply replaced the old geo_apps function:

​https://github.com/django/django/commit/bd563f0f571e7c76b40e8c8d7a0e1f34dcfeb810
​https://github.com/django/django/commit/57f190ed3b77eb105cc86ecfabc455662410d40a

Using discovery_paths this way—to conditionally add test applications from contrib to INSTALLED_APPS—isn't really obvious from the code, though.

[Wojtek Ruszczewski]

I can confirm that removing the GIS toggle already gives a bunch of skipped tests without any failures or errors.

On the other hand, postgres tests can't be skipped that easily -- postgres_tests has some migrations that rely on PostgreSQL specific operations and cause the test database creation to fail with any other backend.

Resolving this would probably require some way to bypass migrations or whole test apps depending on database backend / vendor. I was able to get it to work with an ​ad-hoc solution (that hasn't been properly thought over or researched), but it may be an interesting question how to provide database-specific migrations with reusable apps.

[Tim Graham]

That looks interesting. It would probably be worth discussing the issue of database vendor specific migrations on the DevelopersMailingList. If it's been discussed before I'm not aware of it.

[Claude Paroz]

Wojtek, I think your POC patch is interesting, and would be worth being completed with docs and tests.

[Carl Meyer]

I agree. I think there are probably some aspects that require further thought (like what happens if a later migration is "appropriate for this connection" but has a dependency on one that is not?), but I think the general idea is sound.

I wonder if perhaps it would work better if it were operations rather than entire migrations that were marked as for-certain-db-vendor-only? I could imagine some API like a mark-for-vender wrapper to surround a list of operations in a migration file.

[Markus Holtermann]

The general idea to have some vendor specifics in the migration code base sounds good. But I have to agree with Carl on the problems that arise from not loading a migration. I think the skipping of vendor miss-matches should happen during the actual migrate process. This still allows dependency resolutions and state changes independent from the vendor.

Moving the vendor part into operations would furthermore give us some flexibility for advanced database features. E.g. once contrib.postgres got support for views, these views could be generated within the same migration, one operation could be django.contrib.postgres.operations.CreateView (with a PostgreSQL vendor marker) while a RunSQL operation would run everywhere but on PostgreSQL.

[Claude Paroz]

I finally succeeded in having a seemingly-working patch. Not commit-ready, but a base for discussion:
​https://github.com/django/django/pull/4452

[Tim Graham]

Seems on the right track to me.

[Claude Paroz <claude@…>]

In 8097e54:

Fixed #23879 -- Allowed model migration skip based on feature/vendor

Thanks Carl Meyer for the report and review, and Tim Graham for the
review.

[Claude Paroz <claude@…>]

In 6b6d13b:

Stopped conditional discovery of gis_tests apps

Refs #23879.

[Claude Paroz <claude@…>]

In 36e90d1f:

Stopped special-casing postgres-specific tests

Refs #23879.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2fb872e5:

Refs #23879 -- Made introspection respect required_db_features.