pacificclimate / climdex.pcic

Routines to compute ETCCDI Climdex indices
GNU General Public License v3.0
22 stars 13 forks source link

Add R-CMD-check GitHub Actions Workflow #30

Closed QSparks closed 6 hours ago

QSparks commented 10 months ago

Description: This pull request adds a new GH Actions workflow named 'R-CMD-check' to the repository. The purpose of this workflow is to automate the process of checking the package for errors, warnings, or notes. The workflow uses a modified version of a prebuilt action check-standard from r-lib. It runs R CMD Check on Linux, MacOS, and Windows using the latest release of R and on R-devel.

Addresses an issue where R CMD Check fails due to a warning produced from the comparison (class(quantiles) != "list"). This PR Fixes the class validation using inherits() as a more robust and idiomatic approach.

Changes:

Testing and Verification:

Questions for Reviewer:

Resolves:

29

corviday commented 10 months ago

Would it be preferable to test only on Ubuntu for pushes to non-main branches, and then require cross-platform tests on merge with main?

This is mostly a matter of how long the checks take to run and whether you ever have to wait for them to finish. If they're fast, might as well run everything every time, there's not really a downside except time. It looks like your slowest check is sixteen minutes, which feels on the border to me, I could see going either way with it.

jameshiebert commented 10 months ago

Would it be preferable to test only on Ubuntu for pushes to non-main branches, and then require cross-platform tests on > merge with main?

Yeah, there are a couple ways to look at it. The vast majority of the time we're working on this code, I would expect that we're not doing any thing (or at least intending to do anything) that is platform-specific that would require multi-platform testing. So in a sense the extra testing would only slow down your development cycle while providing limited value. OTOH, if we do start down a path that is inadvertently platform-specific, we might want to know about it sooner rather than later.

I'd lean toward your suggestion (only test cross-platform on merges), but could be convinced either way.

QSparks commented 10 months ago

Thank you both for your replies @jameshiebert & @corviday. While I don't have details on our monthly minute usage, I checked the billing page and because of the extra overhead accrued for MacOS and Windows runners, I think it is in our best interest to reduce cross-platform testing during active development. I'll get started on a separate action to test on merges.

corviday commented 10 months ago

Huh, I didn't know (or had forgotten) that testing minutes were a limited resource.

QSparks commented 10 months ago

Alright, I’ve made a separate action that includes testing on MacOS and Windows for pull requests, and push events will only be checked on Ubuntu!