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

Disable collecting test coverage by default #2456

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Jun 11, 2024

Description

Collecting code coverage is time consuming. Disable it by default and enable it on request by running pytest --cov. This will save some time when developing locally and also reduces the runtime of the GitHub Actions tests by about 1 (Linux) to 2 (OSX) minutes.

Link to documentation: https://esmvaltool--2456.org.readthedocs.build/projects/ESMValCore/en/2456/contributing.html#test-coverage


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.83%. Comparing base (4b0dd41) to head (9fe2d82).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2456   +/-   ##
=======================================
  Coverage   94.83%   94.83%           
=======================================
  Files         251      251           
  Lines       14191    14191           
=======================================
  Hits        13458    13458           
  Misses        733      733           

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

@bouweandela bouweandela marked this pull request as ready for review June 11, 2024 10:09
@@ -3,7 +3,6 @@ addopts =
--mypy
--doctest-modules
--ignore=esmvalcore/cmor/tables/
--cov=esmvalcore
Copy link
Contributor

@valeriupredoi valeriupredoi Jun 11, 2024

Choose a reason for hiding this comment

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

OK, I like the idea of buying us time via not cov-outputting at CI level, but how much time is it saved at user-level? I find it very useful to have the coverage report immediately available after all tests have run, and I bet you my boots if one is not a serious developer, always interested in the coverage report, then they don't even bother running the tests. So I'd say remove it from the CI via --no-cov flag, but leave it for the user - what say you, bud?

Copy link
Member Author

Choose a reason for hiding this comment

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

I usually only look at the coverage report provided by Codecov because it shows me the relevant coverage changes as opposed to everything. I also run the tests way more often to check if I did not mess things up, e.g. before committing, then to check if I covered every changed line of code with tests. But I admit this is just a personal preference.

Let's ask the others: @ESMValGroup/technical-lead-development-team What is your experience with code coverage? Would you prefer to have it on or off on the command line by default? i.e. should the default be pytest --cov or pytest --no-cov? Running the tests with coverage takes 38 seconds on my laptop, while running them without coverage takes 24 seconds.

Copy link
Contributor

Choose a reason for hiding this comment

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

we may want to use --testmon and have both jam and peanut butter, it looks like the previous issues with --cov and --testmon have now been solved tarpas/pytest-testmon#86 NOTE: testmon allows coverage production/updating only for the bits of code that changed

Copy link
Contributor

Choose a reason for hiding this comment

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

Running the tests with coverage takes 38 seconds on my laptop, while running them without coverage takes 24 seconds.

Wow, it's more like ~3 minutes and ~2 minutes on my machine! 😮

My preference would be to have coverage run via a GitHub workflow, but not locally (since I like to test locally frequently, and not for coverage reasons, but I would like to make sure my coverage is good when creating a PR) 👍

Copy link
Member Author

@bouweandela bouweandela Jun 13, 2024

Choose a reason for hiding this comment

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

@ehogan Are you running the tests in parallel? Of your machine has multiple cpu cores you might be able to run the tests faster withpytest -n auto

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @bouweandela, pytest -n auto used 4 workers and reduced the timings to ~2 minutes and ~1 minute 👍 (I'm guessing my Linux machine isn't as good as yours!) 😝

Copy link
Contributor

Choose a reason for hiding this comment

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

nobody make comments about Linux boxes, my Ubuntu 18.04 is flying through those tests 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants