[Django] #21206: Misleading Exception on emty test class: ImportError: Start directory is not importable
Django
not importable
-----------------------------------+------------------------
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------
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:
{{{
#!python
"""
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.
{{{
#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21206>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by prestontimmons):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:1>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by prestontimmons):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:2>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:3>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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`.
{{{
#!python
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:4>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:5>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:6>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by thepapermen):
Hi! Thank you! Sorry for posting wrong second argument!
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:7>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:8>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:9>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:10>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:11>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:12>
Django
not importable
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:13>
Django
not importable
-----------------------------------+--------------------------------------
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by prestontimmons):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:14>
Django
not importable
-----------------------------------+--------------------------------------
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: closed
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"aef019de6113e6c750e3ba49dc1e9f2a179eb0ca"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:15>
Django
not importable
-----------------------------------+--------------------------------------
Reporter: thepapermen | Owner: nobody
Type: Bug | Status: closed
Component: Testing framework | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz <claude@…>):
In [changeset:"18d962f2e6cc8829d60d6f6dfb3ee3855fa5362e"]:
{{{
#!CommitTicketReference repository=""
revision="18d962f2e6cc8829d60d6f6dfb3ee3855fa5362e"
Fixed #21206 -- Fixed test discovery without labels
Added test to verify an empty label performs discovery on the current
working directory.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21206#comment:16>