-
-
Notifications
You must be signed in to change notification settings - Fork 294
VOL-Enabled Tool Testing for h5copy, h5diff, and h5dump #5445
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
Conversation
d3be9a5
to
8f0a0e5
Compare
h5copy_extlinks_trg.h5 | ||
h5copy_ref.h5 | ||
h5copytst.h5 | ||
set(LIST_HDF5_H5COPY_TEST_FILES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indents for continuation lines is twice - 4 spaces instead of two
) | ||
|
||
add_test ( | ||
NAME H5COPY_F-${testname} | ||
COMMAND ${CMAKE_CROSSCOMPILING_EMULATOR} $<TARGET_FILE:h5copy> -f ${fparam} -i ./testfiles/${infile} -o ./testfiles/${testname}.out.h5 ${vparam} ${sparam} ${srcname} ${dparam} ${dstname} ${ARGN} | ||
COMMAND ${CMAKE_COMMAND} -E env ${DISABLE_VOL_ARG} ${CMAKE_CROSSCOMPILING_EMULATOR} $<TARGET_FILE:h5copy> -f ${fparam} -i ./testfiles/${infile} -o ./testfiles/${testname}.out.h5 ${vparam} ${sparam} ${srcname} ${dparam} ${dstname} ${ARGN} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the env ${DISABLE_VOL_ARG} arg - use the set_test_properties ( ENVIRONMENT xxx) command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus you can use a generator construct to set or clear the ENV
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using something like set_tests_properties( ENVIRONMENT $<${local_disable_vol}:HDF5_VOL_CONNECTOR=>)
would wipe the test environment in the case that the VOL is enabled, preventing the active runtime VOL from being used.
Preserving the other test environment values in this case would required reading the current environment for the test and passing it in there, which would be possible but seems more cumbersome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that different then doing the same on the command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, -E env
only modifies the specified variables, while changing the environment with set tests properties completely replaces it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From CMake docs:
Specify environment variables that should be defined for running a test.
Set to a semicolon-separated list list of environment variables and values of the form MYVAR=value. Those environment variables will be defined while running the test. The environment changes from this property do not affect other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NO, as the CMake entry states only sets the ones mentioned.
@@ -1328,24 +1471,24 @@ | |||
# sizeof(long double) != sizeof(double). Use -w80 after the floating-point | |||
# format option since specifying a fixed floating-point precision resets h5dump's | |||
# default number of columns value. | |||
ADD_H5_TEST (tcomplex 0 --enable-error-stack -m %.6f -w80 -d ArrayDatasetFloatComplex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? This seems to ignore the above the comment which details why these arguments were added in the first place.
In general, this PR seems to be trying to fix various things with the tools testing, which I'd argue is separate from enabling tools testing with VOLs and should probably be in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During Linux oneapi testing, this test would fail due to what seemed to be random rounding errors in the complex number output - e.g. 1.00001i instead of 1.000000i. From what I could tell, this was due to something in how h5dump expands floats and not a property of the actual data.
Leaving the h5dump output to the default number of digits fixed this problem. I'll update the comment and split the PR between CI fixes and VOL testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more likely due to the compiler than an issue in h5dump, especially given that the testing has been passing to this point with files generated on Linux. The issue with not using a fixed floating point format specifier for output is that depending on the compiler and values, the values still may not be consistent due to differences in printf
across compilers. In this case it's not really an issue since the values aren't particularly interesting floating point values, but with "real" values you should run into this issue. This is going to be a fundamental problem with getting consistent test files across platforms if they're going to be generated by each platform at runtime of the tests. AFAIK, the testing files have mostly been generated on x86 Linux to this point, though that could be wrong. I consider dropping the floating point format argument as really just masking the problem in this case. I'd almost suggest adding more digits to make sure the full precision of the values is printed, but that's going to be machine-dependent, which also creates another issue for consistent test files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the root cause of inconsistencies is printf
differences across compilers, then I would think that which system generated the testfile wouldn't matter since the current system's printf
will be used regardless of where the testfile came from. I guess that indicates that maybe the different compilers are resulting in some difference in the testfile data after all - I'll look into this some more.
These scripts are added as tests when
HDF5_BUILD_GENERATORS
is on. They're added as tests instead of being executed at build time in order to use the dynamic VOL connector set up at test-time rather than using the VOL enabled at build/configure time. Later tests require the gentest to have been executed before them through use of CMake fixtures.If build generators are disabled, or VOL-enabled tools testing is not possible due to building static tools, then tools testing falls back to directly copying the test files in the repo, and testing using the Native VOL connector.
This is largely a clone of the internal passthrough connector, except that filenames (and external links) have a suffix added to them within the passthrough VOL layer, such that a test which fails to use the connector when intended to will be unable to find the target file. This was done in order to adequately test the ability of the tools' tests to interface with non-standard HDF5 file implementations (e.g. multi-files, object stores, etc.)
Replace direct file copying in tools tests for setup with VOL-enabled file generation
Replace direct file deletion in tools tests with VOL-enabled h5delete usage
Update gentest scripts to correct differences from pre-existing test files/add missing test files
This largely consists of updating filepaths to point to the
testfiles
directory, removing inconsistencies/system-dependent ways of creating datatypes, and disabling generation of files that are are incompatible with testing in this way (e.g. files that have different internal offsets when created through gentest, creating different h5dump output)This is a draft PR until prerequisite fixes to the gentest scripts are merged.
Partial resolution to #5117