-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider splitting/refactoring base_test.py
#100
Comments
I do agree that there is room for improvement. Notably, I always find a bit misleading that the real tests are in And I always need to take 5mn to understand how the fixtures are fed in the base tests. Perhaps levering pytest's parametrize could allow us to make things clearer.
Not sure about this one. I like that I can check what HDF5 file is tested with one glance on the test. Writing tests with big HDF5 files requires external knowledge about these files. |
Haha true.
It's just a lot of repetition: we initialise a dataset path and numpy value, create a file, create the dataset, access it, decode it, and assert that the returned value matches the initial value exactly. If we had one big dictionary of initial values, we could just loop through them to make sure h5grove supports them all and reads them all correctly. This could even be a single test, to be honest - i.e. a smoke test. |
This smoke test would work particularly well for the Generally speaking, I would prefer to have one test per |
Very low priority, but I find that
base_test.py
is becoming quite long and is, generally speaking, difficult to read. I feel like we could make things clearer at least by:/data/
could probably use multiple files)Moreover, there are a lot of things to test:
/data/
,/meta/
, etc.)dtype=safe
,format=bin
, etc.)That's a lot of features and a lot of permutations if we really want to be thorough. It might be worth creating a few big HDF5 files like I did in H5Web (
sample.h5
) to make sure that we test/data/
and/meta/
with as many permutations of dtype, shape,format
, etc., as we can.The text was updated successfully, but these errors were encountered: