refactor core tests

[d-v-b]

This PR refactors the array API tests in test_core.py by adding two methods to TestArray , create_store and create_chunk_store. These methods are called by TestArray.create_array to generate a test-class-specific store and chunk_store instances.

Nearly all the variability across test classes can be expressed as different implementations of create_store, so test classes no longer need their own implementation of create_array, which cuts down the size of each test class.

Different test classes use a variety of different arguments to create_array; these variables are now class attributes which are referenced in create_array.

I see this PR as an opportunity for additional simplification of the test suite, so feel free to add suggestions.

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)

[codecov]

Codecov Report

Merging #1305 (6682089) into main (b9e9f5a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main     #1305    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           35        35            
  Lines        14404     14111   -293     
==========================================
- Hits         14404     14111   -293     

Impacted Files	Coverage Δ
zarr/tests/test_core.py	100.00% <ø> (ø)
zarr/tests/test_storage.py	100.00% <ø> (ø)
zarr/__init__.py	100.00% <100.00%> (ø)
zarr/_storage/absstore.py	100.00% <100.00%> (ø)
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/_storage/v3.py	100.00% <100.00%> (ø)
zarr/attrs.py	100.00% <100.00%> (ø)
zarr/convenience.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/creation.py	100.00% <100.00%> (ø)
... and 23 more

[d-v-b]

another change worth noting: in this PR, TestArray.test_array_init now creates a store via self.create_store instead of via the class attribute self.KVStoreClass, which in main is always KVStore (or KVStoreV3 for the V3 tests). See #1303

[rabernat]

Thanks so much @d-v-b for taking some time to pay down some technical debt and improve our test suite! 🙏

I can't quite tell how much this is WIP, but I'd be happy to review this when you're ready for feedback. Please just request me as a reviewer.

[d-v-b]

@rabernat i'm coming back from a vacation with energy to push this forward. In therms of WIP-ness, all I need to do is check code coverage and then it's ready, assuming the maintainers agree with the strategy behind this compaction of the test suite.

[joshmoore]

At a high-level, the strategy sounds good to me. Minor nitpick: for large(-ish) refactors, adding e.g. the switch from ' to " is a distraction and hurts your diff stats: "975 additions / 999 deletions" 😄 I'll read through more carefully and see if anything jumps out.

Edit: No worries beyond codecov. 👍

[d-v-b]

I'm formatting with black, which was pretty helpful given the length of the test suite and the previously semi-standard formatting. I think this behavior of black is not really configurable, so maybe we just have to accept a noisy diff.

[rabernat]

I'm formatting with black, which was pretty helpful given the length of the test suite and the previously semi-standard formatting. I think this behavior of black is not really configurable, so maybe we just have to accept a noisy diff.

I agree with Josh here. Changes in our linting should be introduced as standalone PRs, without mixing in any other changes. Otherwise it's extremely hard to review.

I think we should be running Black (or even better, Ruff) on Zarr. But this PR is about refactoring our test suite. Let's not make it any harder than it has to be.

Edit with a hopefully more productive suggestion: one path forward that would not require @d-v-b to somehow un-black his changes would be to create a separate PR that runs black on the whole code base and introduces this into the pre-commit workflow, merge, and then rebase this PR on top of that. Then reviewers would be able to focus on the actual test refactor changes (rather than formatting changes), AND we would get the benefits of a black-ed codebase.

[joshmoore]

For @jstriebel's sanity I would very much like to get the sharding PRs in before we reformat everything.

[d-v-b]

apologies for prematurely formatting the code -- it did make development easier, but I didn't anticipate the cost of the noisy diff

[d-v-b]

see #1321 for a blackened, ruff-approved main

[d-v-b]

this diff 56d23b6 is against a formatted test suite, so it's not an atrocious diff.

Let me know if there's more I can do to make this easier for review, and sorry for the noisy commits.

There's some drops in coverage that I would like to resolve before merging this.

[joshmoore]

this diff 56d23b6 is against a formatted test suite, so it's not an atrocious diff.

Thanks for doing this! ❤️

Only issues that pop out:

• Showing 1 changed file with 597 additions and 812 deletions. is largely due to the multiline reformatting (in the first half).
• second half is nice removal of methods 👍
• removal of the hexdigests is a bit of a surprise (but these are annoying)
• can you explain the change in the test inheritance hierarchy?

There's some drops in coverage that I would like to resolve before merging this.

💯 since the state propagates to other PRs based off the same mainline.

[d-v-b]

removal of the hexdigests is a bit of a surprise (but these are annoying)

The ones I removed are from test_array_init, and I removed them because as far as I can tell they are worthless -- in main, test_array_init doesn't use self.create_array to make the arrays that get tested, so every test class is actually running the test with the same store instance. My reasoning was that test_hexdigest is probably a better place for hexdigest tests, and we know that test_hexdigest actually uses subclass-specific store / array creation routines, unlike test_array_init. If test_array_init does anything useful with those hexdigest tests, it will only be for one test class (the base v2 test) and I should see the impact of its removal in the coverage report and I can add a corresponding test to test_hexdigest.

can you explain the change in the test inheritance hierarchy?

All of the test classes vary in two places: the values passed to init_array and Array. Wherever possible, I have made these values class properties (e.g., path and dimension_separator) or methods (create_store, create_chunk_store).

This results in smaller class definitions -- in this PR, many classes only override create_store and expected. There are some notable exceptions, e.g. TestArrayWithFilters, which requires overriding create_array because the filters need access to the dtype that varies in different test methods.

Thus the hierarchy is actually unchanged or simplified -- TestArray is still the base class where all the bulk of the test methods are defined, and in principle TestArrayWithXStore can just inherit from TestArray and override the necessary methods (e.g., TestArray.create_store() and TestArray.expected()). In cases like TestArrayWithXStoreAndYOption, TestArrayWithXStoreAndZOption, e.g. v3 tests, these test classes inherit from TestArrayWithXStore and class properties are overridden as needed to define subclasses. Thus classes that required multiple inheritance to define in main, can be defined with single inheritance in this PR.

[d-v-b]

Some recent changes:

• partial chunk read tests are defined in the base TestArray and gated by the partial_decompress property of a subclass, which removes some duplicated code.
• I undid some of my premature formatting of function calls, so some LOC got shaved off by that.
• Some tests in test_array_init could not pass for CustomMapping as a store, so I changed those tests to accord with their spirit, thus allowing TestArrayWithCustomMapping to pass.
• I added a line to the configuration of pytest in pyproject.toml that instructs pytest to track which test routines are responsible for hitting which lines, which can greatly speed up the process of interpreting coverage reports. I'm not sure if this will work for the html coverage generated online, but it works locally.

[joshmoore]

Argh. Apologies, @d-v-b, I assume the sharding PR led to conflicts as feared.

[d-v-b]

closing this in favor of a new effort: #1462