Skip to content

Conversation

@ValuedMammal
Copy link
Collaborator

@ValuedMammal ValuedMammal commented May 30, 2025

@ValuedMammal ValuedMammal self-assigned this May 30, 2025
@ValuedMammal ValuedMammal added the ci Continuous integration label May 30, 2025
@ValuedMammal ValuedMammal marked this pull request as ready for review June 9, 2025 15:01
- name: Make coverage directory
run: mkdir coverage
- name: Run grcov
run: grcov . --binary-path ./target/debug/ -s . -t lcov --branch --ignore-not-existing --ignore '/*' --ignore '**/tests/**' --ignore '**/examples/**' -o ./coverage/lcov.info
Copy link
Contributor

@nymius nymius Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have in mind a second use of the lcov.info file other than source it to genhtml? Why not using -t html?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving a second look at this, I think lcov.info may be useful to generate diffs between master and the merging branch, but I've not found a good tool to display reports from these diffs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I would remove the --branch flag. I don't know if your are getting any results there, but from my experience the matching column for those is usually 0/0, so not giving useful information. It seems to be not supported actually: mozilla/grcov#520

Copy link
Contributor

@nymius nymius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been testing grcov for bdk-sp and explored different combinations of flag to get the most of coverage reports:

  • --excl-start "#\[cfg\(test\)\]": In all the bdk repositories I have seen unit test are placed at the end of files, and not in the middle like some projects (correct me if I'm wrong). So I think this flag is useful in this case. I wouldn't use --excl-br-line "^\s*#\[cfg\(test\)\].*" for these cases because I've experimented with it, and it ignores all #[test] tagged functions, but the utility functions inside the test module are not ignored, altering the final report numbers.
  • --excl-br-line "^\s*((debug_)?assert(_eq|_ne)?!|#\[derive\()": In this case I'm not so sure about the debug_assert and assert variations, as it may be used in some parts of the target code with actual implications in main logic of the program, but I don't have much of these in the code I have been using to test. In the case of derive I think is useful to exclude them in the case of default derivations. That code is boilerplate and cannot unless implemented manually, in which case the lines will be considered by the coverage report.
  • --excl-line "^\s*\S+\s*(=>)?\s*unreachable!.*": I have some actually legit cases to use unreachable, so as long as the macro is not used lightly, I think is a good way to improve the signal of the report.

@nymius
Copy link
Contributor

nymius commented Jun 30, 2025

After adding docstrings to bdk-sp encoding module, grcov started to produce some false positives, so I started looking at other tools.
I ended up deciding with cargo-llvm-cov because it doesn't have those false positives, and also is able to ignore unit test code.
Although this is possible thanks to nightly attributes, as these tools are exclusively for the developing phase, and not release, I think it is OK to use nightly for these use cases.
cargo-llvm-cov also improves reports by adding branch coverage (thanks to nightly).
--mdcd gives errors on ci so --branch is enough for now.

@ValuedMammal
Copy link
Collaborator Author

Thank you @nymius I'll do some more research on cargo-llvm-cov.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Continuous integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants