more extensive `api` x `store` testing #2447

d-v-b · 2024-10-30T12:19:10Z

The api module contains a lot of our user-facing code, but we are essentially only testing it against LocalStore and MemoryStore. A better situation would be to test the functions in api against all the stores. That's what this PR adds. Note that there are many test failures. I will try to get them all fixed.

This PR also contains a fix for #2444, but the central addition is the (currently failing) test for that scenario.

TODO:

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

… the stores

d-v-b · 2024-10-30T17:21:56Z

this became a bit of a saga.

S3 test fixtures

I moved the s3 test fixture to the main conftest.py so that any test can depend on a RemoteStore instance. I was getting strange errors from our s3 test fixture. they went away after I made the following changes:

set the region to us-east-1
set the moto port automatically instead of hard-coding it. this makes the moto server URL dynamic, so I wired up the test fixtures that need that url to consume the s3_base fixture.

`store` test fixture

I also refactored the store test fixture. It's now possible to request a store class and a store mode with the appropriately formatted string, e.g. @pytest.mark.parametrize('store', ['local_a', 'local_w'], indirect=True) will request a LocalStore with mode="a", and a LocalStore with mode="w". This test fixture was completely broken for RemoteStore, so I fixed that. I also removed a redundant store test fixture from the group tests, and I removed the individual memory_store, local_store, etc test fixtures.

we should default to testing all the stores

IMO our default behavior should be to test relevant APIs against as many stores as possible, at least until slow tests becomes an issue. That being said, I'm happy to narrow the scope of certain tests, e.g. consolidated metadata tests, to only run on MemoryStore if we think that's OK.

`ZipStore` problems

We have a lot of tests that involve creating an array or group with a store, then opening a new array or group with the same store but with a different mode. This is impossible today because ZipStore doesn't do with_mode, so tests where I couldn't hack around this limitation are xfailed (there are several in test_api).

`open` logic

I had test failures relating to some of the logic in zarr.api.asynchronous.open. The version of this function in main is very hard to understand, the version in this PR is marginally simpler. I think it's possible in main to run open(...shape=(10,)) and get back a group, which should not be possible (and is not possible after my changes).

Failing tests

Here's the currently failing tests. I think these are all real problems exposed by adding more tests, but I don't really have any idea how to solve them. Help would be appreciated here!

FAILED tests/test_group.py::test_group_update_attributes_async[zarr2-remote] - botocore.exceptions.HTTPClientError: An HTTP Client raised an unhandled exception: Task <Task pending name='Task-147164' coro=<set_or_delete() running at /Users/runner/work/zarr-python/zarr-python/src/zarr/abc/store.py:425> cb=[gather.<locals>._done_callback() at /Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/tasks.py:820]> got Future <Future pending cb=[BaseSelectorEventLoop._sock_write_done()()]> attached to a different loop
FAILED tests/test_group.py::test_group_update_attributes_async[zarr3-remote] - botocore.exceptions.HTTPClientError: An HTTP Client raised an unhandled exception: Task <Task pending name='Task-147207' coro=<set_or_delete() running at /Users/runner/work/zarr-python/zarr-python/src/zarr/abc/store.py:425> cb=[gather.<locals>._done_callback() at /Library/Frameworks/Python.framework/Versions/3.13/lib/python3.13/asyncio/tasks.py:820]> got Future <Future pending cb=[BaseSelectorEventLoop._sock_write_done()()]> attached to a different loop
FAILED tests/test_group.py::test_members_name[zarr2-remote-True] - DeprecationWarning: datetime.datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.now(datetime.UTC).
FAILED tests/test_group.py::test_members_name[zarr2-remote-False] - DeprecationWarning: datetime.datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.now(datetime.UTC).
FAILED tests/test_group.py::test_members_name[zarr3-remote-True] - DeprecationWarning: datetime.datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.now(datetime.UTC).
FAILED tests/test_group.py::test_members_name[zarr3-remote-False] - DeprecationWarning: datetime.datetime.utcnow() is deprecated and scheduled for removal in a future version. Use timezone-aware objects to represent datetimes in UTC: datetime.datetime.now(datetime.UTC).

d-v-b · 2024-10-31T22:16:10Z

The remaining test falures are in the python 3.13 tests where botocore.auth uses datetime in a deprecated way, resulting in DeprecationWarning, which test_members_name treats as an error. See https://github.com/zarr-developers/zarr-python/actions/runs/11620549084/job/32362570100?pr=2447. Since this is purely a botocore x py3.13 issue, this test qua Zarr test should probably not fail. @jhamman any suggestions for changes to the test_members_name that would avoid this error?

…x/open-v2-array-remotestore

…-python into fix/open-v2-array-remotestore

…emotestore

…ix/open-v2-array-remotestore

d-v-b · 2024-11-15T19:28:27Z

tests/test_api.py

+def test_open_store_with_mode_r(store: Store) -> None:
+    # 'r' means read only (must exist)
+    with pytest.raises(FileNotFoundError):
+        zarr.open(store=store, mode="r")
+
+    z1 = zarr.ones(store=store, shape=(3, 3))
+    assert z1.fill_value == 1
+
+    z2 = zarr.open(store=store, mode="r")
+    assert isinstance(z2, Array)
+    assert z2.fill_value == 1
+    assert (z2[:] == 1).all()
+    with pytest.raises(ValueError):
+        z2[:] = 3


@jhamman this test is failing in this PR. What's the expected behavior here, given the new semantics of read_only for stores? Should we raise an error in zarr.open if the mode is r but the store is not read only?

Note the test below, which is copied from main, passes, but in that case zarr.open takes a path / string and thus has the opportunity to create the store in read only mode.

see #2447 (comment)

…x/open-v2-array-remotestore

…s in storepath creation; get tests to pass

d-v-b · 2024-11-26T10:13:18Z

getting the lasts tests to pass required adding two new methods to the stores: _as_mutable and _as_immutable, that return a non-read-only or a read-only copy of a store class, respectively. This is necessary because without some means of creating read-only copies of stores, there's no way for zarr.open(mutable_store, mode='r') to correctly produce a read-only store class.

I think this PR is pretty important insofar as it opens up the possibility of testing the store classes much more uniformly, it would be great to get this looked at soon!

d-v-b · 2024-11-26T10:43:11Z

A major downside to this PR is the additional time required for tests -- with these changes, tests in CI take nearly 20m to complete! On the other hand, I have found many bugs by testing extensively. Not sure how to find balance here.

dcherian · 2024-12-12T04:02:36Z

Not sure how to find balance here.

Hypothesis property tests! You can even configure it to run for longer on CI and shorter locally. You can also ensure specific cases always run

dcherian · 2024-12-12T04:03:39Z

src/zarr/abc/store.py

@@ -412,6 +412,18 @@ async def getsize_prefix(self, prefix: str) -> int:
        sizes = await concurrent_map(keys, self.getsize, limit=limit)
        return sum(sizes)

+    def _as_immutable(self: Self) -> Self:


in icechunk we have these as set_read_only and set_writeable which I mildly prefer. Simple words are nice.

dcherian · 2024-12-12T04:07:45Z

tests/test_api.py

+@pytest.mark.parametrize("store", ["local", "memory", "remote"], indirect=True)
+def test_create_array(store: Store) -> None:


Suggested change

@pytest.mark.parametrize("store", ["local", "memory", "remote"], indirect=True)

def test_create_array(store: Store) -> None:

from hypothesis import given

import hypothesis.strategies as st

stores = st.sampled_from(["local", "memory", "remote"])

@given(store=stores())

def test_create_array(store: Store) -> None:

chuckwondo · 2025-01-10T18:00:25Z

src/zarr/abc/store.py

@@ -412,6 +412,18 @@ async def getsize_prefix(self, prefix: str) -> int:
        sizes = await concurrent_map(keys, self.getsize, limit=limit)
        return sum(sizes)

+    def _as_immutable(self: Self) -> Self:
+        """
+        Return a mutable copy of the store.


It looks like perhaps the words "mutable" and "immutable" should be swapped between this method and the _as_mutable method, no?

oops, good catch!

d-v-b added 10 commits October 30, 2024 13:14

add filenotfounderror to tupe of handled errors

e309415

revive remote store test fixture, and parameterize api tests with all…

0045351

… the stores

properly construct remotestore instance in test fixture

d670b30

add filenotfounderror to tuple of handled errors

f6d35fb

propagate mode kwarg into open_group

311efaf

propagate mode kwarg into open_group

73911b7

xfail zipstore

9917343

revert removal of type ignores

90df740

speculative refactor of open func

3c3099a

refactor store test fixture

0219425

d-v-b mentioned this pull request Oct 30, 2024

open() fails for path to a v2 group in a RemoteStore #2444

Closed

d-v-b added 8 commits October 31, 2024 16:25

send s3fs fixture to remotestore fixture

bd5eb13

remove unused method from group and corresponding test

1c6044b

explicitly depend on moto server

4c5f06e

explicitly depend on moto server, also in test deps

420ca07

ignore mypy errors properly

bac994d

fixup

426e1cb

annotate store variables as Store

0013d22

add moto[server] to min_deps

fb52a7c

d-v-b marked this pull request as ready for review November 5, 2024 22:25

d-v-b added 8 commits November 5, 2024 23:25

Merge branch 'main' into fix/open-v2-array-remotestore

53b9faf

Merge branch 'main' of github.com:zarr-developers/zarr-python into fi…

2ab4dc3

…x/open-v2-array-remotestore

revert store fixture name convention

9ec0d01

remove unused import

7bc982e

remove mode arg for remotestore

d776ead

fixup

191c012

Merge branch 'main' of github.com:zarr-developers/zarr-python into fi…

da03f87

…x/open-v2-array-remotestore

add mutability altering funcs

9def17d

d-v-b added 17 commits November 15, 2024 14:41

get consolidated tests to pass

d1c95b2

fix for open_array in r+ mode

a04e636

fix broken zipstore delete_dir

e29e072

xfail zipstore in open_array test

cbf1a61

add missing import

cadfef1

Merge branch 'fix/open-v2-array-remotestore' of github.com:d-v-b/zarr…

f085859

…-python into fix/open-v2-array-remotestore

use create instead of open_array in fill_value test

c0f777d

remove xfail for zipstore in test_open_with_mode_r_plus

94e2468

xfail test that fails for any warning on python 3.12 or higher with r…

15b0211

…emotestore

remove xfail for zip

8d3b66c

lint

5e5376f

fix open_array for mode r+

d087e9b

Merge branch 'fix/open-r-plus' of github.com:d-v-b/zarr-python into f…

0c03d4a

…ix/open-v2-array-remotestore

simplify group tests by removing zip and remote store cases

9f45997

simplify test_array

c0d5407

use zarr_format fixture

f946499

add old open_with_mode_r test

c5b3588

d-v-b commented Nov 15, 2024

View reviewed changes

d-v-b added 3 commits November 15, 2024 21:06

Merge branch 'main' of github.com:zarr-developers/zarr-python into fi…

c62de51

…x/open-v2-array-remotestore

Merge branch 'main' of github.com:zarr-developers/zarr-python into fi…

f95c0fb

…x/open-v2-array-remotestore

add _as_immutable and _as_mutable methods to stores; use those method…

0fa90ba

…s in storepath creation; get tests to pass

jhamman added the V3 label Nov 29, 2024

dcherian reviewed Dec 12, 2024

View reviewed changes

dstansby removed the V3 label Dec 12, 2024

chuckwondo reviewed Jan 10, 2025

View reviewed changes

d-v-b mentioned this pull request Jan 11, 2025

[TST] Bring test coverage to 100% #2614

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more extensive `api` x `store` testing #2447

more extensive `api` x `store` testing #2447

d-v-b commented Oct 30, 2024 •

edited

Loading

d-v-b commented Oct 30, 2024

d-v-b commented Oct 31, 2024

d-v-b Nov 15, 2024

d-v-b Nov 26, 2024

d-v-b commented Nov 26, 2024

d-v-b commented Nov 26, 2024

dcherian commented Dec 12, 2024 •

edited

Loading

dcherian Dec 12, 2024

dcherian Dec 12, 2024 •

edited

Loading

chuckwondo Jan 10, 2025

d-v-b Jan 10, 2025

		@pytest.mark.parametrize("store", ["local", "memory", "remote"], indirect=True)
		def test_create_array(store: Store) -> None:

-@pytest.mark.parametrize("store", ["local", "memory", "remote"], indirect=True)
-def test_create_array(store: Store) -> None:
+from hypothesis import given
+import hypothesis.strategies as st
+stores = st.sampled_from(["local", "memory", "remote"])
+@given(store=stores())
+def test_create_array(store: Store) -> None:

more extensive api x store testing #2447

Are you sure you want to change the base?

more extensive api x store testing #2447

Conversation

d-v-b commented Oct 30, 2024 • edited Loading

d-v-b commented Oct 30, 2024

S3 test fixtures

store test fixture

we should default to testing all the stores

ZipStore problems

open logic

Failing tests

d-v-b commented Oct 31, 2024

d-v-b Nov 15, 2024

Choose a reason for hiding this comment

d-v-b Nov 26, 2024

Choose a reason for hiding this comment

d-v-b commented Nov 26, 2024

d-v-b commented Nov 26, 2024

dcherian commented Dec 12, 2024 • edited Loading

dcherian Dec 12, 2024

Choose a reason for hiding this comment

dcherian Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

chuckwondo Jan 10, 2025

Choose a reason for hiding this comment

d-v-b Jan 10, 2025

Choose a reason for hiding this comment

more extensive `api` x `store` testing #2447

more extensive `api` x `store` testing #2447

d-v-b commented Oct 30, 2024 •

edited

Loading

`store` test fixture

`ZipStore` problems

`open` logic

dcherian commented Dec 12, 2024 •

edited

Loading

dcherian Dec 12, 2024 •

edited

Loading