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

CI/coverage: Fix addopts, migrate away from pytest-cov #5649

Conversation

bernhardkaindl
Copy link
Collaborator

@bernhardkaindl bernhardkaindl commented May 24, 2024

To: @stephenchengCloud and @liulinC: for you to review.
Cc: @ashwin9390 pre-review results with Ashwin:

  • Ashwin suggested an improvement to make the pytest report less verbose using -ra (applied!)

The motivation and purpose for this PR is to allow running pytest for a given file. Example:

pytest python3/tests/test_hfx_filename.py
=================================== test session starts ===================================
platform linux -- Python 3.11.9, pytest-8.2.1, pluggy-1.5.0 -- /usr/bin/python3.11
[...]
python3/tests/test_hfx_filename.py::TestRpc::test_db_get_uuid PASSED                [ 20%]
[...]
python3/tests/test_hfx_filename.py::TestParse::test_parse_string PASSED             [100%]

========================================= PASSES ==========================================
================================= short test summary info =================================
PASSED python3/tests/test_hfx_filename.py::TestRpc::test_db_get_uuid
[...]
PASSED python3/tests/test_hfx_filename.py::TestParse::test_parse_string
==================================== 5 passed in 0.04s ====================================

The expected result is to run only the tests in the given test case file (as shown above).
The unexpected (current) behaviour is that all tests are run.

Fix this issue by not forcing to run all tests using addopts:

pyproject.toml:

  • Don't use [tool.pytest.ini_options].addopts to pass test paths: addopts forces those options to be used every time pytest is run, which is very restrictive. Instead, use coverage run to configure coverage options, and support running specific tests by passing them as arguments to pytest: coverage run -m pytest python3/tests/test_xenapi.py

.pre-commit-config.yaml:

  • No longer rely on [tool.pytest.ini_options].addopts to use pytest-cov. Instead, use coverage run to run pytest with coverage, and then coverage xml to get an XML coverage dump of the coverage, as well as coverage html to generate html for viewing coverage locally, and coverage report to generate a brief textual report of the coverage.

    This improves the configuration to show the coverage report after the test results (and not in the middle of them).

@bernhardkaindl bernhardkaindl force-pushed the fix-pytest-addopts-obsolete-pytest-cov branch from dc4d8a3 to 9f1d306 Compare May 24, 2024 12:19
@bernhardkaindl
Copy link
Collaborator Author

bernhardkaindl commented May 24, 2024

Just as a comment, for detailed background:

Initially, using --cov <path> was easiest as it does not need other configuration settings.

Now, that we have the configuration settings for the coverage tool in pyproject.toml, we can use them. This allows us to decide to use one or the other.

Before this PR, you could only:

  • run pytest to run the tests and collect coverage.
    • but it was not obvious how to run only a specific test case for development.

With this PR applied, you can use these commands:

  • pytest to run the default tests as specified in testpaths = ["python3", "scripts", "ocaml/xcp-rrdd"] .
  • pytest <directories> and/or pytest <files> to run pytest for only the given directories or files.
    coverage run && coverage report to check coverage.

As a final test before commit or push:

With or without this PR, to run all test and get/check coverage, you can do:

  • pip install pre-commit (in case you didn't use it before) and then:
  • pre-commit run -av pytest to run pytest with coverage checks (coverage run and run the checks for coverage).
    This runs the pytest configuration in a dedicated environment that pre-commit takes care of.

Running pre-commit is also what GitHub CI now does to run the tests:

  • If you run: pre-commit run -av
    without adding the pytest argument, you run the same CI tests that GitHub CI runs.

@bernhardkaindl bernhardkaindl requested review from gangj and removed request for snwoods May 30, 2024 15:49
@bernhardkaindl
Copy link
Collaborator Author

Thanks stephenchengCloud, psafont and Ashwin for the approvals!

For anyone who reads this has write permission and give the final approval:

I'm looking for a final approval for this Python CI-only PR now.

Feel free to suggest a good approver as well.

This is only a small incremental update fix for Python CI
Background info:

We recently approved and merged two large PRs into feature/py3:

  • A big merge of the master branch into feature/py3
  • An update for configuring pytest to run in a defined Python environment locally (and to use the same Python environment in GitHub CI as well).

The idea for the latter is to be able to run one command (e.g. pre-commit, or pytest, or coverage run) to run the same checks that would run in the GitHub action locally.

The initial approach that I merged in the process configures the pytest configuration setting addopts in pyproject.toml to pass all options to get coverage and run all tests.

Now, after testing it more, when I tried to run a single test file like pytest path/to/file.py, I found that adding the files to addopts (as I did) was not good:

Passing the directories using addopts was bad because it forces pytest to always test those directories, even when you don't want that.

Instead, it's better to just use testpaths to configure the default directories for pytest. This is already configured, and we can remove the paths from addopts.

This fixes the issue of allowing to run only specific tests when that is needed.

The collection of coverage data is moved from using --cov path/to/testdir to use the main coverage.py tool itself directly. We also already configured what paths to test, so we just need to switch coverage collection to use coverage.py using the coverage run, and coverage xml commands (and so on).

The final part of the fix is to fix a problem in the configuration settings for coverage.py.

Only pyproject.toml and .pre-commit-config.yaml are updated to make the needed changes for it.

pyproject.toml Outdated
# For example:
# coverage run -m pytest python3/tests/test_xenapi.py
# Adding specific --cov options using addopts is not recommended as it would
# require to use they pytest-cov plugin, which would conflict with the use of
Copy link
Member

Choose a reason for hiding this comment

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

they -> the

Copy link
Collaborator

@liulinC liulinC left a comment

Choose a reason for hiding this comment

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

Please fix the typo which pointed by @minglumlu , thanks

@bernhardkaindl bernhardkaindl force-pushed the fix-pytest-addopts-obsolete-pytest-cov branch from 9f1d306 to d922db1 Compare May 31, 2024 10:13
@psafont
Copy link
Member

psafont commented May 31, 2024

The failure is because 2 of the ocaml packages provided by the repository got removed. The feature branch needs to be rebased on top of the master branch

@stephenchengCloud
Copy link
Contributor

The failure is because 2 of the ocaml packages provided by the repository got removed. The feature branch needs to be rebased on top of the master branch

Have raised a PR for syncing with the master branch. Please check #5665

pyproject.toml:
- Don't use [tool.pytest.ini_options].addopts to pass test paths:
  addopts forces those options to be used every time pytest is run,
  which is very restrictive. Instead, use `coverage run` to configure
  coverage options, and support running specific tests by passing
  them as arguments to pytest:
  coverage run -m pytest python3/tests/test_xenapi.py

.pre-commit-config.yaml:
- No longer rely on [tool.pytest.ini_options].addopts to use pytest-cov.
  Instead, use `coverage run` to run pytest with coverage, and then
  `coverage xml` to get an xml coverage dump of the coverage, as well as
  `coverage html` to generate html for viewing coverage locally, and
  `coverage report` to generate a brief textual report of the coverage.

  This also improves the configuration to show the coverage report
  after the test results have been shown and not in the middle of it.

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
@bernhardkaindl bernhardkaindl force-pushed the fix-pytest-addopts-obsolete-pytest-cov branch from d922db1 to 0e72cb6 Compare June 9, 2024 10:44
@bernhardkaindl bernhardkaindl merged commit fff26e5 into xapi-project:feature/py3 Jun 9, 2024
14 checks passed
@bernhardkaindl bernhardkaindl deleted the fix-pytest-addopts-obsolete-pytest-cov branch June 17, 2024 14:01
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.

6 participants