Improve test coverage for storage classes #2693
Conversation
| @@ -18,6 +19,8 @@ | |||
|
|
|||
| counter: defaultdict[str, int] | |||
|
|
|||
| T_Store = TypeVar("T_Store", bound=Store) | |||
|
|
|||
|
|
|||
| class LoggingStore(WrapperStore[Store]): | |||
There was a problem hiding this comment.
Should WrapperStore be generic w.r.t T_Store here?
| with await self.store_cls.open(**open_kwargs) as store: | ||
| assert store._is_open | ||
| # Test trying to open an already open store | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
can we check that the error message in the ValueError has the expected content? We don't want this test to succeed because of a ValueError unrelated to the store being already open.
| await store._open() | ||
| assert not store._is_open | ||
|
|
||
| async def test_read_only_store_raises(self, open_kwargs: dict[str, Any]) -> None: |
There was a problem hiding this comment.
contrary to the name, this test doesn't seem to check that an exception is raised
| @@ -135,6 +154,26 @@ async def test_get(self, store: S, key: str, data: bytes, byte_range: ByteReques | |||
| expected = data_buf[start:stop] | |||
| assert_bytes_equal(observed, expected) | |||
|
|
|||
| async def test_get_not_open(self, store_not_open: S) -> None: | |||
There was a problem hiding this comment.
this is rather surprising -- I would expect that a non-open store would not support IO of any kind. what exactly does open mean? cc @jhamman
| async def test_getsize_raises(self, store: S) -> None: | ||
| """ | ||
| Test the result of store.getsize(). | ||
| """ |
There was a problem hiding this comment.
I think the method name and the docstring don't quite match the behavior of the test
| async def test_set_not_open(self, store_not_open: S) -> None: | ||
| """ | ||
| Ensure that data can be written to the store that's not yet open using the store.set method. | ||
| """ |
There was a problem hiding this comment.
tests/test_store/test_core.py
Outdated
| @pytest.mark.parametrize("zarr_format", [2, 3]) | ||
| async def test_contains_group(local_store, write_group: bool, zarr_format: ZarrFormat) -> None: | ||
| """ | ||
| Test contains group method |
There was a problem hiding this comment.
can we parametrize this over path, ensuring that we check a level of nesting? e.g. @pytest.mark.parametrize('path', ['foo', 'foo/bar'])
There was a problem hiding this comment.
and similarly for the contains_array tests
| with pytest.raises(ValueError): | ||
| assert await contains_array(store_path, zarr_format="3.0") |
There was a problem hiding this comment.
| with pytest.raises(ValueError): | |
| assert await contains_array(store_path, zarr_format="3.0") |
deduplicate
There was a problem hiding this comment.
this is a distinct check for contains_array rather than contains_group. I can parameterize these functions to make it more concise and clear.
There was a problem hiding this comment.
ah oops, I missed that these were testing different methods
| with pytest.raises(ValueError): | ||
| await StorePath.open(LocalStore(str(tmpdir), read_only=False), path=None, mode="x") |
There was a problem hiding this comment.
lets parametrize the test over mode instead of repeating nearly identical checks
There was a problem hiding this comment.
looks like we would need to parametrize over (read_only, mode) tuples
| @@ -53,3 +54,17 @@ def test_creates_new_directory(self, tmp_path: pathlib.Path): | |||
|
|
|||
| store = self.store_cls(root=target) | |||
| zarr.group(store=store) | |||
|
|
|||
| def test_invalid_root_raises(self): | |||
There was a problem hiding this comment.
add a docstring explaining what this test is checking
|
this looks great, I had some minor suggestions. |
Co-authored-by: Davis Bennett <[email protected]>
This PR improves the test coverage for the various storage classes. While testing the storage classes, I fixed a few issues:
open()forLoggingStore_is_openproperty and setter forWrapperStorestdoutrather thanstderras the default stream forLoggingStoreZipStoreis open before getting or setting any valuesLoggingStoreandWrapperStoresuch that the types much be equal. This is an opinionated change. For example, previously a LocalStore and LoggingStore instance could be evaluated as equal, whereas now they are distinct.Here's the change in coverage:
src/zarr/storage/memory.pycoverage is low because it includes the GPUStore and I don't have a test environment with cuda. I'm opening this PR now even though it's not at 100% coverage because I don't expect to have much time to work on it during the week and would rather the PR not get stale if the team has time for a review.The set partial values methods are addressed separately because they require discussion (xref #2688).