ClimateImpactLab / dscim

Data-Driven Spatial Climate Impact Model core component code
https://climateimpactlab.github.io/dscim/
Apache License 2.0
5 stars 2 forks source link

Only run pytests/black when dscim src is changed #255

Closed JMGilbert closed 5 months ago

JMGilbert commented 6 months ago

It's driving me a little crazy that the pytests run when I make a change only in the readme/changelog. Especially with the upcoming documentation addition, I think this'll streamline the workflow nicely. My goal with this PR is to only run the pytests when making a change to code.

see: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#onpushpull_requestpull_request_targetpathspaths-ignore

JMGilbert commented 6 months ago

Drawback is that there's no little green check next to PRs that don't make modifications in the src/ or test/ directories.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.63%. Comparing base (d4ab702) to head (5d81924).

:exclamation: Current head 5d81924 differs from pull request most recent head 49415d6. Consider uploading reports for the commit 49415d6 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #255 +/- ## ======================================= Coverage 65.63% 65.63% ======================================= Files 17 17 Lines 1845 1845 ======================================= Hits 1211 1211 Misses 634 634 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

brews commented 5 months ago

Thanks for bringing this up, @JMGilbert. Could you give more details on what sucks? Are commits on a single PR getting tested and holding things up, or something? Is it simply that tests are slow?

@kemccusker, My kneejerk response is to recommend not merging this because this misses some key cases, such as ensuring the package gets built as part of testing (so changes to pyproject.toml) or when dependabot bumps requirements.txt to test against upstream package releases. If we allowed exceptions for these cases we'd then need to remember to whitelist all test-worthy cases in the future. It's pretty likely we'd forget the subtleties of the CI config and the cost for screwing this up is breaking the package in a way that would have been avoidable. Generally, this kind of automated test is your last line of defense against some basic mistakes so you want to keep it easy to maintain and reliable.

If you're working on docs or such you can also test and make docs locally rather than burning github actions minutes. Then, write the PR (or remove "draft" status if you want to write it early) at the end when things are ready to go and you can let it do its final check. I think if you keep PRs as "draft" it doesn't start CI checks?... I don't actually remember, maybe that's something we could try.

I might be okay with blacklisting tests on README or CHANGELOG (w/ paths-ignore) instead of whitelisting everything else. I've seen automated tests catch bad file formatting and code in "examples" sections in README and CHANGELOG, though, (because the CHANGELOG and README were automatically mirrored to a page in readthedocs).

Other strategy is changing how testing is done. Clean up our tests to create tighter and more focused unit tests. Profile tests and see what's really slowing things down. Can we speed things up? Maybe only run a certain set of "smoke" tests on PRs, etc. I don't recommend this until tests run beyond 10 - 15 minutes and people start going nuts.

Hope this is helpful.

JMGilbert commented 5 months ago

@brews Makes sense. It's not too much of a frustration since it's not like I ever need to merge something within the ~7 minutes it takes to run the tests. I was mostly wanting to stop from repeating tests when I make incremental changes to docs while waiting for review.

I'll go ahead and close this. Maybe you all can rope the new RAs into making some of these nice design changes!