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.

Sign up for GitHub

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

Jump to bottom

Disable collecting test coverage by default #2456

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

Disable collecting test coverage by default #2456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
66fc8e7
Disable coverage collection by default
bouweandela Jun 11, 2024
65870a1
Update documentation
bouweandela Jun 11, 2024
010aae7
Run a GitHub action test to see difference
bouweandela Jun 11, 2024
7ef0796
Revert "Run a GitHub action test to see difference"
bouweandela Jun 11, 2024
9fe2d82
Merge branch 'main' of github.com:ESMValGroup/ESMValCore into disable…
bouweandela Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,18 @@ commands:
circleci step halt
fi
test_and_report:
parameters:
args:
type: string
default: ""
steps:
- run:
name: Run tests
command: |
mkdir -p test-reports
. /opt/conda/etc/profile.d/conda.sh
conda activate esmvaltool
pytest -n 4 --junitxml=test-reports/report.xml
pytest -n 4 --junitxml=test-reports/report.xml << parameters.args >>
esmvaltool version
- store_test_results:
path: test-reports/report.xml
Expand Down Expand Up @@ -127,8 +131,9 @@ jobs:
. /opt/conda/etc/profile.d/conda.sh
mkdir /logs
conda activate esmvaltool
pip install .[test] |& tee -a /logs/install.txt
- test_and_report
pip install .[test] > /logs/install.txt 2>&1
- test_and_report:
args: --cov
- save_cache:
key: test-{{ .Branch }}-{{ checksum "cache_key.txt" }}
paths:
Expand Down
13 changes: 10 additions & 3 deletions doc/contributing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,9 +470,16 @@ successful.
Test coverage
~~~~~~~~~~~~~

To check which parts of your code are `covered by unit tests`_, open the file
``test-reports/coverage_html/index.html`` (available after running a ``pytest``
command) and browse to the relevant file.
To check which parts of your code are `covered by unit tests`_, run the command

.. code-block:: bash

pytest --cov

and open the file ``test-reports/coverage_html/index.html`` and browse to the
relevant file. Note that tracking code coverage slows down the test runs,
therefore it is disabled by default and needs to be requested by providing
``pytest`` with the ``--cov`` flag.

CircleCI will upload the coverage results from running the tests to codecov and
Codacy.
Expand Down
2 changes: 1 addition & 1 deletion setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
addopts =
--doctest-modules
--ignore=esmvalcore/cmor/tables/
--cov=esmvalcore
Copy link
Contributor

@valeriupredoi valeriupredoi Jun 11, 2024 •
edited
Loading

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 •
edited
Loading

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 🤣

--cov-report=xml:test-reports/coverage.xml
--cov-report=html:test-reports/coverage_html
--html=test-reports/report.html
Expand All @@ -15,6 +14,7 @@ markers =

[coverage:run]
parallel = true
source = esmvalcore
[coverage:report]
exclude_lines =
pragma: no cover
Expand Down