[V3] Expand store tests #1900
[V3] Expand store tests #1900
Conversation
…ations to implement them; make StoreTests generic w.r.t. the store class being tested; update store.get abc to match actual type signature
src/zarr/testing/store.py
Outdated
| def set(self, store: S, key: str, value: Buffer) -> None: | ||
| """ | ||
| Insert key: value pairs into a store without using the store methods. | ||
| """ | ||
| raise NotImplementedError | ||
|
|
||
| def get(self, store: S, key: str) -> Buffer: | ||
| """ | ||
| Retrieve values from a store without using the store methods. | ||
| """ | ||
|
|
||
| raise NotImplementedError |
There was a problem hiding this comment.
@jhamman I added these two methods so that store.get and store.set could be tested independently. i hope this functionality is consistent with your intention for this class.
| async def test_set_get_bytes_roundtrip(self, store: Store, key: str, data: bytes) -> None: | ||
| await store.set(key, Buffer.from_bytes(data)) | ||
| assert_bytes_equal(await store.get(key), data) |
There was a problem hiding this comment.
this test could fail due to bugs in store.set, or store.get, or both. To disambiguate, I wrote separate tests for store.set and store.get, that, if both pass, are equivalent to a round-trip test.
| if byte_range is not None: | ||
| value = value[byte_range[0] : byte_range[1]] | ||
| return value | ||
| start, length = _normalize_interval_index(value, byte_range) | ||
| return value[start : start + length] |
There was a problem hiding this comment.
the semantics of byte_range were inconsistent between LocalStore and MemoryStore. LocalStore was treating byte_range as (start, length), while MemoryStore was treating it as (start, stop).
We should be consistent, which is why I changed MemoryStore to match LocalStore here, but if people prefer (start, stop) semantics (which is closer to a python slice object), then I'd be happy moving to that, too. for what it's worth, (start, stop) might be more intuitive to python users.
There was a problem hiding this comment.
Sounds good to normalize them. We should favor what works for HTTP including negative start offset. Can we make the semantics more explicit in the signature?
There was a problem hiding this comment.
yes, I think we can. I'm not too familiar with http range requests (reading about it now) but it seems like the range request header actually takes a tuple of [start, end] pairs. interesting!
| class StoreTests(Generic[S]): | ||
| store_cls: type[S] |
There was a problem hiding this comment.
StoreTests is now parametrized by the type of the store being tested, because I am lazy and I wanted my IDE to automatically fill out the type annotations for various methods of this class.
|
i think this is ready for review. I'm deferring changes to the If these changes are OK, we should move on implementing this test suite for a remote store. |
This PR expands the current suite of store tests. The goal is to provide complete coverage of all store methods. I am removing some old storage tests I wrote and instead building on top of tooling @jhamman introduced.
At the time of opening this PR, some
MemoryStoretests are failing, because the new tests reveal a bug inMemoryStore. Those tests will go green when I fix that bug.I am also using this PR to make some changes to the storage classes, if testing reveals any significant friction that would be relieved by such changes. I will explain such changes in this PR as they come up.
For example, the
auto_mkdirattribute forLocalStoreseems unnecessary given that as of v3 we expect as default behavior for arrays to store their chunks in a nested directory style; additionally, it was burdensome to parameterize overauto_mkdirin the testing suite, so I hard-codedauto_mkdirtoTruein theLocalStoremethods that previously used it as an instance attribute. I'm happy to revisit this decision if anyone has use for parametrizing store behavior withauto_mkdir.TODO: