Skip to content

chore: remove tests for private tenuous zarr-python store methods#2175

Open
d-v-b wants to merge 7 commits into
earth-mover:mainfrom
d-v-b:remove-store-private-method-tests
Open

chore: remove tests for private tenuous zarr-python store methods#2175
d-v-b wants to merge 7 commits into
earth-mover:mainfrom
d-v-b:remove-store-private-method-tests

Conversation

@d-v-b

@d-v-b d-v-b commented Jun 3, 2026

Copy link
Copy Markdown

I couldn't run tests locally because compilation failed during linking with cc, but I bet CI will be sufficient for this contribution.

@dcherian
dcherian enabled auto-merge June 3, 2026 21:24
@ianhi

ianhi commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Hi Davis thanks for this. Can you explain the motivation and what you mean by "tenuous" here? These overrides appear to have been doing something useful as without them we get these failures:

FAILED tests/test_zarr/test_store/test_icechunk_store.py::TestIcechunkStore::test_get_bytes[1] - icechunk.IcechunkError: x bad metadata

@d-v-b

d-v-b commented Jun 23, 2026

Copy link
Copy Markdown
Author

Can you explain the motivation and what you mean by "tenuous" here?

The methods being tested in the code I excise here were removed from zarr-python recently.

@d-v-b

d-v-b commented Jun 23, 2026

Copy link
Copy Markdown
Author

haha I never scrolled all the way up to see that these are overrides for the store tests we ship in zarr-python! this PR should be amended to gate on the zarr-python version instead of just removing these overrides!

@dcherian

Copy link
Copy Markdown
Collaborator

IMO these tests all suck, both in IC and in Zarr. They should be property tests and should be in the public testing. api so that external stores can run them.

@d-v-b

d-v-b commented Jun 24, 2026

Copy link
Copy Markdown
Author

IMO these tests all suck, both in IC and in Zarr. They should be property tests and should be in the public testing. api so that external stores can run them.

100%, these tests are no good. And we should support icechunk's needs by supporting test configuration (e.g., choosing the stored values to narrowly contain zarr metadata docs)

@dcherian

Copy link
Copy Markdown
Collaborator

hah forgot that complaint, they should be generating keys that are expected from a reading of the spec!

@d-v-b

d-v-b commented Jun 24, 2026

Copy link
Copy Markdown
Author

hah forgot that complaint, they should be generating keys that are expected from a reading of the spec!

that's complicated! we need to support zarr v2, which includes consolidated metadata (.zmetadata keys) which is not in any spec, and zarr v3's consolidated metadata (spec drafted, but stalled over in zarr-specs) reserves the 'kind field for future use, anticipating storing consolidated metadata in some external key with an unknown name. And then there are chunk key encodings...

@dcherian

Copy link
Copy Markdown
Collaborator

sure... but there's still a small subset of patterns not foo.json with crap in it.

li-em added a commit that referenced this pull request Jul 24, 2026
li-em added a commit that referenced this pull request Jul 24, 2026
li-em added a commit that referenced this pull request Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants