maintenance/improve test coverage #213
Open
+2,095
−335
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@phlexbot format |
Contributor
|
Automatic cmake-format fixes pushed (commit e60d811). |
Contributor
Author
|
@phlexbot format |
Contributor
|
No automatic cmake-format fixes were necessary. |
Contributor
|
Automatic clang-format fixes pushed (commit 2aa7957). |
Contributor
Author
|
@phlexbot python-fix |
Contributor
|
Automatic Python linting fixes pushed (commit 092dec4). |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (63.39%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #213 +/- ##
==========================================
+ Coverage 76.27% 78.03% +1.75%
==========================================
Files 124 124
Lines 2748 3132 +384
Branches 476 561 +85
==========================================
+ Hits 2096 2444 +348
- Misses 454 456 +2
- Partials 198 232 +34
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
greenc-FNAL
force-pushed
the
maintenance/improve-test-coverage
branch
from
38fc738 to
47661fa
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Author
|
@phlexbot format |
Contributor
|
No automatic clang-format fixes were necessary. |
Contributor
|
Automatic cmake-format fixes pushed (commit bb954c8). |
Contributor
Author
|
@phlexbot python-fix |
Contributor
|
Automatic Python linting fixes pushed (commit cfdb09a). |
greenc-FNAL
force-pushed
the
maintenance/improve-test-coverage
branch
3 times, most recently
from
c9ccf06 to
1159796
Compare
Contributor
Author
|
@phlexbot format |
Contributor
|
Automatic clang-format fixes pushed (commit b145a22). |
Contributor
Author
|
@phlexbot format |
Contributor
|
Automatic cmake-format fixes pushed (commit cc928e7). |
Contributor
|
No automatic clang-format fixes were necessary. |
This was referenced Jan 12, 2026
Merged
greenc-FNAL
force-pushed
the
maintenance/improve-test-coverage
branch
from
9b1fa7f to
6fbd59d
Compare
Refactors the `coverage.sh` script to align its behavior, particularly for the Clang toolchain, with the CI coverage workflow. Key changes: - Sets the `LLVM_PROFILE_FILE` environment variable during the test phase for Clang to ensure correct collection of raw profile data, mirroring the CI setup. - Changes the default preset from `coverage-gcc` to `coverage-clang` to match the CI standard. - Adds HTML report generation for the Clang toolchain, bringing it to feature parity with the GCC preset within the script. - Simplifies the logic for the Clang path by removing irrelevant and incorrect GCC-specific staleness checks. - Improves the argument parsing to be more robust and reliable. Refactors the coverage.sh script to align its behavior with the CI workflow. - Implements support for uploading LLVM/Clang coverage reports to Codecov. - Improves the help text to clarify the differences between the GCC and Clang workflows. - Adds validation for the --preset flag. - Changes the exit code to 1 when no commands are provided. - Improves comments and file URL generation.
- Add pytest-cov integration to test/python/CMakeLists.txt - Update CreateCoverageTargets.cmake to add coverage-python target - Add python command to coverage.sh script - Update .github/workflows/coverage.yaml to collect Python coverage - Include Python coverage files in artifact bundles and Codecov uploads Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
- Add PHLEX_INSTALL environment variable to coverage-python target - Update documentation in CreateCoverageTargets.cmake header - Ensure Python coverage target has necessary environment setup Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
- Add python command to coverage.sh commands table - Document Python coverage requirements (pytest, pytest-cov) - Add example for Python-only coverage generation - Update command descriptions to note C++ and Python support Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
- Remove unused HAS_PYTEST variable from test/python/CMakeLists.txt - Remove unused PYTEST_EXECUTABLE find_program call - Use Python3_EXECUTABLE -m pytest consistently throughout Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
- Add fallback detection for both python3 and python commands - Use detected Python command for pytest-cov check - Make script more robust across different Python installations Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
- Change from find_package(Python3) to find_package(Python 3.12) - Replace all Python3_FOUND and Python3_EXECUTABLE references with Python_* - Ensure consistent Python interpreter detection across all CMake files - Aligns CreateCoverageTargets.cmake with test/python/CMakeLists.txt Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
… handling - Fixed critical bug in check_codeql_alerts.py where alerts with valid locations were not being added to results - Added validation in workflow to handle missing comment_path gracefully - Enhanced error messages for fork PRs with instructions on viewing alerts - Updated CodeQL-README.md with comprehensive documentation for viewing alerts from fork PRs Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
**Files:** modulewrap.cpp, lifelinewrap.cpp
* **Feature: RAII for Python Objects**
* **Change:** Introduced `PyObjectPtr` (a `std::shared_ptr` alias with a custom `PyObjectDeleter`) to manage Python object reference counts.
* **Rationalization:** Manual reference counting (`Py_INCREF`/`Py_DECREF`) is error-prone, especially in the presence of C++ exceptions. If an exception is thrown, manual decrements might be skipped, leading to memory leaks.
* **Resolution:** `PyObjectPtr` ensures that `Py_DECREF` is called automatically when the pointer goes out of scope, even during stack unwinding.
* **Fix: Robust Annotation Parsing**
* **Change:** Rewrote the argument parsing logic in `parse_args` to iterate over the `__annotations__` dictionary using `PyDict_Next` and explicitly skip the `"return"` key.
* **Root Cause:** The previous implementation relied on `PyDict_Values`, which returns all values including the return type annotation. Depending on dictionary iteration order (which can vary or be insertion-ordered), the return type could be mistakenly interpreted as an input argument type.
* **Diagnosis:** Likely diagnosed by observing type mismatch errors when Python functions had return type annotations.
* **Fix: Flexible Input Conversion (List vs. NumPy)**
* **Change:** Replaced rigid macro-based vector converters with explicit implementations (`py_to_vint`, `py_to_vuint`, etc.) that accept both Python `list` and NumPy `ndarray` objects.
* **Root Cause:** The previous converters strictly expected NumPy arrays. Users passing standard Python lists would cause runtime errors or type mismatches.
* **Resolution:** The new converters check the input type (`PyList_Check` vs `PyArray_Check`) and handle data extraction accordingly.
* **Fix: Memory Safety in Cyclic GC**
* **Change:** Added `PyObject_GC_UnTrack(pyobj)` in `ll_dealloc` (`lifelinewrap.cpp`).
* **Root Cause:** Python objects that support cyclic garbage collection must be untracked before deallocation to prevent the GC from visiting invalid memory. Missing this can lead to segfaults during interpreter shutdown or garbage collection cycles.
* **Fix: Type String Matching**
* **Change:** Replaced brittle fixed-offset string comparisons (e.g., `inp_type.compare(pos, ...)`) with robust substring searching (`suffix.find(...)`). Corrected a typo where `double64]]` was checked instead of `float64]]`.
* **Root Cause:** The fixed-offset logic assumed a specific string format for type signatures, which could break if the format changed slightly. The typo prevented `float64` arrays from being correctly identified.
**Files:** CMakeLists.txt, CMakeLists.txt
* **Enhancement: Reduced Build Dependencies**
* **Change:** Removed the dependency on the external `packaging` Python module in CMakeLists.txt.
* **Rationalization:** The build system previously used `packaging.version` to check module versions. This required the `packaging` library to be installed in the build environment.
* **Resolution:** Implemented a lightweight, inline version parser (splitting strings by `.`) to perform the check using only the standard library.
* **Fix: GCC 14+ Warning Suppression**
* **Change:** Added `-Wno-maybe-uninitialized` to compile options for GCC 14.1+.
* **Root Cause:** Newer GCC versions have more aggressive static analysis that produces false positives for uninitialized variables in complex C++ templates used by the project.
**Files:** copilot-instructions.md
* **New Feature:** Added a comprehensive instructions file for GitHub Copilot.
* **Rationalization:** To standardize the behavior of AI assistants working in the repository, ensuring they follow project-specific coding standards (formatting, error handling) and workflow guidelines.
User: Now, please summarize test code changes and additions, including rationale for the test, how they improve test coverage, any problems they exposed, and what categories of regression they might detect in future code.
GitHub Copilot: Ran terminal command: git show 15cac5f test/python/
**Files:** vectypes.py, test_types.py, pyvectypes.jsonnet, pytypes.jsonnet, verify_extended.py
* **Rationale:**
* The existing tests primarily covered basic integer and string types.
* There was a gap in coverage for:
* **Floating point types** (`float`, `double`).
* **Unsigned integers** (`unsigned int`, `unsigned long`).
* **64-bit integers** (`long`, `int64_t`).
* **NumPy array interoperability** (passing vectors from C++ to Python as NumPy arrays).
* These tests were added to verify the robustness of the new `modulewrap.cpp` converters.
* **Coverage Improvements:**
* **`py:types`**: Validates scalar type conversion between C++ and Python for `float`, `double`, and `unsigned int`.
* **`py:vectypes`**: Validates vector/array conversion. It tests:
* Creation of NumPy arrays from scalar inputs (`collectify_*`).
* Summation of NumPy arrays back to scalars (`sum_array_*`).
* Handling of all major numeric types: `int32`, `uint32`, `int64`, `uint64`, `float32`, `float64`.
* **verify_extended.py**: Introduces specialized verifiers (`VerifierFloat`, `VerifierUInt`, etc.) that handle type-specific assertions (e.g., epsilon comparison for floats).
* **Problems Exposed:**
* **Integer Overflow/Underflow**: The `py:vectypes` test exposed a logic error in source.cpp where large 64-bit hashes were being used in arithmetic (`100 - id`), causing underflow for unsigned types and wrapping for signed types. This was fixed by introducing modulo arithmetic to keep values small and predictable.
* **Type Mismatches**: The strict type checking in the new tests likely exposed the need for the robust annotation parsing and explicit type converters implemented in `modulewrap.cpp`.
* **Regression Detection:**
* **Type Conversion Breakages**: These tests will fail if future changes to `modulewrap.cpp` break the mapping between C++ types (like `std::vector<int>`) and Python types (like `numpy.ndarray` or `list`).
* **Precision Loss**: The float/double tests will catch regressions where 64-bit precision is accidentally truncated to 32-bit.
* **Sign Errors**: The unsigned integer tests will detect if unsigned values are incorrectly cast to signed values (e.g., treating `UINT_MAX` as `-1`).
**Files:** CMakeLists.txt, source.cpp
* **Rationale:**
* To support the new tests and ensure the test environment is consistent with real-world usage.
* To fix flaky or incorrect test data generation.
* **Changes:**
* **CMakeLists.txt**:
* Added `py:vectypes` and `py:types` to the test suite.
* Enhanced `PYTHONPATH` setup to explicitly include `Python_SITELIB` and `Python_SITEARCH`. This ensures tests running in embedded environments (like Spack) can find installed packages.
* Replaced the external `packaging` dependency with a simple inline version parser for the module check.
* **source.cpp**:
* Expanded the C++ data provider to generate all required types (`float`, `double`, `uint`, `int64`, `uint64`).
* **Fix**: Changed data generation logic from `id.number()` to `id.number() % N` to prevent integer overflow and ensure deterministic summation results.
**Files:** adder.py, all_config.py, reducer.py, sumit.py, verify.py
* **Rationale:**
* To comply with the project's stricter linting rules (`ruff`, `mypy`) introduced in this commit.
* **Changes:**
* **Formatting**: Applied standard Python formatting (whitespace, indentation).
* **Linting**: Fixed issues like:
* Comparison to `False` (changed `== False` to `is False` or kept as is with `# noqa` if intentional for testing).
* Missing docstrings or blank lines.
* Unused imports.
* **Type Hinting**: Added or corrected type hints to satisfy `mypy`.
* **Regression Detection:**
* **Static Analysis**: By enforcing these standards, the CI pipeline can now detect syntax errors, undefined variables, and type inconsistencies before tests are even run.
Apply clang-format fixes
Apply cmake-format fixes
Improve Python list support and test coverage
- plugins/python/src/modulewrap.cpp:
- Added support for `list['double']` string representation in input converters.
- Implemented output transformation support for `list[double]` and `list['double']`, mapping them to `py_to_vdouble`.
- This enables Phlex to correctly handle Python lists of floats/doubles when strict typing is required.
- test/python/vectypes.py:
- Refactored to include specific helper functions (e.g., `collectify_float64_list`, `sum_list_int64`) that return standard Python lists instead of NumPy arrays.
- Updated registration logic to use distinct node names (e.g., `name="sum_int32"`) to prevent collisions between list and NumPy test cases.
- This ensures that the new C++ list conversion paths are explicitly exercised.
- test/python/pyveclists.jsonnet:
- Added a new test configuration to run the list-specific vector tests.
- test/python/CMakeLists.txt:
- Registered the new `py:veclists` test.
- test/demo-giantdata/unfold_transform_fold.cpp:
- Increased workload (n_spills from 10 to 100) to fix a race condition in the pipelined execution test.
- The test verifies that `fold` operations begin before `unfold` operations complete.
- Previously, the workload was too small, allowing the `unfold` phase to finish completely before the `fold` phase started, causing the test to falsely fail by observing "batched" instead of "pipelined" execution.
Effect on Coverage:
- The changes significantly improve coverage in `plugins/python/src/modulewrap.cpp`.
- Specifically, the `py_to_vdouble` function now has confirmed execution hits on the list processing path (previously 0 hits).
- The `py_to_vint` function also shows increased coverage for list processing.
- This confirms that the bindings now correctly intercept and convert Python list objects to Phlex vector types.
Fix CodeQL alert: Empty except block in verify_extended.py
Apply clang-format fixes
Improve Python test coverage
- Added test/python/test_mismatch.py and pymismatch.jsonnet to test error handling in modulewrap.cpp when input labels and types do not match.
- Added test/python/test_coverage.py and pycoverage.jsonnet to test list[int], list[float], and list[double] input converters.
- Updated test/python/CMakeLists.txt to include the new tests and fixed the regex for py:mismatch to match the actual error message.
Apply cmake-format fixes
- Added variant.py helper from PR Framework-R-D#245 - Modified modulewrap.cpp to recognize Variant wrapper via phlex_callable - Updated adder.py to use Variant helper for type-specific registration - Removed debug print statements from verify_extended.py - Removed commented-out mutex code from modulewrap.cpp - Removed debug message() calls from CMakeLists.txt - Fixed LaTeX syntax in copilot-instructions.md (use Unicode ↔) Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
- Fixed docstring in adder.py to reference 'Addable' instead of 'Number' - Fixed error message in variant.py to use correct class name 'Variant' - Added clarifying comments in modulewrap.cpp about reference counting Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
…th metaclass - Add type aliases for C++ types (unsigned_int, unsigned_long, long, double) - Use metaclass to set __name__ property correctly for C++ wrapper identification - Update function signatures to use type aliases instead of string annotations - Fix mypy unreachable code warnings by adding union types for array/list params - All ruff and mypy checks now pass Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
- Remove unnecessary complexity in __name__ property getter - Always set _cpp_name in __new__ with fallback to class name - Makes code more maintainable and easier to understand Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
…ettings Fix ruff F722 and mypy errors for Python 3.12+ type annotations in vectypes.py
greenc-FNAL
force-pushed
the
maintenance/improve-test-coverage
branch
from
6fbd59d to
5e1d2f2
Compare
Contributor
|
Once this one is in, I have additional coverage improvements here: #245 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Non-test changes:
1. Python/C++ Interoperability Enhancements
Files: modulewrap.cpp, lifelinewrap.cpp
Feature: RAII for Python Objects
PyObjectPtr(astd::shared_ptralias with a customPyObjectDeleter) to manage Python object reference counts.Py_INCREF/Py_DECREF) is error-prone, especially in the presence of C++ exceptions. If an exception is thrown, manual decrements might be skipped, leading to memory leaks.PyObjectPtrensures thatPy_DECREFis called automatically when the pointer goes out of scope, even during stack unwinding.Fix: Robust Annotation Parsing
parse_argsto iterate over the__annotations__dictionary usingPyDict_Nextand explicitly skip the"return"key.PyDict_Values, which returns all values including the return type annotation. Depending on dictionary iteration order (which can vary or be insertion-ordered), the return type could be mistakenly interpreted as an input argument type.Fix: Flexible Input Conversion (List vs. NumPy)
py_to_vint,py_to_vuint, etc.) that accept both Pythonlistand NumPyndarrayobjects.PyList_CheckvsPyArray_Check) and handle data extraction accordingly.Fix: Memory Safety in Cyclic GC
PyObject_GC_UnTrack(pyobj)inll_dealloc(lifelinewrap.cpp).Fix: Type String Matching
inp_type.compare(pos, ...)) with robust substring searching (suffix.find(...)). Corrected a typo wheredouble64]]was checked instead offloat64]].float64arrays from being correctly identified.2. Build System & CI Improvements
Files: CMakeLists.txt, CMakeLists.txt
Enhancement: Reduced Build Dependencies
packagingPython module in CMakeLists.txt.packaging.versionto check module versions. This required thepackaginglibrary to be installed in the build environment..) to perform the check using only the standard library.Fix: GCC 14+ Warning Suppression
-Wno-maybe-uninitializedto compile options for GCC 14.1+.3. Documentation
Files: copilot-instructions.md
Test code changes and additions:
1. New Tests:
py:vectypesandpy:typesFiles: vectypes.py, test_types.py, pyvectypes.jsonnet, pytypes.jsonnet, verify_extended.py
Rationale:
float,double).unsigned int,unsigned long).long,int64_t).modulewrap.cppconverters.Coverage Improvements:
py:types: Validates scalar type conversion between C++ and Python forfloat,double, andunsigned int.py:vectypes: Validates vector/array conversion. It tests:collectify_*).sum_array_*).int32,uint32,int64,uint64,float32,float64.VerifierFloat,VerifierUInt, etc.) that handle type-specific assertions (e.g., epsilon comparison for floats).Problems Exposed:
py:vectypestest exposed a logic error in source.cpp where large 64-bit hashes were being used in arithmetic (100 - id), causing underflow for unsigned types and wrapping for signed types. This was fixed by introducing modulo arithmetic to keep values small and predictable.modulewrap.cpp.Regression Detection:
modulewrap.cppbreak the mapping between C++ types (likestd::vector<int>) and Python types (likenumpy.ndarrayorlist).UINT_MAXas-1).2. Test Infrastructure Enhancements
Files: CMakeLists.txt, source.cpp
Rationale:
Changes:
py:vectypesandpy:typesto the test suite.PYTHONPATHsetup to explicitly includePython_SITELIBandPython_SITEARCH. This ensures tests running in embedded environments (like Spack) can find installed packages.packagingdependency with a simple inline version parser for the module check.float,double,uint,int64,uint64).id.number()toid.number() % Nto prevent integer overflow and ensure deterministic summation results.3. Code Quality & Modernization
Files: adder.py, all_config.py, reducer.py, sumit.py, verify.py
Rationale:
ruff,mypy) introduced in this commit.Changes:
False(changed== Falsetois Falseor kept as is with# noqaif intentional for testing).mypy.Regression Detection: