Skip to content

Generate VOL tests for h5copy #5551

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

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from

Conversation

mattjala
Copy link
Contributor

  • If a VOL connector is provided at build time, CMake will create counterparts of the h5copy tests for that connector, with names of the form HDF5_VOL_<vol_name>-<test_name>

  • Add h5copy tests to the VOL workflows

The h5copy tests aren't added to the adios2 and the LOG VOL CI, because the workflows don't use fetchcontent to set up those connectors.

The h5copy tests are mostly disabled for the REST VOL because it doesn't factor in current working directory to the HSDS paths it accesses, so the h5copy tests fail due to being unable to find the files.

@mattjala mattjala added the Priority - 2. Medium It would be nice to have this in the next release label May 28, 2025
@mattjala mattjala requested a review from lrknox as a code owner May 28, 2025 15:14
@mattjala mattjala added the Component - Tools Command-line tools like h5dump, includes high-level tools label May 28, 2025
@mattjala mattjala added the Component - Testing Code in test or testpar directories, GitHub workflows label May 28, 2025
@github-project-automation github-project-automation bot moved this to To be triaged in HDF5 - TRIAGE & TRACK May 28, 2025
@hyoklee hyoklee moved this from To be triaged to In progress in HDF5 - TRIAGE & TRACK May 30, 2025
@hyoklee hyoklee added this to the Release 2.0.0 milestone May 30, 2025
bmribler
bmribler previously approved these changes Jun 8, 2025
@mattjala mattjala requested a review from Copilot June 11, 2025 16:26
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for generating VOL-specific h5copy tests in CMake and adjusts the REST VOL CI to skip h5copy tests due to HSDS path limitations.

  • Introduces HDF5_GET_VOL_TGT_INFO macro in HDF5Macros.cmake to collect connector name, plugin path, and derive a clean VOL name.
  • Updates .github/workflows/vol_rest.yml to exclude H5COPY tests on the REST VOL CI.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
config/HDF5Macros.cmake New macro to extract VOL connector properties and environment.
.github/workflows/vol_rest.yml Excludes h5copy tests (H5COPY) from the REST VOL CI workflow.
Comments suppressed due to low confidence (1)

@fortnern
Copy link
Member

fortnern commented Jun 17, 2025

Ideally we should find a way to get rid of the repeated code in the h5copy cmake file, but this looks good otherwise (as far as I can tell, I am not a cmake expert) and should be fine for now

fortnern
fortnern previously approved these changes Jun 17, 2025
)

set_tests_properties (HDF5_VOL_${vol}-H5COPY_F-${testname}-clear-objects PROPERTIES
ENVIRONMENT "${env}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, we should probably avoid modifying any of these properties when using the native VOL. Currently, this seems like it would pass NONE for the environment, whereas we shouldn't be passing anything. It would also be good to avoid the generator expressions below that are trying to fix things up when using the native VOL.

ADD_H5_TEST (compact 0 ${HDF_FILE1}.h5 -v -s compact -d compact)
ADD_H5_TEST (compound 0 ${HDF_FILE1}.h5 -v -s compound -d compound)
# Add a test for the native connector and each external VOL connector
foreach (vol_idx RANGE 0 ${num_ext_vols})
Copy link
Collaborator

@jhendersonHDF jhendersonHDF Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is handy for preventing duplication of the ADD_ macros below, but it complicates the logic a bit when the native VOL is being used and has other issues due to VOL testing really being separate from regular testing. For example, tests that use VOL plugins should currently only be run when shared tools are built (or, the flipside of that, avoided when static tools are built), whereas the regular tools tests using the native VOL can always be run. I think it would go a long way if we were able to collapse the vast majority of the different ADD_ macros and ideally have one for regular testing and one for VOL testing, though it's not clear if that would be more complicated than what we currently have.

In the worst case, one alternative might be to move the foreach (VOL ...) logic into each of the macros so that we don't duplicate the calls here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the primary motivation is to separate VOL testing from regular testing, does moving the per-VOL loop to inside the ADD_ macros make that any simpler? It seems like any conditions we want to enforce as requisite for VOL testing (e.g. shared libraries) are simpler to set up when all the ADD_ macros fall within a single loop.

The other case - where VOL tests are managed by an invocation of a counterpart ADD_VOL_ call for each ADD_ - seems cumbersome and likely to lead to newly added tests being accidentally left out of some test suites. The majority of tool tests can be run against most VOL connectors, so the default case being to run the test for native and non-native connectors makes sense.

I agree that it would be good to condense the number of distinct ADD_ macros, especially because they are duplicated across the different tools

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say the primary motivation is trying to separate VOL testing from regular testing while also trying to prevent the need for duplicating calls to the ADD_ macros, since, as you mention, it opens the possibility for someone to forget to add new calls for both the native VOL and external VOLs. Moving the foreach logic into each macro allows us to keep only one set of calls to the macros and allows us to easily separate the VOL-specific test setup logic from the logic for the native VOL; mixing logic between "regular" testing and external VOL testing is always going to be messy. However, it will likely make each of those macros substantially larger, which is why I think the ideal case is condensing the majority of the existing macros.

In many cases, the macro variants aren't substantially different from each other, so I think this is work that needs to happen regardless. The current situation of having many variants of tools test macros doesn't lend itself well to adding in VOL testing without either duplication or hacky workarounds for the native VOL.

The majority of tool tests can be run against most VOL connectors, so the default case being to run the test for native and non-native connectors makes sense.

As I recall, several tools tests had to be skipped or left out for the majority of VOL connectors the last time this was tested, so I'm not sure this is necessarily the case. And to me this is all the more reason to keep VOL testing as separate from regular testing as possible.

@mattjala mattjala force-pushed the h5copy_vol_testing_fetchcontent branch from ebd8ae4 to 1044a84 Compare June 20, 2025 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Testing Code in test or testpar directories, GitHub workflows Component - Tools Command-line tools like h5dump, includes high-level tools Priority - 2. Medium It would be nice to have this in the next release
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

5 participants