Skip to content

Commit 467dda7

Browse files
d-v-bclaude
andauthored
chore: indexing test cleanup (#4001)
* docs: design spec for indexing test cleanup Plan to consolidate and speed up tests/test_indexing.py: shrink oversized arrays (>=3 chunks/axis, partial edge), replace np.random selection loops with hand-picked parametrized cases via the Expect/ExpectFail dataclasses, one-behavior-per-test isolation, and docstrings throughout. Includes a prerequisite step deduplicating the two divergent Expect dataclass pairs (tests/conftest.py vs tests/test_codecs/conftest.py) onto one canonical pair. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: implementation plan for indexing test cleanup Task-by-task plan: Expect dataclass dedup (Part 0) then per-family rewrites of tests/test_indexing.py to parametrized Expect/ExpectFail cases on smaller arrays (Part 1), with final verification and speed measurement (Part 2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: make canonical Expect/ExpectFail frozen Prepares for deduplicating the second Expect pair in test_codecs/conftest.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: migrate test_chunk_grids to canonical Expect/ExpectFail Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: migrate test_cast_value to canonical Expect/ExpectFail Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: migrate test_scale_offset to canonical Expect/ExpectFail Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: delete duplicate Expect dataclasses in test_codecs/conftest.py All consumers now use the canonical Expect/ExpectFail from tests/conftest.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: rewrite orthogonal 1d bool indexing as parametrized cases Smaller array (30 elems, chunks of 7), hand-picked deterministic masks replacing np.random sparsity sweep, error paths split into their own test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: add Task 0.6 (optional ExpectFail.msg + raises helper) Structural fix for the msg="" footgun discovered during Task 1.1: msg becomes optional and regex-matched via a case.raises() helper, with an escape flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: make ExpectFail.msg optional with a raises() helper msg defaults to None (assert exception type only) and is treated as a regex, with an escape flag for literal messages. Removes the msg="" footgun that the repo's filterwarnings=["error"] config turned into a failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: clearer single-true mask idiom in orthogonal 1d exemplar np.arange(30) == 7 reads better than np.eye(1, 30, 7)[0]; matters because the other indexing families copy this exemplar. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: rewrite orthogonal 1d int indexing as parametrized cases Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: rewrite orthogonal 2d indexing as parametrized cases Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: rewrite orthogonal 3d indexing as parametrized cases Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: rewrite set-orthogonal indexing family as parametrized cases Reuses the get-orthogonal case tables; deletes the per-dimensionality set helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: rewrite basic 1d/2d selection families as parametrized cases Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: restore basic-indexing integer-list rejection coverage The basic-selection rewrite dropped two assertions that get_basic_selection rejects integer-list selections (1D direct, 2D nested in a tuple); restore them as dedicated get-only tests, since z[...] falls back to fancy indexing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: rewrite coordinate selection family as parametrized cases Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: rewrite block selection family as parametrized cases Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: rewrite mask selection family as parametrized cases Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: shrink arrays in selection_out and numpy-equivalence tests Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: add behavior docstrings to remaining indexing tests Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: drop redundant pytest.mark.asyncio decorators asyncio_mode = "auto" already collects async test functions; the explicit marks were no-ops. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: add changelog fragment for indexing test cleanup Rename XXXX.misc.md to the PR number when the PR is opened. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: explain msg=None on the basic-1d string bad case Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: remove LLM plans * docs: rename changelog --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 6827a61 commit 467dda7

8 files changed

Lines changed: 1074 additions & 1022 deletions

File tree

changes/4001.misc.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Consolidated the array indexing test suite (`tests/test_indexing.py`): the loop-and-`np.random` based selection tests were rewritten as deterministic, parametrized `Expect`/`ExpectFail` cases on small arrays, error paths were split into their own named tests, and the two divergent `Expect` test-case dataclass pairs were unified onto the canonical one in `tests/conftest.py` (whose `ExpectFail` now has an optional regex `msg` and a `raises()` helper). Test-only change with no effect on the public API.

tests/conftest.py

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import math
44
import os
55
import pathlib
6+
import re
67
import sys
78
from collections.abc import Mapping, Sequence
89
from dataclasses import dataclass, field
@@ -50,6 +51,7 @@
5051

5152
if TYPE_CHECKING:
5253
from collections.abc import Generator
54+
from contextlib import AbstractContextManager
5355
from typing import Any, Literal
5456

5557
from _pytest.compat import LEGACY_PATH
@@ -64,7 +66,7 @@
6466
from zarr.core.dtype.wrapper import ZDType
6567

6668

67-
@dataclass
69+
@dataclass(frozen=True)
6870
class Expect[TIn, TOut]:
6971
"""A test case with explicit input, expected output, and a human-readable id."""
7072

@@ -73,14 +75,27 @@ class Expect[TIn, TOut]:
7375
id: str
7476

7577

76-
@dataclass
78+
@dataclass(frozen=True)
7779
class ExpectFail[TIn]:
78-
"""A test case that should raise an exception."""
80+
"""A test case that should raise an exception.
81+
82+
`msg` is a regex matched against the exception text (pytest's native
83+
`match=` semantics). Leave it `None` to assert only the exception type. Set
84+
`escape=True` when `msg` is a literal that contains regex metacharacters
85+
such as `(`, `[`, or `.`; `escape` has no effect when `msg` is `None`.
86+
"""
7987

8088
input: TIn
8189
exception: type[Exception]
8290
id: str
83-
msg: str
91+
msg: str | None = None
92+
escape: bool = False
93+
94+
def raises(self) -> AbstractContextManager[pytest.ExceptionInfo[Exception]]:
95+
if self.msg is None:
96+
return pytest.raises(self.exception)
97+
pattern = re.escape(self.msg) if self.escape else self.msg
98+
return pytest.raises(self.exception, match=pattern)
8499

85100

86101
async def parse_store(

tests/test_chunk_grids.py

Lines changed: 66 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
1-
import re
21
from typing import Any
32

43
import numpy as np
54
import pytest
65

7-
from tests.test_codecs.conftest import Expect, ExpectErr
6+
from tests.conftest import Expect, ExpectFail
87
from zarr.core.chunk_grids import (
98
ChunkLayout,
109
_guess_regular_chunks,
@@ -131,95 +130,118 @@ def test_chunk_layout_nested() -> None:
131130
@pytest.mark.parametrize(
132131
"case",
133132
[
134-
ExpectErr(input=(0, 100), msg="Chunk size must be positive", exception_cls=ValueError),
135-
ExpectErr(input=(-2, 100), msg="Chunk size must be positive", exception_cls=ValueError),
136-
ExpectErr(input=([], 100), msg="must not be empty", exception_cls=ValueError),
137-
ExpectErr(input=([10, -1, 10], 100), msg="must be positive", exception_cls=ValueError),
138-
ExpectErr(input=([10, 0, 10], 20), msg="must be positive", exception_cls=ValueError),
139-
ExpectErr(input=([10, 20], 100), msg="do not sum to span", exception_cls=ValueError),
133+
ExpectFail(
134+
input=(0, 100),
135+
exception=ValueError,
136+
id="zero-uniform",
137+
msg="Chunk size must be positive",
138+
),
139+
ExpectFail(
140+
input=(-2, 100),
141+
exception=ValueError,
142+
id="negative-uniform",
143+
msg="Chunk size must be positive",
144+
),
145+
ExpectFail(input=([], 100), exception=ValueError, id="empty-list", msg="must not be empty"),
146+
ExpectFail(
147+
input=([10, -1, 10], 100),
148+
exception=ValueError,
149+
id="negative-element",
150+
msg="must be positive",
151+
),
152+
ExpectFail(
153+
input=([10, 0, 10], 20), exception=ValueError, id="zero-element", msg="must be positive"
154+
),
155+
ExpectFail(
156+
input=([10, 20], 100), exception=ValueError, id="wrong-sum", msg="do not sum to span"
157+
),
140158
# Nested/RLE form for a single dim is rejected with offending indices.
141-
ExpectErr(
159+
ExpectFail(
142160
input=([[3, 3], 1], 7),
161+
exception=TypeError,
162+
id="rle-single-dim",
143163
msg="non-integer element(s) ([3, 3],) at indices (0,)",
144-
exception_cls=TypeError,
164+
escape=True,
145165
),
146166
# Multiple non-int elements: all offending indices reported.
147-
ExpectErr(
167+
ExpectFail(
148168
input=([1, [2, 2], 1, [3]], 9),
169+
exception=TypeError,
170+
id="multiple-non-ints",
149171
msg="non-integer element(s) ([2, 2], [3]) at indices (1, 3)",
150-
exception_cls=TypeError,
172+
escape=True,
151173
),
152174
# Strings are non-integers and should be reported the same way.
153-
ExpectErr(
175+
ExpectFail(
154176
input=([2, "3", 5], 10),
177+
exception=TypeError,
178+
id="string-element",
155179
msg="non-integer element(s) ('3',) at indices (1,)",
156-
exception_cls=TypeError,
180+
escape=True,
157181
),
158182
],
159-
ids=[
160-
"zero-uniform",
161-
"negative-uniform",
162-
"empty-list",
163-
"negative-element",
164-
"zero-element",
165-
"wrong-sum",
166-
"rle-single-dim",
167-
"multiple-non-ints",
168-
"string-element",
169-
],
183+
ids=lambda c: c.id,
170184
)
171-
def test_normalize_chunks_1d_errors(case: ExpectErr[tuple[Any, int]]) -> None:
185+
def test_normalize_chunks_1d_errors(case: ExpectFail[tuple[Any, int]]) -> None:
172186
"""Invalid 1D chunk specifications are rejected with informative error messages."""
173187
chunks, span = case.input
174-
with pytest.raises(case.exception_cls, match=re.escape(case.msg)):
188+
with case.raises():
175189
normalize_chunks_1d(chunks, span=span)
176190

177191

178192
@pytest.mark.parametrize(
179193
"case",
180194
[
181-
ExpectErr(
195+
ExpectFail(
182196
input=(None, (100,)),
197+
exception=ValueError,
198+
id="none",
183199
msg="None is not a valid chunk input",
184-
exception_cls=ValueError,
185200
),
186201
# `True` is rejected explicitly because bool is a subclass of int — without
187202
# this guard, `chunks=True` would silently produce size-1 chunks.
188-
ExpectErr(
203+
ExpectFail(
189204
input=(True, (100,)),
205+
exception=ValueError,
206+
id="true",
190207
msg="True is not a valid chunk input",
191-
exception_cls=ValueError,
192208
),
193-
ExpectErr(input=("foo", (100,)), msg="dimensions", exception_cls=ValueError),
194-
ExpectErr(input=((100, 10), (100,)), msg="dimensions", exception_cls=ValueError),
195-
ExpectErr(input=((10,), (100, 100)), msg="dimensions", exception_cls=ValueError),
209+
ExpectFail(input=("foo", (100,)), exception=ValueError, id="string", msg="dimensions"),
210+
ExpectFail(
211+
input=((100, 10), (100,)), exception=ValueError, id="too-many-dims", msg="dimensions"
212+
),
213+
ExpectFail(
214+
input=((10,), (100, 100)), exception=ValueError, id="too-few-dims", msg="dimensions"
215+
),
196216
# End-to-end: per-dim RLE surfaces through normalize_chunks_nd.
197-
ExpectErr(
217+
ExpectFail(
198218
input=([[6, 4], [[3, 3], 1]], (10, 10)),
219+
exception=TypeError,
220+
id="rle-inner-dim",
199221
msg="non-integer element(s) ([3, 3],) at indices (0,)",
200-
exception_cls=TypeError,
222+
escape=True,
201223
),
202224
],
203-
ids=["none", "true", "string", "too-many-dims", "too-few-dims", "rle-inner-dim"],
225+
ids=lambda c: c.id,
204226
)
205-
def test_normalize_chunks_nd_errors(case: ExpectErr[tuple[Any, tuple[int, ...]]]) -> None:
227+
def test_normalize_chunks_nd_errors(case: ExpectFail[tuple[Any, tuple[int, ...]]]) -> None:
206228
"""Invalid N-D chunk specifications are rejected with informative error messages."""
207229
chunks, shape = case.input
208-
with pytest.raises(case.exception_cls, match=re.escape(case.msg)):
230+
with case.raises():
209231
normalize_chunks_nd(chunks, shape)
210232

211233

212234
@pytest.mark.parametrize(
213235
"case",
214236
[
215237
# uniform-chunks branch: one int → broadcast across span via np.full.
216-
Expect(input=(1000, 100_000), expected=[1000] * 100),
238+
Expect(input=(1000, 100_000), output=[1000] * 100, id="uniform"),
217239
# explicit-per-chunk branch.
218-
Expect(input=([10, 20, 30, 40], 100), expected=[10, 20, 30, 40]),
240+
Expect(input=([10, 20, 30, 40], 100), output=[10, 20, 30, 40], id="explicit-list"),
219241
# -1 sentinel branch: one chunk covering the full span.
220-
Expect(input=(-1, 100), expected=[100]),
242+
Expect(input=(-1, 100), output=[100], id="full-span-sentinel"),
221243
],
222-
ids=["uniform", "explicit-list", "full-span-sentinel"],
244+
ids=lambda c: c.id,
223245
)
224246
def test_normalize_chunks_1d_returns_int64_array(
225247
case: Expect[tuple[Any, int], list[int]],
@@ -230,4 +252,4 @@ def test_normalize_chunks_1d_returns_int64_array(
230252
assert isinstance(result, np.ndarray)
231253
assert result.dtype == np.int64
232254
assert result.ndim == 1
233-
assert result.tolist() == case.expected
255+
assert result.tolist() == case.output

tests/test_codecs/conftest.py

Lines changed: 0 additions & 20 deletions
This file was deleted.

0 commit comments

Comments
 (0)