Opened 17 years ago

Closed 12 years ago

Last modified 12 years ago

#4501 closed New feature (fixed)

Coverage support for tests

Reported by: bugs@… Owned by: Christopher Medrela
Component: Documentation Version:
Severity: Normal Keywords:
Cc: ned@…, chromano@…, tom@…, krzysiumed@…, ionel.mc@…, kmike84@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Make {{ python manage.py test --with-coverage }} comand, which will use coverage module to display coverage stats.

Attachments (6)

4501-coverage.diff (2.7 KB ) - added by Eric Holscher 16 years ago.
Initial patch
4501-coverage-2.diff (6.5 KB ) - added by Eric Holscher 16 years ago.
Very hacky, but it seems to work. Just need to whittle this down, maybe.
4501-coverage-soc.diff (26.4 KB ) - added by Eric Holscher 15 years ago.
Initial cleanup of the summer of code work on this ticket.
4501_v4.diff (1.5 KB ) - added by Christopher Medrela 13 years ago.
4501_v5.diff (1.7 KB ) - added by Christopher Medrela 13 years ago.
4501_v6.diff (1.7 KB ) - added by Christopher Medrela 13 years ago.

Download all attachments as: .zip

Change History (45)

comment:1 by mir@…, 17 years ago

Triage Stage: UnreviewedDesign decision needed

comment:2 by Russell Keith-Magee, 17 years ago

Resolution: wontfix
Status: newclosed

I'm going to mark this as wontfix, because it would rely upon the availability of an external module. The licensing for this module is listed as "UNKNOWN", and it isn't entirely Ned's work - it's based on Gareth Rees' original.

If you can clarify the licensing arrangement, and Ned is amenable to the idea of integrating his coverage module with Django (he's a Django user, so he might be), feel free to reopen this ticket.

comment:3 by Marat Radchenko <slonopotamusorama@…>, 17 years ago

Resolution: wontfix
Status: closedreopened
Triage Stage: Design decision neededUnreviewed

I'd like to reopen this ticket.

  1. http://nedbatchelder.com/code/modules/coverage.py appendix C contains licensing info.
  2. Ned is for integrating his module into Django: http://www.mail-archive.com/django-users@googlegroups.com/msg40221.html

comment:4 by Jacob, 17 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Almad, 16 years ago

./manage.py test --with-coverage can be used with [Django: sane testing http://devel.almad.net/trac/django-sane-testing/], for those interested.

comment:6 by Eric Holscher, 16 years ago

Owner: changed from Adrian Holovaty to Eric Holscher
Status: reopenednew

Marking this as assigned to me. I have some code that does this externally. If this can be done in a custom test runner, and requires a third party module; is there a reason that this shouldn't just be third party?

Jacob marked it accepted, so I assume this would go in core with a test import on coverage.py, and if it is there it would be enabled with a flag.

comment:7 by Jacob, 16 years ago

I'd like to see this in core myself for the same reason we have a test suite in core: anything we do to make testing easier is good for Django users. Selfishly, I don't want to have to manage an external dependancy just to generate coverage.

Eric, if you're going to work on this, please make sure that it can generate coverage both for Django itself -- as part of runtests.py -- and for someone's projects, ignoring Django core and the stdlib.

by Eric Holscher, 16 years ago

Attachment: 4501-coverage.diff added

Initial patch

comment:8 by Eric Holscher, 16 years ago

I attached an initial patch that implements the functionality in the ./manage.py test runner. I have run into a couple of design decisiony things that I want to get feedback on while trying to create this patch.

First off: Making it work for runtests.py and ./manage.py test means that logically I would want to put this code in Django's simple test runner. In order to do this, I would want to make a use_coverage argument to the run_tests() command. However, this is currently an exposed API, where people can write their own test runners. So if we try and pass another argument to it and someone is using a third party test runner, it will break. So that is out of the question.

So the next logical thing to do would be to put the code into runtests.py and ./manage.py test, around the calls to run_tests, and then print out the results afterwards. This approach seems to make sense, and means that the common code should probably be moved out into a utils module some where. I threw a start_coverage and stop_coverage function into django.test.utils.

This is a basic hashing out of what would eventually need to be committed. The API is currently ugly (start is passed verbosity so it knows when to print an error, this should probably just raise an exception and be handled in the client code with a try block). Stop is stopping (one line), and then printing out results. However, I just threw this together to see if this is the right direction to go in.

Also of note, running the entire test suite with coverage.py takes a considerably longer amount of time than without. This makes sense, but hopefully there are some ways to speed things up. Without coverage enabled, the tests were taking around 280 seconds. With coverage enabled they took around 1400 seconds. Everything did pass though, it just took a long time. Presumably one won't be running with coverage enabled often, so this performance hit is manageable. I will be looking at ways to shave some time off though for sure.

SQLAlchemy has an implementation of similar code here: http://github.com/brosner/sqlalchemy/blob/a03faa1575bc3e1a56a93e0deb0f6ea7988c0491/test/testlib/config.py#L80

by Eric Holscher, 16 years ago

Attachment: 4501-coverage-2.diff added

Very hacky, but it seems to work. Just need to whittle this down, maybe.

comment:9 by Eric Holscher, 16 years ago

Looks like I need to be following doing the os.walk stuff inside of all the models/apps that I'm pulling in, and ignoring the tests actual code. I got some output that was interesting, but lacking a lot of detail that I needed. http://dpaste.com/5089/ I think I understand where I need to go from here, basically just making sure that I include all of the files inside an app (and not just the models.py, which is the "class"). I also don't need to be importing Django's test cases, because we're trying to get the coverage of the code, and not the tests.

I'll take another swing at this later this week.

comment:10 by George Song, 16 years ago

Hi Eric, I was wondering what your progress is on this ticket. I didn't realize this was an open ticket and did a lot of work around coverage over the weekend. In any case, here's actual HTML output from my tests, is this kind of what we're looking for? My test runner generates the simple plain text output to sys.stdout as well, but I didn't find that too useful.

http://lehrhaus.55minutes.com/test_report/

In any case, I have the following parameters in my settings:

# Specify which style you want the listing to be in
# 'path' or 'module'
# 'path': '/myhome/project/package/module'
# 'module': 'package.module'
COVERAGE_TEST_LIST_STYLE = 'module'

# Specify regular expressions of code blocks you would like to ignore
# in the coverage test
COVERAGE_TEST_EXCLUDES = (
    'def __unicode__\(self\):', 'def get_absolute_url\(self\):',
    )
    
# Specify regular expressions of paths to exclude
COVERAGE_TEST_PATH_BLACKLIST = (
    r'django%sdjango' %re.escape(os.path.sep),
    r'.svn', 'tests', 'settings', '__init__', 'urls',
    'common.*views.*test',
    )

COVERAGE_TEST_HTML_OUTPUT_DIR = 'test_html'

I'm ignoring core Django modules, but can obviously include them if we wanted to. I'm not that familiar with runtests so I didn't bother looking at that at all. I approached this from the perspective of the application developer, not the framework developer. My idea was to package it up as a third-party app with a custom manage.py command.

comment:11 by George Song, 16 years ago

I ran runtests without any modifications and came up with the following report:

http://test.55minutes.com/test_report/

comment:12 by Eric Holscher, 16 years ago

gsong: Neat! Thanks for sharing. Bonus points for the pretty output :)

If you could post a patch or a link to a third party place where this code is hosted, that would be awesome. It looks like your code is doing things correctly. I haven't had time to come back to this ticket. My patch on it is lacking, and your approach is looks better.

Releasing it as a third party package probably makes sense since the 1.2 release is a long ways off, but hopefully we can work on getting it in for that release as well.

comment:13 by Ned Batchelder, 16 years ago

Cc: ned@… added

comment:14 by Eric Holscher, 16 years ago

Keywords: gsoc added
milestone: 1.2
Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:15 by George Song, 16 years ago

In the meantime, I've released my Django test coverage app as open source: https://www.ohloh.net/p/django-test-coverage

comment:16 by anonymous, 15 years ago

Cc: chromano@… added

comment:17 by QingFeng, 15 years ago

4501-coverage-2.diff is error.

use_coverage = start_coverage(verbosity) 

use_coverage is None.

Change:

start_coverage(verbosity) 

by Eric Holscher, 15 years ago

Attachment: 4501-coverage-soc.diff added

Initial cleanup of the summer of code work on this ticket.

comment:18 by Eric Holscher, 15 years ago

This approach has a couple of major points that need to be discussed before merging.

First off, it turns the Default test runner into a class instead of a function. This class then has the run_tests function on it. It would be easy enough to not replace the default runner, and keep that as a function. The different coverage runners could also be turned into functions, however that would be a lot hackier, and in that case I think that having it be class based is a big win. It also allows people to override it and make a custom runner if they want, ex. for figleaf.

The other big point of contention is the way that modules are collected. If you just run coverage.report() without passing in the modules to include and exclude, you get an ugly traceback including core python. The module_utils that are included here strike me as way too magical, and should probably not go into core. That said, they do currently work for what we need them to do.

Looking at how nose does this is probably what we want to copy, and it looks easy enough:

http://somethingaboutorange.com/mrl/projects/nose/0.11.1/plugins/cover.html

I'll try and get this working today.

comment:19 by Ramiro Morales, 15 years ago

FYI r12255 introduced a class-based test runner that will be available in 1.2. maybe the code in the last patch can be adapted to the new structure and facilities?

comment:20 by Russell Keith-Magee, 15 years ago

milestone: 1.2
Version: SVN

comment:21 by Gabriel Hurley, 14 years ago

Severity: Normal
Type: New feature

comment:22 by Tom Christie, 13 years ago

Cc: tom@… added
Easy pickings: unset
UI/UX: unset

Are we still interested in getting this in? I might try to find some time to work on improving the patch if so.

comment:23 by Paul McMillan, 13 years ago

Keywords: gsoc removed

Yes we're absolutely still interested in this. With the dropping of 2.4, addition of Unittest2, and the removal of doctests from the Django test suite, this should be much easier to do now. There's a lot of history behind this ticket, so do at least skim that to get a sense of the magnitude of the task.

comment:24 by Julien Phalip, 13 years ago

Has patch: set

I've discussed about this with ericholsher, PaulM and russellm at DjangoCon. We all agree that code coverage is important and that developers should be encouraged to use it in their projects.

I am, however, a bit wary of adding a lot of code in Django core to "support" coverage.py. Adding several settings to map the various functionalities of coverage.py seems a bit overkill. Especially considering that it is already possible to put all those settings in a separate configuration file [1]. It seems that "manage.py test --with-coverage" would add very little value over "coverage run manage.py test".

So, instead, my preference would be to add some documentation in the testing docs [2] strongly promoting the use of coverage.py.

See also #16817, which is discussing the addition of docs about code coverage with the Django test suite itself.

[1] http://nedbatchelder.com/code/coverage/config.html

[2] https://docs.djangoproject.com/en/dev/topics/testing/

by Christopher Medrela, 13 years ago

Attachment: 4501_v4.diff added

comment:25 by Christopher Medrela, 13 years ago

Needs documentation: unset
Needs tests: unset
Owner: changed from Eric Holscher to Christopher Medrela
Patch needs improvement: unset
Status: newassigned

comment:26 by Julien Phalip, 13 years ago

Patch needs improvement: set

Thanks for the patch! It looks good, although a few improvements could be made, in particular:

  • start with a short (a sentence or two) explanation of what test coverage actually is, so that newbies understand why it's important. Links to more detailed explanations (wikipedia?) could also be provided.
  • use named commands (coverage run, coverage report, etc).
  • mention the --source flag on 'coverage run' to only collect stats on your app(s).

Cheers!

by Christopher Medrela, 13 years ago

Attachment: 4501_v5.diff added

comment:27 by Christopher Medrela, 13 years ago

Patch needs improvement: unset

comment:28 by Christopher Medrela, 13 years ago

Cc: krzysiumed@… added

comment:29 by Ionel Cristian Maries, 13 years ago

Cc: ionel.mc@… added

comment:30 by myusuf3, 13 years ago

Patch needs improvement: set

this patch doesn't apply cleanly.

comment:31 by myusuf3, 13 years ago

Patch needs improvement: unset

made a mistake. its late. will continue tomorrow.

comment:32 by myusuf3, 13 years ago

Patch needs improvement: set

the documentation patch applies. the code change patch does not.

comment:33 by Christopher Medrela, 13 years ago

Patch needs improvement: unset

I noticed small grammar mistake in my patch ('append' instead of 'appended'). I attached new patch (file 4501_v6.diff).

myusuf3, I don't understand. There is only one patch (now it's 4501_v6.diff) that should be included into trunk and this patch applies cleanly.

Btw can we include this patch into django 1.4? This ticket actually is not new feature - it's small documentation improvement.

Last edited 13 years ago by Christopher Medrela (previous) (diff)

by Christopher Medrela, 13 years ago

Attachment: 4501_v6.diff added

comment:34 by Claude Paroz, 13 years ago

I think it should still be proofread by a native speaker.

Even if it misses the 1.4 release target, I think we can always backport it later.

comment:35 by Mikhail Korobov, 13 years ago

Cc: kmike84@… added

comment:36 by Jannis Leidel, 12 years ago

Triage Stage: AcceptedReady for checkin

comment:37 by Tim Graham, 12 years ago

Component: Testing frameworkDocumentation

comment:38 by Tim Graham <timograham@…>, 12 years ago

Resolution: fixed
Status: assignedclosed

In 7ef2781ca0ce48872e21dce2f322c9e4106d1cfd:

Fixed #4501 - Documented how to use coverage.py with Django tests.

Thanks krzysiumed for the draft patch.

comment:39 by Tim Graham <timograham@…>, 12 years ago

In 1be0515fe9940a7a727680a775395fe5f0c12b1d:

[1.4.x] Fixed #4501 - Documented how to use coverage.py with Django tests.

Thanks krzysiumed for the draft patch.

Backport of 7ef2781ca0ce48872e21dce2f322c9e4106d1cfd from master.

Note: See TracTickets for help on using tickets.
Back to Top