Skip to content

Enable stateful tests for LocalStore #2804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/hypothesis.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ jobs:
path: .hypothesis/
key: cache-hypothesis-${{ runner.os }}-${{ github.run_id }}

- name: Upload coverage
uses: codecov/codecov-action@v5
with:
token: ${{ secrets.CODECOV_TOKEN }}
verbose: true # optional (default = false)

- name: Generate and publish the report
if: |
failure()
Expand Down
1 change: 1 addition & 0 deletions changes/2804.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
:py:class:`LocalStore` learned to ``delete_dir``. This makes array and group deletes more efficient.
5 changes: 3 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ run = "run-coverage --no-cov"
run-pytest = "run"
run-verbose = "run-coverage --verbose"
run-mypy = "mypy src"
run-hypothesis = "pytest --hypothesis-profile ci tests/test_properties.py tests/test_store/test_stateful*"
run-hypothesis = "run-coverage --hypothesis-profile ci --run-slow-hypothesis tests/test_properties.py tests/test_store/test_stateful*"
list-env = "pip list"

[tool.hatch.envs.doctest]
Expand Down Expand Up @@ -398,7 +398,8 @@ filterwarnings = [
"ignore:.*is currently not part in the Zarr format 3 specification.*:UserWarning",
]
markers = [
"gpu: mark a test as requiring CuPy and GPU"
"gpu: mark a test as requiring CuPy and GPU",
"slow_hypothesis: slow hypothesis tests",
]

[tool.repo-review]
Expand Down
13 changes: 13 additions & 0 deletions src/zarr/storage/_local.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,19 @@
else:
await asyncio.to_thread(path.unlink, True) # Q: we may want to raise if path is missing

async def delete_dir(self, prefix: str) -> None:
# docstring inherited
self._check_writable()
path = self.root / prefix
if path.is_dir():
shutil.rmtree(path)
elif path.is_file():
raise ValueError(f"delete_dir was passed a {prefix=!r} that is a file")

Check warning on line 218 in src/zarr/storage/_local.py

View check run for this annotation

Codecov / codecov/patch

src/zarr/storage/_local.py#L218

Added line #L218 was not covered by tests
else:
# Non-existent directory
# This path is tested by test_group:test_create_creates_parents for one
pass

async def exists(self, key: str) -> bool:
# docstring inherited
path = self.root / key
Expand Down
13 changes: 13 additions & 0 deletions src/zarr/testing/stateful.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@
# -------------------- store operations -----------------------
@rule(name=node_names, data=st.data())
def add_group(self, name: str, data: DataObject) -> None:
# Handle possible case-insensitive file systems (e.g. MacOS)
if isinstance(self.store, LocalStore):
name = name.lower()

Check warning on line 73 in src/zarr/testing/stateful.py

View check run for this annotation

Codecov / codecov/patch

src/zarr/testing/stateful.py#L72-L73

Added lines #L72 - L73 were not covered by tests
if self.all_groups:
parent = data.draw(st.sampled_from(sorted(self.all_groups)), label="Group parent")
else:
Expand All @@ -90,6 +93,9 @@
name: str,
array_and_chunks: tuple[np.ndarray[Any, Any], tuple[int, ...]],
) -> None:
# Handle possible case-insensitive file systems (e.g. MacOS)
if isinstance(self.store, LocalStore):
name = name.lower()
array, chunks = array_and_chunks
fill_value = data.draw(npst.from_dtype(array.dtype))
if self.all_groups:
Expand Down Expand Up @@ -135,6 +141,7 @@
# self.model.rename(from_group, new_path)
# self.repo.store.rename(from_group, new_path)

@precondition(lambda self: self.store.supports_deletes)
@precondition(lambda self: len(self.all_arrays) >= 1)
@rule(data=st.data())
def delete_array_using_del(self, data: DataObject) -> None:
Expand All @@ -149,6 +156,7 @@
del group[array_name]
self.all_arrays.remove(array_path)

@precondition(lambda self: self.store.supports_deletes)
@precondition(lambda self: len(self.all_groups) >= 2) # fixme don't delete root
@rule(data=st.data())
def delete_group_using_del(self, data: DataObject) -> None:
Expand Down Expand Up @@ -284,6 +292,10 @@
def supports_writes(self) -> bool:
return self.store.supports_writes

@property
def supports_deletes(self) -> bool:
return self.store.supports_deletes

Check warning on line 297 in src/zarr/testing/stateful.py

View check run for this annotation

Codecov / codecov/patch

src/zarr/testing/stateful.py#L297

Added line #L297 was not covered by tests


class ZarrStoreStateMachine(RuleBasedStateMachine):
""" "
Expand Down Expand Up @@ -366,6 +378,7 @@
model_vals_ls,
)

@precondition(lambda self: self.store.supports_deletes)
@precondition(lambda self: len(self.model.keys()) > 0)
@rule(data=st.data())
def delete(self, data: DataObject) -> None:
Expand Down
11 changes: 8 additions & 3 deletions src/zarr/testing/strategies.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import sys
from typing import Any

import hypothesis.extra.numpy as npst
Expand Down Expand Up @@ -209,7 +210,7 @@


def key_ranges(
keys: SearchStrategy = node_names, max_size: int | None = None
keys: SearchStrategy = node_names, max_size: int = sys.maxsize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of our public API, so changing the default here is an API change. What was the motivation for changing it - does hypothesis not work well with max_value=None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleans up the use of min below, that's all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

) -> SearchStrategy[list[int]]:
"""
Function to generate key_ranges strategy for get_partial_values()
Expand All @@ -218,10 +219,14 @@
[(key, (range_start, range_end)),
(key, (range_start, range_end)),...]
"""

def make_request(start: int, length: int) -> RangeByteRequest:
return RangeByteRequest(start, end=min(start + length, max_size))

Check warning on line 224 in src/zarr/testing/strategies.py

View check run for this annotation

Codecov / codecov/patch

src/zarr/testing/strategies.py#L223-L224

Added lines #L223 - L224 were not covered by tests

byte_ranges = st.builds(
RangeByteRequest,
make_request,
start=st.integers(min_value=0, max_value=max_size),
end=st.integers(min_value=0, max_value=max_size),
length=st.integers(min_value=0, max_value=max_size),
)
key_tuple = st.tuples(keys, byte_ranges)
return st.lists(key_tuple, min_size=1, max_size=10)
18 changes: 18 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,24 @@ def zarr_format(request: pytest.FixtureRequest) -> ZarrFormat:
raise ValueError(msg)


def pytest_addoption(parser: Any) -> None:
parser.addoption(
"--run-slow-hypothesis",
action="store_true",
default=False,
help="run slow hypothesis tests",
)


def pytest_collection_modifyitems(config: Any, items: Any) -> None:
if config.getoption("--run-slow-hypothesis"):
return
skip_slow_hyp = pytest.mark.skip(reason="need --run-slow-hypothesis option to run")
for item in items:
if "slow_hypothesis" in item.keywords:
item.add_marker(skip_slow_hyp)


settings.register_profile(
"ci",
max_examples=1000,
Expand Down
26 changes: 12 additions & 14 deletions tests/test_store/test_stateful.py
Original file line number Diff line number Diff line change
@@ -1,25 +1,24 @@
# Stateful tests for arbitrary Zarr stores.
import pytest
from hypothesis.stateful import (
Settings,
run_state_machine_as_test,
)

from zarr.abc.store import Store
from zarr.storage import LocalStore, MemoryStore, ZipStore
from zarr.storage import LocalStore, ZipStore
from zarr.testing.stateful import ZarrHierarchyStateMachine, ZarrStoreStateMachine

pytestmark = pytest.mark.slow_hypothesis


def test_zarr_hierarchy(sync_store: Store):
def mk_test_instance_sync() -> ZarrHierarchyStateMachine:
return ZarrHierarchyStateMachine(sync_store)

if isinstance(sync_store, ZipStore):
pytest.skip(reason="ZipStore does not support delete")
if isinstance(sync_store, MemoryStore):
run_state_machine_as_test(
mk_test_instance_sync, settings=Settings(report_multiple_bugs=False, max_examples=50)
)

run_state_machine_as_test(mk_test_instance_sync)


def test_zarr_store(sync_store: Store) -> None:
Expand All @@ -28,11 +27,10 @@ def mk_test_instance_sync() -> None:

if isinstance(sync_store, ZipStore):
pytest.skip(reason="ZipStore does not support delete")
elif isinstance(sync_store, LocalStore):
pytest.skip(reason="This test has errors")
elif isinstance(sync_store, MemoryStore):
run_state_machine_as_test(mk_test_instance_sync, settings=Settings(max_examples=50))
else:
run_state_machine_as_test(
mk_test_instance_sync, settings=Settings(report_multiple_bugs=True)
)

if isinstance(sync_store, LocalStore):
# This test uses arbitrary keys, which are passed to `set` and `delete`.
# It assumes that `set` and `delete` are the only two operations that modify state.
# But LocalStore, directories can hang around even after a key is delete-d.
pytest.skip(reason="Test isn't suitable for LocalStore.")
run_state_machine_as_test(mk_test_instance_sync)