search

taiki-e / cargo-llvm-cov

Cargo subcommand to easily use LLVM source-based code coverage (-C instrument-coverage).
Apache License 2.0
933 stars 57 forks source link

Include `cargo-llvm-cov` version in the JSON output #285

Closed dnaka91 closed 1 year ago

dnaka91 commented 1 year ago

I'm building a small tool that depends on the llvm-cov JSON output. One thing that happened is, that one user had a ancient version of llvm-cov-pretty around that automatically stripped path prefixes to make them relative to the project. That became an optional flag later.

I understand the JSON output comes mainly from llvm-cov, but I saw that some deduplication steps are already done as post-processing step.

Therefore, I was wondering if it would be okay to inject the version of cargo-llvm-cov into the root object of said structure?

Currently, I do a manual check that the version of cargo-llvm-cov isn't too old, but that gives me no guarantee that the global version that I look up from my tool is the actual version the user invoked. If it's directly in the JSON struct, I'd have a stronger guarantee that this is the generated the output of the tool the user invoked and can directly check the version (my tool is meant to get the cargo-llvm-cov JSON report piped in).

From the JSON perspective, this should cause any issues, I suppose? Most JSON parsers simply ignore any extra fields that are no defined manually.

taiki-e commented 1 year ago

I would accept a PR to add additional data to the JSON output.

llvm-cov-pretty

By the way, I have used it a little, and it looks great.

I have seen several times before that people want reports in a different design (e.g., https://github.com/taiki-e/cargo-llvm-cov/issues/86), and your tool might solve some of them.

I think it is worth mentioning your tool in the cargo-llvm-cov readme.

(From llvm-cov-pretty's readme)

Reduced clutter from instantiation annotations.

I'm happy to know that people think these annotations are annoying. I thought similarly and added the --hide-instantiations option option, but I didn't make it default because I wasn't sure if other people wanted it.

dnaka91 commented 1 year ago

I'm glad you like my HTML report design :blush:

Then I'll prepare a PR to inject the cargo-llvm-cov version into the JSON output, and a separate one to add a mention to this project's readme.

Initially I thought whether it's worth creating a PR to directly integrate the custom HTML report into cargo-llvm-cov, but it seemed good to have it separate. This project as a convenient wrapper around the llvm-cov invocation for Rust, and then my (and potentially other projects) that can generate alternative views of the output.

But if you think it would be good to combine both tools, I'll consider it :+1:

Regarding "Reduced clutter from instantiation annotations.", I actually like those annotations, as they can be helpful if you want to make sure you really covered all parts of some derive macros.

What I meant with that statement is, that in contrast to the regular llvm-cov report, there is only one single annotation with the list of missing invocations per source line, instead of one annotation for each. That saves a lot of vertical space.

Later on I made the annotations opt-in, because I still have some bugs to sort out to display it exactly like llvm-cov does (have cases where there are sometimes missing and sometimes too many annotations). Also, the person who gave me the initial idea to make this tool, mentioned that they don't need those annotations.