Simplify tests/helpers/maturin.py: inline pin checks, consolidate wheel snapshot (#97, #99) - #240
Conversation
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Summary
WalkthroughSeparate Maturin pin, toolchain, wheel-build, and snapshot checks into focused modules. Add shared pin-reading support. Add coverage for missing wheel metadata. Update helper documentation and packaged test entries. ChangesMaturin test separation
Possibly related PRs
Suggested labels: Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 inconclusive)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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".
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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)
|
Thanks — took the simplification from the first comment and measured the caching half rather than guessing. The second comment looks to be against a superseded revision. 1. Pin-helper file readsApplied the deduplication (abf8142). "pyproject.toml": _read_expected_maturin_version(root),Declined the caching, with numbers. A full run of the module performs seven reads totalling ~69 KiB:
Measured wall-clock for all seven reads together: 0.124 ms, against a 1.32 s module runtime — 0.009%, and that 1.32 s is with an already-built wheel; a cold run spends seconds in On the "keep the helpers simpler" half: a module-level cache or fixture would make them less simple, not more — it adds shared mutable state, an invalidation question (these files are read to detect drift, so a stale cache is a correctness hazard rather than a perf trade), and a test-isolation concern. That is exactly the speculative-abstraction pattern #97 and #99 asked this PR to remove, so adding one here would work against its purpose. The delegation above captures the real simplification without any of that. 2.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Three of the four applied in 8dc5353; one was already satisfied. Details below. 1.
|
| Command | Outcome |
|---|---|
uv run pytest cuprum/unittests/test_maturin_build.py |
16 passed (incl. the wheel-build snapshot) |
make check-fmt |
pass |
make lint |
pass (ruff, interrogate 100%, pylint 10.00/10, clippy) |
make typecheck |
pass |
make test |
pass — full Python suite, Rust nextest 57/57 |
make markdownlint / make nixie |
pass |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
Rebasing onto main combined two independent decompositions of the same surface. Main extracted wheel inspection into `tests/helpers/maturin_wheel.py` and added `maturin_script_locatable` with three tests; this branch inlined the pin readers into their sole consumer. Both landed, which left `test_maturin_build.py` at 446 lines — past the 400-line limit, and mixing three concerns. Split it the way the helpers already are: pin synchronization in `test_maturin_pins.py`, the availability detectors in `test_maturin_toolchain.py` beside the `toolchain_available` tests they sit next to, and the wheel build and snapshot in `test_maturin_build.py`. One reader resists inlining. `read_expected_maturin_version` now has two consumers — the pin comparison and the wheel snapshot's `Generator` assertion — which is exactly the second concrete consumer this branch's own re-use policy names as the condition for sharing. It moves to `_maturin_pin_support.py`; the readers with one consumer stay put. Repoint `test_manylinux_container_ref_properties.py`, which imported the container regex from the module that was split, and refresh the wheel manifest for the new files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a799f4a to
19a3d58
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/developers-guide.md (1)
1660-1662: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the
toolchain_available()implementation description.State that
toolchain_available()importsmaturinwith
importlib.import_module. It does not useimportlib.util.find_spec.
Retain thecargoandrustcavailability checks. The current text describes
the wrong broken-import boundary.Triage:
[type:docstyle]Based on supplied helper context,
toolchain_availableuses
importlib.import_module.🤖 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 1660 - 1662, Update the toolchain_available() documentation to state that it imports maturin via importlib.import_module, removing the incorrect importlib.util.find_spec description while retaining the cargo and rustc PATH availability checks.
🤖 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 `@cuprum/unittests/_maturin_pin_support.py`:
- Around line 21-56: Expand the docstrings for require_pin_match, read_text, and
read_expected_maturin_version to follow NumPy style, adding Parameters, Returns,
and Raises sections that describe each argument, return value, and
AssertionError or file-reading exception raised. Preserve the existing behavior
and implementation of these helpers.
In `@cuprum/unittests/test_manylinux_container_ref_properties.py`:
- Line 8: Move the shared _MANYLINUX_CONTAINER_SHA256_RE definition from
test_maturin_pins.py into _maturin_pin_support.py, then update both
test_maturin_pins.py and test_manylinux_container_ref_properties.py to import it
from the support module. Remove the test-to-test import and preserve the
existing regex behavior.
In `@cuprum/unittests/test_maturin_build.py`:
- Around line 167-205: Remove the redundant standalone
test_wheel_build_snapshot_rejects_wheel_without_metadata test, while preserving
the missing_metadata case in test_wheel_build_snapshot_reports_missing_dist_info
as the sole coverage for absent METADATA and its exact diagnostic.
In `@cuprum/unittests/test_maturin_pins.py`:
- Around line 88-96: Update test_installed_maturin_matches_expected_pin to
verify maturin through importlib.import_module("maturin") rather than
shutil.which("maturin"), skipping only when the import raises ImportError.
Remove the unused shutil import and adjust the docstring to describe the current
interpreter’s maturin module.
In `@cuprum/unittests/test_maturin_toolchain.py`:
- Around line 152-153: Update the test around toolchain_available() to capture
the raised RuntimeError with pytest.raises as exc_info, then assert
str(exc_info.value) exactly equals the complete expected diagnostic, preserving
the existing explanatory assertion message.
In `@tests/helpers/maturin.py`:
- Around line 1-11: Update the module docstring to identify
cuprum/unittests/test_maturin_pins.py as the owner of pin-synchronization
checks, and separately state that test_maturin_build.py consumes the shared
expected-version reader for wheel validation. Remove the current wording that
identifies test_maturin_build.py as the sole consumer.
---
Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 1660-1662: Update the toolchain_available() documentation to state
that it imports maturin via importlib.import_module, removing the incorrect
importlib.util.find_spec description while retaining the cargo and rustc PATH
availability checks.
🪄 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: 1d68aaa7-0b23-467a-a895-f66c3ef08b89
📒 Files selected for processing (8)
cuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/_maturin_pin_support.pycuprum/unittests/test_manylinux_container_ref_properties.pycuprum/unittests/test_maturin_build.pycuprum/unittests/test_maturin_pins.pycuprum/unittests/test_maturin_toolchain.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)
Six review findings on the maturin test split. `_MANYLINUX_CONTAINER_SHA256_RE` had two consumers, one of which reached into the other test module for a private name. Move it beside `read_expected_maturin_version` in `_maturin_pin_support.py`, which is what the two-consumer threshold already asks for, and make it public since it is now a shared symbol rather than a module-local one. Document the read failures the helpers can raise. `read_text` and `read_expected_maturin_version` touch the filesystem while returning plain `str`, so `OSError` and `UnicodeDecodeError` were reachable but unadvertised. They stay uncaught deliberately — a renamed CI file is a genuine failure, and its own traceback names the path better than a wrapper would — but that is now stated rather than implied. Skip the installed-version check on the maturin *module* rather than on a `PATH` launcher. The build runs `python -m maturin` in the current interpreter, so a launcher from another environment would have let the test compare a version the build never uses. Drop `test_wheel_build_snapshot_rejects_wheel_without_metadata`: the `missing_metadata` parametrized case covers the same failure and compares the message exactly. Its rationale about `metadata_name` being derived by substitution moves into the case that survives. Assert the toolchain import diagnostic exactly, for the reason already applied to the wheel cases — `match=` searches, so a prefix or suffix would slip through. Correct three stale claims in the guide: `toolchain_available` uses `import_module`, not `find_spec`; the manylinux command targets `test_maturin_pins.py` after the split; and the skip boundary is the module, not `PATH`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cuprum/unittests/test_maturin_pins.py (1)
133-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd diagnostics to the changed assertions.
The same test-guideline gap appears at all four sites. Add a short contract-specific message to each assertion.
cuprum/unittests/test_maturin_pins.py#L133-L137: describe why the invalid container reference must be rejected.cuprum/unittests/test_manylinux_container_ref_properties.py#L21-L21: describe why a valid digest reference must match.cuprum/unittests/test_manylinux_container_ref_properties.py#L35-L35: describe why mutable tags must be rejected.cuprum/unittests/test_manylinux_container_ref_properties.py#L43-L43: describe why truncated digests must be rejected.As per path instructions, “Use
assert …, "message"over bare asserts.”🤖 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 `@cuprum/unittests/test_maturin_pins.py` around lines 133 - 137, Add contract-specific assertion messages at all four sites: in cuprum/unittests/test_maturin_pins.py lines 133-137, explain that invalid or mutable manylinux AArch64 container references must be rejected; in cuprum/unittests/test_manylinux_container_ref_properties.py lines 21, 35, and 43, explain respectively why valid digest references must match, mutable tags must be rejected, and truncated digests must be rejected. Use assert condition, "message" for each assertion.Source: Path instructions
🤖 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.
Outside diff comments:
In `@cuprum/unittests/test_maturin_pins.py`:
- Around line 133-137: Add contract-specific assertion messages at all four
sites: in cuprum/unittests/test_maturin_pins.py lines 133-137, explain that
invalid or mutable manylinux AArch64 container references must be rejected; in
cuprum/unittests/test_manylinux_container_ref_properties.py lines 21, 35, and
43, explain respectively why valid digest references must match, mutable tags
must be rejected, and truncated digests must be rejected. Use assert condition,
"message" for each assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 798198ac-538d-4410-ae0a-8fbae3d3e29c
📒 Files selected for processing (7)
cuprum/unittests/_maturin_pin_support.pycuprum/unittests/test_manylinux_container_ref_properties.pycuprum/unittests/test_maturin_build.pycuprum/unittests/test_maturin_pins.pycuprum/unittests/test_maturin_toolchain.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 guide had two passages describing what `test_installed_maturin_matches_expected_pin` gates on. The previous round fixed one; this paragraph still said it gates "on a `maturin` CLI being present on `PATH`", contradicting both the code and the corrected text further down. State the actual boundary — the running interpreter being able to import the module — and the reason it matters: the build runs `python -m maturin`, so a launcher earlier on `PATH` can belong to an entirely different environment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Four assertions on the manylinux pin pattern carried no message, so a failure reported only `assert None` or `assert <re.Match>` — true but useless to whoever hits it. Give each one the contract it is defending rather than a restatement of the expression: a mutable tag can be republished under the same name, a truncated digest identifies no single image, and rejecting a valid 64-character digest would block a legitimate bump. Each message interpolates the reference that failed, so the case is visible without re-running under a debugger. Verified by breaking the pattern and confirming all four messages surface. The Developer Documentation warning in the same round needed no change: it cites the paragraph corrected in 5e7efed, and the review was submitted against 3775d96. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both warnings from this round, addressed. Developer Documentation — already fixed. The cited paragraph was corrected in 5e7efed; this review was submitted against 3775d96, so it evaluated the version before that commit. The guide now says the test gates on the running interpreter being able to import Linked Issues — taking the second branch of the suggested resolution, "document and justify the second concrete consumer". Both issues state the governing rule as "defer any re-extraction into a shared helper module until a second concrete consumer exists" (
Everything else stayed inlined: On the literal wording: the issues name Worth noting the two findings in this same round point opposite ways: this check asks for Rationale is recorded in the module docstring of |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The finding cited four sites and called them "all four". There were six: `test_manylinux_aarch64_container_pin_regex_rejects_missing_comment` and `test_manylinux_aarch64_container_usage_regex_rejects_literal_image` carry the same guideline gap and went uncited. Both defend something a bare `assert None` cannot convey. The pin pattern must reject a digest with no trailing comment, because that comment records which tag the digest came from — without it a reviewer cannot tell what a bump is bumping to. The usage pattern must reject a literal image reference, because the build step has to read `env.MANYLINUX_AARCH64_CONTAINER`; otherwise the pin every other test in this module checks is not what the job actually pulls. Verified by inverting both conditions and confirming each message surfaces. No bare assertions remain in any file this PR touches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Both remaining 1. Developer Documentation — already resolved at headThe finding says lines 1581–1585 "still say the installed-version test gates on a CLI in PATH". That wording was removed by At head,
That matches the code, try:
importlib.import_module("maturin")
except ImportError:
pytest.skip("maturin is not installed in this interpreter.")A sweep of No edit made. 2. Linked Issues check — the second concrete consumer exists, and pre-dates this PRThe recommendation offers a choice: inline the pin logic, or "document and justify the second concrete consumer that requires the new shared module".
On
Inlining it would mean either duplicating the reader in both modules or reinstating a private cross-module import. The first is exactly what CodeScene flagged: There is also a hard constraint. Pylint's Worth being explicit that the issues' substantive complaints were actioned: the speculative platform-variability normalization is gone, the pin readers with only one consumer stayed inlined in The justification is already documented where a reader will meet it:
No edit made. Verification run: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
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: