From de88dd48bc73812bffcef2f96046647da1ec42d5 Mon Sep 17 00:00:00 2001 From: Deepak Cherian Date: Tue, 17 Sep 2024 21:36:18 -0600 Subject: [PATCH 1/6] Generalize stateful store test --- src/zarr/testing/strategies.py | 3 +- tests/v3/conftest.py | 11 ++++++- tests/v3/test_store/test_stateful_store.py | 34 +++++++++++++++------- 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index 83de3d92ce..916df39662 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -32,7 +32,8 @@ ) array_names = node_names attrs = st.none() | st.dictionaries(_attr_keys, _attr_values) -paths = st.lists(node_names, min_size=1).map(lambda x: "/".join(x)) | st.just("/") +keys = st.lists(node_names, min_size=1).map(lambda x: "/".join(x)) +paths = st.just("/") | keys np_arrays = npst.arrays( # TODO: re-enable timedeltas once they are supported dtype=npst.scalar_dtypes().filter(lambda x: x.kind != "m"), diff --git a/tests/v3/conftest.py b/tests/v3/conftest.py index 41cd359346..d216094401 100644 --- a/tests/v3/conftest.py +++ b/tests/v3/conftest.py @@ -10,6 +10,8 @@ from hypothesis import HealthCheck, Verbosity, settings from zarr import AsyncGroup, config +from zarr.abc.store import Store +from zarr.core.sync import sync from zarr.store import LocalStore, MemoryStore, StorePath, ZipStore from zarr.store.remote import RemoteStore @@ -20,7 +22,6 @@ from _pytest.compat import LEGACY_PATH - from zarr.abc.store import Store from zarr.core.common import ChunkCoords, MemoryOrder, ZarrFormat @@ -77,6 +78,14 @@ async def store(request: pytest.FixtureRequest, tmpdir: LEGACY_PATH) -> Store: return await parse_store(param, str(tmpdir)) +@pytest.fixture(params=["local", "memory", "zip"]) +def sync_store(request: pytest.FixtureRequest, tmpdir: LEGACY_PATH) -> Store: + result = sync(parse_store(request.param, str(tmpdir))) + if not isinstance(result, Store): + raise TypeError("Wrong store class returned by test fixture! got " + result + " instead") + return result + + @dataclass class AsyncGroupRequest: zarr_format: ZarrFormat diff --git a/tests/v3/test_store/test_stateful_store.py b/tests/v3/test_store/test_stateful_store.py index 1ecbd87cc1..555ecf5106 100644 --- a/tests/v3/test_store/test_stateful_store.py +++ b/tests/v3/test_store/test_stateful_store.py @@ -1,21 +1,23 @@ # Stateful tests for arbitrary Zarr stores. - - import hypothesis.strategies as st +import pytest from hypothesis import assume, note from hypothesis.stateful import ( RuleBasedStateMachine, + Settings, + initialize, invariant, precondition, rule, + run_state_machine_as_test, ) from hypothesis.strategies import DataObject import zarr from zarr.abc.store import AccessMode, Store from zarr.core.buffer import BufferPrototype, cpu, default_buffer_prototype -from zarr.store import MemoryStore -from zarr.testing.strategies import key_ranges, paths +from zarr.store import ZipStore +from zarr.testing.strategies import key_ranges, keys class SyncStoreWrapper(zarr.core.sync.SyncMixin): @@ -103,13 +105,17 @@ class ZarrStoreStateMachine(RuleBasedStateMachine): https://hypothesis.readthedocs.io/en/latest/stateful.html """ - def __init__(self) -> None: + def __init__(self, store) -> None: super().__init__() self.model: dict[str, bytes] = {} - self.store = SyncStoreWrapper(MemoryStore(mode="w")) + self.store = SyncStoreWrapper(store) self.prototype = default_buffer_prototype() - @rule(key=paths, data=st.binary(min_size=0, max_size=100)) + @initialize() + def init_store(self): + self.store.clear() + + @rule(key=keys, data=st.binary(min_size=0, max_size=100)) def set(self, key: str, data: DataObject) -> None: note(f"(set) Setting {key!r} with {data}") assert not self.store.mode.readonly @@ -118,7 +124,7 @@ def set(self, key: str, data: DataObject) -> None: self.model[key] = data_buf @precondition(lambda self: len(self.model.keys()) > 0) - @rule(key=paths, data=st.data()) + @rule(key=keys, data=st.data()) def get(self, key: str, data: DataObject) -> None: key = data.draw( st.sampled_from(sorted(self.model.keys())) @@ -128,7 +134,7 @@ def get(self, key: str, data: DataObject) -> None: # to bytes here necessary because data_buf set to model in set() assert self.model[key].to_bytes() == (store_value.to_bytes()) - @rule(key=paths, data=st.data()) + @rule(key=keys, data=st.data()) def get_invalid_keys(self, key: str, data: DataObject) -> None: note("(get_invalid)") assume(key not in self.model.keys()) @@ -186,7 +192,7 @@ def empty(self) -> None: # make sure they either both are or both aren't empty (same state) assert self.store.empty() == (not self.model) - @rule(key=paths) + @rule(key=keys) def exists(self, key: str) -> None: note("(exists)") @@ -227,4 +233,10 @@ def check_keys(self) -> None: note("checking keys / exists / empty") -StatefulStoreTest = ZarrStoreStateMachine.TestCase +def test_zarr_hierarchy(sync_store: Store): + def mk_test_instance_sync(): + return ZarrStoreStateMachine(sync_store) + + if isinstance(sync_store, ZipStore): + pytest.skip(reason="ZipStore does not support delete") + run_state_machine_as_test(mk_test_instance_sync, settings=Settings(report_multiple_bugs=False)) From 4ff48a68819db49dc7943a704ea61295e5897d00 Mon Sep 17 00:00:00 2001 From: Deepak Cherian Date: Tue, 17 Sep 2024 21:53:53 -0600 Subject: [PATCH 2/6] Fix for localstore --- src/zarr/testing/strategies.py | 2 +- tests/v3/test_store/test_stateful_store.py | 28 ++++++++++++++-------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index 916df39662..77e51f57e0 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -155,7 +155,7 @@ def basic_indices(draw: st.DrawFn, *, shape: tuple[int], **kwargs): # type: ign ) -def key_ranges(keys: SearchStrategy = node_names) -> SearchStrategy[list]: +def key_ranges(keys: SearchStrategy = node_names) -> SearchStrategy[list[int]]: """ Function to generate key_ranges strategy for get_partial_values() returns list strategy w/ form:: diff --git a/tests/v3/test_store/test_stateful_store.py b/tests/v3/test_store/test_stateful_store.py index 555ecf5106..e402682dd8 100644 --- a/tests/v3/test_store/test_stateful_store.py +++ b/tests/v3/test_store/test_stateful_store.py @@ -16,7 +16,7 @@ import zarr from zarr.abc.store import AccessMode, Store from zarr.core.buffer import BufferPrototype, cpu, default_buffer_prototype -from zarr.store import ZipStore +from zarr.store import LocalStore, ZipStore from zarr.testing.strategies import key_ranges, keys @@ -183,9 +183,13 @@ def clear(self) -> None: self.store.clear() self.model.clear() + assert self.store.empty() + assert len(self.model.keys()) == len(list(self.store.list())) == 0 @rule() + # Local store can be non-empty when there are subdirectories but no files + @precondition(lambda self: not isinstance(self.store.store, LocalStore)) def empty(self) -> None: note("(empty)") @@ -201,9 +205,9 @@ def exists(self, key: str) -> None: @invariant() def check_paths_equal(self) -> None: note("Checking that paths are equal") - paths = list(self.store.list()) + paths = sorted(self.store.list()) - assert list(self.model.keys()) == paths + assert sorted(self.model.keys()) == paths @invariant() def check_vals_equal(self) -> None: @@ -220,16 +224,18 @@ def check_num_keys_equal(self) -> None: @invariant() def check_keys(self) -> None: + # For local store, this can be empty if there are no files, + # but there are subfolders that weren't cleared. keys = list(self.store.list()) - if len(keys) == 0: - assert self.store.empty() is True + # if len(keys) == 0: + # assert self.store.empty() is True - elif len(keys) != 0: - assert self.store.empty() is False + # elif len(keys) != 0: + # assert self.store.empty() is False - for key in keys: - assert self.store.exists(key) is True + for key in keys: + assert self.store.exists(key) is True note("checking keys / exists / empty") @@ -239,4 +245,6 @@ def mk_test_instance_sync(): if isinstance(sync_store, ZipStore): pytest.skip(reason="ZipStore does not support delete") - run_state_machine_as_test(mk_test_instance_sync, settings=Settings(report_multiple_bugs=False)) + if isinstance(sync_store, LocalStore): + pytest.skip(reason="This test does not handle subdirectories") + run_state_machine_as_test(mk_test_instance_sync, settings=Settings(report_multiple_bugs=True)) From 7e8c1c2a620309492bf3e9cc0d8be7014649eb1d Mon Sep 17 00:00:00 2001 From: Deepak Cherian Date: Tue, 17 Sep 2024 22:03:54 -0600 Subject: [PATCH 3/6] cleanup --- tests/v3/test_store/test_stateful_store.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/v3/test_store/test_stateful_store.py b/tests/v3/test_store/test_stateful_store.py index e402682dd8..a753abe18c 100644 --- a/tests/v3/test_store/test_stateful_store.py +++ b/tests/v3/test_store/test_stateful_store.py @@ -224,18 +224,18 @@ def check_num_keys_equal(self) -> None: @invariant() def check_keys(self) -> None: - # For local store, this can be empty if there are no files, - # but there are subfolders that weren't cleared. keys = list(self.store.list()) - # if len(keys) == 0: - # assert self.store.empty() is True + # NOTE: A local store can be non-empty if there are no files, + # but there are subfolders that weren't cleared. + if len(keys) == 0: + assert self.store.empty() is True - # elif len(keys) != 0: - # assert self.store.empty() is False + elif len(keys) != 0: + assert self.store.empty() is False - for key in keys: - assert self.store.exists(key) is True + for key in keys: + assert self.store.exists(key) is True note("checking keys / exists / empty") From ae42bff166beb510eb35059bfe665da934ff4924 Mon Sep 17 00:00:00 2001 From: Deepak Cherian Date: Mon, 23 Sep 2024 21:34:07 -0600 Subject: [PATCH 4/6] Address feedback --- tests/v3/conftest.py | 4 ++-- tests/v3/test_store/test_stateful_store.py | 23 +++++++++++----------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/tests/v3/conftest.py b/tests/v3/conftest.py index 20b77d888e..5bb2048219 100644 --- a/tests/v3/conftest.py +++ b/tests/v3/conftest.py @@ -79,8 +79,8 @@ async def store(request: pytest.FixtureRequest, tmpdir: LEGACY_PATH) -> Store: @pytest.fixture(params=["local", "memory", "zip"]) -def sync_store(request: pytest.FixtureRequest, tmpdir: LEGACY_PATH) -> Store: - result = sync(parse_store(request.param, str(tmpdir))) +def sync_store(request: pytest.FixtureRequest, tmp_path: LEGACY_PATH) -> Store: + result = sync(parse_store(request.param, str(tmp_path))) if not isinstance(result, Store): raise TypeError("Wrong store class returned by test fixture! got " + result + " instead") return result diff --git a/tests/v3/test_store/test_stateful_store.py b/tests/v3/test_store/test_stateful_store.py index a753abe18c..4a2cbf5b3a 100644 --- a/tests/v3/test_store/test_stateful_store.py +++ b/tests/v3/test_store/test_stateful_store.py @@ -17,7 +17,8 @@ from zarr.abc.store import AccessMode, Store from zarr.core.buffer import BufferPrototype, cpu, default_buffer_prototype from zarr.store import LocalStore, ZipStore -from zarr.testing.strategies import key_ranges, keys +from zarr.testing.strategies import key_ranges +from zarr.testing.strategies import keys as zarr_keys class SyncStoreWrapper(zarr.core.sync.SyncMixin): @@ -105,7 +106,7 @@ class ZarrStoreStateMachine(RuleBasedStateMachine): https://hypothesis.readthedocs.io/en/latest/stateful.html """ - def __init__(self, store) -> None: + def __init__(self, store: Store) -> None: super().__init__() self.model: dict[str, bytes] = {} self.store = SyncStoreWrapper(store) @@ -115,7 +116,7 @@ def __init__(self, store) -> None: def init_store(self): self.store.clear() - @rule(key=keys, data=st.binary(min_size=0, max_size=100)) + @rule(key=zarr_keys, data=st.binary(min_size=0, max_size=100)) def set(self, key: str, data: DataObject) -> None: note(f"(set) Setting {key!r} with {data}") assert not self.store.mode.readonly @@ -124,7 +125,7 @@ def set(self, key: str, data: DataObject) -> None: self.model[key] = data_buf @precondition(lambda self: len(self.model.keys()) > 0) - @rule(key=keys, data=st.data()) + @rule(key=zarr_keys, data=st.data()) def get(self, key: str, data: DataObject) -> None: key = data.draw( st.sampled_from(sorted(self.model.keys())) @@ -134,8 +135,8 @@ def get(self, key: str, data: DataObject) -> None: # to bytes here necessary because data_buf set to model in set() assert self.model[key].to_bytes() == (store_value.to_bytes()) - @rule(key=keys, data=st.data()) - def get_invalid_keys(self, key: str, data: DataObject) -> None: + @rule(key=zarr_keys, data=st.data()) + def get_invalid_zarr_keys(self, key: str, data: DataObject) -> None: note("(get_invalid)") assume(key not in self.model.keys()) assert self.store.get(key, self.prototype) is None @@ -196,7 +197,7 @@ def empty(self) -> None: # make sure they either both are or both aren't empty (same state) assert self.store.empty() == (not self.model) - @rule(key=keys) + @rule(key=zarr_keys) def exists(self, key: str) -> None: note("(exists)") @@ -217,13 +218,13 @@ def check_vals_equal(self) -> None: assert self.model[key].to_bytes() == store_item @invariant() - def check_num_keys_equal(self) -> None: - note("check num keys equal") + def check_num_zarr_keys_equal(self) -> None: + note("check num zarr_keys equal") assert len(self.model) == len(list(self.store.list())) @invariant() - def check_keys(self) -> None: + def check_zarr_keys(self) -> None: keys = list(self.store.list()) # NOTE: A local store can be non-empty if there are no files, @@ -239,7 +240,7 @@ def check_keys(self) -> None: note("checking keys / exists / empty") -def test_zarr_hierarchy(sync_store: Store): +def test_zarr_hierarchy(sync_store: Store) -> None: def mk_test_instance_sync(): return ZarrStoreStateMachine(sync_store) From 27efef01f79de77e10d1bf6c923a791b09e2e8dd Mon Sep 17 00:00:00 2001 From: Deepak Cherian Date: Tue, 24 Sep 2024 09:18:48 -0600 Subject: [PATCH 5/6] Limit binary size, and range for key_ranges --- src/zarr/testing/strategies.py | 7 +++++-- tests/v3/test_store/test_stateful_store.py | 12 ++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index 77c1e0da51..0c46174a26 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -160,7 +160,9 @@ def basic_indices(draw: st.DrawFn, *, shape: tuple[int], **kwargs): # type: ign ) -def key_ranges(keys: SearchStrategy = node_names) -> SearchStrategy[list[int]]: +def key_ranges( + keys: SearchStrategy = node_names, max_size: int | None = None +) -> SearchStrategy[list[int]]: """ Function to generate key_ranges strategy for get_partial_values() returns list strategy w/ form:: @@ -169,7 +171,8 @@ def key_ranges(keys: SearchStrategy = node_names) -> SearchStrategy[list[int]]: (key, (range_start, range_step)),...] """ byte_ranges = st.tuples( - st.none() | st.integers(min_value=0), st.none() | st.integers(min_value=0) + st.none() | st.integers(min_value=0, max_value=max_size), + st.none() | st.integers(min_value=0, max_value=max_size), ) key_tuple = st.tuples(keys, byte_ranges) key_range_st = st.lists(key_tuple, min_size=1, max_size=10) diff --git a/tests/v3/test_store/test_stateful_store.py b/tests/v3/test_store/test_stateful_store.py index 4a2cbf5b3a..1d0de338b9 100644 --- a/tests/v3/test_store/test_stateful_store.py +++ b/tests/v3/test_store/test_stateful_store.py @@ -20,6 +20,8 @@ from zarr.testing.strategies import key_ranges from zarr.testing.strategies import keys as zarr_keys +MAX_BINARY_SIZE = 100 + class SyncStoreWrapper(zarr.core.sync.SyncMixin): def __init__(self, store: Store) -> None: @@ -116,7 +118,7 @@ def __init__(self, store: Store) -> None: def init_store(self): self.store.clear() - @rule(key=zarr_keys, data=st.binary(min_size=0, max_size=100)) + @rule(key=zarr_keys, data=st.binary(min_size=0, max_size=MAX_BINARY_SIZE)) def set(self, key: str, data: DataObject) -> None: note(f"(set) Setting {key!r} with {data}") assert not self.store.mode.readonly @@ -144,7 +146,9 @@ def get_invalid_zarr_keys(self, key: str, data: DataObject) -> None: @precondition(lambda self: len(self.model.keys()) > 0) @rule(data=st.data()) def get_partial_values(self, data: DataObject) -> None: - key_range = data.draw(key_ranges(keys=st.sampled_from(sorted(self.model.keys())))) + key_range = data.draw( + key_ranges(keys=st.sampled_from(sorted(self.model.keys())), max_size=MAX_BINARY_SIZE) + ) note(f"(get partial) {key_range=}") obs_maybe = self.store.get_partial_values(key_range, self.prototype) observed = [] @@ -246,6 +250,6 @@ def mk_test_instance_sync(): if isinstance(sync_store, ZipStore): pytest.skip(reason="ZipStore does not support delete") - if isinstance(sync_store, LocalStore): - pytest.skip(reason="This test does not handle subdirectories") + # if isinstance(sync_store, LocalStore): + # pytest.skip(reason="This test does not handle subdirectories") run_state_machine_as_test(mk_test_instance_sync, settings=Settings(report_multiple_bugs=True)) From 6e9eeb1f2898ed7de2fba23ea7785dc7d60e19a4 Mon Sep 17 00:00:00 2001 From: Deepak Cherian Date: Fri, 27 Sep 2024 10:43:24 -0600 Subject: [PATCH 6/6] small cleanup --- tests/v3/test_store/test_stateful_store.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/v3/test_store/test_stateful_store.py b/tests/v3/test_store/test_stateful_store.py index f1a1482f25..1a43b55fef 100644 --- a/tests/v3/test_store/test_stateful_store.py +++ b/tests/v3/test_store/test_stateful_store.py @@ -227,12 +227,10 @@ def check_num_zarr_keys_equal(self) -> None: def check_zarr_keys(self) -> None: keys = list(self.store.list()) - # NOTE: A local store can be non-empty if there are no files, - # but there are subfolders that weren't cleared. - if len(keys) == 0: + if not keys: assert self.store.empty() is True - elif len(keys) != 0: + else: assert self.store.empty() is False for key in keys: @@ -246,6 +244,6 @@ def mk_test_instance_sync(): if isinstance(sync_store, ZipStore): pytest.skip(reason="ZipStore does not support delete") - # if isinstance(sync_store, LocalStore): - # pytest.skip(reason="This test does not handle subdirectories") + if isinstance(sync_store, LocalStore): + pytest.skip(reason="This test has errors") run_state_machine_as_test(mk_test_instance_sync, settings=Settings(report_multiple_bugs=True))