Code

Opened 9 months ago

Closed 7 months ago

#21206 closed Bug (fixed)

Misleading Exception on empty test class: ImportError: Start directory is not importable

Reported by: thepapermen Owned by: nobody
Component: Testing framework Version: 1.6-beta-1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Django 1.6 test runner produces misleading exception if you try to run a test case which does not contain any methods with names starting with test.

Issues of this type are time-consuming to debug, since the exception puts you on the wrong path of searching for faulty and/or circular imports.

To reproduce:

"""
This file is placed in application/tests.py. `application` directory does contain an __init__.py file.
"""
from django.test import TestCase

class AnEmptyTest(TestCase):
    """
    Test case without method names that start with `test`.
    """
    pass

./manage.py test application.tests.AnEmptyTest will raise an error: ImportError: Start directory is not importable: 'application.tests.AnEmptyTest'. If you add an empty method with name starting with test, test runner will run fine.

from django.test import TestCase

class ANonEmptyTest(TestCase):
    """
    This will run fine.
    """
    def test_something(self):
        pass

Desired behavior for AnEmptyTest: launch test runner and run zero tests.

Attachments (0)

Change History (15)

comment:1 Changed 9 months ago by prestontimmons

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

This exception comes from the unittest discover method.

By adding an empty unittest on the Python path, this can be reproduced outside of Django with python -m unittest discover path.to.EmptyTestCase.

Marking this as accepted for now, but fixing it would require us to further mask the unittest discovery behavior in Django's test runner.

comment:2 Changed 9 months ago by prestontimmons

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 9 months ago by carljm

Yeah, I agree this is bad behavior, but its bad behavior that should really be fixed upstream in unittest, not in Django.

I'll leave this open in case anyone suggests a really slick way to work around it in Django, and just so we can continue to track the issue and any upstream progress, but in general I don't think it's up to Django to fix bugs in the Python standard library.

comment:4 Changed 9 months ago by thepapermen

I was going to report the bug upstream, but I wasn't able to reproduce it with python's unittest module. Indeed, it looks like it is a Django test runner bug.

I'm testing it with python2.7:amd64/sid 2.7.5-8 uptodate.

import unittest

class EmptyTestCase(unittest.TestCase):
    pass

Here is the output of python -m unittest discover /path/to/python/module path.to.EmptyTestCase:

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK
Version 0, edited 9 months ago by thepapermen (next)

comment:5 Changed 9 months ago by carljm

Yeah, on a closer look, you're right. Although this error message is reproducible with python -m unittest discover path.to.EmptyTestCase, it's not tied to an empty test case; it's simply because in unittest "discover" does not accept dotted paths to test case classes, only file paths or dotted package paths to start discovery at. Django's discovery runner attempts to combine unittest's discovery ability with its ability to load tests directly from a dotted module / testcase path, by autodetecting which one each provided test label is, and the bug here lies in that autodetection. We assume that if we try loadTestsFromName and no tests are found, that the given label must have been a package instead, and so we try discovery. When the given label was in fact an empty test case class, this error occurs.

In order to fix this, we'll need to do some further checks in the if not (tests and tests.countTestCases()) block to verify that the provided label actually maps to a package before we try discovery on it.

comment:6 Changed 9 months ago by carljm

BTW, in your provided example command line of python -m unittest discover /path/to/python/module path.to.EmptyTestCase, the path.to.EmptyTestCase portion is not having the effect you think it does. python -m unittest discover, unlike the Django runner, accepts only one discovery start dir. The second argument, if given, is actually the test-files pattern (i.e. to override the default of test*.py). So no matter how many tests you add to EmptyTestCase, your provided command line will never run any tests.

comment:7 Changed 9 months ago by thepapermen

Hi! Thank you! Sorry for posting wrong second argument! In my case the command python -m unittest discover $(pwd) test.EmptyTestCase did actually try to run all the tests in the directory provided as first argument (before posting the previous reply I have tried to add real test methods to the test class and did insure that they do run, just as a sanity check). Still, I didn't perform enough research. My bad. :P

Last edited 9 months ago by thepapermen (previous) (diff)

comment:8 Changed 9 months ago by prestontimmons

Good catch.

I posted a test case for this here:

https://github.com/prestontimmons/django/commit/0170eff8350285e47aa2561d475725d4436df3cc

Simply changing the check to if tests is None seems to fix the problem and passes all discovery tests, but for some reason it causes test_runner.tests.DeprecationDisplayTest to fail.

Is there ever a case that discovery should run if loadTestsFromName is called?

comment:9 Changed 9 months ago by carljm

Yes, there is such a case, though apparently we don't have good tests for it. If a test label is some.package, where some/package/ is a directory with an __init__.py and then various test files within it, loadTestsFromName will successfully return a test suite containing zero tests (it only looks directly in the some.package module namespace, doesn't recurse to contained modules). In that case, what the user almost certainly meant was "run all the tests within some.package", so we run discovery using some.package (i.e. some/package/) as the start dir.

comment:10 Changed 9 months ago by prestontimmons

Ah, yes. Looks like we're missing a test for that.

That indeed fails if we only check if tests is None.

I went ahead and added a test for this case in my branch.

https://github.com/prestontimmons/django/compare/ticket-21206

comment:11 Changed 9 months ago by prestontimmons

I added a fix and PR here:

https://github.com/django/django/pull/1705

I'm not happy with the use of import_module. If anyone has another way to tell a dotted path is a package, I'll try it out.

comment:12 Changed 9 months ago by carljm

  • Has patch set
  • Patch needs improvement set

I suggested a few small changes on the PR, but basically I think you've got the right approach. Thanks for taking this one on!

comment:13 Changed 7 months ago by prestontimmons

Added final updates in pull request: https://github.com/django/django/pull/1705/files

The pull adds two commits. One that fixes this issue, and one that adds two additional test cases that weren't covered in the original discover runner tests.

I think this is ready to go.

comment:14 Changed 7 months ago by prestontimmons

  • Patch needs improvement unset
  • Summary changed from Misleading Exception on emty test class: ImportError: Start directory is not importable to Misleading Exception on empty test class: ImportError: Start directory is not importable

comment:15 Changed 7 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In aef019de6113e6c750e3ba49dc1e9f2a179eb0ca:

Fixed #21206 -- No longer run discovery if the test label doesn't point to a package or directory.

Thanks thepapermen for the report and Carl Meyer for the review.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.