maintenance/improve test coverage #213
Conversation
|
@phlexbot format |
|
Automatic cmake-format fixes pushed (commit e60d811). |
|
@phlexbot format |
|
No automatic cmake-format fixes were necessary. |
|
Automatic clang-format fixes pushed (commit 2aa7957). |
|
@phlexbot python-fix |
|
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}
|
@phlexbot format |
|
No automatic clang-format fixes were necessary. |
|
Automatic cmake-format fixes pushed (commit bb954c8). |
|
@phlexbot python-fix |
|
Automatic Python linting fixes pushed (commit cfdb09a). |
greenc-FNAL
force-pushed
the
maintenance/improve-test-coverage
branch
2 times, most recently
from
ab62a60 to
a7c8fdd
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.
**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.
greenc-FNAL
force-pushed
the
maintenance/improve-test-coverage
branch
from
c9ccf06 to
1159796
Compare
|
@phlexbot format |
|
Automatic clang-format fixes pushed (commit b145a22). |
|
@phlexbot format |
|
Automatic cmake-format fixes pushed (commit cc928e7). |
|
No automatic clang-format fixes were necessary. |
There was a problem hiding this comment.
Pull request overview
This PR significantly enhances test coverage and robustness for Python/C++ interoperability in the Phlex framework. The changes focus on fixing memory safety issues, improving type conversion flexibility, and adding comprehensive tests for numeric types that were previously untested.
Key changes:
- Memory Safety Fixes: Introduced RAII-based Python object management (
PyObjectPtr) and fixed cyclic GC handling to prevent memory leaks and segfaults - Type Conversion Improvements: Replaced rigid NumPy-only converters with flexible implementations supporting both Python lists and NumPy arrays, fixed annotation parsing bugs, and corrected type string matching errors
- Test Coverage Expansion: Added comprehensive tests for floating-point types, unsigned integers, 64-bit integers, and NumPy array interoperability through new test suites (
py:vectypes,py:types)
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| test/python/verify_extended.py | New verifier classes for extended type testing (int, uint, long, ulong, float, double) |
| test/python/vectypes.py | New algorithms exercising NumPy array types with int32/64, uint32/64, float32/64 |
| test/python/test_types.py | New scalar type conversion tests for float, double, and unsigned int |
| test/python/verify.py | Code formatting updates (spacing, quotes) |
| test/python/sumit.py | Code formatting cleanup |
| test/python/reducer.py | Code formatting cleanup |
| test/python/adder.py | Code formatting cleanup |
| test/python/all_config.py | Code formatting cleanup |
| test/python/pyvectypes.jsonnet | Configuration for vector types test suite |
| test/python/pytypes.jsonnet | Configuration for scalar types test suite |
| test/python/source.cpp | Extended data providers for all numeric types with modulo arithmetic to prevent overflow |
| test/python/CMakeLists.txt | Added new test targets and improved PYTHONPATH handling for Spack environments |
| plugins/python/src/modulewrap.cpp | Major refactoring: RAII memory management, robust annotation parsing, flexible list/array converters, fixed type string matching |
| plugins/python/src/lifelinewrap.cpp | Added GC untracking before deallocation to fix memory safety issue |
| plugins/python/README.md | New comprehensive documentation explaining Python plugin architecture |
| plugins/python/CMakeLists.txt | Removed external packaging dependency, inline version parser |
| scripts/coverage.sh | Enhanced coverage workflow with better preset handling and Clang support |
| scripts/README.md | Updated documentation for coverage preset options |
| scripts/sarif-alerts.py | Formatting fix (blank line) |
| .github/copilot-instructions.md | Added development workflow guidelines and testing conventions |
| .github/actions/generate-build-matrix/generate_matrix.py | Formatting fix (blank line) |
| .devcontainer/devcontainer.json | New VS Code dev container configuration |
| .devcontainer/cmake_wrapper.sh | New environment setup wrapper for CMake in dev containers |
| .devcontainer/ctest_wrapper.sh | New environment setup wrapper for CTest in dev containers |
| .actrc | Configuration for local GitHub Actions testing |
| phlex.code-workspace | New VS Code workspace configuration with IDE settings |
| ci/Dockerfile | Improved Spack repository setup order and added mypy installation |
| CMakeLists.txt | Extended GCC 14+ warning suppressions, formatting cleanup |
|
|
||
| def __call__(self, value: "long") -> None: # type: ignore # noqa: F821 | ||
| """Check if value matches expected sum.""" | ||
| print(f"VerifierLong: value={value}, expected={self._sum_total}") |
There was a problem hiding this comment.
Debug print statement should be removed or converted to proper logging. Print statements in production code can clutter output and may impact performance in high-volume scenarios.
|
|
||
| def __call__(self, value: "float") -> None: | ||
| """Check if value matches expected sum.""" | ||
| sys.stderr.write(f"VerifierFloat: value={value}, expected={self._sum_total}\n") |
There was a problem hiding this comment.
Inconsistent logging output. This verifier writes to sys.stderr while VerifierDouble (line 90) writes to stdout via print(). Consider standardizing all debug output to use the same stream or removing debug statements entirely.
| // static std::mutex g_py_mutex; | ||
|
|
There was a problem hiding this comment.
Commented-out mutex code should either be removed or properly documented with a TODO/FIXME explaining why it's preserved. The presence of this code in multiple locations suggests incomplete refactoring or debugging code left in place.
| // static std::mutex g_py_mutex; |
| static PyObjectPtr vint_to_py(std::shared_ptr<std::vector<int>> const& v) | ||
| { | ||
| PyGILRAII gil; | ||
| // std::lock_guard<std::mutex> lock(g_py_mutex); |
There was a problem hiding this comment.
Commented-out mutex lock appears in multiple vector converter functions. This pattern repetition suggests these should either be removed entirely or consolidated with proper documentation explaining the threading model decision.
| message(STATUS "Python_SITELIB: ${Python_SITELIB}") | ||
| message(STATUS "Python_SITEARCH: ${Python_SITEARCH}") | ||
| set(TEST_PYTHONPATH ${CMAKE_CURRENT_SOURCE_DIR}) | ||
| # Always add site-packages to PYTHONPATH for tests, as embedded python might | ||
| # not find them especially in spack environments where they are in | ||
| # non-standard locations | ||
| if(Python_SITELIB) | ||
| set(TEST_PYTHONPATH ${TEST_PYTHONPATH}:${Python_SITELIB}) | ||
| endif() | ||
| if(Python_SITEARCH AND NOT "${Python_SITEARCH}" STREQUAL "${Python_SITELIB}") | ||
| set(TEST_PYTHONPATH ${TEST_PYTHONPATH}:${Python_SITEARCH}) | ||
| endif() | ||
|
|
||
| if(DEFINED ENV{VIRTUAL_ENV}) | ||
| set(TEST_PYTHONPATH ${TEST_PYTHONPATH}:${Python_SITELIB}:${Python_SITEARCH}) | ||
| # Keep this for backward compatibility or if it adds something else | ||
| endif() | ||
| set(TEST_PYTHONPATH ${TEST_PYTHONPATH}:$ENV{PYTHONPATH}) | ||
| message(STATUS "TEST_PYTHONPATH: ${TEST_PYTHONPATH}") |
There was a problem hiding this comment.
Debug print statements (lines 149-150, 166) should be removed. These message() calls will appear in all CMake configuration runs, cluttering output unnecessarily.
.devcontainer/ctest_wrapper.sh
Outdated
| @@ -0,0 +1,3 @@ | |||
| #!/bin/bash | |||
| . /entrypoint.sh | |||
| exec ctest "$@" | |||
There was a problem hiding this comment.
Shell script file should end with exactly one newline character and no trailing blank lines according to the project's text formatting standards.
|
|
||
| def __call__(self, value: "unsigned long") -> None: # type: ignore # noqa: F722 | ||
| """Check if value matches expected sum.""" | ||
| print(f"VerifierULong: value={value}, expected={self._sum_total}") |
There was a problem hiding this comment.
Debug print statement should be removed or converted to proper logging. Print statements in production code can clutter output and may impact performance in high-volume scenarios.
|
|
||
| def __call__(self, value: "double") -> None: # type: ignore # noqa: F821 | ||
| """Check if value matches expected sum.""" | ||
| print(f"VerifierDouble: value={value}, expected={self._sum_total}") |
There was a problem hiding this comment.
Debug print statement should be removed or converted to proper logging. Print statements in production code can clutter output and may impact performance in high-volume scenarios.
| static_assert(sizeof...(Args) == N, "Argument count mismatch"); | ||
|
|
||
| PyGILRAII gil; | ||
| // std::lock_guard<std::mutex> lock(g_py_mutex); |
There was a problem hiding this comment.
Commented-out mutex lock should either be removed or properly documented with a TODO/FIXME explaining why it's preserved. If thread safety is a concern, this should be addressed explicitly rather than leaving commented code.
| static_assert(sizeof...(Args) == N, "Argument count mismatch"); | ||
|
|
||
| PyGILRAII gil; | ||
| // std::lock_guard<std::mutex> lock(g_py_mutex); |
There was a problem hiding this comment.
Commented-out mutex lock should either be removed or properly documented with a TODO/FIXME explaining why it's preserved. If thread safety is a concern, this should be addressed explicitly rather than leaving commented code.
greenc-FNAL
force-pushed
the
maintenance/improve-test-coverage
branch
from
cc928e7 to
de2d683
Compare
- 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.
|
@phlexbot format |
|
No automatic cmake-format fixes were necessary. |
|
Automatic clang-format fixes pushed (commit 7a5919e). |
greenc-FNAL
force-pushed
the
maintenance/improve-test-coverage
branch
from
7a5919e to
3d5372d
Compare
|
@greenc-FNAL, thanks for the PR. Due to the size and scope of this PR, we'll need to give this a more careful look after the first prototype is released. |
- 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.
|
@phlexbot format |
|
No automatic clang-format fixes were necessary. |
|
Automatic cmake-format fixes pushed (commit 34f38f6). |
2 participants
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: