Test cleanup: fixtures, coverage, and stub removal#132
Open
jhughes-mw wants to merge 7 commits into
Open
Conversation
Replace hand-rolled directory and permission juggling with the matlab.unittest fixtures built for the purpose: - tZarrRead, tZarrInfo: inherit SharedZarrTestSetup (PathFixture + WorkingFolderFixture) instead of re-declaring addSrcCodePath. This also stops tZarrRead/tooBigArray from leaking a bigData/ folder into the repo, since writes now land in the temp working folder. Fixture paths move from dataFiles/grp_v2/... to grp_v2/... to match the copied-contents layout. - Relative-path tests (tZarrRead, tZarrInfo): use CurrentFolderFixture instead of a hardcoded ../test/... path that only resolved from the original test folder. - tZarrCreate/createArrayRelativePath: use WorkingFolderFixture instead of manual mkdir + pwd capture + addTeardown(cd). - tZarrAttributes/noWritePermissions: isolate the read-only array in a TemporaryFolderFixture. Drops the '+w' teardown, which forced a specific permission state rather than restoring the original and mutated shared fixture data. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
createArrayRemoteDefaultSyntax and createArrayRemoteUserDefinedSyntax were empty method bodies (marked "Move to a separate file") added when the suite was first created and never implemented. An empty MATLAB test method passes unconditionally, so they silently reported green while covering nothing. Implementing real remote (S3) write tests requires a CI-friendly mock S3 endpoint (no live AWS dependency); that work is tracked in separate GitHub issues. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cover public methods the suite never exercised, raising overall line coverage from ~94.7% to ~98.1%: - Zarr.isZarrGroup and Zarr.isZarrArray - ZarrDatatype.fromTensorstoreType (round-trip and invalid-type error) - Zarr.createGroup: folder creation, and the read-only .zgroup open failure (via a TemporaryFolderFixture, so no real data is modified) - Zarr.write scalar-shape branch, via the scalarData fixture whose stored shape is a true scalar [1] (zarrcreate expands scalars to [1 N], so this branch is otherwise unreachable from the public API) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tools/run_coverage.m runs the full test suite with code coverage over the source folder (excluding test/, matching codecov.yml) and produces an HTML report plus a Cobertura XML under test/. Mirrors what CI collects, for reproducing coverage locally. Generated artifacts are gitignored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
+ Coverage 95.81% 99.23% +3.42%
==========================================
Files 8 8
Lines 263 263
==========================================
+ Hits 252 261 +9
+ Misses 11 2 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The read-only-folder approach does not fail on Windows: the directory read-only attribute is ignored by the OS for file creation, so the .zgroup / .zattrs write succeeded and no error was raised. (The original noWritePermissions test passed on Windows only incidentally, because it marked a pre-existing .zattrs *file* read-only via recursion.) Make the specific target file read-only instead of its folder. A read-only file is honored on both Windows and Unix, so the open-for- write failure fires on all platforms. Write permission is restored on teardown so the fixture can clean up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jm9176
requested changes
Jul 9, 2026
| function verifyCreateGroupMakesFolder(testcase) | ||
| % Verify that createGroup creates the directory when it does not | ||
| % already exist, and writes a valid .zgroup file into it. | ||
| groupPath = fullfile(pwd, "brandNewGroup"); |
Member
There was a problem hiding this comment.
Can you create it TemporaryFolder?
| % Verify that createGroup creates the directory when it does not | ||
| % already exist, and writes a valid .zgroup file into it. | ||
| groupPath = fullfile(pwd, "brandNewGroup"); | ||
| testcase.verifyFalse(isfolder(groupPath),... |
Member
There was a problem hiding this comment.
This can be removed. Claude likes to add too much of this verification stuff, which is redundant.
|
|
||
| Zarr.createGroup(groupPath); | ||
|
|
||
| testcase.verifyTrue(isfolder(groupPath),... |
Member
There was a problem hiding this comment.
is this needed if you are already checking for group and you already have verification for .zgroup file? looks redundant to me, but upto you.
| % | ||
| % Run from anywhere; paths are resolved relative to this file. | ||
|
|
||
| % Copyright 2026, The MathWorks, inc. |
Member
There was a problem hiding this comment.
Isn't it "Copyright 2026 The Mathworks, Inc."?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test-suite cleanup and coverage improvements. No source (
Zarr.m/ZarrPy.py/ user-facing function) behavior changes — test-only, plus a dev tool.Changes
tZarrReadandtZarrInfonow inheritSharedZarrTestSetup(PathFixture + WorkingFolderFixture) instead of re-declaringaddSrcCodePath. Relative-path tests useCurrentFolderFixture;tZarrCreate/createArrayRelativePathusesWorkingFolderFixture;tZarrAttributes/noWritePermissionsisolates its read-only array in aTemporaryFolderFixture(dropping a'+w'teardown that forced a specific permission state rather than restoring the original).tZarrRead/tooBigArraypreviously wrote abigData/folder into the working tree because the class had noWorkingFolderFixture. Writes now land in the temp working folder.createArrayRemoteDefaultSyntax/createArrayRemoteUserDefinedSyntaxwere empty method bodies that passed unconditionally (silently green while covering nothing). Real remote (S3) write tests need a CI-friendly mock S3 endpoint — tracked in separate issues (please link).Zarr.isZarrGroup/isZarrArray,ZarrDatatype.fromTensorstoreType,Zarr.createGroup(folder creation + read-only open failure), and theZarr.writescalar-shape branch.tools/run_coverage.m, a local coverage runner mirroring CI (generated artifacts gitignored).Testing
Full suite: 117 passed, 2 skipped (Windows-only), 0 failed.