Fix Python coverage target environment and availability#318
Conversation
|
@phlexbot format |
|
Automatic markdownlint fixes pushed (commit 5a70083). |
|
No automatic jsonnetfmt fixes were necessary. |
|
No automatic clang-format fixes were necessary. |
|
Automatic cmake-format fixes pushed (commit 45ec36d). |
There was a problem hiding this comment.
Pull request overview
This PR updates the Python coverage configuration so coverage reports include the in-repo phlex Python package under plugins/python/python, not just the test/python directory.
Changes:
- Add an additional
--covpath forplugins/python/pythonin the pytest invocations used for coverage. - Expand
.coveragercsourceentries to include../../plugins/python/python. - Add a markdown write-up explaining the coverage issue and fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
test/python/CMakeLists.txt |
Adds --cov=${PROJECT_SOURCE_DIR}/plugins/python/python to the pytest command used when coverage is enabled. |
Modules/private/CreateCoverageTargets.cmake |
Adds the same --cov path to the coverage-python custom target. |
test/python/.coveragerc |
Extends source to include the plugins Python directory. |
COVERAGE_FIX_EXPLANATION.md |
Adds documentation describing the problem and solution. |
Comments suppressed due to low confidence (1)
COVERAGE_FIX_EXPLANATION.md:122
- File ends with an extra blank line; repo text-file rules require exactly one newline at EOF with no trailing blank lines. Remove the trailing empty line at the end of the document.
- __Codecov config__: `codecov.yml` (coverage reporting configuration)
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #318 +/- ##
==========================================
+ Coverage 80.19% 80.33% +0.13%
==========================================
Files 126 127 +1
Lines 3070 3102 +32
Branches 547 547
==========================================
+ Hits 2462 2492 +30
- Misses 381 385 +4
+ Partials 227 225 -2
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
greenc-FNAL
requested review from
Copilot and
knoepfel
and removed request for
greenc-FNAL
|
@greenc-FNAL, the coverage report indicates that the |
|
I'll interrogate the SARIF file and see if there's info there. It may not show up until after merge even if it's accounted for though. There's a few things I don't understand about this, yet. |
|
No automatic clang-format fixes were necessary. |
|
@copilot https://github.com/Framework-R-D/phlex/actions/runs/22006226496/job/63590495144 The workflows were run from |
The coverage-python target was inside the if(HAS_CPPYY) block, causing "ninja: error: unknown target 'coverage-python'" when cppyy is not available. Moved the target definition to after all environment setup (TEST_PYTHONPATH, PYTHON_TEST_ENVIRONMENT) so it's always available when pytest-cov is installed. Added fallback for when cppyy is not available: - Uses simpler pytest command for standalone Python tests - Runs unit_test_variant.py to test the phlex Python package - Still collects coverage for both test and plugins directories This ensures the coverage-python target exists regardless of cppyy availability. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Found the issue! The Fixed in commit 8292e5e by moving the
The target is now always available when |
|
@phlexbot format |
|
No automatic markdownlint fixes were necessary. |
|
No automatic jsonnetfmt fixes were necessary. |
|
Automatic cmake-format fixes pushed (commit a6008c3). |
|
No automatic clang-format fixes were necessary. |
|
@copilot Successful run this time, but it looks like we're monitoring test files for coverage? |
Updated .coveragerc to properly exclude all test files from coverage: - Removed '.' from source list (only measure plugins/python/python) - Added omit patterns for unit_test_*.py files - Added omit patterns for all files in test directories (*/test/*, */tests/*) This ensures only the actual phlex package code is measured, not test utilities like adder.py, reducer.py, verify.py, vectypes.py, etc. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
... Fixed in commit 95a08d1. The issue was that Changes:
Coverage now only measures the actual phlex package code, excluding test utilities like adder.py, reducer.py, verify.py, etc. |
|
@phlexbot format |
|
No automatic markdownlint fixes were necessary. |
|
No automatic cmake-format fixes were necessary. |
|
No automatic jsonnetfmt fixes were necessary. |
|
No automatic clang-format fixes were necessary. |
|
@knoepfel This is ready for review/merge. |
Fix Python Coverage Target Environment and Availability
Problem
Python coverage monitoring was not properly including
plugins/python/python/phlex/__init__.pyin reports, and test files were incorrectly included in coverage measurements.Root Causes
coverage-pythontargets were defined, causing conflictsPYTHONPATHwith Python site-packagesif(HAS_CPPYY)block, causing errors when cppyy unavailableSolution
test/python/CMakeLists.txt--cov=${PROJECT_SOURCE_DIR}/plugins/python/pythonto pytest commandcoverage-pythontarget outsideHAS_CPPYYblock for universal availabilityunit_test_variant.py)PYTHONPATH=${TEST_PYTHONPATH},PHLEX_INSTALL=${PYTHON_TEST_PHLEX_INSTALL}Modules/private/CreateCoverageTargets.cmakecoverage-pythontargettest/python/.coveragerc../../plugins/python/python(removed.to exclude test dir)unit_test_*.pyfiles*/test/*,*/tests/*)COVERAGE_FIX_EXPLANATION.mdResult
The
coverage-pythontarget now:plugins/python/python/phlex(not test files)✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.