Extend test discovery to include unittest2 test suite runner

[Jannis Leidel]

For projects wanting to support a more traditional approach to test suite discovery I'd like to propose to ship an optional subclass of the DjangoTestSuiteRuner class as described in @carljm's gist: ​https://gist.github.com/1450104

It uses unittest2's test discovery feature behind the scenes as described in ​http://docs.python.org/dev/library/unittest.html#unittest-test-discovery

[Tomek Paczkowski]

It's backwards compatible and for me a clear win: it will be possible to find tests outside of installed apps.

[anonymous]

IMO this should be the default, entirely replace django's manual test finding with discovery in both all applications as well as in a (setting?) defined list of extra places.

[Alex Gaynor]

That was me :)

[Carl Meyer]

Besides test discovery, the other thing this gives you is a more powerful way to specify which tests to run via commandline, by using real dotted path rather than the pseudo-dotted-path applabel.TestCaseClass.testcasemethod. Right now, if your tests within an app are split into submodules, there is no way to say "run just the tests in this module." You have to run either the entire app's tests, or a single TestCase subclass. With this runner, you can just give the real dotted path to the module.

So we could make this the default for new startprojects without breaking back-compat. The biggest question is how to set TEST_DISCOVERY_ROOT by default. This is a bit of a puzzle, since we currently don't preset any settings with paths (e.g. users have to set TEMPLATE_DIRS and STATICFILES_DIRS themselves), and I'm not really interested in reintroducing any magic "find the project directory" code. I suppose one option would be to have TEST_DISCOVERY_ROOT default to empty, and if it's empty discover tests in all INSTALLED_APPS (but with robust discovery that includes all files matching the test* pattern, including sub-modules, not the "models.py and tests.py only" approach that we have now). Then if someone prefers keeping tests outside of apps, they could set TEST_DISCOVERY_ROOT to their preferred location.

[Carl Meyer]

Replying to anonymous:

IMO this should be the default, entirely replace django's manual test finding with discovery in both all applications as well as in a (setting?) defined list of extra places.

Oh, somehow missed that comment. Yeah, that would be another option, to always do test discovery in all apps, and then have a setting that's a list of additional locations (TEST_DIRS?). The downside of that for me is that I would really like to have a way to disable running reusable app tests, and only run the tests internal to my project (whether they are kept in project-internal apps, or as I tend to prefer, in one tests directory for the whole project). So I'd like to have some (optional) way to say "please _don't_ do test discovery in all INSTALLED_APPS".

[Carl Meyer]

#17366 is related.

[Carl Meyer]

Summary of discussion with Jacob on IRC:

• There's no reason for Django to ship two test runners, and test discovery doesn't bite in production, so the back-compat bar is lower. We should just do what we think is right and document the changes well.
• Ignoring tests in models.py is fine, that's ugly and nobody should be doing it anyway (we do it 36 times in our own test suite, so we'd need to fix that).
• There should be a way to a) discover tests in all INSTALLED_APPS, b) discover tests in a single app, c) discover tests under an arbitrary supplied root path, and d) discover tests rooted in the current directory. These could all be done with flags to manage.py test; e.g. manage.py test discovers based in ., --installed flag does all installed apps, --app=app for a single app, --root=some/path, etc. It would be ok to change the default from "all installed apps" to "discovery based in CWD", if it's easy for people to get back to "all installed apps" with a flag.

Aymeric also suggested a TEST_APPS setting defaulting to INSTALLED_APPS, to set the default set of apps to discover tests in. This is something I've seen a lot of projects in the wild do.

[Jannis Leidel]

Agreed, those points sound excellent. Marking as accepted and changing the subject to follow the direction of this ticket.

[Russell Keith-Magee]

+1 to everything Jacob said, with one caveat: There is a historical reason for looking for tests in models.py -- that's the way you could easily find doctests on any of your model methods. I'm not arguing that this is a reason to include models.py, but given that there was method in the madness of including models.py in the search path in the first place, if we choose to remove models.py, we need to make sure the change is documented.

I'm also interested to know how this would impact on any plans to introduce better support for "suites"; I've had a couple of discussions in recent times, as well as a long running desire to provide a way to separate true unit test (e.g., check that contrib.auth works) from integration tests (e.g., checking that contrib.auth is deployed correctly) from acceptance tests (e.g., testing that contrib.auth works in practice against a browser test suite). I haven't given any specific thought to how this would be implemented, but if we're going to make a change to test discovery, it would be good to know that the idea has been considered, even if we dont' deliver on the actual capability.

[Carl Meyer]

Replying to russellm:

+1 to everything Jacob said, with one caveat: There is a historical reason for looking for tests in models.py -- that's the way you could easily find doctests on any of your model methods. I'm not arguing that this is a reason to include models.py, but given that there was method in the madness of including models.py in the search path in the first place, if we choose to remove models.py, we need to make sure the change is documented.

Certainly I think all changes in behavior, including models.py no longer being included in test discovery, need to be well documented in the release notes (as well as updating the testing documentation to match the new behavior).

I'm also interested to know how this would impact on any plans to introduce better support for "suites"; I've had a couple of discussions in recent times, as well as a long running desire to provide a way to separate true unit test (e.g., check that contrib.auth works) from integration tests (e.g., checking that contrib.auth is deployed correctly) from acceptance tests (e.g., testing that contrib.auth works in practice against a browser test suite). I haven't given any specific thought to how this would be implemented, but if we're going to make a change to test discovery, it would be good to know that the idea has been considered, even if we dont' deliver on the actual capability.

So my best idea of how this could be implemented would be by making use of unittest2 skipIf/skipUnless decorators, in combination with an option to manage.py test to set some arbitrary "flags" that the skip decorators could access. So for instance, a decorator that lets you annotate a test with @skipUnlessFlag("integration"), and python manage.py test --flag=integration. This is just a rough idea with very little research done - I'd want to take a closer look at how e.g. nose and pytest handle test annotations, or if unittest2 provides anything already in this direction that I'm not aware of. Open questions would also include whether it should just be a generic "test annotation/flag" system, or something more specifically targeted to split "unit" vs "integration" tests (which IMO would only make sense if it also came along with some automatic settings-isolation features for the unit tests, or something).

Anyway, I think that's all pretty much orthogonal to test discovery, which is what this patch is about. I'd be opposed to any solution for splitting unit and integration tests that relied on putting the different types of tests in magical locations; that's the only type of solution I can think of that would intersect with the concerns of this ticket.

[Aymeric Augustin]

#13873 might be fixed as a side effect of this ticket.

[Carl Meyer]

Moving a conversation from #17966 that's OT there to this ticket:

Replying to rob:

I find that often the strange edge-cases are caught by running the tests in contrib, which my own tests may not necessarily cover (though this should obviously be rectified, if such a situation is discovered).

For example, the behaviour of custom middleware on login/logout views. I'd feel somewhat uncomfortable excluding Django's tests from the build (is there even a way to do this - or must you specify each of your own apps explicitly on the command line?)

I guess it would be more productive to look at a specific real example of a "strange edge-case" that was caught by a contrib test, as I don't have any such examples in my own experience (while I have seen many cases of spurious failures).

Currently with the built-in test runner there is no automatic way to exclude django.contrib (or other third-party app) tests, you have to manually list the apps you want to test. This can be wrapped up in a script. Once this ticket is fixed, that will change.

[Jannis Leidel]

In case anyone is interested, I've released django-discover-runner which uses unittest2's test discovery: ​https://github.com/jezdez/django-discover-runner

[myusuf3]

open pull request on github.
​https://github.com/django/django/pull/319

[Claude Paroz]

Looks good, but currently if I run python manage.py test django.contrib.messages in my project, no tests are run (currently, the files containing the tests do not match the test*.py pattern). Is this something that we intend to fix soon after this commit, by renaming affected contrib test files?

[Jannis Leidel]

Moving back to accepted as Carl mentioned in the pull request over at Github:

Sorry I haven't done a fuller comment on this sooner. I worked on this some at the DjangoCon sprints and talked it over quite a bit with Russ, Jacob, and others. I have some work locally but haven't pushed it yet since it's not yet in a fully working state. I'll try to clean that work up and get it pushed soon. Specifically the things that IMO are not ready yet with this patch:

@jacobian wanted to avoid adding any new settings with this; things like the test discovery pattern should be set instead as command-line arguments.

If we are deprecating the old runner, it's no good to have the new runner inherit from the deprecated runner; that's just postponing a bunch of work to whoever does the final deprecation removal, deprecation removals should not require big code reorganizations. Instead the new runner needs to be standalone with all required functionality, and the old runner should inherit from the new runner, so when it is removed nothing else is affected.

Docs - I don't think we should commit things without adequate docs and then clean it up later. Writing the docs helps us figure out if we've made mistakes in the API. Docs are a first-class citizen.

The current code does a kind of funny thing where it tries to intelligently decide whether to use discovery or loadTestsFromModule depending what you pass on the command line and what it finds there. I did this because it was functional and what I wanted for an external project, but I'm not sure this approach belongs in Django - this is just going right back down the path of inventing our own unique discovery process. I think we should maybe stick closer to the native behavior of unittest, and discuss with voidspace possible improvements to that behavior. This requires more discussion.

And that's really the key - while I'd like to see this change in Django, it's a significant developer-facing change and I think it's worth taking the time to make sure we do it right. And that means not committing something quick before feature freeze just to get it in, it means plenty of time for consideration and review on django-developers. Unfortunately I didn't have time this summer to drive that, but I'm hoping I (or someone else?) will in the 1.6 cycle.

[Aymeric Augustin]

It seems to me that this overlaps a lot with #6712.

[Aymeric Augustin]

It would be nice to tackle #18727 at the same time.

[jcatalan]

I've been looking at ticket #17366 during PyCon Sprints and it might be good to consider some kind of warning or notification to django users currently having tests in places that will stop being looked at (ie. models.py) with this new discover process.

[Carl Meyer]

Things remaining to do here (at least those I'm thinking of at the moment):

1) Figure out how to use ​http://hg.python.org/cpython/file/f3032825f637/Lib/unittest/loader.py#l187 to replace ​http://hg.python.org/cpython/file/f3032825f637/Lib/unittest/loader.py#l187

2) Replace settings with command-line arguments (match the command-line args used by python -m unittest discover as closely as possible).

3) Documentation (needs to include prominence in release notes).

4) Get the Django test suite running with the new runner.

5) Remove Django's custom doctest stuff (#18727).

[Carl Meyer]

#4 in the above list needs to include fixing all the places Django's own test suite relies on tests being loaded from models.py. And #3 needs to include prominent mention that tests will not be loaded from models.py anymore. This fixes #17366.

[Carl Meyer]

Oops, in (1) above the second link should be ​https://github.com/carljm/django/compare/master...ticket-17365#L1R30 -- our runner should pass its args to discover in all cases, because discover has direct support for dotted paths already. We shouldn't do what discover-test-runner currently does with its args, which is use loadTestsFromNames and then try discovery if that doesn't find any tests.

[Preston Timmons]

I thought I'd give this a shot.

Here's an up-to-date branch based on carljm's one.

​https://github.com/prestontimmons/django/tree/ticket-17365

And the commit:

​https://github.com/prestontimmons/django/commit/68142a4ef14a5bcf7938eaa7279373ebd2de1ca1

I added tests for these cases:

# Discover tests under current directory
python manage.py test

# Test under different root
python manage.py test root="../tests

# Test with top level
python manage.py test --root="../tests --top_level=../../top

# Use different pattern
python manage.py test --pattern="tests_*"

# Test single installed app with discovery
python manage.py test contact

# Test multiple apps with discovery
python manage.py test myapp1 myapp2 myapp3

# Test specific test module
python manage.py test myapp1.tests_views
python manage.py test myapp1.test_models

# Test with mixed arguments
python manage.py test myapp.test_views myapp.test_models

# Test single testcase
python manage.py test contact.tests.ContactTest

# Test single method
python manage.py test contact.tests.ContactTest.test_contact

# Test all installed apps with discovery
python manage.py test --installed

Notes:

In this branch, the django.test.simple.DjangoTestSuiteRunner is still the default. Users have to opt-in to the DiscoverRunner.

This means the old style tests are default unless a user updates their testrunner setting to django.test.runner.DiscoverRunner.

Doctest support:

Is the plan to do a hard cut-off of doctest support?

For instance, the page below introduces the user to both at the start. I started in on some docs, but not sure if I should retrofit the existing docs, or do a deeper restructure.

​https://docs.djangoproject.com/en/dev/topics/testing/

If we do a restructure, it needs to be clear to the user that they must manually update their TEST_RUNNER setting to use the new discovery tests.

Backward-incompatibility

As decided, the new runner won't support doctests or tests in models.py.

A more subtle change is that the discover runner won't run tests specified in an init.py of the tests module, like this:

myapp/
    tests/
        __init__.py 

That won't match a discovery pattern.

The unittest docs cover this in detail here:

​http://docs.python.org/2/library/unittest.html#load-tests-protocol

I don't think it's a big issue, because most times the init.py is only set up to import from other test files. If those other files are named "tests*.py", then they will be found automatically.

It's worth a note in the docs somewhere for those migrating.

Caveats:

I ran into problems with dotted paths to a method on a TestCase.

Whether using django.test.TestCase, or django.utils.unittest.TestCase, this line always returned False.

    issubclass(parent, case.TestCase)

​https://github.com/prestontimmons/django/commit/68142a4ef14a5bcf7938eaa7279373ebd2de1ca1#L4R125

This caused the methods to be called wrong.

For some reason, this fixed it. I don't know the reason why.

    issubclass(parent, unittest.TestCase)

[Preston Timmons]

The latest updates break the Django runtests.py. That's going to take more work.

To make initial testing easier I created a standalone app:

​https://github.com/prestontimmons/discover-runner-tests

It covers most of the fiddly cases we've ran into.

When the implementation of build_suite works with confidence, runtests.py can be tackled.

[Carl Meyer]

The latest work on this is now happening in ​https://github.com/carljm/django/compare/master...ticket-17365-new based on Preston's work.

[Carl Meyer]

I think this pull request is ready to go; needs review and people playing with it: ​https://github.com/django/django/pull/1050

Merging that pull request also fixes #17366 and #18727, and closes as no-longer-relevant #11077, #17032, and #18670.

[Aymeric Augustin]

I reviewed the docs changes and I don't have any significant remarks.

[Carl Meyer <carl@…>]

In 9012833af857e081b515ce760685b157638efcef:

Fixed #17365, #17366, #18727 -- Switched to discovery test runner.

Thanks to Preston Timmons for the bulk of the work on the patch, especially
updating Django's own test suite to comply with the requirements of the new
runner. Thanks also to Jannis Leidel and Mahdi Yusuf for earlier work on the
patch and the discovery runner.

Refs #11077, #17032, and #18670.

[Florian Apolloner <florian@…>]

In e23a5f9a4730ddecb8f3950ee2936716f458c506:

Fixed a regression in the test runner loading of runtests.py.

Refs #17365, #17366, #18727.

[Florian Apolloner <florian@…>]

In 2bf403ecbd958bfb269794b36e61b69f0aede4cf:

Fixed a regression from e23a5f9a4730ddecb8f3950ee2936716f458c506.

Excluded postgis specific gis tests from other spatial databases.

Refs #17365, #17366, #18727.

[Carl Meyer <carl@…>]

In cd79f337233be3f2dfa22316314c9d4834093e31:

Fixed #20503 - Moved doctest utilities in with the rest of the deprecated test code.

The DocTestRunner and OutputChecker were formerly in
django.test.testcases, now they are in django.test.simple. This avoids
triggering the django.test._doctest deprecation message with any import
from django.test. Since these utility classes are undocumented internal
API, they can be moved without a separate deprecation process.

Also removed the deprecation warnings specific to these classes, as they are
now covered by the module-level warning in django.test.simple.

Thanks Anssi for the report.

Refs #17365.

[Tim Graham <timograham@…>]

In bf5430a20b65b3e76a2f8cd2580101e0baa59f82:

Removed django.test.simple and django.test._doctest per deprecation timeline.

refs #17365, #17366, #18727.