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.

Sign up for GitHub

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

Jump to bottom

Add codecov reporting to travis #4538

Closed
wants to merge 2 commits into from
Closed

Add codecov reporting to travis #4538

wants to merge 2 commits into from

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Jun 9, 2017

This is currently WIP. Depends on jhass/crystal-build-docker#2. Doesn't currently work on OSX. Displays loads of errors to the build log.

This PR uses the kcov tool to provide coverage reports on crystal code. Example output here. I think there are bugs with the crystal debug line information, for example this empty line of documentation got 37 hits. I've seen similar impossible debug information when profiling using callgrind + kcachegrind so I suspect the issue is with crystal not kcov. I will shortly create an issue in the kcov repo reporting the ld errors in the build log.

RX14
RX14 commented Jun 9, 2017
View reviewed changes
bin/ci
@@ -147,7 +149,7 @@ EOF
prepare_build() {
on_linux verify_linux_environment

on_linux docker pull "jhass/crystal-build-$ARCH"
on_linux docker pull "rx14/crystal-build-$ARCH"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: get jhass/crystal-build-docker#2 merged and move back to @jhass's image.

RX14
RX14 commented Jun 9, 2017
View reviewed changes
bin/ci Outdated
with_build_env 'make crystal spec doc'
with_build_env 'make crystal doc'
on_linux with_build_env 'make spec coverage_output=.build/coverage'
on_osx with_build_env 'make spec'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Installing kcov on osx is pretty hard and would require compiling kcov every build. Is it worth it when the coverage on linux is likely to be the same? I think so as kcov is actually a really useful test of the debug information's accuracy.

@RX14
Copy link
Contributor Author

RX14 commented Jun 9, 2017

This PR will likely help #3801 once the coverage data is correct.

@bcardiff
Copy link
Member

bcardiff commented Jun 9, 2017

I like this, but would be better to decouple things a bit and use a Docker image to run kcov over a binary which is built on jhass docker image? I wouldn't expect changes in the Makefile to achieve this kind of reports.

@RX14
Copy link
Contributor Author

RX14 commented Jun 9, 2017 •
edited
Loading

@bcardiff running coverage reports is surely something someone does locally as well as on travis. kcov produces some great HTML reports so I see no reason not to include the functionality in the makefile. Besides, bin/ci would get ugly as it would have to build the all_spec binary using the makefile then run it seperately in a container. On top of that the binary is dynamically linked so we'd need to install the correct library files into the new container. Overall I think the current approach is the best.

@RX14 RX14 force-pushed the feature/travis-coverage branch from dfa9cf9 to 8cd1acb Compare August 20, 2017 15:57
@RX14
Copy link
Contributor Author

RX14 commented Aug 20, 2017

Any progress on this? I've just rebased it on master since e811cf9 broke it.

@straight-shoota
Copy link
Member

Do we want to proceed with this? This PR hasn't been touched in 2,5 years.
I tried building coverage report with kcov and it didn't show anything. Not sure if I did something wrong (but according to this PR there shouldn't be much to it) or if it just broke somewhere in between.

In any case, I figure we can close this because it would probably be easier to start anew.

@RX14
Copy link
Contributor Author

RX14 commented Feb 18, 2020

I think with the debug location fixes in #8538 this PR will be viable again.

This is a good stress test, anyway.

@RX14
Copy link
Contributor Author

RX14 commented Feb 19, 2020

We're actually doing pretty well!

@asterite
Copy link
Member

@RX14 Wow, that's really nice!

I wonder if then we could get spec to include a way to generate a coverage. That would really be awesome.

@RX14
Copy link
Contributor Author

RX14 commented Feb 19, 2020

I wonder if then we could get spec to include a way to generate a coverage. That would really be awesome.

That would be doable, though a generic --wrapper-program="kcov --include-path=src .build/coverage" would be my preference.

I'm not sure if we want to "bless" kcov, though i'm fine with it.

@RX14
Copy link
Contributor Author

RX14 commented Feb 19, 2020

So, do we want to move forward with this PR? 👍/👎?

@RX14 RX14 mentioned this pull request Mar 24, 2020
Merged
@straight-shoota straight-shoota added pr:needs-work A PR requires modifications by the author. topic:infrastructure labels Apr 8, 2020
@j8r
Copy link
Contributor

j8r commented Jan 3, 2021

For information, Travis CI is no longer used (#10078).
The coverage can still be added to GitHub Actions and Circle CI.

@straight-shoota
Copy link
Member

Closing. PRs for adding code coverage to our current CI setup would be welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
pr:needs-work A PR requires modifications by the author. topic:infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RX14 @bcardiff @straight-shoota @asterite @j8r