test: consolidate vlen F-contiguous regression tests#4122
Merged
d-v-b merged 1 commit intoJul 6, 2026
Conversation
github-actions
Bot
added
the
needs release notes
Follow-up test-quality cleanup for the zarr-developersgh-3558 fix (zarr-developers#4116): - Merge test_vlen_string_f_contiguous and test_vlen_bytes_f_contiguous into one test parametrized over (dtype, fill_value, elements), matching the parametrized style of test_vlen_string in the same file. - Drop the chunks=(2,2) case: with (3,3) data it produces only partial chunks, which the codec pipeline recopies to C order before encoding, so it never exercised the F-contiguous path it claimed to cover. Use chunks == shape (a single complete chunk) so the F-contiguous buffer reaches the codec untouched. - Build the F-contiguous input directly via reshape(order="F") instead of wrapping a C-order reshape in np.asarray(..., order="F"), and assert data.flags.f_contiguous so a future simplification can't silently defeat the regression guard. - Add a docstring stating the verified behavior. Assisted-by: ClaudeCode:claude-opus-4.8
d-v-b
force-pushed
the
claude/vlen-f-contiguous-test-cleanup
branch
from
dcbe087 to
6e93a8b
Compare
Contributor
Author
|
I'm going to self-merge because this is a small internal cleanup. |
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
1 participant
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.
🤖 AI text below 🤖
Follow-up test-quality cleanup for the gh-3558 fix merged in #4116. No source changes — the fix itself is already on
main; this only tidies the regression tests it added.What and why
The
test_vlen_string_f_contiguous/test_vlen_bytes_f_contiguoustests added in #4116 had a few issues surfaced in review:chunks=(2,2)case never exercised the F-contiguous path. With(3,3)data, a(2,2)chunking produces only partial chunks, which_merge_chunk_arrayrecopies into a fresh C-order buffer before the codec sees them — so that parametrization gave false coverage confidence. Only the whole-array (chunks == shape) case actually hands an F-contiguous buffer to the codec. This PR keeps just that case.np.asarray(np.array(...).reshape(3, 3), order="F")), so a future "simplification" dropping the outernp.asarraycould silently stop testing the F path. Now it's built directly withreshape((3, 3), order="F")and guarded withassert data.flags.f_contiguous.(dtype, fill_value, elements), matching the parametrized style oftest_vlen_stringin the same file.Verification
pytest tests/test_codecs/test_vlen.py -k f_contiguous→ 2 passed (string + bytes).prek run mypy --all-files,ruff check,ruff format --checkall pass.🤖 Generated with Claude Code