Code

Opened 7 years ago

Closed 19 months ago

Last modified 19 months ago

#4501 closed New feature (fixed)

Coverage support for tests

Reported by: bugs@… Owned by: krzysiumed
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 ericholscher 5 years ago.
Initial patch
4501-coverage-2.diff (6.5 KB) - added by ericholscher 5 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 ericholscher 4 years ago.
Initial cleanup of the summer of code work on this ticket.
4501_v4.diff (1.5 KB) - added by krzysiumed 2 years ago.
4501_v5.diff (1.7 KB) - added by krzysiumed 2 years ago.
4501_v6.diff (1.7 KB) - added by krzysiumed 2 years ago.

Download all attachments as: .zip

Change History (45)

comment:1 Changed 7 years ago by mir@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 7 years ago by russellm

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

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 Changed 6 years ago by Marat Radchenko <slonopotamusorama@…>

  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Design decision needed to Unreviewed

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 Changed 6 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 5 years ago by Almad

./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 Changed 5 years ago by ericholscher

  • Owner changed from adrian to ericholscher
  • Status changed from reopened to new

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 Changed 5 years ago by jacob

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.

Changed 5 years ago by ericholscher

Initial patch

comment:8 Changed 5 years ago by ericholscher

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

Changed 5 years ago by ericholscher

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

comment:9 Changed 5 years ago by ericholscher

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 Changed 5 years ago by gsong

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 Changed 5 years ago by gsong

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

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

comment:12 Changed 5 years ago by ericholscher

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 Changed 5 years ago by nedbatchelder

  • Cc ned@… added

comment:14 Changed 5 years ago by ericholscher

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

comment:15 Changed 5 years ago by gsong

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

comment:16 Changed 5 years ago by anonymous

  • Cc chromano@… added

comment:17 Changed 4 years ago by qingfeng

4501-coverage-2.diff is error.

use_coverage = start_coverage(verbosity) 

use_coverage is None.

Change:

start_coverage(verbosity) 

Changed 4 years ago by ericholscher

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

comment:18 Changed 4 years ago by ericholscher

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 Changed 4 years ago by ramiro

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 Changed 4 years ago by russellm

  • milestone 1.2 deleted
  • Version SVN deleted

comment:21 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to New feature

comment:22 Changed 3 years ago by tomchristie

  • 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 Changed 3 years ago by PaulM

  • 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 Changed 3 years ago by julien

  • 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/

Changed 2 years ago by krzysiumed

comment:25 Changed 2 years ago by krzysiumed

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from ericholscher to krzysiumed
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:26 Changed 2 years ago by julien

  • 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!

Changed 2 years ago by krzysiumed

comment:27 Changed 2 years ago by krzysiumed

  • Patch needs improvement unset

comment:28 Changed 2 years ago by krzysiumed

  • Cc krzysiumed@… added

comment:29 Changed 2 years ago by IonelMaries

  • Cc ionel.mc@… added

comment:30 Changed 2 years ago by myusuf3

  • Patch needs improvement set

this patch doesn't apply cleanly.

comment:31 Changed 2 years ago by myusuf3

  • Patch needs improvement unset

made a mistake. its late. will continue tomorrow.

comment:32 Changed 2 years ago by myusuf3

  • Patch needs improvement set

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

comment:33 Changed 2 years ago by krzysiumed

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

Version 0, edited 2 years ago by krzysiumed (next)

Changed 2 years ago by krzysiumed

comment:34 Changed 2 years ago by claudep

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 Changed 2 years ago by kmike

  • Cc kmike84@… added

comment:36 Changed 21 months ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:37 Changed 19 months ago by timo

  • Component changed from Testing framework to Documentation

comment:38 Changed 19 months ago by Tim Graham <timograham@…>

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

In 7ef2781ca0ce48872e21dce2f322c9e4106d1cfd:

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

Thanks krzysiumed for the draft patch.

comment:39 Changed 19 months ago by Tim Graham <timograham@…>

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.

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.