test(py): declare numpy so the ZPayloadView tests cannot silently skip - #274
Open
YuanYuYuan wants to merge 5 commits into
Open
test(py): declare numpy so the ZPayloadView tests cannot silently skip#274YuanYuYuan wants to merge 5 commits into
YuanYuYuan wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Ensures NumPy-based ZPayloadView buffer-sharing tests execute instead of skipping.
Changes:
- Adds NumPy as a test extra.
- Installs test dependencies in the Python venv.
- Fails tests when NumPy is unavailable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
scripts/test-python.nu |
Installs the test extra during setup. |
crates/hiroz-py/tests/test_payload_view.py |
Replaces conditional skipping with failure. |
crates/hiroz-py/pyproject.toml |
Declares the NumPy test dependency. |
Comments suppressed due to low confidence (1)
scripts/test-python.nu:50
pip install -e '.[test]'invokes the maturin PEP 660 backend and builds the Rust extension, after whichmaturin developbuilds it again. Because the second build changesRUSTFLAGS, Cargo cannot reuse the first compilation, doubling this expensive setup work for every distro. PassRUSTFLAGSto the editable install and use it as the single extension build.
run-cmd "cd crates/hiroz-py; source .venv/bin/activate && pip install -e '.[test]'" --shell bash --distro (get-distro)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`TestZPayloadViewNumpy` guarded on `pytest.importorskip("numpy")`, and
numpy was installed by nothing -- not pyproject dependencies, not an
extra, not the test script. The venv is built with plain `python -m venv`
so it inherits nothing from the devShell. Both tests had therefore always
skipped, and every run ended "86 passed, 2 skipped".
What never ran is the only assertion in the suite that proves the
zero-copy claim:
assert arr.flags["OWNDATA"] is False
If ZPayloadView regressed to copying, the whole suite would still pass.
Install numpy as a test dependency, and replace the importorskip with a
real import so a missing numpy fails instead of skipping. A permanently
skipped test is indistinguishable from a passing one in the summary line.
Closes #266
Adversarial review caught that the new failure message instructs the user to "install it with the test extra", and no such extra existed -- pyproject.toml has no [project.optional-dependencies] at all. numpy was installed by exactly one imperative line in the test script. A contributor doing the natural manual setup (venv, pip install -e ., maturin develop, pytest) previously got 2 skips and now gets 2 hard errors whose stated remedy fails with "no extra 'test'". Declare test = ["numpy>=1.21"] and have the script install via it, so the dependency is declared in one place and the message is true.
Copilot was right and the claim was overstated in three places. OWNDATA == False proves numpy borrowed the buffer ZPayloadView exported rather than copying it. It does not prove the transport was zero-copy: ZPayloadView::new falls back to PayloadBytes::Owned for a fragmented payload, and numpy borrows that owned copy just as happily. The transport-level check is `payload.is_zero_copy_py`, asserted by test_payload_view_is_zero_copy, which was already running. The reason to fix #266 is unchanged -- two tests had never executed and a permanent skip reads as a pass -- but the value claim now matches what the assertion can support.
YuanYuYuan
force-pushed
the
test/numpy-zero-copy
branch
from
August 14, 2026 18:23
515fdbf to
e739a2d
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
TestZPayloadViewNumpyguarded onpytest.importorskip("numpy"). No file in this repo declared numpy. This PR declares numpy as a test extra, installs it during venv setup, and makes its absence fail instead of skip.Relates to #266.
The defect
Every place that could declare numpy, before this change:
crates/hiroz-py/pyproject.tomldependenciesmsgspecandhiroz-msgs-pyonlycrates/hiroz-py/pyproject.tomloptional extras[project.optional-dependencies]sectionsetup-venvinscripts/test-python.nupython -m venv; installshiroz-msgs-pyandhiroz-pyonlyhiroz-msgs-pymsgspec>=0.18.0onlynumpy resolves at test time from the ROS environment's
site-packages. That source is ambient, not declared. If it disappears:Important
#266 states that these tests "have never run". The evidence below contradicts that. The ROS environment supplied numpy before this change, so
importorskippassed through. Both tests ran. This PR's justification does not depend on that premise.What this PR does
testextra (numpy>=1.21)crates/hiroz-py/pyproject.tomlpip install -e '.[test]'during venv setupsetup-venvinscripts/test-python.nuimportorskipwith an import that callspytest.failcheck_numpyThis declares the dependency once. The failure message names the command that installs it.
Evidence
There is no failing baseline. This is a hardening change.
Measurements on head
515fdbf4b1e49ba0eddf8f4507111da353394282:TestZPayloadViewNumpy::test_numpy_frombuffer PASSED,TestZPayloadViewNumpy::test_numpy_zero_copy_verification PASSED48 passed, 7 warnings in 17.01s— 0 skippedpip install -e '.[test]'reportsRequirement already satisfied: numpy>=1.21 in <ros-env>/site-packages (from hiroz-py==0.1.0) (2.4.4)That green is contingent. Nothing declared the dependency, nothing pinned it, nothing recorded that these tests need it. Declaring it converts "green because numpy happened to be present" into "green because numpy is required and present". This PR restores no lost coverage, because no coverage is currently lost.
Breaking changes
None. This adds one optional extra and changes the test environment.
hiroz-py's runtime dependencies do not change, sopip install hiroz-pydoes not pull numpy.Coverage this does not have
None of these block the change. They bound what the evidence above establishes.
pytest.failpath is never exercisedexcept ImportErrorbranch is marked# pragma: no cover. No CI job runs the suite without numpy, so reading establishes the loud-failure behaviour, not execution.numpy>=1.21, with no lockfile. A future numpy release that changes buffer-protocol behaviour is still accepted.OWNDATA is Falseis not a transport-level checkZPayloadViewexported instead of copying it. It does not prove the transport was zero-copy.G3 in detail:
ZPayloadView::newfalls back toPayloadBytes::Ownedfor a fragmented payload, and numpy borrows that owned copy just as happily. The transport-level check ispayload.is_zero_copy_py, asserted separately bytest_payload_view_is_zero_copy.