7903842: Negative coverage in report for records

[lkuskov]

The Javap report had several issues marked in the picture below, which have been fixed:

The expected report is:

---

Progress

• Change must not contain extraneous whitespace

Issue

• CODETOOLS-7903842: Negative coverage in report for records (Bug - P2)

Reviewers

• Alexandre Iline - Reviewer ⚠️ Added manually

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jcov.git pull/48/head:pull/48
$ git checkout pull/48

Update a local copy of the PR:
$ git checkout pull/48
$ git pull https://git.openjdk.org/jcov.git pull/48/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 48

View PR using the GUI difftool:
$ git pr show -t 48

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jcov/pull/48.diff

Webrev

Link to Webrev Comment

[lkuskov]

/reviewer credit @shurymury

[bridgekeeper]

👋 Welcome back lkuskov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@lkuskov This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

7903842: Negative coverage in report for records

Reviewed-by: shurailine

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@lkuskov
Reviewer shurailine successfully credited.

[mlbridge]

Webrevs

• 00: Full (a146e216)

[shurymury]

Can you please add a test similar to one added by https://bugs.openjdk.org/browse/CODETOOLS-7903867?

[shurymury]

Please note that the regular report is also showing negative coverage, not only the javap report.

Does this fix addresses both reports? Is the root cause the same for the two reports or different?

If you will integrate this, you will need to open another bug for the regular report.

[shurymury]

FYI: I have created a test for this problem (only javap report, for now): #52

[bridgekeeper]

@lkuskov This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@lkuskov This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[lkuskov]

/open

[openjdk]

@lkuskov This pull request is now open

[shurymury]

I think it would be better to rename "line coverage" to "instruction coverage" in this report. Some other word can be used, but "instruction" is the best I can come up with.

The reasons are:

1. Line is associated with a source line. What's listed in the reworked report are not lines, they are byte code instructions. One may argue that a line could correspond to a javap utility output line, but that is superficial.
2. If the column name the same as in a corresponding report with java code, first action of anybody looking on the report would be to compare the numbers. Compare line coverage from one report to the line coverage in the other report. Without knowledge that the report is showing something else, it is impossible to explain the numbers.

[shurymury]

The test added by https://bugs.openjdk.org/browse/CODETOOLS-7903867 is only checking that there is no negative coverage. An update to it is needed to check that the right coverage is reported.

[shurymury]

I think it would be better to rename "line coverage" to "instruction coverage" in this report. Some other word can be used, but "instruction" is the best I can come up with.

@dbessono What are your thoughts on "line coverage" vs "instruction coverage"?

[lkuskov]

The term 'code line' (line of 4.7.3. The Code Attribute) or the simplified 'line' is intuitively clear and sufficient to be used there. All lines that can't be covered in the javap report don't have green or red markings like comment lines in Java sources and are clearly excluded from the meaning of covered/non-covered.

[dbessono]

While not being deep in the context it seems that the report body was not reworked to show something else than code lines containing code instructions. The report still renders lines marked as green or red, not the specific instructions highlighted in green or red.

Another thing to note is that the reported bug looks related to incorrect calculations, not to the column naming. So any suggested renaming might better go under a different suggested enhancement?