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

Cobertura output #106

Closed svenstaro closed 1 year ago

svenstaro commented 2 years ago

Hey! This is currently my favorite Rust coverage tool but I'm missing Cobertura output for GitLab. Currently, the only way seems to be to use the unmaintained Python package lcov-to-cobertura-xml but that seems annoying to me. Any chance we can get native GitLab-compatible Cobertura output in this tool? :)

taiki-e commented 2 years ago

I would accept a PR to add support for this output format.

FWIW, in #77, there was a discussion on another way that works on GitLab.

Kage-Yami commented 2 years ago

taiki-e: FWIW, in #77, there was a discussion on another way that works on GitLab.

Just for reference, the discussion in that issue is about pulling out just the coverage percentage to display in GitLab's UI, whereas Cobertura reports are used in GitLab for visualising per-line coverage in merge request diffs (with little green/red pips in the diff's gutter).

mike-kfed commented 1 year ago

Hi @taiki-e I re-implemented the mentioned python script in Rust. I achieved feature parity, however I am not confident that the coverage counts are correctly parsed (mostly because the original code ignores a lot of data / resets counting at strange points). I likely need some guidance how to verify that it does stuff correctly, beyond the existing small unit-tests.

It can be used as a library, which is probably of interest to you. And, since I built this for you, I wait with releasing this to crates.io until you are happy with the API. Here's the code https://github.com/mike-kfed/lcov2cobertura - basic usage can be seen in src/main.rs

I tried my best to write idiomatic code, code comments are often missing but that will come once I have some feedback if it's usable like that^^

taiki-e commented 1 year ago

@mike-kfed Great!

It can be used as a library, which is probably of interest to you. And, since I built this for you, I wait with releasing this to crates.io until you are happy with the API.

Thanks. I'm not familiar with Cobertura format so cannot review the correctness of the implementation, but the API looks good except for a few nits:

I achieved feature parity, however I am not confident that the coverage counts are correctly parsed (mostly because the original code ignores a lot of data / resets counting at strange points). I likely need some guidance how to verify that it does stuff correctly, beyond the existing small unit-tests.

At least if it works well in your test case, I think it would be reasonable to add Cobertura support using lcov2cobertura to cargo-llvm-cov as a preview feature and get feedback from users.

mike-kfed commented 1 year ago

@taiki-e Thanks a lot for looking at my code and the quick feedback!

Thanks. I'm not familiar with Cobertura format so cannot review the correctness of the implementation, but the API looks good except for a few nits:

I am not familiar either, I just like your coverage tool and would like to use it for our Gitlab - translating the python script was a good exercise :)

  • In Rust, fn often means function or closure, so I was not sure what parse_fn does until I looked at the documentation; a name like parse_file might be more preferable.
  • It is maybe preferable that the excludes parameter for parse_fn and parse_file be a slice rather than a vec to allow the list to be reused.
  • coverage_as_string may preferably be coveragetostring because it allocates a string; see also API Guidelines

Thanks for those points, valuable insights! All of them have been addressed just now.

At least if it works well in your test case, I think it would be reasonable to add Cobertura support using lcov2cobertura to cargo-llvm-cov as a preview feature and get feedback from users.

So far I am happy with the results I get, but again I too am unfamiliar with cobertura. I'll read up on how to release my crate and then I'll work on a PR to integrate it with cargo-llvm-cov.

mike-kfed commented 1 year ago

My codebase does not need it at all, but given existing bug report for GitLab I also implemented splitting of large coverage XML files. GitLab has a 10MB upload limit, this could be useful for big codebases. Not sure this splitting should be part of cargo-llvm-cov though, probably better to point users in the docs to helper tools like mine?

taiki-e commented 1 year ago

It might make sense to add a GitLab example to the "Continuous Integration" section in readme and mention the helper tool in it.

taiki-e commented 1 year ago

Added as --cobertura flag in 0.5.1 (#224). Thanks @mike-kfed for working on this!