Skip to content

Commit

Permalink
Only import gis test apps for geo-enabled DBs
Browse files Browse the repository at this point in the history
  • Loading branch information
@claudep
claudep committed Jul 9, 2013
1 parent bd563f0 commit 57f190e
Showing 1 changed file with 10 additions and 5 deletions.
15 changes: 10 additions & 5 deletions tests/runtests.py
View file
Expand Up @@ -12,13 +12,11 @@
from django.utils import six

CONTRIB_MODULE_PATH = 'django.contrib'
CONTRIB_GIS_TESTS_PATH = 'django.contrib.gis.tests'

TEST_TEMPLATE_DIR = 'templates'

RUNTESTS_DIR = os.path.abspath(os.path.dirname(upath(__file__)))
CONTRIB_DIR = os.path.dirname(upath(contrib.__file__))
CONTRIB_GIS_TESTS_DIR = os.path.join(CONTRIB_DIR, 'gis', 'tests')

TEMP_DIR = tempfile.mkdtemp(prefix='django_')
os.environ['DJANGO_TEST_TEMP_DIR'] = TEMP_DIR
Expand Down Expand Up @@ -52,11 +50,18 @@


def get_test_modules():
from django.contrib.gis.tests.utils import HAS_SPATIAL_DB
modules = []
for modpath, dirpath in (
discovery_paths = [
(None, RUNTESTS_DIR),
(CONTRIB_MODULE_PATH, CONTRIB_DIR),
(CONTRIB_GIS_TESTS_PATH, CONTRIB_GIS_TESTS_DIR)):
(CONTRIB_MODULE_PATH, CONTRIB_DIR)
]
if HAS_SPATIAL_DB:
discovery_paths.append(
('django.contrib.gis.tests', os.path.join(CONTRIB_DIR, 'gis', 'tests'))
)

for modpath, dirpath in discovery_paths:
for f in os.listdir(dirpath):
if ('.' in f or
# Python 3 byte code dirs (PEP 3147)
Expand Down

6 comments on commit 57f190e

@carljm
Copy link
Member

@carljm carljm commented on 57f190e Jul 9, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claudep Can you clarify the reason for this commit (since it's not linked to a ticket or any other rationale)? With the test discovery patch we made an intentional choice to treat the GIS tests less magically and include them along with everything else, using the normal test-skipping decorators in the test files themselves to skip specific tests that cannot be run with a non-spatial backend. This also makes test counts more consistent, since the not-run tests are reported as skipped rather than omitted entirely.

If this choice was causing a specific problem I'm not necessarily opposed to reversing it, but I'd like to know what the problem was.

@carljm
Copy link
Member

@carljm carljm commented on 57f190e Jul 9, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, never mind - I see the previous commit now. I guess we hadn't removed the magic for the test apps within django.contrib.gis.tests, and you're doing that now. I am still curious though why this follow-up commit was necessary.

@claudep
Copy link
Member Author

@claudep claudep commented on 57f190e Jul 9, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I broke the test suite with the previous commit. The problem is that loading gis test apps with a non gis-enabled backend chokes when trying to create gis database fields. See http://ci.djangoproject.com/job/Django/2866/database=sqlite3,python=python2.7/console

@carljm
Copy link
Member

@carljm carljm commented on 57f190e Jul 9, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I saw that in the IRC backlog just now. I suppose the other option would be to make the model definitions in models.py conditional on HAS_SPATIAL_DB.

@claudep
Copy link
Member Author

@claudep claudep commented on 57f190e Jul 9, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would also be an option. Just tell me if you'd prefer that option.

@carljm
Copy link
Member

@carljm carljm commented on 57f190e Jul 9, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, I don't think it's worth changing. It's effectively the same thing; either we add them to INSTALLED_APPS conditionally as you do here, or we always add them to INSTALLED_APPS and make their model definitions conditional. Not seeing a significant advantage to either approach.

Please sign in to comment.