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

Everything is highlihted as uncovered #4

Open ju1ius opened 1 year ago

ju1ius commented 1 year ago

Describe the bug

Hi,

I just installed llvm-cov-pretty, and ran it on a project.

At first glance the metrics seem to match the cargo llvm-cov html report.

However in the file source view, everything single executable line is highlighted as uncovered (wrapped in a <td class="uncovered">). That's a lot of red.

Expected behavior

Lines should be highlighted correctly.

What operating system are you using?

Linux

Additional context

Here's the formatted JSON object for a small file showing 100% line coverage: ```json { "branches": [], "expansions": [], "filename": "/path/to/mod.rs", "segments": [ [21, 5, 22, true, true, false], [23, 6, 0, false, false, false], [26, 5, 22, true, true, false], [28, 6, 0, false, false, false], [38, 5, 1, true, true, false], [40, 6, 0, false, false, false], [44, 5, 6, true, true, false], [46, 6, 0, false, false, false] ], "summary": { "branches": { "count": 0, "covered": 0, "notcovered": 0, "percent": 0 }, "functions": { "count": 4, "covered": 4, "percent": 100 }, "instantiations": { "count": 70, "covered": 29, "percent": 41.42857142857143 }, "lines": { "count": 12, "covered": 12, "percent": 100 }, "regions": { "count": 4, "covered": 4, "notcovered": 0, "percent": 100 } } } ```
ju1ius commented 1 year ago

OK, I think I've found the source of the issue. Let me try to explain a bit.

The project I'm testing this on is compiled as a cdylib which is meant to be loaded inside an host environment. Thus it has almost no unit tests using libtest. Most of the test suite resides in a workspace crate for integration tests, using custom test binaries with harness = false.

The cargo llvm-cov command has to be invoked as recommended by the docs:

source <(cargo llvm-cov show-env --export-prefix)
export CARGO_TARGET_DIR="$${CARGO_LLVM_COV_TARGET_DIR}"
cargo llvm-cov clean --workspace
cargo build --workspace \
  && cargo test --workspace \
  && cargo llvm-cov report --json --output-path=coverage.json

The issue in this case seems to be that the functions execution_count (the functions.regions[4] field in coverage.json) is always set to 0, unless they are themselves unit tests. In other words, the functions.regions[4] field is always set to 0 for functions that are run by the integration tests.

Then the logic in merge_function_info kicks in and marks all these lines as uncalled, which in turns force them to be highlighted as uncovered.

The end result is that the only lines highlighted as covered are lines that are themselves part of unit tests. Everything else (the stuff we really care about) is highlighted as uncovered.

I know nothing about llvm-cov report format so I can't tell if the merge_function_info is correct or not. But if it is, maybe there could be a command-line flag to switch off this behaviour in order to better support this kind of setup?

dnaka91 commented 1 year ago

Thank you for the bug report and addition of further context.

I never did external tests in my projects, thus simply never encountered the situation. But I was recently playing with the idea of adding an option that disables the function invocation overlay.

Sadly, llvm-cov uses some internal structures that are not the same as the JSON output, so I can't fully map the logic in llvm-cov-pretty. Plus, the LLVM code is pretty complex, and I'm not a C++ expert :sweat:.

Therefore, the behavior in llvm-cov-pretty is the result of putting JSON and HTML report next to each other and try to make sense of the numbers in the JSON format until it seems to match with the HTML report.

One big difference is that the function coverage in llvm-cov is inline, whereas I mark the whole line as uncovered if the coverage reports 0. Maybe at some point I'll do the same as llvm-cov, but the addition of syntax highlighting complicates things.


Just let me confirm whether I understand correctly: you'd like to be able to disable the function instantiation overlay that's being done over the regular file coverage?

If that's the case, it aligns with my idea of adding a CLI flag that allows to disable said overlay.

ju1ius commented 1 year ago

Just let me confirm whether I understand correctly: you'd like to be able to disable the function instantiation overlay that's being done over the regular file coverage?

Sorry, I'm confused, what is the "function instantiation overlay"?

dnaka91 commented 1 year ago

So llvm-cov splits the coverage information into certain categories. As you can see in the overview page that llvm-cov-pretty generates, it splits coverage into line, function and region coverage. There is a fourth, branch coverage, but that isn't currently supported for Rust.

In the coverage JSON file, there are the file coverages which represent the line coverage information. Then there is function instantiation coverage which represents the function and region coverage.

For the coverage on the source code, it first applies the line coverage, and then uses the function coverage on top of that.

That behavior is the same in the regular llvm-cov output, where you can see it as individual bits (like a specific function call), that are marked as covered/uncovered within a single line (or multiple).

I agree the term "function instantiation" is odd, as you don't really instantiate a function but rather call it... It's the terminology used inside LLVM code :shrug:.

How about I simply add that flag, and you try it out to see if that's what you need. I was playing with the idea to add that flag anyway. It'd skip the merge_function_info basically.

ju1ius commented 1 year ago

I agree the term "function instantiation" is odd

I was in fact confused by the term "overlay".

The way I understand the term "instanciation" is related to generics, i.e.:

// Given a generic function:
fn foo<T>(param: T) {}
// The following is an instanciation of foo<T> as foo<&'static str>
let _ = foo("bar");
// The following is an instanciation of foo<T> as foo<u8>
let _ = foo(42u8);
// Thus llvm-cov reports:
// count = 1 for foo<&'static str>
// count = 1 for foo<u8>
// Unexecuted instanciation for foo<_> (for any other T)

In the source view of the llvm-cov html report, the source lines of foo would be show three times: one w/ the global counts, and one for each instanciation w/ their respective counts. Plus an additional block for the unexecuted instanciation for foo<_>.

In the source view of the llvm-cov-pretty html report:

Now - correctly if I'm wrong as I just glanced over the code - maybe the issue is that merge_function_info marks lines as uncovered if any of it's function instanciation is unexecuted, whereas it should mark them as uncovered if all of it's function instanciations are unexecuted?

dnaka91 commented 1 year ago

The way I understand the term "instanciation" is related to generics

True, as you get a version of the generic function with each type it's used with.

Now - correctly if I'm wrong as I just glanced over the code - maybe the issue is that merge_function_info marks lines as uncovered if any of it's function instanciation is unexecuted, whereas it should mark them as uncovered if all of it's function instanciations are unexecuted?

I feel it's neither right nor wrong :sweat_smile:. In the original llvm-cov report, it's still marked as red if the variant is missing, just not for the whole line. I wanted to avoid having it look too good, giving a false sense of security as it would show as covered even though there are some instantiations that weren't called.

But only marking it as uncovered if all of those are uncovered is a good idea. Maybe it's worth introducing another color (yellow) to mark a line as partially covered, for when some of them are not covered.

By the way, you can get similar annotations with the --show-instantiations flag. Just, I didn't quite figure out the correct way of how llvm-cov decides when to omit them (as it doesn't show all of them, only some). Therefore, I made it an opt-in.

ju1ius commented 1 year ago

In fact it turns out my issue is not related to generics.

It seems related to the fact that the report is collected in several passes, i.e. one pass with #[cfg(test)] where only a few things are executed, and one pass for each of the integration test binairies where all the real work is done.

Since a #[test] function if cfg-gated it will have only one entry in the report and thus always be marked as executed.

But a library function will have one entry when run w/ #[cfg(test)], in which it is not executed, and one entry per integration test binary, in which it is. Thus the whole function will be highlighted as uncovered because it was not executed in unit tests.

Given all the former, I agree that:

only marking it as uncovered if all of those are uncovered is a good idea.

And that:

it's worth introducing another color (yellow) to mark a line as partially covered, for when some of them are not covered.

👍🏻

dnaka91 commented 1 year ago

You might be able to merge the coverage data with llvm-profdata merge :thinking:. Or, if you put it in the right place, cargo-llvm-cov can do it for you as it merges profile data under the hood.

Just, cargo-llvm-cov does some deduplication in the JSON afterward, that I potentially rely on in llvm-cov-pretty.

Looking at the implementation of cargo-llvm-cov it by default puts the *.profraw data in the target/llvm-cov-target folder and merges all files that follow the pattern of <crate-name>-*.profraw. As long as you copy the other coverage files in that pattern, and then call cargo llvm-cov report on it, it might work.

But that's just an idea to get all the coverage data into one file. Might not even work in the end.


As for the 2 improvements I mentioned, I'll work on those and let you know once I make a new release.

dnaka91 commented 1 year ago

Sorry, a little late with the ping. I released a new version a bit ago, which marks partially covered lines as yellow now, and adds a new flag to disable the function coverage override.

Have you had any success with the llvm-profdata merge to combine the coverage data?