Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support region coverage on codecov #20

Closed
taiki-e opened this issue Jun 7, 2021 · 10 comments · Fixed by #249
Closed

Support region coverage on codecov #20

taiki-e opened this issue Jun 7, 2021 · 10 comments · Fixed by #249
Labels
C-enhancement Category: A new feature or an improvement for an existing one help wanted Call for participation: Help is requested to fix this issue

Comments

@taiki-e
Copy link
Owner

taiki-e commented Jun 7, 2021

readme says:

Currently, only line coverage is available on Codecov. This is because -Z instrument-coverage does not support branch coverage and Codecov does not support region coverage.

However, we can probably support region coverage on codecov by converting llvm-cov's json coverage format to codecov's json coverage format.

related:

@taiki-e taiki-e added the C-enhancement Category: A new feature or an improvement for an existing one label Jun 7, 2021
@taiki-e taiki-e added the help wanted Call for participation: Help is requested to fix this issue label Jan 6, 2022
@flying-sheep
Copy link

flying-sheep commented Dec 8, 2022

So it seems that codecov doesn’t support full branch coverage? (“region coverage“ is just branch coverage without ignoring that there can be multiple branch arms in one line, right?)

Is there any tool other than llvm-cov show -format=html itself that visualizes full branch coverage?

@taiki-e
Copy link
Owner Author

taiki-e commented Dec 8, 2022

Sorry, I can't understand the question. (See this LLVM patch for what branch coverage means in the LLVM source-based code coverage and for example report it generates.)

@flying-sheep
Copy link

flying-sheep commented Dec 9, 2022

Never mind the definitions: What I mean is that llvm-cov seems (to my knowledge) to be the only tool that is able to visualize which exact parts of the source code have been executed and which haven’t.

Oher tools will show e.g. the following as “covered” or “partially covered” because they work line-based:

@taiki-e
Copy link
Owner Author

taiki-e commented Dec 15, 2022

Ah, thanks for the clarification. I also don't know of other tools that actually show areas that are not covered.

@andrewgazelka
Copy link
Contributor

andrewgazelka commented Mar 29, 2023

@taiki-e I am interested in potentially taking a stab at this. Do you already have a Rust deserialized version of CoverageMappingFormat?

@taiki-e
Copy link
Owner Author

taiki-e commented Mar 29, 2023

Do you already have a Rust deserialized version of CoverageMappingFormat?

LlvmCovJsonExport is the data structure for that.

pub struct LlvmCovJsonExport {

@andrewgazelka
Copy link
Contributor

This is because -Z instrument-coverage does not support branch coverage

@taiki-e so

pub(crate) branches: Option<Vec<serde_json::Value>>,
will always be None?

@taiki-e
Copy link
Owner Author

taiki-e commented Mar 30, 2023

will always be None?

IIRC, it will be empty list (Some(vec![]))

EDIT:

@andrewgazelka
Copy link
Contributor

andrewgazelka commented Mar 30, 2023

So we would want to check a separate arg like --codecov-json here, right? If this were true we would convert LlvmCovJsonExport to the codecov JSON format. Is there a better place to do this conversion?

pub fn normalize_output(output_path: &Utf8Path, args: &[&str]) -> Result<()> {
if args.contains(&"--json") {
let s = fs::read_to_string(output_path)?;
let mut json = serde_json::from_str::<cargo_llvm_cov::json::LlvmCovJsonExport>(&s).unwrap();
if !args.contains(&"--summary-only") {
json.demangle();
}
fs::write(output_path, serde_json::to_vec_pretty(&json)?)?;
}
#[cfg(windows)]
{
let s = fs::read_to_string(output_path)?;
// In json \ is escaped ("\\\\"), in other it is not escaped ("\\").
fs::write(output_path, s.replace("\\\\", "/").replace('\\', "/"))?;
}
Ok(())
}

@andrewgazelka
Copy link
Contributor

@taiki-e here is a draft PR. I still need to test and add unit tests. Is there anything you would change?

#249

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: A new feature or an improvement for an existing one help wanted Call for participation: Help is requested to fix this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants