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 coverage testing #4765

Merged
merged 5 commits into from Feb 22, 2023
Merged

Add coverage testing #4765

merged 5 commits into from Feb 22, 2023

Conversation

lbdreyer
Copy link
Member

🚀 Pull Request

Description

This PR enables coverage testing using pytest-cov and coverage reporting using codacy.

There are a few different coverage reporting tools we can choose from:

  • coveralls (used in cartopy),
  • codecov (used in cf-units, iris-esmg-regrid, nc-time-axis, etc), and
  • codacy (not currently used although we seem to have it enabled but not running for cf-units)

I don't have a particularly strong preference between these tools, but a benefit of codacy (as @bjlittle pointed out) is that it provides extra features such as code quality inspection. It also has security scanning with bandit but that only seems to be available on the paid tiers unfortunately. Another benefit I have found when trialing these tools is that I have had difficulty with the codecov uploader and it would often fail silently, whereas codacy was quite simple to get working.

To implement codacy in this PR only required adding a single line to the .cirrus.yml file. Replacing this with a different coverage reporting tool (e.g. codecov) should be a simple change.

Outside of the changes in this PR, in order to get codacy working, we would also need to do the following:

  • Add the Iris repo to codacy (requires admin permissions which I don't have)
  • Add the CODACY_PROJECT_TOKEN to the Cirrus-CI settings (I tested this in my branch)

To see what it looks like on my branch you can see the main dashboard here and a report of coverage per files here

noxfile.py Show resolved Hide resolved
@trexfeathers trexfeathers self-assigned this May 31, 2022
Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

Thanks @lbdreyer! Codacy looks informative and worth a try.

A few minor points for you.


This definitely deserves a What's New entry. And while you're at it please could you do one for #4734 because we missed that one.

  • Add the CODACY_PROJECT_TOKEN to the Cirrus-CI settings (I tested this in my branch)

Assuming you mean via the Cirrus GUI, rather than having the token raw in a repo file?

  • Add the Iris repo to codacy (requires admin permissions which I don't have)

@bjlittle / @jamesp can you take care of this point? Thanks

lib/iris/tests/runner/_runner.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
requirements/ci/nox.lock/py38-linux-64.lock Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@bjlittle
Copy link
Member

@lbdreyer Fancy rebasing to pickup the migration of CI from cirrus-ci to GHA?

@trexfeathers trexfeathers removed their assignment Jul 5, 2022
@bouweandela
Copy link
Member

This would be a really nice feature to have!

In my experience, Codacy is good for linting pull requests (we've been using it for several years in the ESMValCore project now, see here), but for code coverage, Codecov has been more useful to us because it makes the coverage and coverage changes much more visible by writing a comment on the pull request (example). Since we started using Codecov a bit over a year ago, the amount of code without coverage we have has halved (see here).

@lbdreyer lbdreyer force-pushed the add_cov branch 2 times, most recently from f395458 to 2c0a437 Compare February 20, 2023 23:13
@bouweandela
Copy link
Member

Great to see some action again on this pull request!

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@11da71b). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4765   +/-   ##
=======================================
  Coverage        ?   89.25%           
=======================================
  Files           ?       88           
  Lines           ?    22197           
  Branches        ?     4858           
=======================================
  Hits            ?    19811           
  Misses          ?     1641           
  Partials        ?      745           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lbdreyer
Copy link
Member Author

This would be a really nice feature to have!

In my experience, Codacy is good for linting pull requests (we've been using it for several years in the ESMValCore project now, see here), but for code coverage, Codecov has been more useful to us because it makes the coverage and coverage changes much more visible by writing a comment on the pull request (example). Since we started using Codecov a bit over a year ago, the amount of code without coverage we have has halved (see here).

Thanks @bouweandela that is helpful to know about your experiences with codacy!

With that in mind I propose we use codecov and if at a later date we wish to explore codacy a bit more, we can look at implementing that. But for now codecov gives us what we want: coverage reporting on the PR

@lbdreyer
Copy link
Member Author

@trexfeathers
Thanks for reviewing this previously!

I have hopefully now addressed your comments.

I have also adopted codecov instead, which you can see from this report Note the report isn't showing anything yet as it doesn't have anything to compare to. Once this is merged, future PRs based of main should show the coverage change.

Do you mind taking another look?

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

Everything looks good, thanks @lbdreyer! But I know there's one more thing you want to do.

noxfile.py Outdated Show resolved Hide resolved
@trexfeathers trexfeathers merged commit 0b54d58 into SciTools:main Feb 22, 2023
@lbdreyer
Copy link
Member Author

Thanks @trexfeathers !

@bouweandela
Copy link
Member

Very nice! Now I can see which of the lines are covered by tests: #5015 (comment). That will make it much easier to understand if I can assume everything is fine or if I need to write extra tests when contributing.

tkknight added a commit to tkknight/iris that referenced this pull request Apr 22, 2023
* upstream/main: (23 commits)
  Lockfiles and pydata-sphinx-theme fix (SciTools#5188)
  Allow smarter weights (cubes, coordinates, cell measures, or ancillary variables) for aggregation (SciTools#5084)
  removed cell measure mask check and error (SciTools#5181)
  Updated environment lockfiles (SciTools#5177)
  Lazy weighted RMS calculation (SciTools#5017)
  Add coverage badge to README.md (SciTools#5176)
  Add coverage testing (SciTools#4765)
  Whats new updates for v3.4.1 .
  NetCDF thread safety take two (SciTools#5095)
  Updated environment lockfiles (SciTools#5163)
  Plugin support (SciTools#5144)
  Expand scope of common contributor links (SciTools#5159)
  Replace apparently retired UDUNITS documentation link. (SciTools#5153)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5150)
  Fixing typo's in Gitwash. (SciTools#5145)
  add readme #showyourstripes (SciTools#5141)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5143)
  Iris ❤ Xarray docs page. (SciTools#5025)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5136)
  Updated citation (SciTools#5116)
  ...
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

4 participants