Rust: snapshot tests for consume_stream UTF-8 replacement output (#105) - #241
Conversation
Reviewer's GuideAdds Unix-only Rust snapshot tests using Sequence diagram for consume_stream_files UTF-8 snapshot testssequenceDiagram
participant TestRunner
participant consume_snapshot_tests
participant consume_stream_files
participant insta
TestRunner->>consume_snapshot_tests: pure_ascii_decodes_verbatim
consume_snapshot_tests->>consume_stream_files: consume_stream_files(read_fd, write_fd, buffer_size)
consume_stream_files-->>consume_snapshot_tests: decoded_text
consume_snapshot_tests->>insta: assert_snapshot(decoded_text)
TestRunner->>consume_snapshot_tests: invalid_bytes_become_replacement_characters
consume_snapshot_tests->>consume_stream_files: consume_stream_files(read_fd, write_fd, buffer_size)
consume_stream_files-->>consume_snapshot_tests: decoded_text
consume_snapshot_tests->>insta: assert_snapshot(decoded_text)
TestRunner->>consume_snapshot_tests: incomplete_trailing_sequence_is_replaced_at_eof
consume_snapshot_tests->>consume_stream_files: consume_stream_files(read_fd, write_fd, buffer_size)
consume_stream_files-->>consume_snapshot_tests: decoded_text
consume_snapshot_tests->>insta: assert_snapshot(decoded_text)
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 ignored due to path filters (1)
📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Summary
WalkthroughChangesAdd Unix-only end-to-end UTF-8 snapshot and property tests for UTF-8 stream snapshot tests
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 3 inconclusive)
✅ Passed checks (15 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@rust/cuprum-rust/src/consume_snapshot_tests.rs`:
- Around line 7-10: Add end-to-end PyO3 integration cases in
TestRustConsumeStream for multibyte UTF-8 sequences split across buffer
boundaries and incomplete trailing bytes at EOF, matching the documented Python
oracle categories. Keep the existing invalid-byte coverage and ensure these
scenarios cross the Python/Rust boundary before retaining the module’s
full-coverage claim.
- Around line 73-77: Extend the equality assertions in the invalid-byte and
incomplete-tail tests around consume_snapshot_tests.rs to include
consume(payload, 3) alongside the existing buffer sizes 1 and 64. Ensure both
replacement-case tests verify identical output across one-byte, three-byte, and
whole-payload reads.
🪄 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: f93ffd86-9684-4256-a4b7-7d86c0dab710
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rust/cuprum-rust/Cargo.tomlrust/cuprum-rust/src/consume_snapshot_tests.rsrust/cuprum-rust/src/lib.rs
🔗 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)
|
Both addressed in 4f6693d — one applied as asked, one that turned out to be right for a different and more serious reason than stated. 2. Three-byte buffer in the replacement tests ✅Applied. Both the invalid-byte and incomplete-tail tests now assert identical output across one-byte, three-byte, and whole-payload reads, matching the multibyte test. The three-byte case is genuinely the interesting middle ground for the incomplete tail, since it splits the truncated euro sign differently again. 1. The Python integration cases — investigated, and the docstring corrected insteadThe literal request was to add multibyte-split and incomplete-trailing cases to
Each calls But your underlying instinct was right, and the reason is worse than a missing test. Chasing "ensure these scenarios cross the Python/Rust boundary", I ran them — and all seven The
I verified this locally: after So adding the two requested cases would have added no coverage at all — they would have skipped alongside the existing four, while making the suite look better than it is. That seemed strictly worse than the honest alternative, so I corrected the overstated claim instead. The docstring no longer says these snapshots mirror what the Python oracle "covers"; it records that the Python suite declares the categories, that they are gated on an extension the test path never builds, and that these Rust-side snapshots are consequently the coverage that actually executes. Happy to add the Python cases anyway if you would rather have them staged for whenever the extension does get built — say the word. Worth raising separatelyThat the Rust-backend Python tests never execute in CI is a real gap well beyond this PR: it silently disables every Validation
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai Both warnings addressed — one by implementing it, one by showing the premise is misplaced and raising the real defect. Testing (Property / Proof) — taken, in 0240976. Two properties now sit beside the four snapshots in Mutation-verified rather than assumed: never marking the final chunk — so an incomplete trailing sequence is dropped — fails the lossy-equivalence property; treating every chunk as final — so multi-byte sequences split at read boundaries — fails both. Linked Issues — the tests are not missing. That single occurrence is inside Adding more Python-side boundary tests to this PR would not change that; they would skip alongside the existing ones and make CI look better covered while testing nothing. I have raised #258 for the actual fix — build the extension in the test jobs, and add a The general rule I would like applied in future: when a gated test suite appears absent from a diff, check whether the gate ever opens in CI before asking for more tests behind the same gate. The module docstring in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Rust test module outline ==='
ast-grep outline rust/cuprum-rust/src/consume_snapshot_tests.rs --items all || true
echo '=== Rust property/snapshot assertions and helper constraints ==='
rg -n -C 5 'proptest!|from_utf8_lossy|buffer_size|512|assert_snapshot|consume\(' \
rust/cuprum-rust/src/consume_snapshot_tests.rs
echo '=== Make build target ==='
rg -n -A 8 -B 2 '^build:' Makefile
echo '=== Workflow maturin/test-job contexts ==='
rg -n -A 14 -B 14 'maturin develop|make build|make test|typecheck-test|benchmark-ratchet' \
.github/workflows/ci.yml
echo '=== Python Rust-stream gate contexts ==='
rg -n -A 8 -B 5 '_rust_backend_native|rust_streams|rust_consume_stream|pytest\.skip' \
cuprum/unittests/test_rust_streams.py \
cuprum/unittests/test_rust_streams_boundary_property.py \
cuprum/unittests/test_rust_extension.py \
cuprum/unittests/test_rust_splice.py \
cuprum/unittests/test_backend.py
echo '=== Issue `#258` ==='
gh issue view 258 --repo leynos/cuprum --json number,title,state,url,bodyLength of output: 74531
Withdraw the Python integration-test finding. Keep checking whether a gated suite actually executes in CI before requesting additional tests behind that gate. 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@rust/cuprum-rust/src/consume_snapshot_tests.rs`:
- Around line 154-162: Update the property test function
decoding_is_independent_of_the_buffer_size to reject cases where first and
second are equal by adding the appropriate property assumption before the
assertion, while preserving the existing buffer-size bounds and comparison.
🪄 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: e53399db-1703-40f3-b957-02903a612093
📒 Files selected for processing (2)
rust/cuprum-rust/proptest-regressions/consume_snapshot_tests.txtrust/cuprum-rust/src/consume_snapshot_tests.rs
🔗 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)
c65eef8 to
78e53e5
Compare
consume_stream_files' read-and-decode loop had no Rust-level test pinning its observable UTF-8 output; only the incremental decoder in utf8.rs was property-tested, and the PyO3 boundary is covered on the Python side (test_rust_streams.py::TestRustConsumeStream). Add src/consume_snapshot_tests.rs with insta inline-snapshot tests that drive the full loop through a real pipe for the four categories the Python oracle suite also covers: - pure ASCII decodes verbatim - multi-byte sequences split across buffer boundaries (buffer_size 1) decode identically to a whole-payload buffer - invalid bytes become U+FFFD - an incomplete trailing sequence resolves to U+FFFD at EOF The boundary-independence assertions pin that the output does not depend on where reads split a multi-byte sequence. insta is added as a default-features-off dev-dependency. Closes #105 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both replacement tests now assert identical output across one-byte,
three-byte, and whole-payload reads, matching the multibyte test. The
three-byte case is the interesting middle ground for the incomplete tail,
since it splits the truncated euro sign differently again.
The module docstring claimed these snapshots covered "the four categories
the Python oracle suite also covers", which overstated what is verified.
TestRustConsumeStream does declare all four against rust_consume_stream —
ASCII, a boundary-split snowman at buffer_size 2, invalid bytes, and a
truncated trailing sequence — each checked against
payload.decode("utf-8", errors="replace"). But those cases are gated on
the compiled extension being importable, and nothing on the test path
builds it: make build only runs uv sync, and the CI typecheck-test job
runs make typecheck and make test without maturin develop, which appears
only in the benchmark-ratchet job. They therefore skip, locally and in CI.
Adding the two further Python cases the review asked for would have added
no coverage for the same reason, so the docstring now states the position
accurately instead: the Python suite declares the categories, these
snapshots are what actually executes for them.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The pre-merge Developer Documentation check noted that the guide covers the Rust test stack and syrupy but never mentions insta, which this branch adds as a dev-dependency. Add a paragraph to "Rust property testing and verification" stating when to reach for insta over a property test — when the valuable assertion is the exact output text rather than a property of it — and why the snapshots here are inline rather than .snap files: the expected text stays beside the case producing it, there are no snapshot files to review or prune, and accepting a deliberate change means editing the literal rather than running cargo insta. Records the default-features = false pin too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The four snapshots pin what the loop produces for hand-picked payload categories, but they cannot show the categories are exhaustive — a payload whose multi-byte sequence straddles a read boundary in some way none of the four happens to hit would go unnoticed. Add two properties over arbitrary bytes and buffer sizes: the loop's output equals `String::from_utf8_lossy` of the input, and two different buffer sizes decode the same payload identically. Payloads stay under 512 bytes so the helper's unbuffered pipe write cannot block, which is the precondition it documents. Verified by mutation rather than assumed: never marking the final chunk (so an incomplete trailing sequence is dropped) fails one property, and treating every chunk as final (so multi-byte sequences split at read boundaries) fails both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Whitaker lint requires a module to document itself from the inside rather than from the item above it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`first` and `second` were drawn independently, so roughly one case in sixty-four compared a payload against itself at a single buffer size. Those cases hold for any implementation, correct or not, and so assert nothing about boundary independence. Reject equal pairs with `prop_assume!`. Name the shared bound `MAX_BUFFER` rather than repeating the literal, so the two properties cannot drift apart. An offset-derived second size would avoid the rejection entirely, but it needs `%`, which the workspace denies via `clippy::integer-division-remainder-used`. At a 1-in-64 rejection rate the assumption costs nothing measurable and states the intent directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The committed seeds record cases from mutation testing, not real failures: `payload = [194]` is an incomplete two-byte UTF-8 lead, which only failed against a deliberately-broken decoder that never marked the final chunk. Checking them in re-runs seeds that have never failed against correct code, and implies to a reader that they once did. The cases they pin are already covered by name — the incomplete-tail snapshot test asserts exactly that input. Ignore the directory so the file cannot return. It has appeared three times across these branches, each time from a verification run rather than a genuine shrink. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
15eafac to
a8b7c24
Compare
The snapshot guidance did not say that the same four input categories are also covered at the Python/Rust boundary, nor why those cases do not run. A reader could reasonably conclude either that the Rust cases duplicate existing coverage, or that they discharge the boundary entirely. Neither is true. Name `TestRustConsumeStream` and what it asserts, state that `make build` only synchronizes dependencies so the extension is never built for the test jobs, and point at the two issues that track the fix: `#258` for building the extension with a fail-loud guard, and `#265` for the PyO3 `errno` defect that currently fails one extension-enabled test. Say plainly what the snapshots and properties are: the coverage of the read-and-decode loop that executes on every commit, and not a substitute for executed boundary coverage. Documentation only. 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: 1
🤖 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 @.gitignore:
- Around line 212-215: Remove the rust/cuprum-rust/proptest-regressions/ entry
from .gitignore so genuine Proptest regression seeds remain trackable and can be
committed for regression coverage.
🪄 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: 7acd14a8-caac-463f-ac59-6dcb15d2fb0e
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.gitignoredocs/developers-guide.mdrust/cuprum-rust/Cargo.tomlrust/cuprum-rust/src/consume_snapshot_tests.rsrust/cuprum-rust/src/lib.rs
🔗 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)
Ignoring `rust/cuprum-rust/proptest-regressions/` stopped mutant-derived seeds returning, but it also foreclosed the case the directory exists for: a genuine shrink that should be replayed ahead of the generated cases. A silently-dropped seed is the worse failure of the two, because nothing signals the loss. Remove the ignore and address the original problem where it starts. Proptest reads `PROPTEST_DISABLE_FAILURE_PERSISTENCE`, which sets `failure_persistence` to `None`, so a verification run against deliberately-broken code can be told not to write a seed at all rather than relying on someone noticing the stray file afterwards. Document both halves: commit a seed from a real failure, and disable persistence when proving a property is non-vacuous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai please create a new GitHub issue for the following: Testing (Unit And Behavioural) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
for number in 258 265; do
echo "=== Issue #${number} ==="
gh issue view "$number" \
--repo leynos/cuprum \
--json number,title,state,url,body,assignees
doneLength of output: 7828 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="leynos/cuprum"
pr_url="https://github.com/leynos/cuprum/pull/241"
comment_url="$(
gh api "repos/${repo}/issues/241/comments" --paginate \
--jq '.[] | select(.user.login == "leynos" and (.body | contains("please create a new GitHub issue for the following"))) | .html_url' \
| tail -n1
)"
if [ -z "$comment_url" ]; then
echo "Could not locate the requesting comment URL." >&2
exit 1
fi
body=$(cat <<EOF
## Summary
Add an executed Python/Rust boundary regression test for I/O errors from \`rust_consume_stream\`.
The tests in \`rust/cuprum-rust/src/consume_snapshot_tests.rs\` correctly exercise the private \`consume_stream_files\` read-and-decode loop. They do not exercise error conversion through the public Python/Rust boundary.
## Required work
1. After the CI extension build from `#258` is available, add a focused test that calls \`rust_consume_stream\` with an invalid or closed file descriptor.
2. Assert that the Python-facing call raises the expected \`OSError\`.
3. Assert a usable OS error number. Accept the platform-specific values already used by existing tests: \`errno.EBADF\` or \`errno.EINVAL\`.
4. Retain the existing pipe, snapshot, and property tests for \`consume_stream_files\`. Do not replace them with the boundary test.
5. Run the new test in CI with the native extension enabled.
## Rationale
The current Rust tests validate decoding behaviour below the PyO3 boundary. An executed \`rust_consume_stream\` I/O-error assertion validates descriptor failure handling and error translation at the public Python/Rust boundary.
`#258` must first build the native extension in test CI and fail loudly when it is unavailable. `#265` must preserve \`OSError.errno\` across PyO3 before this assertion can pass consistently.
## Affected areas
- \`cuprum/unittests/test_rust_streams.py\`
- \`rust/cuprum-rust/src/lib.rs\` and the \`rust_consume_stream\` error-conversion path, if `#265` requires a fix
- \`.github/workflows/ci.yml\` and test setup from `#258`
## Acceptance criteria
- CI builds and imports \`cuprum._rust_backend_native\` before it runs the extension-gated test suite.
- A test calls \`rust_consume_stream\` with an I/O failure and observes \`OSError\`.
- The test verifies \`.errno\` is \`errno.EBADF\` or \`errno.EINVAL\`.
- The extension-gated test suite executes in CI instead of skipping.
- The existing \`consume_stream_files\` snapshots and properties remain.
## Related work
- `#258` — build the native extension in test CI and make a missing extension fail loudly.
- `#265` — preserve \`OSError.errno\` across the PyO3 boundary.
## Backlinks
- Pull request: ${pr_url}
- Requesting comment: ${comment_url}
- Requested by: `@leynos`
EOF
)
issue_url=$(gh issue create \
--repo "$repo" \
--title "Add executed rust_consume_stream I/O-error boundary coverage" \
--body "$body" \
--assignee "leynos")
echo "$issue_url"Length of output: 3100
The issue links to PR You are interacting with an AI system. |
Summary
consume_stream_files(the read-and-decode loop behind the PyO3rust_consume_stream) had no Rust-level test pinning its observable UTF-8 output. Only the incremental decoder inutf8.rswas property-tested (againstString::from_utf8_lossy), and the compile-time UI tests only check PyO3 macro patterns. This addsinstasnapshot tests exercising the full loop.What's added
rust/cuprum-rust/src/consume_snapshot_tests.rsdrives real pipe descriptors throughconsume_stream_filesand pins the exact output for the four categories from #105:pure_ascii_decodes_verbatimcuprum reads pipesmultibyte_sequences_split_across_buffer_boundarieshéllo, 世界! ☕invalid_bytes_become_replacement_charactersx\xffy\x80zincomplete_trailing_sequence_is_replaced_at_eofeuro sign: \xe2\x82The multi-byte, invalid, and incomplete cases additionally assert boundary-independence — a one-byte buffer, a three-byte buffer, and a whole-payload buffer all decode to the same text — so a regression in the bounds-checked slicing or
final_chunkhandling surfaces as a concrete diff.instais added as adefault-features = falsedev-dependency (v1.48.0); the snapshots are inline, so there are no separate.snapfiles to manage.Boundary already covered on the Python side
The issue's second bullet (integration through the Python/Rust boundary) is already satisfied by
cuprum/unittests/test_rust_streams.py::TestRustConsumeStream, which feeds ASCII, a boundary-split multi-byte scalar (buffer_size=2), invalid bytes, and an incomplete trailing sequence throughrust_consume_streamand asserts againstpayload.decode("utf-8", errors="replace"). These Rust snapshots complement that by pinning the exact decoded text one layer down, at the read loop itself.Validation
Full gates green:
make check-fmt,make lint(clippy-D warnings,cargo doc -D warnings, interrogate 100%, pylint 10.00/10),make test(pytest 755 passed / 47 skipped; Rust nextest 61/61 including the four new cases).Closes #105
🤖 Generated with Claude Code
Summary by Sourcery
Add Rust snapshot tests to pin the UTF-8 replacement behavior of the consume_stream_files read loop using real pipe descriptors.
Build:
Tests: