[cmake-developers] Support for coverage.py coverage on the topic stage

[Patrick Reynolds]

Eike,

This is fantastic feedback. Thanks!

I've responded to specific issues inline.

TL;DR: I fixed a lot of these points. This should be closer to merge-ability.

-- 
Patrick Reynolds
Technical Leader
Kitware, Inc.
919 869 8848



On Saturday, September 28, 2013 at 5:36 AM, Rolf Eike Beer wrote:

> Am Freitag, 27. September 2013, 11:30:52 schrieb Patrick Reynolds:
> > Hi folks,
> > 
> > I just pushed a topic to the stage that adds support for coverage.py python
> > coverage. It actually should work for anything the exports cobertura
> > coverage.xml files.
> > 
> > The topic is here:
> > 
> > http://public.kitware.com/gitweb?p=stage/cmake.git;a=shortlog;h=refs/heads/A
> > dd-coverage.py-Coverage
> > 
> 
> 
> You are doomed! Now that you want to introduce a new coverage handler I'll 
> complain as long until you add a testcase for it, i.e. putting a sample output 
> file in the Tests/ directory and verify that CTest will generate the expected 
> results ;)
> 
> 
> 

Challenge accepted! The test is added and I pushed the updated branch:

http://public.kitware.com/gitweb?p=stage/cmake.git;a=shortlog;h=refs/heads/Add-coverage.py-Coverage
> 
> But there are some things about your code and even about older code. First, 
> you do not set error from cont.error in 
> cmCTestCoverageHandler::ProcessHandler().
> 
> 
> 

Fixed on the aforementioned branch. 
> 
> Second, this whole gathering code there will badly fail once 2 coverage 
> systems are present and the second one gives an error. Say gcov finds 2 files. 
> Then python has an error, returns -1. The file count is 1, no error is 
> propagated. This is not your fault, but please fix this in one commit first 
> before adding your new handler. And maybe make this block from gcov to mumps 
> somehow use an array, vector, list, or something and just iterate over it, so 
> you only have to add your handler into the array.
> 
> 
> 

Hmmm. I agree with you on this one; however, I think this may be beyond the scope of my contribution. If it's okay with the community, I would like to add a bug in Mantis for this overall and treat it as a separate topic. 
> 
> Then I'm very unhappy with the coverage database being in the source 
> directory. One setup I daily use at work is having one checkout and run 2 
> builds from it with different compilers. In your case the coverage results will 
> mix. Even worse, they will always sum up, because usually noone deletes a file 
> from the source directory before running a test.
> 
> 
> 

Fixed on the aforementioned branch, again. :)
> 
> Another important question: can python handle the situation where 2 test 
> programs run in parallel and want to write to the coverage file? Or will they 
> just happily trash each others results? That's a problem I fixed in 2.8.12 
> which would have happened for all other coverage generators.
> 
> 
> 

I have not verified this experimentally, but coverage.py claims to support this through there '-p', parallel option. 
> 
> Another thing totally unrelated to your code I see when looking at the code is 
> that the Bullseye coverage handling seems broken:
> 
> int cmCTestCoverageHandler::HandleBullseyeCoverage(
> cmCTestCoverageHandlerContainer* cont)
> {
> const char* covfile = cmSystemTools::GetEnv("COVFILE");
> if(!covfile || strlen(covfile) == 0)
> {
> cmCTestLog(this->CTest, HANDLER_VERBOSE_OUTPUT,
> " COVFILE environment variable not found, not running "
> " bullseye\n");
> return 0;
> 
> /// nothing happened, return 0
> 
> }
> cmCTestLog(this->CTest, HANDLER_VERBOSE_OUTPUT,
> " run covsrc with COVFILE=["
> << covfile
> << "]" << std::endl);
> if(!this->RunBullseyeSourceSummary(cont))
> {
> cmCTestLog(this->CTest, ERROR_MESSAGE,
> "Error running bullseye summary.\n");
> 
> 
> //// error case, return 0
> 
> return 0;
> }
> cmCTestLog(this->CTest, DEBUG, "HandleBullseyeCoverage return 1 "
> << std::endl);
> 
> //// default, probably everything fine: return 1
> 
> return 1;
> }
> 
> 
> if(this->HandleBullseyeCoverage(&cont))
> {
> return cont.Error;
> }
> 
> Break if everything is fine, continue on error or when no files are found? All 
> other handlers support being run in parallel, why not Bullseye?
> 
> 
> 

I'll leave this one to folks who are better versed in the arcane art of Bullseye...
> 
> Eike
> --
> 
> Powered by www.kitware.com (http://www.kitware.com)
> 
> Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html
> 
> Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ
> 
> Follow this link to subscribe/unsubscribe:
> http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
> 
> 
> 


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20130929/e7fbeef6/attachment.html>