Do not allow exceptions in ManifestStore #683
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #683 +/- ##
===========================================
- Coverage 88.51% 88.24% -0.28%
===========================================
Files 33 33
Lines 1802 1803 +1
===========================================
- Hits 1595 1591 -4
- Misses 207 212 +5
🚀 New features to boost your workflow:
|
|
@TomNicholas the tests required major changes because we can no longer point to fake files (which would previously just return NaNs). I got 3/4 of the way through the DMR++ tests and then decided to open #685 regarding the relative file tests and remove some unit tests that seemed redundant with a better approach of property tests (#394). In summary, I'd appreciate your review with the understanding that we can put more work into improving the DMRPP tests after the release. |
There was a problem hiding this comment.
The actual changes to the error-checking code seem fine to me.
Just so I understand - the new dmrpp fixture actually points to real (temporary) netcdf data, whereas previously it was just a shell of some kind?
FYI @ayushnag @betolink. It would be nice to get to the point where the DMR++ reader can live outside of VirtualiZarr's main repo...
| "0.0": {"path": "file:///foo.0.0.nc", "offset": 0, "length": 8}, | ||
| "1.0": {"path": "file:///foo.0.0.nc", "offset": 0, "length": 8}, | ||
| "2.0": {"path": "file:///foo.0.0.nc", "offset": 0, "length": 8}, |
There was a problem hiding this comment.
Why did the length of these need to change?
There was a problem hiding this comment.
Because the chunks are emulating two 4 byte numbers.
There was a problem hiding this comment.
But it doesn't actually matter for these tests right? You're just making it neater.
That's right. I assume the dmrpp fixtures were created from some real data, but the matching files weren't documented or available to the tests.
👍 https://github.com/virtual-zarr could be a good spot 😄 |
Zarr python allows some exceptions to pass because nodes in the hierarchy that contain only fill values can be represented by missing files. We have a different way of representing missing files after #668, so we should never be expecting to have a failed get request due to a FileNotFoundError. I don't think we should expect the other errors used in regular Zarr store implementations.
TODO:
TestToVirtualXarraytestsdocs/releases.rstapi.rst