The bad news is, this issue is going to be slightly more than I've said here. The good news is that the property-based tests caught some edge cases.

Nice, I've been meaning to add this to Zarr:

@st.compositedefv3_array_metadata(draw:st.DrawFn)->bytes:fromzarr.codecs.bytesimportBytesCodecfromzarr.core.chunk_gridsimportRegularChunkGridfromzarr.core.chunk_key_encodingsimportDefaultChunkKeyEncodingfromzarr.core.metadata.v3importArrayV3Metadata# separator = draw(st.sampled_from(['/', '\\']))shape=draw(array_shapes)ndim=len(shape)chunk_shape=draw(npst.array_shapes(min_dims=ndim,max_dims=ndim))dtype=draw(zrst.v3_dtypes())fill_value=draw(npst.from_dtype(dtype))dimension_names=draw(st.none()|st.lists(st.none()|simple_text,min_size=ndim,max_size=ndim)    )metadata=ArrayV3Metadata(shape=shape,data_type=dtype,chunk_grid=RegularChunkGrid(chunk_shape=chunk_shape),fill_value=fill_value,attributes=draw(simple_attrs),dimension_names=dimension_names,chunk_key_encoding=DefaultChunkKeyEncoding(separator="/"),# FIXMEcodecs=[BytesCodec()],storage_transformers=(),    )returnmetadata.to_buffer_dict(prototype=default_buffer_prototype())["zarr.json"]

What do you think of aarray_metadata_json(zarr_formats...) strategy that just returns the JSON and we can test whether that satisfies the spec for V2 and V3?

I love the idea. I was thinking the other day that the obvious path out of the bugs that are currently popping up would be property-based testing, so I was pretty pleased to see that there's already some work in that direction

Out of curiosity, do we have something like json schema that we could apply against the outputs to at least verify structure? We'd still need to define all the rules that exist in terms of value/type dependencies etc. but that's an easy win if it exists somewhere

Out of curiosity, do we have something like json schema that we could apply against the outputs to at least verify structure?

Don't know. ping@jhamman@d-v-b

I'm not aware of a JSON schema definition for the array metadata. If one existed, it would necessarily only support partial validation, because JSON schema can't express certain invariants in the metadata document, like the requirement that dimensional attributes (shape, chunk_shape, etc) be consistent.

A fairly easy alternative way to handle this would be to simply write a test that takes thearrays strategy, extracts the metadata, converts to JSON, and then asserts that the JSON meets spec (as best as we can).

I still think a generic metadata strategy is probably useful.

Yeah, you'd definitely need json-schemaand custom validation rules that encode relationships among different fields

Did some refactoring and organization of property-based testing code and added round trip testing forArrayV2Metadata (which hopefully captures some important expectations about these things. Still need to fully satisfy the linter and make sure similar guarantees are tested forArrayV3Metadata

Hye@moradology I added anarray_metadata strategy in

Definitely - do you mind my pulling the generators up into their own submodule as done here, given their similar and unique functionality?

I think testing/strategies.py is right place. Everything in there already handles the v2 vs v3 complexity

@dcherian Maybe I got out ahead of myself. Do you think the small amount of extra organization I added is undesirable? Basically, I brokestrategies.py out intostrategies/array_metadata_generators.py,strategies/array_generators.py,strategies/dtype_generators.py while usingstrategies/__init__.py to keep imports the same.

My thought was just that continuing to add property-based tests is probably a good idea and the singlestrategies.py file will likely get a bit long

It feels a bit premature to me. In any case, I find it nice to keep such refactoring PRs separate so that we can review (much) smaller diffs. In my experience, keeping the diffs small is the best way to keep open-source contributions easy to merge.

How about just keeping them in strategies for now and we can refactor later as needed.

Sounds like a plan. As a relatively new contributor to this library, I appreciate the firm opinions!

Thankyou for considering the opinions in a constructive manner!

OK, so to get the property-based tests working I had to make some decisions about what serialization strategies looked like. I tried to follow the instructions availablehere, but other issues that have come to the surface in the history ofzarr-python suggest there are likely blindspots in this document.This part, in particular, deserves special attention

A bit confused about the code coverage going down. The tested constraints are definitely a bit tighter than they were, if anything

@d-v-b or@dcherian - thoughts on this set of changes? No pressure, I just don't want this to get too stale if possible. The PR is now non-trivial, but I think it does a decent job of organizing and defining serialization/deserialization behaviors that were previously hard to see at a glance or else not well defined. It also verifies round tripping with property-based tests, which is how certain undefined behaviors were uncovered (e.g. 'NaT' fordatetime instances)

this looks good to me, I'm going to merge soon unless there are objections. good to keep things moving.

Glad I got the pushback to go through things again because the property-based tests came across an error related to floating point precision at really big numbers that (in extremely rare cases of integers nearsys.maxsize) was causing problems. Also trimmed some redundancy

Well, this timeout error is pretty annoying! Can't reliably reproduce it locally but don't really like fiddling with the timeout settings either. Oh well. I think it should be fine to ignore timeout settings fortest_basic_indexing

@dstansby Bumping this again just to make sure I don't lose track of it. If you all need some help doing PR reviews etc (obviously there's a lot going on) I'd be happy to lend a hand on smaller/less controversial PRs!