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

Add GHA workflow to compute code coverage via examples #1785

Merged
merged 22 commits into from
May 21, 2024

Conversation

IndrajeetPatil
Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil commented Dec 1, 2022

Closes #1675

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.86%. Comparing base (fc2268f) to head (3f8f9ba).
Report is 2 commits behind head on main.

Current head 3f8f9ba differs from pull request most recent head e3e3794

Please upload reports for the commit e3e3794 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1785      +/-   ##
==========================================
+ Coverage   98.15%   98.86%   +0.71%     
==========================================
  Files         125      112      -13     
  Lines        5738     4841     -897     
==========================================
- Hits         5632     4786     -846     
+ Misses        106       55      -51     

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

@IndrajeetPatil IndrajeetPatil marked this pull request as ready for review May 17, 2024 11:57
@IndrajeetPatil IndrajeetPatil changed the title WIP: Add GHA workflow to compute code coverage via examples Add GHA workflow to compute code coverage via examples May 17, 2024
# examples present but not run
"R/lint.R",
"R/use_lintr.R",
# mostly internal utilities
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can work on removing some of these exceptions in the future.

@MichaelChirico
Copy link
Collaborator

Thanks for keeping at this PR.

I'm not sure at a high level how to think of this PR... are we expecting ourselves to achieve (asymptotically) 100% coverage, twice? (once through tests/ code and again through \examples{}?

Would it be better if we just focused on getting (asymptotically) 100% coverage between tests/\examples{}? i.e., including examples for coverage is just used as supplemental coverage to what we get from tests?

@IndrajeetPatil
Copy link
Collaborator Author

I'm not sure at a high level how to think of this PR... are we expecting ourselves to achieve (asymptotically) 100% coverage, twice?

No, I think the goals here are quite different-

  • code coverage through tests: targeted at developers and used as a check that most LOC are covered by unit tests
  • code coverage through examples: targeted at users and used as a check that the key parts of the API are covered well in examples

I don't think there is much to gain from achieving 100% code coverage in the latter case, and this is why we have such an extensive exceptions list. The goal is to make sure that we have enough examples for key functions, and code coverage is just a way to check that; maybe there is another way to check this, but I am not aware of one.

Does that sound reasonable?

@MichaelChirico
Copy link
Collaborator

Does it make sense to have such a test in CI? It feels like something we should monitor occasionally, but is not high importance to match on every PR.

In particular the long list of files_to_exclude seems like a maintenance headache to me.

Next, what does # nocov look like for examples? Would we have to hind them in \dontshow{} blocks to avoid cluttering the user-facing code?

@IndrajeetPatil
Copy link
Collaborator Author

Does it make sense to have such a test in CI?

I think so, yes. We have added linters for which either there were no examples or not sufficient number of examples.

It feels like something we should monitor occasionally, but is not high importance to match on every PR.

Sure, we don't need to run it on every PR. We could set up a cron job to run this workflow on a weekly or monthly basis on the default branch.

In particular the long list of files_to_exclude seems like a maintenance headache to me.

This is going to be a problem only if we frequently add new files that contain utility or internal-only functions. But, based on my experience in the last 2 years, this is rarely the case, and it may become increasingly rare going forward.

Next, what does # nocov look like for examples?

Can you give me an example where we will need to do this?
(Note that the 90% threshold is set for the entire codebase, and not on a file-by-file basis.)

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented May 20, 2024

Can you give me an example where we will need to do this?

Sorry, I was confusing myself. I guess what I had in mind is a separate sentinel like # nocovExamples we could define for this CI, that might also get us out of all the files_to_exclude logic, WDYT? (wouldn't need to be done in this PR if you agree, we can start adding those incrementally aftewards)

Sure, we don't need to run it on every PR. We could set up a cron job to run this workflow on a weekly or monthly basis on the default branch.

Yes, let's go with monthly for now.

@IndrajeetPatil
Copy link
Collaborator Author

I guess what I had in mind is a separate sentinel like # nocovExamples we could define for this CI, that might also get us out of all the files_to_exclude logic, WDYT?

Yes, that's something we can slowly work on.

Copy link
Collaborator

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

LGTM to try it out, thanks for your persistence!

@MichaelChirico MichaelChirico merged commit ea1685b into main May 21, 2024
20 checks passed
@MichaelChirico MichaelChirico deleted the 1675_workflow_examples branch May 21, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GHA for examples coverage?
3 participants