Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding test coverage #705

Closed
wants to merge 5 commits into from
Closed

Conversation

sumedhghaisas
Copy link
Member

The test coverage is not complete though the basic framework is ready.

Completed things are -

  • Using gcov and lcov to calculate test coverage
  • Added cmake option BUILD_WITH_COVERAGE to build mlpack with appropriate GCC flags
  • Added cmake target mlpack_coverage to generate coverage with all tests
  • bash script 'mlpack_coverage' will be generated in build folder which takes test suite as parameter('-t') for generate coverage for specific test
  • Coverage result will be available in build/coverage/index.html

TODO list:

  1. Coverage results get accumulated with every run. We need to make sure the previous result gets deleted for every run.
  2. Where to show these results? and how to upload it automatically?
  3. Some hacks are needed for templated functions and classes.

Opening this pull request for discussion.

* Using gcov and lcov to calculate test coverage
* Added as cmake cmake option BUILD_WITH_COVERAGE
* Added cmake target mlpack_coverage
@sumedhghaisas sumedhghaisas self-assigned this Jun 24, 2016
@sumedhghaisas
Copy link
Member Author

Adding coverage results of my latest run. Line coverage: 88.9%, functional coverage: 93.3%
@rcurtin I have not seen such good coverage even in corporate world :)

coverage.tar.gz

@zoq
Copy link
Member

zoq commented Jun 24, 2016

Interesting idea, I'm not sure if this works, however, do you think we could use coveralls.io for this. I think that would make things a lot easier and probably less complex. It basically comes down to:

  1. Build with BUILD_WITH_COVERAGE=ON
  2. Add - coveralls ... to the travis config.

@sumedhghaisas
Copy link
Member Author

sumedhghaisas commented Jun 26, 2016

I have improved the coverage to accommodate the inline functions. @zoq Yes coveralls.io is a good idea but I thought its just for posting coverage. But I will take a look at it. Adding improved coverage - the values have decreased a little but the coverage calculation is improved :)
Uploading coverage.tar.gz…

@sumedhghaisas
Copy link
Member Author

I found this on coveralls.io documentation for C/C++ code. https://github.com/eddyxu/cpp-coveralls . It also suggests gcov for coverage calculation.

@zoq
Copy link
Member

zoq commented Jun 26, 2016

So, we could build with BUILD_WITH_COVERAGE=ON, as you already suggested, and avoid all the steps in mlpack_coverage.in by using the cpp-coveralls script?

@sumedhghaisas
Copy link
Member Author

I think cpp-coveralls script is for uploading the result to the coveralls server. Their instructions says -

  1. Build project with gcov support
  2. Run tests
  3. upload

@sumedhghaisas
Copy link
Member Author

@zoq Ohh I looked more into cpp-coveralls. I was wrong. It also runs gcov internally and uploads. So we need to build library with gcov support and run coveralls. But I think we also should keep the mlpack_coverage script, so as to generate the report locally. Coveralls with dryrun only generates .gcov files, the visual view is only available online, but there might be a situation where we need to see the results locally. Currently I am trying to configure cpp-coveralls for mlpack.

* lcov will be used for local coverage computation.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3d34028 on sumedhghaisas:test_coverage into * on mlpack:master*.

@sumedhghaisas sumedhghaisas force-pushed the test_coverage branch 2 times, most recently from b452aa1 to d8da0a2 Compare June 29, 2016 19:29
@rcurtin
Copy link
Member

rcurtin commented Jun 29, 2016

Hm, I like this idea, it could be nice to add.

It seems to me like it should not be necessary to keep the mlpack_coverage.in script (maybe we can move it to jenkins-conf?); I am not sure of situations where users will want to check test coverage themselves, and if they do, we can make it a little more complex (i.e. "download mlpack_coverage.in, then run it").

It looks like coveralls is not displaying correct results quite yet, I will take another look and dig around when that is working. :)

- sudo apt-get install lcov
- sudo apt-get install ruby
- sudo gem install coveralls-lcov
- printf "Y\n" > apt_input.txt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line shouldn't be necessary anymore. :) You can also use apt-get -y if you need, or even yes | apt-get install ....

@rcurtin
Copy link
Member

rcurtin commented Jun 29, 2016

Another thought is, we can set up a Jenkins job specifically to do coverage testing, to avoid making Travis do too much. Compiling with debugging symbols will make the tests take longer and might put us over the 1hr time limit. But there is no limit for Jenkins, so we can just run on Jenkins and submit those results to coveralls.io, I think. I am not 100% clear on how the service works at this time.

@sumedhghaisas
Copy link
Member Author

sumedhghaisas commented Jun 29, 2016

Currently the problem is with gcov version. Gcc 4.8 does not work with gcov 4.6. I am trying to upgrade the gcov version. The problem with cpp-coveralls is, it is not getting the header file coverage correct. I don;t know if its a known issue. I also looked at the shogun coverage, as they also use cpp-coveralls. It seems they are just ignoring the header files. I don;t think thats a good idea. Its better to use mlpack_coverage.sh with coveralls-lcov which can upload the lcov info file to coveralls. I like the jenkins idea, but there is no harm in keeping mlpack_coverage in cmake configuration. Its not for users but for developers, like a developer can test the coverage for his specific module before sending in the PR.

@sumedhghaisas sumedhghaisas force-pushed the test_coverage branch 4 times, most recently from 4a616a0 to 92c63e9 Compare June 30, 2016 08:45
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 92c63e9 on sumedhghaisas:test_coverage into * on mlpack:master*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants