Add h5dump tests that require subfiling VFD #3879
Conversation
hyoklee
requested review from
lrknox,
derobins,
byrnHDF,
fortnern,
jhendersonHDF,
qkoziol,
vchoi-hdfgroup,
bmribler,
glennsong09,
mattjala and
brtnfld
as code owners
| if (vfdtest STREQUAL "subfiling") | ||
| foreach (h5_tfile ${HDF5_SF_VFD_H5DUMP_FILES}) | ||
| file(COPY "${PROJECT_SOURCE_DIR}/testfiles/${h5_tfile}" DESTINATION "${PROJECT_BINARY_DIR}/${vfdtest}") | ||
| execute_process( |
There was a problem hiding this comment.
Why is this doing a process that could be done by a CMake command? We should not be using platform specific commands by choice. We should not assume that something is only ever going to be on one platform.
There was a problem hiding this comment.
Why use the inode in the first place - the files are unique without the number. I don't see the advantage of the complexity.
There was a problem hiding this comment.
The inode is used by the Subfiling VFD to generate unique filenames, so the CMake code would need to be able to generate the names of the other resulting files when creating files with the Subfiling VFD during testing.
There was a problem hiding this comment.
But isn't that in the original name? Seems like there needs to be more documentation on this test, because it isn't obvious.
There was a problem hiding this comment.
The problem is we can't add generated files here easily because the inode value for the file could change, meaning that when the Subfiling VFD tries to open test_subfiling_stripe_sizes.h5, it will also look for
test_subfiling_stripe_sizes.h5.subfile_XXXXXXXX_1_of_1 and test_subfiling_stripe_sizes.h5.subfile_XXXXXXXX.config, but we wouldn't have a way of predicting what the inode value for XXXXXXXX would be and so we have to grab the inode value. On the other hand, we probably don't necessarily need a generated file at all to test Subfiling with h5dump if we just create the file on the fly.
glennsong09
added
Priority - 2. Medium ⏹
|
We need to have a test with more than one subfile. One subfile is just a regular HDF5. |
There was a problem hiding this comment.
Where were these test files taken from? I'd expect the configuration files to have aggregator_count, hdf5_file, subfile_dir fields and a list of the names of all the subfiles.
There was a problem hiding this comment.
I removed them manually because they did not affect the h5dump test result. Test files are based on subfiling test under testpar.
There was a problem hiding this comment.
In general, it's probably best to keep the original config files intact so that future versions of the VFD don't have issues with the test files if it requires certain fields in the future for verification purposes. That said, I think this is fine for now.
The test HDF5 file,
test_subfiling_stripe_sizes.h5, in this PR is a subfiling VFD "stub" file.It depends on the corresponding subfile
test_subfiling_stripe_sizes.h5.subfile_1_of_1and config filetest_subfiling_stripe_sizes.h5.subfile.config.h5dump can't dump the HDF5 file unless h5dump uses subfiling VFD.
These tests pass because VFD CTest defines
HDF5_DRIVERinconfig/cmake/vfdTest.cmake:If you comment it out, these tests will fail.
I will create another PR that will make these tests pass even if HDF5_DRIVER is not set.