Enabling attribute tests for reading data from groups #104
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
=======================================
Coverage 96.66% 96.66%
=======================================
Files 8 8
Lines 210 210
=======================================
Hits 203 203
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
test/tZarrAttributes.m
Outdated
| actAttr.attr1 = grpInfo.group_description; | ||
| actAttr.attr2 = grpInfo.group_level; |
There was a problem hiding this comment.
Instead of creating an artificial "actual" struct value from two fields of the actual output of zarrinfo, I think I would have preferred a separate verifyEqual for each field. In addition to being conceptually nicer (IMHO), I think it would also make it easier to see which attribute has an issue if the test fails.
| @@ -53,11 +52,17 @@ function verifyAttrOverwrite(testcase) | |||
| end | |||
|
|
|||
| function verifyGroupAttributeInfo(testcase) | |||
| % Write attribute info using zarrwriteatt function to a group. | |||
| testcase.assumeTrue(false,'Filtered until Issue-35 is fixed.'); | |||
| % Verify group attribute info. | |||
There was a problem hiding this comment.
Do we already have a test for zarrwriteatt for groups?
There was a problem hiding this comment.
I have updated the test file creation by adding zarrwriteatt calls to a group. Accordingly, I have updated the testpoint to read and verify attributes using this change.
test/tZarrAttributes.m
Outdated
| actAttr.attr1 = grpInfo.group_description; | ||
| actAttr.attr2 = grpInfo.group_level; | ||
|
|
||
| expAttr.attr1 = 'This is a sample Zarr group'; |
There was a problem hiding this comment.
hang on, in the file itself the attribute names are "group_description" and "group_level", but we are using attr1 and attr2 for the field names? A bit confusing..
Would avoid this issue if checking each field separately which would be my preference (see other comment) :)
There was a problem hiding this comment.
Discussed offline. Fixed.
Enabled tests for #35