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

feat: reintroduce coverage and upload to codecov.io #229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

YeungOnion
Copy link
Contributor

Bringing back codecov #147

I went with tarpaulin because it seemed easy to setup and use, especially with the container on dockerhub. I don't believe that coverage reports need the same level of confidence as passing the tests that are written, but can be useful in deciding how to focus efforts.

But I'm new to this, so feedback would be appreciated.

@YeungOnion YeungOnion requested a review from henryjac May 15, 2024 21:52
@YeungOnion YeungOnion force-pushed the test-ci branch 2 times, most recently from 2ecf336 to 31708c1 Compare May 15, 2024 22:44
Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.52%. Comparing base (49a5209) to head (0050684).
Report is 190 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
- Coverage   91.76%   91.52%   -0.25%     
==========================================
  Files          44       50       +6     
  Lines        7360    10332    +2972     
==========================================
+ Hits         6754     9456    +2702     
- Misses        606      876     +270     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@YeungOnion
Copy link
Contributor Author

It appears that tarpaulin is accurately reading tests that rely on the testing_boiler macro, I don't know where else to check for inaccuracy, so others' input would be helpful.

Copy link
Contributor

@FreezyLemon FreezyLemon left a comment

Choose a reason for hiding this comment

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

I don't have any experience with tarpaulin, but it seems straightforward enough. Some actions are outdated though.

.github/workflows/coverage.yml Outdated Show resolved Hide resolved
.github/workflows/coverage.yml Outdated Show resolved Hide resolved
@YeungOnion YeungOnion force-pushed the test-ci branch 2 times, most recently from d702f11 to 7f83dcf Compare May 27, 2024 00:40
@YeungOnion
Copy link
Contributor Author

Should we get tests back to >90% before the release or not fail ci to get the release out?

Note: since v14 we've been below 90%, thinking maybe changing it to 80%?

@FreezyLemon
Copy link
Contributor

Does it make sense to enforce a coverage > x% as part of the CI pipeline for every PR? Codecov will create a report (see above) for all PRs which makes it obvious whether the coverage increases or decreases. Setting an arbitrary minimum could also lead to a situation like "current coverage is 95%, so it's fine if this PR drops it a little". But maybe I'm overthinking.

Higher coverage could still be a goal for the next release (or the one after that) even without the explicit CI check.

@FreezyLemon
Copy link
Contributor

BTW: Looks like moving from nightly to stable caused a permissions error. Could be xd009642/tarpaulin#406 and may be solved by moving to a non-docker solution. FYI taiki-e/install-action supports tarpaulin, so that could be a good alternative.

@YeungOnion YeungOnion changed the title feat: reintroduce coverage with tarpaulin and codecov.io feat: reintroduce coverage and upload to codecov.io May 28, 2024
@YeungOnion
Copy link
Contributor Author

Note: Still looking into this option, but I force-pushed to test the action.

In looking into taiki-e/install-action I found their nextest project, which appears more friendly and designed for CI.

It and the wrapper they developed to compiling with instrument-coverage, cargo-llvm-cov it seems capable of removing the need to rerun tests for collecting coverage data.

YeungOnion added a commit that referenced this pull request Jun 7, 2024
doc: remove codecov badge before release to crates.io
  badge is pending its correctness, see PR #229 for progress
YeungOnion added a commit that referenced this pull request Jun 8, 2024
doc: remove codecov badge before release to crates.io
  badge is pending its correctness, see PR #229 for progress
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.

None yet

2 participants