dnaka91 / llvm-cov-pretty

More beautiful HTML reports for llvm-cov/cargo-llvm-cov
GNU Affero General Public License v3.0
28 stars 3 forks source link

"80% function coverage" but you can't tell where the problem is #7

Open zackw opened 1 year ago

zackw commented 1 year ago

Describe the bug

I'm attaching three HTML files to this report. The first is llvm-cov show -format=html output for one file of a project I'm working on, and the other two are llvm-cov-pretty output for the same file, one generated with --skip-function-coverage and the other without. (All files have been put through htmltidy and then renamed with a .txt extension so Github will let me upload them.)

In the basic llvm-cov output, one can easily see that there is only one coverage region that was missed in testing: the lambda on line 73 was never called.

In llvm-cov-pretty output, the summary report at the top agrees with the basic llvm-cov output, but it's impossible to tell where the uncovered region is. In the file generated with --skip-function-coverage, every line of code is marked as fully covered (green background). In the file generated without that flag, line 73 is marked as partially covered but so are 17 other lines. Furthermore, the "jump to first uncovered line" link does not work (it has no href attribute).

(I happen to know, because I've been tinkering with coverage reports for this project for a while now, that those 17 other lines would appear to be partially covered in llvm-cov output as well if I had used -show-instantiations=true. What's going on is, except for line 73, each region is reached at least once in at least one of the monomorphizations of the generic functions in this file, but not in every monomorphization. Achieving 100% coverage of every monomorphization is not nearly as important as ensuring that there aren't any code paths that are never tested at all, so it's really important to be able to distinguish these two cases.)

I'm also attaching the .json file I used as input to llvm-cov-pretty.

Expected behavior

In llvm-cov-pretty output:

  1. Line 73 should be visibly different from all other lines in this file.
  2. The "jump to first uncovered line" link should take me to line 73.
  3. Preferably, line 73 should have its uncovered regions highlighted, more or less the same as how llvm-cov does it.
  4. It would be nice if each region of a generic function, that is reached at least once in some monomorphization of that function, would be treated as fully covered but there would be some kind of unobtrusive indication that it's not reached in all monomorphizations, and you could click on it to see details (the <summary> and <details> tags can do this without needing JS).

To reproduce

Please let me know if you need me to pull together a minimized test case for this problem.

What operating system are you using?

Linux


llvm-cov show -format=html output
llvm-cov-pretty output
llvm-cov-pretty --skip-function-coverage output
coverage.json.gz

dnaka91 commented 1 year ago

With the latest version v0.1.7 the behavior changed slightly and now you should at least get a different color for lines that are partially covered (yellow instead of red).

I completely agree that it would be best to get the same inline highlighting as in llvm-cov, but it is a bit tricky. A big blocker here is the code highlighting, which creates its own tags, and I'd have to mangle those together with the locations from the JSON output.

Not impossible, but simply not trivial, and I just didn't get to it as I don't need that inline highlighting as much myself.

Similar to llvm-cov you can enable some extra annotations with the --show-instantiations flag in llvm-cov-pretty as well. It's opt in, because I wasn't able to derive a pattern yet, which decides when to show those annotations. You can see in llvm-cov that not every partially covered line has those annotations, and it's not really clear from the JSON output either.

Nice idea with the <summary>/<details> to make those regions collapsible. So far it doesn't really seem needed as it can be controlled by a flag, but I might add that at some point.


In regard to the broken link, you're absolutely right. I must have missed that and never really generate a proper href attribute for that link. Fixed that just now in the latest commits.

zackw commented 1 year ago

Thanks for the quick fix to the "jump to first uncovered line does nothing" issue.

I probably shouldn't have combined three issues into one bug report; I think I made you miss the more important one of the remaining two. Let me go over it again.

The most important of the two remaining issues, and the one you may have missed, is that line 73 does not get marked as the cause of the incomplete overall coverage indicated at the top of the report. With --skip-function-coverage, every line is marked as fully covered (green); without --skip-function-coverage, line 73 is indeed marked as partially covered (yellow) but so are a bunch of other lines and there's no way to tell that line 73 is different from those other lines.

The additional information added by --show-instantiations is basically useless and, in particular, does not offer any clue that line 73 is different. See for yourself. I'm fairly sure this is the same problem as why -show-instantiations=true is usually useless in plain llvm-cov output -- it's not particularly useful to demand that every instantiation of a particular region be covered. Much more important to know that at least one instantiation of a region was covered -- and that's exactly what makes line 73 different: there is a region on that line that was never executed, not in any of the instantiations.

The other issue (showing uncovered regions within a line as plain llvm-cov does) is more of an enhancement request and I completely agree that mangling the region highlights together with the syntax highlighting is nontrivial and not urgent.

dnaka91 commented 1 year ago

Sorry, it wasn't quite clear to me in the initial description, and thank you for clarification.

I agree that it's not really helpful to demand every instantiation be executed. That is similar to how 100% coverage is not realistic and (in my opinion) rather harmful. Was just initially marking those as not covered to be on the safe side of not marking something as covered if in fact some parts are not.

To make the line 73 marked correctly, you'd like to have it marked as uncovered instead of partially covered (basically any line where all instantiations never got executed), right?

A bit difficult to tell the difference in the HTML reports without the styling, sorry. Maybe it's possible to re-upload as tar.gz or zip file (or does GitHub block that?).