Minor bugfixes to generate Gcov/Lcov branch coverage #11489

MichaelKlemm · 2020-05-26T11:56:15Z

When using bazel to generate C++ branch coverage BA entries are created instead of BRDA lines. As far as these BA lines could not be used by genhtml, I changed it be processed as BRDA. Doing so the program throws an exception because of the wrong condition, trying to convert a '-' to a number. Inverted this condition to have the correct behavior of '-' which means wasExecuted = false and executionCount = 0.

googlebot · 2020-05-26T11:56:20Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

MichaelKlemm · 2020-05-26T11:58:56Z

@googlebot I signed it!

googlebot · 2020-05-26T11:58:59Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

MichaelKlemm · 2020-05-26T14:02:35Z

recheck

ulfjack

It might be better to split this into two changes, and also add tests for each of the bugfixes.

...st/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/LcovPrinter.java

Replaced obsolete 'BA:' tag by 'BRDA:' to be able to generate a valid branch coverage report.

On taken indicator '-' the current implementation of the LcovParser was setting the branch as executed and tried to convert the sign '-' to a number. Doing so an exception was thrown. Just inverted the condition to fix the problem.

MichaelKlemm · 2020-05-27T14:01:05Z

It might be better to split this into two changes, and also add tests for each of the bugfixes.

I know, one is the parsing and the other one the printing. Implementing tests will strongly combine these two, so I would suggest to keep it together.

ulfjack · 2020-05-27T16:23:00Z

Unfortunately, I don't know the history of the BA lines, but I wouldn't be surprised if the tool just changed silently at some minor version. The coverage tools seem to have a habit of doing that. :-/

The change looks reasonable to me, but I didn't do a careful review of the tests, and I also didn't research the BA / BRDA thing.

@meisterT can you take a look as well, or find someone to take a look?

meisterT · 2020-05-27T17:24:59Z

Sorry, I have no clue about this.
cc @lberki

MichaelKlemm · 2020-05-28T15:03:45Z

Are there any other tools being used besides the lcov project (genhtml, geninfo, ...) using the *.dat output? I don't want to destroy compatibility but as far as I know there is only the one specification at the LCOV project and this is not using BA at all. So BRDA seems to be the way to go and it's also working quite well with my local C++ environment using branch coverage.

How to proceed here?

ulfjack · 2020-06-01T11:09:30Z

I think a couple tools use lcov internal output for further processing. Regardless, if lcov HEAD accepts this and doesn't accept or document BA, I think it's fine to change it to BRDA unconditionally.

ulfjack · 2020-06-02T11:18:32Z

So, the BA lines date back to lcov 1.9 (https://chromium.googlesource.com/chromium/src/third_party/lcov/+/dd41bfc42b684e08eb9205301932bae3056ff8e4%5E%21/). BRDA was introduced in 1.10 (http://ltp.sourceforge.net/coverage/lcov/changes.php):

commit b5c1bdddd1380be3ad12952ed2747df3744e227e 
Author: Peter Oberparleiter <[email protected]>
Date:   Wed Oct 10 08:20:21 2012 +0000

    lcov: finalizing release 1.10

I think it's fine not to support an 8-year-old release at this point.

ulfjack · 2020-06-02T11:20:09Z

@lberki doesn't seem to be very responsive. Lots of work to do?

oquenchil · 2020-06-08T11:13:48Z

...est/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/LcovParser.java

@@ -280,7 +248,7 @@ private boolean parseBRDALine(String line) {

      boolean wasExecuted = false;


This line can be deleted.

oquenchil · 2020-06-08T11:16:54Z

...CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/BranchCoverage.java

@@ -22,7 +22,7 @@

  static BranchCoverage create(int lineNumber, int nrOfExecutions) {
    return new AutoValue_BranchCoverage(
-        lineNumber, /*blockNumber=*/ "", /*branchNumber=*/ "", nrOfExecutions);
+        lineNumber, /*blockNumber=*/ "0", /*branchNumber=*/ "0", nrOfExecutions);


Are the two numbers here valid GCC IDs?

Below in this file it says it should be -1 for non-gcc emitted coverage. I'm guessing that if we don't have the values present, it should be -1 and not 0?

oquenchil · 2020-06-08T11:18:01Z

Thank you for the change! I have a couple of nits.

oquenchil · 2020-06-08T11:47:25Z

Actually hold on addressing my comments. A colleague has just told me that BA may be important internally. I will ping this again once I know more.

ulfjack · 2020-06-15T13:33:09Z

@oquenchil any updates? It's been a week...

oquenchil · 2020-06-15T13:56:39Z

The problem that we are facing with coverage is that we have quite the knowledge gap right now and not enough people. I'm trying to fill that gap but I have been very busy with other issues.

The previous expert on coverage in the team told me that the BA line is important internally but they didn't remember why. I wrote an email today to reach out to 2 other people in the company that may have more context.

I will ping this once they reply.

ulfjack · 2020-06-15T15:24:15Z

Would it help if this change added support for BRDA lines without converting to or from BA lines? Would that be an option, @MichaelKlemm ?

oquenchil · 2020-06-17T09:49:58Z

I asked this as well because from the reply I got yesterday it wasn't clear. If I cannot tell you for certain tomorrow, I will try importing this PR with the BA deletion anyway and then we might find out later in which way it was important.

Meanwhile, @MichaelKlemm could you please address the 2 nits?

oquenchil · 2020-06-17T13:53:29Z

Ok, I have more information now. We definitely have internal tools still reporting BA lines. But we will change that. Meanwhile, if you can change this PR to be able to parse BA lines and treat them as BRDA, then that should be enough not to break anything as far as I can tell.

Thank you!

meisterT · 2020-07-30T06:51:30Z

What is the status of this PR?

oquenchil · 2020-07-30T09:00:40Z

Ok, I have more information now. We definitely have internal tools still reporting BA lines. But we will change that. Meanwhile, if you can change this PR to be able to parse BA lines and treat them as BRDA, then that should be enough not to break anything as far as I can tell.

Thank you!

It doesn't look like the internal tools will be changed any time soon, so this PR should keep the BA lines.

c-mita · 2020-08-07T14:51:03Z

We're currently in the process of fixing issues around the handling of lcov; fixes to the parser are part of that, as is changing the generators to not output "BA" lines.

Unfortunately we cannot remove support for BA lines yet as they are still used internally at Google.

MichaelKlemm requested a review from lberki as a code owner May 26, 2020 11:56

googlebot added the cla: no label May 26, 2020

googlebot added cla: yes and removed cla: no labels May 26, 2020

ulfjack reviewed May 26, 2020

View reviewed changes

...st/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/LcovPrinter.java Outdated Show resolved Hide resolved

MichaelKlemm added 5 commits May 27, 2020 15:46

tools: coverage: fix convertation of branch coverage

fedb124

Replaced obsolete 'BA:' tag by 'BRDA:' to be able to generate a valid branch coverage report.

tools: coverage: removed handler for obsolete BA lines

ec30ce2

tools: coverage: add test for branch coverage / BRDA lines

56fb330

tools: coverage: make block/branch number always contain a valid value

766ac38

MichaelKlemm force-pushed the master branch from 883693a to 766ac38 Compare May 27, 2020 14:02

meisterT added the team-Rules-Server Issues for serverside rules included with Bazel label May 27, 2020

ulfjack approved these changes Jun 2, 2020

View reviewed changes

oquenchil reviewed Jun 8, 2020

View reviewed changes

jin assigned oquenchil Jun 15, 2020

c-mita mentioned this pull request Aug 6, 2020

Fix parsing of BRDA lines in LcovParser #11899

Closed

c-mita closed this Aug 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor bugfixes to generate Gcov/Lcov branch coverage #11489

Minor bugfixes to generate Gcov/Lcov branch coverage #11489

MichaelKlemm commented May 26, 2020

googlebot commented May 26, 2020

MichaelKlemm commented May 26, 2020

googlebot commented May 26, 2020

MichaelKlemm commented May 26, 2020

ulfjack left a comment

MichaelKlemm commented May 27, 2020

ulfjack commented May 27, 2020

meisterT commented May 27, 2020

MichaelKlemm commented May 28, 2020

ulfjack commented Jun 1, 2020

ulfjack commented Jun 2, 2020

ulfjack commented Jun 2, 2020

oquenchil Jun 8, 2020

oquenchil Jun 8, 2020

oquenchil commented Jun 8, 2020

oquenchil commented Jun 8, 2020

ulfjack commented Jun 15, 2020

oquenchil commented Jun 15, 2020

ulfjack commented Jun 15, 2020

oquenchil commented Jun 17, 2020

oquenchil commented Jun 17, 2020

meisterT commented Jul 30, 2020

oquenchil commented Jul 30, 2020

c-mita commented Aug 7, 2020

		@@ -280,7 +248,7 @@ private boolean parseBRDALine(String line) {

		boolean wasExecuted = false;

Minor bugfixes to generate Gcov/Lcov branch coverage #11489

Minor bugfixes to generate Gcov/Lcov branch coverage #11489

Conversation

MichaelKlemm commented May 26, 2020

googlebot commented May 26, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

MichaelKlemm commented May 26, 2020

googlebot commented May 26, 2020

MichaelKlemm commented May 26, 2020

ulfjack left a comment

Choose a reason for hiding this comment

MichaelKlemm commented May 27, 2020

ulfjack commented May 27, 2020

meisterT commented May 27, 2020

MichaelKlemm commented May 28, 2020

ulfjack commented Jun 1, 2020

ulfjack commented Jun 2, 2020

ulfjack commented Jun 2, 2020

oquenchil Jun 8, 2020

Choose a reason for hiding this comment

oquenchil Jun 8, 2020

Choose a reason for hiding this comment

oquenchil commented Jun 8, 2020

oquenchil commented Jun 8, 2020

ulfjack commented Jun 15, 2020

oquenchil commented Jun 15, 2020

ulfjack commented Jun 15, 2020

oquenchil commented Jun 17, 2020

oquenchil commented Jun 17, 2020

meisterT commented Jul 30, 2020

oquenchil commented Jul 30, 2020

c-mita commented Aug 7, 2020