#4501 closed New feature (fixed)
Coverage support for tests
Reported by: | 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)
Change History (45)
comment:1 by , 17 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:2 by , 17 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
comment:3 by , 17 years ago
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
Triage Stage: | Design decision needed → Unreviewed |
I'd like to reopen this ticket.
- http://nedbatchelder.com/code/modules/coverage.py appendix C contains licensing info.
- Ned is for integrating his module into Django: http://www.mail-archive.com/django-users@googlegroups.com/msg40221.html
comment:4 by , 17 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:5 by , 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 , 16 years ago
Owner: | changed from | to
---|---|
Status: | reopened → 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 by , 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.
comment:8 by , 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 , 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 , 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 , 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 , 16 years ago
I ran runtests
without any modifications and came up with the following report:
comment:12 by , 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 , 16 years ago
Cc: | added |
---|
comment:14 by , 16 years ago
Keywords: | gsoc added |
---|---|
milestone: | → 1.2 |
Needs documentation: | set |
Needs tests: | set |
Patch needs improvement: | set |
comment:15 by , 15 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 , 15 years ago
Cc: | added |
---|
comment:17 by , 15 years ago
4501-coverage-2.diff is error.
use_coverage = start_coverage(verbosity)
use_coverage is None.
Change:
start_coverage(verbosity)
by , 15 years ago
Attachment: | 4501-coverage-soc.diff added |
---|
Initial cleanup of the summer of code work on this ticket.
comment:18 by , 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 , 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 , 15 years ago
milestone: | 1.2 |
---|---|
Version: | SVN |
comment:21 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:22 by , 13 years ago
Cc: | 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 , 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 , 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.
by , 13 years ago
Attachment: | 4501_v4.diff added |
---|
comment:25 by , 13 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Owner: | changed from | to
Patch needs improvement: | unset |
Status: | new → assigned |
comment:26 by , 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 , 13 years ago
Attachment: | 4501_v5.diff added |
---|
comment:27 by , 13 years ago
Patch needs improvement: | unset |
---|
comment:28 by , 13 years ago
Cc: | added |
---|
comment:29 by , 13 years ago
Cc: | added |
---|
comment:31 by , 13 years ago
Patch needs improvement: | unset |
---|
made a mistake. its late. will continue tomorrow.
comment:32 by , 13 years ago
Patch needs improvement: | set |
---|
the documentation patch applies. the code change patch does not.
comment:33 by , 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.
by , 13 years ago
Attachment: | 4501_v6.diff added |
---|
comment:34 by , 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 , 13 years ago
Cc: | added |
---|
comment:36 by , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:37 by , 12 years ago
Component: | Testing framework → Documentation |
---|
comment:38 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → 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.