Opened 9 years ago

Closed 9 years ago

Last modified 5 years ago

#23879 closed Cleanup/optimization (fixed)

We should use test-skipping, not conditional discovery in

Reported by: Carl Meyer 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


Currently the get_test_modules function in has hardcoded skips in place for GIS and Postgres-specific tests. We should remove this logic from 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).
  1. 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.

Change History (17)

comment:1 by Claude Paroz, 9 years ago

Triage Stage: UnreviewedAccepted
Version: 1.6master

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.

comment:2 by Carl Meyer, 9 years ago

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 django.contrib.gis and get a bunch of skipped tests, some passed tests, no failures or errors.

comment:3 by Carl Meyer, 9 years ago

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.

comment:4 by Preston Timmons, 9 years ago


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:

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

comment:5 by Wojtek Ruszczewski, 9 years ago

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.

Last edited 9 years ago by Wojtek Ruszczewski (previous) (diff)

comment:6 by Tim Graham, 9 years ago

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.

comment:7 by Claude Paroz, 9 years ago

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

comment:8 by Carl Meyer, 9 years ago

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.

comment:9 by Markus Holtermann, 9 years ago

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.

comment:10 by Claude Paroz, 9 years ago

Has patch: set

I finally succeeded in having a seemingly-working patch. Not commit-ready, but a base for discussion:

comment:11 by Tim Graham, 9 years ago

Patch needs improvement: set

Seems on the right track to me.

comment:12 by Claude Paroz, 9 years ago

Patch needs improvement: unset

comment:13 by Tim Graham, 9 years ago

Patch needs improvement: set

comment:14 by Claude Paroz <claude@…>, 9 years ago

Resolution: fixed
Status: newclosed

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

comment:15 by Claude Paroz <claude@…>, 9 years ago

In 6b6d13b:

Stopped conditional discovery of gis_tests apps

Refs #23879.

comment:16 by Claude Paroz <claude@…>, 9 years ago

In 36e90d1f:

Stopped special-casing postgres-specific tests

Refs #23879.

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 2fb872e5:

Refs #23879 -- Made introspection respect required_db_features.

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