Skip to content

Conversation

@guzman-raphael
Copy link
Contributor

@guzman-raphael guzman-raphael commented Apr 17, 2025

Now that we have our code coverage integrated into the dev flow and the report published somewhere, things have improved. However, it was still missing some tweaking so that it could be the ideal setup for coverage checks.

To address that, the config here:

  • Lets partial coverage count as normal coverage.
    • Apparently llvm-cov also supports generating cobertura coverage reports which is also allowed to be uploaded to codecov.io. This kind of coverage treats partial coverage as fully covered (it might also be configurable) which better matches the reports generated from the stdout line coverage summary.
  • Requires patch coverage to be 100%.
    • codecov.io apparently differentiates between 2 kinds of coverage:
      • project coverage: overall codebase coverage and what most people normally think of
      • patch coverage: coverage of only the diff in PR's i.e. how much a contributor has changed is actually covered
    • Using the codecov.yaml file is how you can modify the codecov.io experience from the default. Codecov.io seems to prefer patch over project coverage and disables project coverage by default. I'd normally use project coverage but there appears to be a bug in it currently. Therefore, I've set and modified patch coverage to achieve the same intent: new/modified code introduced should be completely covered.
  • Excludes coverage for any executables (bin/*) and the top-level lib.rs

Also added a badge on our README to highlight our current project code coverage and updated the docs.

@nauticalab nauticalab deleted a comment from codecov bot Apr 17, 2025
@codecov
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@guzman-raphael guzman-raphael marked this pull request as ready for review April 17, 2025 20:32
@Synicix
Copy link
Contributor

Synicix commented Apr 18, 2025

Is there a way to ignore certain files for coverage. One issue I see with this is error.rs can't be fully cover in patches, thus it would fail every time when a new pull request adds errors.

Copy link
Contributor

@Synicix Synicix 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 a question about the patch coverage restriction. Check the github pull request comments.

@guzman-raphael
Copy link
Contributor Author

guzman-raphael commented Apr 18, 2025

@Synicix There is a way to ignore files but I think there's a better solution.

Ideally, new features/enhancements would be considered complete if they include docs, pass existing tests, and are covered by new tests i.e. "testable". Keep in mind that patch only considers code that a contributor adds/changes.

For errors, this should be doable so long as the error state is possible to replicate.

Gave it another pass and added a couple of commits that shows some examples how to do that and simplified some stuff. With them I was able to raise the project coverage of core::error.rs to 89.55% and uniffi::error.rs to 100%.

core::error.rs can be raised further but it's not a big priority right now. The nice thing about requiring 100% patch coverage on merge is that we don't grow the amount of untested code over time.

@guzman-raphael guzman-raphael requested a review from Synicix April 18, 2025 21:31
@guzman-raphael
Copy link
Contributor Author

Man... still so much to learn from this codecov.io tool, it is pretty cool.

I just learned that we can set the status check to informational. When set, that basically always lets the coverage check pass but adds the results purely as info to the PR.

I'm now leaning towards going with that so we can "live with it" for a while without blocking PR merges. That way we still have the opportunity of treating it case by case i.e. letting the reviewer decide whether to enforce it or allow exceptions. We can always switch it back later once we become more comfortable with how it is affecting our dev flow.

Issuing one more commit to apply this config.

@Synicix
Copy link
Contributor

Synicix commented Apr 22, 2025

Hmm, that is really cool and yeah, I agree, running with warning type check right now might be a good trial run to see how much it interferes before we fully commit it as a check.

@Synicix Synicix merged commit 64a2dd3 into nauticalab:dev Apr 22, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants