Simplify tests/helpers/maturin.py: inline pin checks, consolidate wheel snapshot (#97, #99) - #240
Simplify tests/helpers/maturin.py: inline pin checks, consolidate wheel snapshot (#97, #99)#240leynos wants to merge 4 commits into
Conversation
tests/helpers/maturin.py had grown to 394 lines / 15 defs serving one consumer (cuprum/unittests/test_maturin_build.py). Per #97 and #99: - Inline all pin-reading logic (read_expected_maturin_version, read_maturin_pins, read_manylinux_aarch64_container_ref, workflow_uses_manylinux_aarch64_container_ref, _require_pin_match) and its regexes directly into the sole consumer as private helpers. - Consolidate wheel-snapshot parsing from six functions (_header_value, _parse_metadata, _normalise_wheel_entry, _locate_dist_info_wheel, _parse_wheel_header, wheel_build_snapshot) to three by folding the single-purpose extractors into wheel_build_snapshot and _parse_metadata. - Fold _maturin_module_available into toolchain_available. - Document the defer-until-second-consumer policy in the module docstring. The wheel entry-normalisation is retained unchanged: every branch (dist-info suffixes, /sboms/, the .cpython-<platform>.so extension) is exercised by the test_maturin_wheel_build_snapshot syrupy snapshot across the build matrix, so none is speculative. Snapshot output is byte- identical. Helper drops to 218 lines / 6 defs. Closes #97 Closes #99 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideSimplifies the maturin test helper by inlining pin- and workflow-related logic into its sole test consumer and collapsing wheel snapshot parsing into fewer, more cohesive functions, while tightening the toolchain availability check and documenting reuse policy. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughChangesInline maturin and manylinux pin validation in the build tests, refactor wheel metadata and archive-member parsing, and document the narrower shared-helper boundary. Add coverage for wheels missing required metadata members. Maturin helper simplification
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the pin-synchronisation helpers in
test_maturin_build.py,pyproject.tomland the workflow/action YAML files are read multiple times within the same test run (e.g.,_read_expected_maturin_versionand_read_maturin_pinseach re-openpyproject.toml), so consider caching these file contents per test module (e.g., via a small module-level cache or a fixture) to avoid redundant I/O and keep the helpers simpler. - In
wheel_build_snapshot, the inline logic for locating the.dist-info/WHEELentry and deriving theMETADATAentry is now embedded directly in the function; if you intend to reuse this logic elsewhere, consider extracting a small internal helper (e.g.,_dist_info_paths) to clarify the intent and make the error handling around missing WHEEL/METADATA entries easier to test and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the pin-synchronisation helpers in `test_maturin_build.py`, `pyproject.toml` and the workflow/action YAML files are read multiple times within the same test run (e.g., `_read_expected_maturin_version` and `_read_maturin_pins` each re-open `pyproject.toml`), so consider caching these file contents per test module (e.g., via a small module-level cache or a fixture) to avoid redundant I/O and keep the helpers simpler.
- In `wheel_build_snapshot`, the inline logic for locating the `.dist-info/WHEEL` entry and deriving the `METADATA` entry is now embedded directly in the function; if you intend to reuse this logic elsewhere, consider extracting a small internal helper (e.g., `_dist_info_paths`) to clarify the intent and make the error handling around missing WHEEL/METADATA entries easier to test and maintain.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6494aa38bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Consolidating the wheel parsing pushed wheel_build_snapshot to a cyclomatic complexity of 10 (threshold 9) and the module mean to 4.43 (threshold 4). Extract _read_wheel_members (archive read + member lookup) and _parse_wheel_header (generator + Root-Is-Purelib), leaving wheel_build_snapshot as a thin orchestrator. Behaviour and snapshot output are unchanged; CodeScene health returns to 10.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The developers guide still described tests/helpers/maturin.py as providing shared helpers for the pin-contract tests, which is the opposite of the boundary this branch implements: the pin checks are now private helpers in cuprum/unittests/test_maturin_build.py and only the wheel build/snapshot machinery remains shared. Rewrite the section intro to state both halves of the split, name the retained module's narrow scope, and record the defer-until-second-consumer reuse policy the abstraction policy in AGENTS.md requires. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 1412-1415: Update the wheel builder scope description around
build_native_wheel_artifact, toolchain_available, and wheel_build_snapshot to
state that build_native_wheel_artifact uses the maturin module available to the
current Python interpreter rather than selecting a pinned version. Also describe
the pin check and snapshot generator assertion as the mechanisms enforcing
version alignment.
In `@tests/helpers/maturin.py`:
- Around line 164-178: In _read_wheel_members, validate that the derived
metadata_name appears in entry_names before calling archive.read(metadata_name).
Raise the documented AssertionError with the missing .dist-info/METADATA message
when absent, while preserving the existing read and decode flow when present.
- Around line 71-80: The public function toolchain_available in
tests/helpers/maturin.py (lines 71-80) needs a NumPy-style Returns section
documenting that it returns True only when cargo, rustc, and the maturin module
are available. Add the corresponding Returns section to the other changed public
function in tests/helpers/maturin.py (lines 204-208), documenting its stable
snapshot mapping for a wheel.
- Line 127: Update the changed docstrings in the relevant parsing helpers,
including the one describing RFC 2822 METADATA headers and the sections at the
referenced locations, to use Oxford -ize spellings: replace “normalised”,
“Normalise”, and “normalises” with “normalized”, “Normalize”, and “normalizes”
while preserving the existing documentation content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c88aed4c-c59f-41bd-9c32-9cbed57e0797
📒 Files selected for processing (3)
cuprum/unittests/test_maturin_build.pydocs/developers-guide.mdtests/helpers/maturin.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
|
The Testing (Unit And Behavioural) row came back What this PR changes. It is a pure refactor with no behaviour change: pin-reading logic moves from Strongest evidence — the snapshot is byte-identical. Coverage of the changed surface. All 14 tests in the module pass, and every retained and inlined symbol is exercised:
Gates. No new behaviour is introduced by this PR, so no new tests are warranted beyond the existing suite continuing to pass against the refactored code — which it does. |
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Changed files =='
git diff --name-only origin/main...HEAD -- \
cuprum/unittests/test_maturin_build.py \
tests/helpers/maturin.py \
docs/developers-guide.md
printf '%s\n' '== Test module outline =='
ast-grep outline cuprum/unittests/test_maturin_build.py --items all \
--match 'maturin|pin|workflow|container' --view expanded || true
printf '%s\n' '== Helper module outline =='
ast-grep outline tests/helpers/maturin.py --items all \
--match 'wheel|member|metadata|snapshot' --view expanded || true
printf '%s\n' '== Pin-reading helpers and calls =='
rg -n -A18 -B4 \
'def _read_(expected_maturin_version|maturin_pins|manylinux_aarch64_container_ref)|def _workflow_uses_manylinux_aarch64_container_ref|read_text\(' \
cuprum/unittests/test_maturin_build.py
printf '%s\n' '== Wheel member extraction and snapshot use =='
rg -n -A35 -B5 \
'def _read_wheel_members|def wheel_build_snapshot|ZipFile|dist-info/WHEEL|METADATA' \
tests/helpers/maturin.pyLength of output: 1413 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Test module: relevant declarations and uses =='
rg -n -A24 -B5 \
'^(def _read_expected_maturin_version|def _read_maturin_pins|def _read_manylinux_aarch64_container_ref|def _workflow_uses_manylinux_aarch64_container_ref)|\b(_read_expected_maturin_version|_read_maturin_pins|_read_manylinux_aarch64_container_ref|_workflow_uses_manylinux_aarch64_container_ref)\(' \
cuprum/unittests/test_maturin_build.py
printf '%s\n' '== Test module: every filesystem read =='
rg -n -C3 '\.read_text\(|open\(' cuprum/unittests/test_maturin_build.py || true
printf '%s\n' '== Helper module: wheel member extraction and caller =='
rg -n -A45 -B6 \
'^(def _read_wheel_members|def wheel_build_snapshot)|ZipFile|dist-info/WHEEL|METADATA' \
tests/helpers/maturin.pyLength of output: 13460
|
Review round on the helper simplification: - _read_wheel_members derived the METADATA member name and read it without checking membership, so a wheel missing that member raised KeyError from ZipFile.read rather than the AssertionError wheel_build_snapshot documents. Assert membership first, matching the existing WHEEL guard, and cover both missing-member cases with a parametrised test. Verified non-vacuous: removing the guard reproduces the KeyError the finding predicted. - build_native_wheel_artifact runs `python -m maturin` under the current interpreter, so it uses whichever maturin that environment provides; it does not select a pinned version. Correct that claim in both the docstring and the developers guide, and name the two mechanisms that actually enforce alignment: the installed-CLI pin check and the snapshot test's Generator assertion. - Add NumPy-style Returns sections to toolchain_available and wheel_build_snapshot, matching the Parameters/Raises sections already used in this module. - Move docstring prose to Oxford -ize spelling per AGENTS.md. Identifiers keep the -ise spelling that prevails across the repository, since the rule governs comments rather than names. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 1403-1410: Update the regex ownership wording in the documentation
around the “shared regex” reference so it identifies the private test-local
regexes in test_maturin_build.py, consistent with the documented helpers and
regexes being inlined there. Do not describe them as shared.
- Around line 1417-1420: Update the documentation around
test_installed_maturin_matches_expected_pin to describe its actual check: it
verifies the installed maturin package version from current-interpreter metadata
via importlib.metadata.version("maturin"), while only gating on the CLI being
available on PATH; alternatively, change the implementation to obtain the
version with maturin --version.
In `@tests/helpers/maturin.py`:
- Around line 178-185: Reduce the _read_wheel_members docstring to a single-line
summary of its return value, removing the Raises section and its assertion
details while preserving any non-obvious guarantee needed by callers.
- Around line 81-84: Update the maturin availability check in the helper to
import the maturin module directly instead of calling importlib.util.find_spec,
and keep the ImportError fallback setting maturin_available to False. The check
must only report availability when the import succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b00727e6-01fd-4901-8543-64e2040a5c87
📒 Files selected for processing (3)
cuprum/unittests/test_maturin_build.pydocs/developers-guide.mdtests/helpers/maturin.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
| These checks live in `cuprum/unittests/test_maturin_build.py`, split across two | ||
| boundaries. The **pin-synchronization** checks are inlined there as private | ||
| helpers (`_read_maturin_pins`, `_read_expected_maturin_version`, | ||
| `_read_manylinux_aarch64_container_ref`, and their regexes): they read | ||
| repository files, have a single consumer, and gain nothing from indirection. | ||
| The **native wheel build and snapshot** machinery stays in | ||
| `tests/helpers/maturin.py`, because it wraps `subprocess` and `zipfile` | ||
| introspection that does not inline cleanly. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the regex ownership description below.
Replace “shared regex” in Lines 1446-1450 with wording that identifies the
private test-local regexes; Lines 1403-1406 now explicitly document that the
helpers and regexes are inlined in test_maturin_build.py.
Triage: [type:docstyle]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/developers-guide.md` around lines 1403 - 1410, Update the regex
ownership wording in the documentation around the “shared regex” reference so it
identifies the private test-local regexes in test_maturin_build.py, consistent
with the documented helpers and regexes being inlined there. Do not describe
them as shared.
Source: Coding guidelines
| selecting a version itself. Two separate mechanisms enforce alignment with the | ||
| declared pin: `test_installed_maturin_matches_expected_pin` compares the | ||
| installed CLI against the `pyproject.toml` pin, and the snapshot test asserts | ||
| the built wheel's `Generator` matches that same pin, so a wheel built by an |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe the installed-version check accurately.
Replace “compares the installed CLI” with the current-interpreter package
metadata check, or invoke maturin --version. The test gates on a CLI being on
PATH but obtains the version through importlib.metadata.version("maturin").
Triage: [type:docstyle]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/developers-guide.md` around lines 1417 - 1420, Update the documentation
around test_installed_maturin_matches_expected_pin to describe its actual check:
it verifies the installed maturin package version from current-interpreter
metadata via importlib.metadata.version("maturin"), while only gating on the CLI
being available on PATH; alternatively, change the implementation to obtain the
version with maturin --version.
| try: | ||
| return importlib.util.find_spec("maturin") is not None | ||
| maturin_available = importlib.util.find_spec("maturin") is not None | ||
| except ImportError: | ||
| return False | ||
|
|
||
|
|
||
| def toolchain_available() -> bool: | ||
| """Return whether the Rust toolchain and maturin are available.""" | ||
| maturin_available = False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
python - <<'PY'
import importlib.util
print(f"find_spec: {importlib.util.find_spec('maturin')!r}")
try:
import maturin
except ImportError as exc:
print(f"import failed: {exc!r}")
else:
print("import succeeded")
PYRepository: leynos/cuprum
Length of output: 231
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="tests/helpers/maturin.py"
python - <<'PY'
from pathlib import Path
path = Path("tests/helpers/maturin.py")
print(path.read_text())
PYRepository: leynos/cuprum
Length of output: 10028
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,180p' tests/helpers/maturin.pyRepository: leynos/cuprum
Length of output: 6514
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
nl -ba tests/helpers/maturin.py | sed -n '60,100p'
printf '\n---\n'
nl -ba tests/helpers/maturin.py | sed -n '170,200p'
printf '\n---\n'
rg -n "toolchain_available|find_spec\\(\"maturin\"|import maturin|python -m maturin|maturin" tests -nRepository: leynos/cuprum
Length of output: 191
🌐 Web query:
Python importlib.util.find_spec does not import module broken installation spec importlib docs
💡 Result:
The function importlib.util.find_spec is designed to find the specification (ModuleSpec) for a module without actually performing a full import of that module [1][2][3]. However, it is important to be aware of a specific side effect: if you provide a dotted module name (a submodule), importlib.util.find_spec will automatically import the parent module [1][2][4][3]. Regarding your concern about broken installations or module resolution, here is how the behavior is handled: 1. Parent Module Import: Because find_spec performs this parent import to determine the correct path for a submodule, it will trigger the execution of the parent module's code (e.g., its init.py) [4][5][3]. If the parent module is broken or has a side effect on import, this will occur even though you are only attempting to "find" the spec of the child [3]. 2. Error Handling and Exceptions: If the module cannot be found, the function returns None [1][6]. However, depending on the Python version and the specific circumstances (such as a parent package lacking a path attribute or not existing), it may raise exceptions like ModuleNotFoundError or ValueError instead of returning None [4][7][8]. 3. Broken Installations: If you are encountering issues where find_spec returns an unexpected result (e.g., finding a module in a different directory) or fails, this often stems from how sys.path is configured or the presence of namespace packages that lack init.py files [9]. Since find_spec relies on the standard import machinery, it behaves exactly as the import statement would regarding path discovery [1][3]. If your goal is to verify existence without triggering any side effects from imports, you may need to inspect sys.path or use finders directly from sys.meta_path, rather than relying on the higher-level find_spec function, which inherently triggers the import of parent packages to resolve submodule locations [1][4][3].
Citations:
- 1: https://docs.python.org/3/library/importlib.html
- 2: https://docs.python.org/3.10/library/importlib.html
- 3: https://stackoverflow.com/questions/14050281/how-to-check-if-a-python-module-exists-without-importing-it
- 4: https://github.com/python/cpython/blob/3.11/Lib/importlib/util.py
- 5: https://github.com/python/cpython/blob/ec2385e315fc6b28d92dfb4c97fefcbb1e7daa68/Lib/importlib/util.py
- 6: https://runebook.dev/en/docs/python/library/importlib/importlib.util.find_spec
- 7: https://github.com/python/cpython/blob/refs/heads/main/Lib/importlib/util.py
- 8: importlib.find_spec raises AttributeError when parent is not a package/module python/cpython#74621
- 9: https://bugs.python.org/issue46241
Import maturin here instead of using find_spec(). A spec can exist even when the module fails at import time, which would let this helper report the toolchain as available and push the failure to python -m maturin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/helpers/maturin.py` around lines 81 - 84, Update the maturin
availability check in the helper to import the maturin module directly instead
of calling importlib.util.find_spec, and keep the ImportError fallback setting
maturin_available to False. The check must only report availability when the
import succeeds.
| def _read_wheel_members(whl_path: Path) -> tuple[list[str], str, str]: | ||
| """Return the entry names and the decoded ``WHEEL``/``METADATA`` payloads. | ||
|
|
||
| Raises | ||
| ------ | ||
| AssertionError | ||
| If no ``.dist-info/WHEEL`` entry is present. | ||
| If the archive is missing either ``.dist-info`` member. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reduce this private helper docstring to one line.
Remove the redundant Raises section; the public
wheel_build_snapshot() docstring owns the caller-facing contract.
As per path instructions, “Use a single-line summary for private functions and
methods.” Based on learnings, private helper docstrings should retain only
non-obvious guarantees.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/helpers/maturin.py` around lines 178 - 185, Reduce the
_read_wheel_members docstring to a single-line summary of its return value,
removing the Raises section and its assertion details while preserving any
non-obvious guarantee needed by callers.
Sources: Path instructions, Learnings
Summary
tests/helpers/maturin.pyhad grown to 394 lines / 15 definitions while serving a single consumer,cuprum/unittests/test_maturin_build.py— the over-extraction flagged in #97 and #99. This PR inlines the pin-synchronisation logic into that sole consumer and consolidates the wheel-snapshot machinery.Changes
read_expected_maturin_version,read_maturin_pins,read_manylinux_aarch64_container_ref,workflow_uses_manylinux_aarch64_container_ref,_require_pin_match, and all five pin regexes now live as private helpers intest_maturin_build.py. They have exactly one caller each._header_value,_locate_dist_info_wheel, and_parse_wheel_headerare folded intowheel_build_snapshot/_parse_metadata, leaving three primary parsing functions (wheel_build_snapshot,_parse_metadata,_normalise_wheel_entry) as Simplify tests/helpers/maturin.py: consolidate 12 functions into 2–3 and defer abstraction until second consumer exists #99 asks._maturin_module_availableintotoolchain_available.The retained wheel
build/snapshotmachinery wrapssubprocessandzipfileintrospection that does not inline cleanly, so per #97 it stays as a minimal helper.Result: helper drops to 218 lines / 6 definitions.
Why the entry-normalisation is kept
Every
_normalise_wheel_entrybranch — the.dist-infosuffixes,/sboms/, and the.cpython-<platform>.soextension module — is exercised by thetest_maturin_wheel_build_snapshotsyrupy snapshot, which runs across the wheel-build matrix. None is speculative, and removing any would change verified output.Validation
test_maturin_wheel_build_snapshotpasses with byte-identical snapshot output (behaviour preserved).make check-fmt,make lint(ruff, interrogate 100%, pylint 10.00/10),make test(755 passed / 47 skipped; Rust nextest 57/57).Closes #97
Closes #99
🤖 Generated with Claude Code
Summary by Sourcery
Inline maturin pin-synchronisation helpers into the maturin build tests and slim down the shared maturin helper to focus on wheel build/snapshot introspection.
Enhancements:
Documentation:
Tests: