Code review comment for lp:~mpontillo/maas/code-coverage

Revision history for this message
Mike Pontillo (mpontillo) wrote :

Raphael, I like including the tests in the report for the reason you mention, but agree it would be nice to hide them, since they get in the way (and give a false impression of coverage percentage, assuming they all run!). I don't really like the sorting in the report; the test directories are sorted and appear inline with the code under test.

My original thought was to create a "coverage" target that did the same thing as the "test" target, but my Makefile skills are rusty, so I couldn't figure out how to do it without duplicating code in the Makefile. Then I considered a couple of use cases:

(1) You might want to run coverage manually, then invoke the 'make' targets to create the report
(2) You might want to run a coverage report even if the tests fail, so that you can see which lines of code were executed.

For those reasons, I didn't make the coverage targets depend on the 'test' target. But I think there's a good argument to be made for doing so, and perhaps we should create helper scripts for running the tests, and invoke *those* from the Makefile, rather than adding individual commands to the Makefile.

Nice catch on the dependencies. I suppose I must have had those packages installed already. I don't know why they aren't dependencies of python-coverage; seems like that was overlooked when that was packaged..?

Gavin, thanks for your patch! I applied and pushed it into the review branch. I'll test out your changes.

« Back to merge proposal