Skip to content

Conversation

@real-or-random
Copy link
Contributor

Related to #704.

We could also additionally use something like https://codecov.io/ to get a nice report but I'm not sure if that's worth it.

@real-or-random real-or-random force-pushed the coverage branch 3 times, most recently from cffb9a4 to 5b935eb Compare January 3, 2020 18:47
Also make `make clean` and `.gitignore` aware of gcov.
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Adding coverage reports to CI is a good idea. I don't think the summary alone is enough in code review. If someone PRs a change I would like to check if all lines and all branches of the new code are taken. The summary only shows a number of lines (and branches with -b) and waht percentage is exec'd. Even if the numbers stay constant with respect to master that does not mean that the new code is fully tested.

Would it be possible to have travis provide the html coverage report or push it somewhere (github pages, ...)? On the other hand I have no experience with codecov.io, but I have a slight preference for gcov because it works and it does not require a third party.

Fwiw with f8ca924 travis either shows gcovr not found or shows an empty code coverage report.

@gmaxwell
Copy link
Contributor

gmaxwell commented Jan 6, 2020

because it works and it does not require a third party.

You can also use it locally in an edit/compile/test loop.

On a real CI system it's easy to have the auto builds make their work product available, e.g. https://mf4.xiph.org/jenkins/job/opus-coverage/ws/coverage/index.html (and as I link this, I see someone broke branch coverage reporting in that project, presumably due to applying an os upgrade that upgraded lcov, sad. lol)

But at least previously travis was super anal about not making work product from CI builds easily available-- presumably because they didn't want to get turned into free web hosting. I dunno if that has changed.

@real-or-random
Copy link
Contributor Author

real-or-random commented Jan 6, 2020

Adding coverage reports to CI is a good idea. I don't think the summary alone is enough in code review. If someone PRs a change I would like to check if all lines and all branches of the new code are taken. The summary only shows a number of lines (and branches with -b) and waht percentage is exec'd. Even if the numbers stay constant with respect to master that does not mean that the new code is fully tested.

True, lines would help a lot more. I thought about running this locally but you're right. For the summary alone, we don't need to setup travis, that's useless.

Would it be possible to have travis provide the html coverage report or push it somewhere (github pages, ...)? On the other hand I have no experience with codecov.io, but I have a slight preference for gcov because it works and it does not require a third party.

As Greg points out that's hard with Travis. With tools like codevoc.io, this is easier:
https://docs.codecov.io/docs/viewing-source-code
Or using coveralls.io:
https://coveralls.io/builds/1918/source?filename=lib%2Fcoveralls.rb

Third parties: Yes, I think that's like with Travis. It's convenient but we should not rely on it or trust it. My concern is rather that we prefer to keep things simple, and just running gcovr is indeed simpler.
And we don't to add tools that disappear in 3 months and we need to redo the work. I'm not sure to be honest. I think I'll can spend some time playing with codecov.io and see how it feels.

Fwiw with f8ca924 travis either shows gcovr not found or shows an empty code coverage report.

Yep, I need to fix this.

@pmconrad
Copy link

pmconrad commented Jan 6, 2020

But at least previously travis was super anal about not making work product from CI builds easily available-- presumably because they didn't want to get turned into free web hosting.

For the specific case of code analysis (including test coverage), travis supports SonarQube for example.

@gmaxwell
Copy link
Contributor

gmaxwell commented Jan 7, 2020

@real-or-random I think have a solution to the travis output problem. You can have secret credentials in travis: https://docs.travis-ci.com/user/encryption-keys/

So travis CI could be setup to be able to submit work-product output to an external host. As in ... just scp the gcovr html output to somehost:public_html/libsecp256k1/coverage/pr1234/.

I don't think this is a replacement for a real CI setup that is able to do benchmarking or spend non-negligible amounts of time testing (e.g. running serious fuzz tests) --- but it would be fairly low effort to setup. (and if it breaks you just stop getting ci reports from the PR builds, which isn't the end of the world).

@gmaxwell
Copy link
Contributor

The coverage report should exclude tests.c or otherwise it will always report very low figures (test failures), which will obscure real reductions in branch coverage (which should be very close to 100%).

@real-or-random
Copy link
Contributor Author

Closing in favor of #954.

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.

4 participants