Refactor core tests, round 2#1462
Conversation
github-actions
Bot
added
the
needs release notes
Codecov Report
@@ Coverage Diff @@
## main #1462 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 37 37
Lines 15026 14723 -303
==========================================
- Hits 15026 14723 -303
|
github-actions
Bot
removed
the
needs release notes
|
@rabernat this is ready for eyes |
rabernat
left a comment
rabernat
left a comment
There was a problem hiding this comment.
This is fantastic work @d-v-b! 🙌
Overall, what I love is that you have eliminated a ton of boilerplate code. I have a few comments mostly in the vein of "could we go even farther"?
Another open question I have is whether to bring in pytest conveniences like parametrization and temporary directories.
Happy to leave both thoseitems for future work if you prefer. Or we could keep pushing forward here.
| root = '' | ||
| KVStoreClass = KVStore | ||
| path = '' | ||
| compressor = Zlib(level=1) |
There was a problem hiding this comment.
Curious about why we are setting a default compressor here, and / or why it is different from the built-in default.
There was a problem hiding this comment.
I did that to make the test pass :) My observation is that historically, different test classes used different compressors (I have no idea why), and because of the hexdigest tests, these compressors get immured with the rest of the tests in that test class. I think this alone argues for rethinking the hexdigest tests.
| # keyword arguments for array initialization | ||
| init_array_kwargs = { | ||
| "path": kwargs.pop("path", self.path), | ||
| "compressor": self.compressor, |
There was a problem hiding this comment.
Why not
| "compressor": self.compressor, | |
| "compressor": kwargs.pop("compressor", self.compressor), |
?
Just trying to understand how this class is intended to be used by developers...
|
|
||
| # initialize at path | ||
| store = self.KVStoreClass(dict()) | ||
| store = self.create_store() |
There was a problem hiding this comment.
Kind of feels like we should be in a separate test here. Is anything reused below from above?
There was a problem hiding this comment.
no, nothing is reused, so it's a great opportunity for just making a separate test
| path = mktemp(suffix=".anydbm") | ||
| atexit.register(atexit_rmglob, path + "*") |
There was a problem hiding this comment.
It would really be best to replace all of this stuff with pytest tempdir fixtures.
There was a problem hiding this comment.
yeah, I had the same feeling. I wanted to keep the initial PR close to the original tests, but I'm experimenting with a fixture-style locally.
| init_array(store, **kwargs) | ||
| return Array(store, read_only=read_only, cache_metadata=cache_metadata, | ||
| cache_attrs=cache_attrs, write_empty_chunks=write_empty_chunks) | ||
| compressor = Blosc(cname="zstd", clevel=1, shuffle=1) |
There was a problem hiding this comment.
Feels like we should be using parametrized fixtures here rather than making a class for each compressor.
There was a problem hiding this comment.
Absolutely. Locally, I'm experimenting with ripping out al the OOP stuff and just having parameterized functions. We'll see how that compares.
| dtype = kwargs.get('dtype') | ||
| compressor = Zlib(1) | ||
|
|
||
| def create_array(self, shape: Union[int, Tuple[int, ...]], **kwargs): |
There was a problem hiding this comment.
Rather than re-implementing this whole method in the subclass, could you add filters to the base TestArray class attributes?
… kwarg in create_array.
|
I'm making progress even more refactoring of the tests, but it's getting a bit more involved -- I'm entering territory where I'm merging / splitting / parametrizing individual tests, and I'm less sure that coverage will be preserved. This messier refactoring could take a while, so I think I recommend we settle for the current state of this PR for now. I can follow up with deeper refactors later on. How does this sound @rabernat ? |
2 participants
It's the same thing as #1305, but without ruff + black linting applied to the whole codebase, and thus a much cleaner diff.
Some type hints were improved outside the core tests to ensure that mypy was happy.
TODO: