Code

Opened 2 years ago

Closed 11 months ago

Last modified 4 weeks ago

#17365 closed New feature (fixed)

Extend test discovery to include unittest2 test suite runner

Reported by: jezdez Owned by: myusuf3
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: tomek@…, streeter, aurelio@…, mike@…, lemaire.adrien@…, slacy@…, kkumler, robgolding63, lrekucki@…, scotty@…, myusuf3, mpessas, luiz.vital@…, mindsocket, hv@…, benkonrath Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Attachments (0)

Change History (51)

comment:1 Changed 2 years ago by jezdez

  • Summary changed from Add discovery test suite runner to Add optional unittest2 discovery test suite runner

comment:2 Changed 2 years ago by oinopion

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

comment:3 Changed 2 years ago by oinopion

  • Cc tomek@… added

comment:4 follow-up: Changed 2 years ago by 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.

comment:5 Changed 2 years ago by Alex

That was me :)

comment:6 Changed 2 years ago by carljm

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.

comment:7 in reply to: ↑ 4 Changed 2 years ago by carljm

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".

comment:8 Changed 2 years ago by carljm

#17366 is related.

comment:9 Changed 2 years ago by carljm

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.

comment:10 Changed 2 years ago by jezdez

  • Summary changed from Add optional unittest2 discovery test suite runner to Extend test discovery to inlcude unittest2 test suite runner
  • Triage Stage changed from Design decision needed to Accepted

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

comment:11 Changed 2 years ago by streeter

  • Cc streeter added

comment:12 follow-up: Changed 2 years ago by 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.

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.

comment:13 Changed 2 years ago by jezdez

  • Summary changed from Extend test discovery to inlcude unittest2 test suite runner to Extend test discovery to include unittest2 test suite runner

comment:14 in reply to: ↑ 12 Changed 2 years ago by carljm

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.

comment:15 Changed 2 years ago by aaugustin

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

comment:16 Changed 2 years ago by aurelio

  • Cc aurelio@… added

comment:17 Changed 2 years ago by carbonXT

  • Cc mike@… added

comment:18 Changed 2 years ago by Fandekasp

  • Cc lemaire.adrien@… added

comment:19 Changed 2 years ago by carljm

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.

comment:20 Changed 2 years ago by slacy

  • Cc slacy@… added

comment:21 Changed 2 years ago by kkumler

  • Cc kkumler added

comment:22 Changed 2 years ago by robgolding63

  • Cc robgolding63 added

comment:23 Changed 2 years ago by jezdez

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

comment:24 Changed 23 months ago by lrekucki

  • Cc lrekucki@… added

comment:25 Changed 22 months ago by scotty@…

  • Cc scotty@… added

comment:26 Changed 20 months ago by myusuf3

  • Cc myusuf3 added
  • Needs documentation set
  • Owner changed from nobody to myusuf3

comment:27 Changed 20 months ago by mpessas

  • Cc mpessas added

comment:28 Changed 20 months ago by luizvital

  • Cc luiz.vital@… added

comment:29 Changed 20 months ago by mindsocket

  • Cc mindsocket added

comment:30 Changed 20 months ago by myusuf3

  • Has patch set
  • Needs documentation unset

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

comment:31 Changed 19 months ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:32 Changed 19 months ago by claudep

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?

comment:33 Changed 19 months ago by jezdez

  • Triage Stage changed from Ready for checkin to Accepted

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.

comment:34 Changed 13 months ago by aaugustin

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

comment:35 Changed 13 months ago by aaugustin

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

comment:36 Changed 13 months ago by 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.

comment:37 Changed 13 months ago by carljm

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).

Version 0, edited 13 months ago by carljm (next)

comment:38 Changed 13 months ago by carljm

#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.

comment:39 Changed 13 months ago by carljm

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.

comment:40 Changed 13 months ago by prestontimmons

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)

comment:41 Changed 13 months ago by prestontimmons

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.

comment:42 Changed 13 months ago by carljm

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

comment:43 Changed 12 months ago by guettli

  • Cc hv@… added

comment:44 Changed 12 months ago by benkonrath

  • Cc benkonrath added

comment:45 Changed 11 months ago by carljm

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.

comment:46 Changed 11 months ago by aaugustin

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

comment:47 Changed 11 months ago by Carl Meyer <carl@…>

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

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.

comment:48 Changed 11 months ago by Florian Apolloner <florian@…>

In e23a5f9a4730ddecb8f3950ee2936716f458c506:

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

Refs #17365, #17366, #18727.

comment:49 Changed 11 months ago by Florian Apolloner <florian@…>

In 2bf403ecbd958bfb269794b36e61b69f0aede4cf:

Fixed a regression from e23a5f9a4730ddecb8f3950ee2936716f458c506.

Excluded postgis specific gis tests from other spatial databases.

Refs #17365, #17366, #18727.

comment:50 Changed 11 months ago by 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.

comment:51 Changed 4 weeks ago by Tim Graham <timograham@…>

In bf5430a20b65b3e76a2f8cd2580101e0baa59f82:

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

refs #17365, #17366, #18727.

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.