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

Switch coverage from gcov to lcov #1000

Closed
htuch opened this issue May 19, 2017 · 10 comments
Closed

Switch coverage from gcov to lcov #1000

htuch opened this issue May 19, 2017 · 10 comments
Labels
area/build stale stalebot believes this issue/PR has not been touched recently

Comments

@htuch
Copy link
Member

htuch commented May 19, 2017

We need this for two reasons:

  1. clang coverage.

  2. Excluding lines such as NOT_IMPLEMENTED and NOT_REACHED.

We know lcov is really slow from bazelbuild/bazel#1118, but since we use a single test binary it shouldn't be too bad, maybe a couple of minutes for the test (vs. 30 mins + when running with all the tests producing individual lcov reports).

@mattklein123
Copy link
Member

Sweet. This would also resolve #858

@mattklein123 mattklein123 modified the milestone: 1.4.0 May 19, 2017
@htuch
Copy link
Member Author

htuch commented May 19, 2017

Now we just need to find someone enthusiastic enough to enter the Bazel coverage vortex of doom. Count me in as a P2 if there is no such volunteer.

@htuch
Copy link
Member Author

htuch commented May 19, 2017

This would also probably have the nice side effect that we wouldn't need to be forcing gcovr 3.3, which isn't standard in Ubuntu 14.04.

@mattklein123
Copy link
Member

Frankly, I think someone from the Bazel team should enter their own vortex of doom and deal with this so they can "eat their own dogfood."

@htuch
Copy link
Member Author

htuch commented May 21, 2017

So, one issue we have today in a non-CI setting is that the clang runs actually fail silently, with the only symptom a 0.00 coverage results and empty coverage HTML for the Envoy sources. It looks like under the hood that gcov segfaults, gcovr hides it. I'm going to make some doc and add some run_envoy_bazel_coverage.sh safeguards to deal with this. @rlazarus @hennna.

htuch added a commit to htuch/envoy that referenced this issue May 21, 2017
clang isn't compatible with gcov (see envoyproxy#858). We plan to switch to lcov in the future (see envoyproxy#1000).
For now, just make sure we fail visibly and document.
htuch added a commit that referenced this issue May 22, 2017
clang isn't compatible with gcov (see #858). We plan to switch to lcov in the future (see #1000).
For now, just make sure we fail visibly and document.
@htuch htuch assigned alyssawilk and unassigned htuch Aug 3, 2017
@mattklein123 mattklein123 modified the milestones: 1.4.0, 1.5.0 Aug 4, 2017
@alyssawilk
Copy link
Contributor

I have a build with have lcov "working" (horribly hackery generating HTML reports) but it currently claims we don't have coverage for a whole bunch of trailing braces despite a number of attempts to minimize code optimization. As far I can tell is a known issue with clang and lcov and you "fix" by using coverage annotations to functionally comment out all of the optimized end braces

We could go and dehackify my scripts and // LCOV_EXCL_LINE a bunch of end braces, or we can stick with gcov. I lean towards the latter because while a one-off LCOV_EXCL_LINE sweep isn't bad, the annoyance of dealing with it for incremental code changes makes me want to stick with gcov until lcov is more aggressive about identifying these correctly.

@mattklein123 thoughts?

@mattklein123
Copy link
Member

@alyssawilk yeah I agree. Now that we have the ability to easily generate coverage for people as part of CI, it seems this is less urgent. Perhaps we can just leave this issues open for tracking and wait for clang/lcov/bazel to fix this properly?

@alyssawilk
Copy link
Contributor

SGTM.

My horrible horrible horrible hacks documented for posterity over in #1800

Basically:
*comment out all the tests that crashed coverage runs
*create a docker image with lcov installed. I created a local one and tagged the image ID 'latest'
I suggest just adding it upstream and rolling back to avoid the extra docker hassle.
*edit bazel/envoy_build_system.bzl and add "-fprofile-arcs" "-ftest-coverage" (others optional, I was playing around)
*move foo.sh to /tmp/envoy-docker-build/tmp/_bazel_bazel/

  • change all my hard-coded bazel directory hashes to your hard coded directory hash :-P
    IMAGE_ID=latest ./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.covbuild'
    IMAGE_ID=latest ./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.cov'

Look at /tmp/envoy-docker-build/tmp/_bazel_bazel/your_hash_here/output/index.html

@alyssawilk alyssawilk removed this from the 1.5.0 milestone Oct 3, 2017
@alyssawilk alyssawilk removed their assignment Oct 3, 2017
@mattklein123 mattklein123 added the help wanted Needs help! label Oct 28, 2017
@mattklein123 mattklein123 removed the help wanted Needs help! label Jun 25, 2018
@stale
Copy link

stale bot commented Jul 26, 2018

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 26, 2018
@stale
Copy link

stale bot commented Aug 2, 2018

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

@stale stale bot closed this as completed Aug 2, 2018
jpsim pushed a commit that referenced this issue Nov 28, 2022
These are specified in the Kotlin interface, but not in Swift. They'll be wired up separately.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this issue Nov 29, 2022
These are specified in the Kotlin interface, but not in Swift. They'll be wired up separately.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

No branches or pull requests

3 participants