Skip to content

coverage: use LLVM and bazel native coverage#7552

Merged
htuch merged 17 commits intoenvoyproxy:masterfrom
lizan:llvm-cov3
Jul 16, 2019
Merged

coverage: use LLVM and bazel native coverage#7552
htuch merged 17 commits intoenvoyproxy:masterfrom
lizan:llvm-cov3

Conversation

@lizan
Copy link
Member

@lizan lizan commented Jul 11, 2019

Signed-off-by: Lizan Zhou lizan@tetrate.io

Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:

lizan added 8 commits July 11, 2019 23:01
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan lizan requested a review from htuch July 13, 2019 00:44
@lizan lizan marked this pull request as ready for review July 13, 2019 00:54
@lizan
Copy link
Member Author

lizan commented Jul 13, 2019

This is ready, still using single binary though LLVM and bazel native coverage. CI fails because coverage is below threshold (96.94%). The report is here and should be compared to coverage report from #7566: https://248596-65214191-gh.circle-artifacts.com/0/coverage/index.html

lizan added 4 commits July 13, 2019 01:05
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@mattklein123
Copy link
Member

@lizan agreed let's drop the % required for now and ship it. Looking through there is legit coverage missing so maybe we can raise it again one day.

Q: Which project(s) would we file issues against to get the ability to exclude lines back? It seems like something that shouldn't be that hard to do and maybe we could list as a starter project somewhere. (Either way let's open an issue to track further coverage improvements.)

/wait

@lizan
Copy link
Member Author

lizan commented Jul 13, 2019

@mattklein123 With #7568 I think 97.0% is the threshold we can use for now. It is 0.5% down from currently, which is the gap of excluded lines.

Q: Which project(s) would we file issues against to get the ability to exclude lines back? It seems like something that shouldn't be that hard to do and maybe we could list as a starter project somewhere. (Either way let's open an issue to track further coverage improvements.)

It is llvm-cov. Yeah from a quick glance it shouldn't be too hard to do.

@lizan
Copy link
Member Author

lizan commented Jul 14, 2019

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #7552 (comment) was created by @lizan.

see: more, trace.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks a ton for working on this. Can we have a follow up issue for excluding lines and maybe link to an LLVM issue that we can track? One other question.

/wait-any

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
Copy link
Member Author

lizan commented Jul 14, 2019

There is already one in LLVM actually: https://bugs.llvm.org/show_bug.cgi?id=34277

mattklein123
mattklein123 previously approved these changes Jul 15, 2019
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Amazing! (Might want to see if @htuch wants to take a quick look since he is more familiar with all of this, but LGTM and we can always ship and iterate.)

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Fantastic, two minors nit and LGTM. This is a huge win!

@htuch
Copy link
Member

htuch commented Jul 16, 2019

@asraa FYI, this should change how coverage is done in OSS Envoy and it should make it faster to iterate around the nghttp2 coverage effort.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
htuch
htuch previously approved these changes Jul 16, 2019
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM once you sort out coverage fail.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit cd620ad into envoyproxy:master Jul 16, 2019
zuercher pushed a commit that referenced this pull request Jul 17, 2019
#7552 breaks master coverage build.

Risk Level: Low
Testing: cannot be tested via presubmit only
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
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.

3 participants