Skip to content

Conversation

mattjala
Copy link
Contributor

@mattjala mattjala commented Sep 2, 2025

Depends on #5761 and #5752. Will be marked ready for review once those are merged


Important

Add VOL tests for h5dump, update test generation logic, and modify test configurations.

  • Tests:
    • Add VOL tests for h5dump in CMakeTests.cmake and CMakeTestsXML.cmake.
    • Modify ADD_H5_TEST macro to support VOL-specific tests and filtering.
    • Add gent_tvms() function in h5dumpgentest.c and h5dumpgentest.h for generating test files.
  • Configuration:
    • Update .github/workflows/vol_rest.yml to exclude H5DUMP from certain tests.
    • Add NATIVE_ONLY flag to ADD_H5_TEST macro for native VOL connector tests.
  • Functions:
    • Modify write_dset() in h5dumpgentest.c to accept tid_memory parameter for memory datatype.
    • Update gent_fcontents() and gent_fvalues() to use new write_dset() signature.

This description was created by Ellipsis for 5dfa416. You can customize this summary. It will automatically update as commits are pushed.

@mattjala mattjala added this to the Release 2.0.0 milestone Sep 2, 2025
@mattjala mattjala added Component - Tools Command-line tools like h5dump, includes high-level tools Component - Testing Code in test or testpar directories, GitHub workflows labels Sep 2, 2025
@github-project-automation github-project-automation bot moved this to To be triaged in HDF5 - TRIAGE & TRACK Sep 2, 2025
@mattjala mattjala marked this pull request as ready for review September 17, 2025 19:07
@nbagha1 nbagha1 moved this from To be triaged to Scheduled/On-Deck in HDF5 - TRIAGE & TRACK Sep 22, 2025
@brtnfld brtnfld added the HDFG-internal Internally coded for use by the HDF Group label Sep 24, 2025
brtnfld
brtnfld previously approved these changes Sep 24, 2025

# Mask out the h5dump-assigned anonymous committed datatype numbers
# Datatype number may have a forward slash before #
list (APPEND filters_in "DATATYPE[ \t]*\"(/?#)[0-9]+\"")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm somewhat iffy on this change, only because applying filters to the output for the native connector has the potential to mask real issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it's not a perfect solution, but given that verification is done through h5dump output containing these potentially inconsistent elements, I'm not aware of a cleaner solution. The way the masking is done leaves the files on-disk the same, so those can at least be checked directly to see e.g. real sizes and offsets. Only a few specifically marked tests apply these filters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My issue with this one is more about catching the issues at test time. Sure someone can always manually inspect the files, but they would need to know there is something wrong in the first place before deciding to do that.

The idea is that at least for the native VOL, the address/size/etc. of something changing in the file would be an interesting point to note. The test failure that would result could signal to the developer that something is wrong in the tools, or at least signal that files should be re-generated. This probably isn't too likely in practice, but it would be nice if we could avoid applying these filters for the native VOL.

jhendersonHDF
jhendersonHDF previously approved these changes Sep 24, 2025
Copy link
Collaborator

@jhendersonHDF jhendersonHDF left a comment

Choose a reason for hiding this comment

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

Approving as the masking of some values for the native VOL is a fairly minor issue, though it would be nice if it could be resolved. There's also a small conflict in the REST VOL workflow that needs resolved.

@nbagha1 nbagha1 moved this from Scheduled/On-Deck to In progress in HDF5 - TRIAGE & TRACK Sep 25, 2025
@nbagha1 nbagha1 moved this from In progress to Scheduled/On-Deck in HDF5 - TRIAGE & TRACK Sep 25, 2025
jhendersonHDF
jhendersonHDF previously approved these changes Sep 26, 2025
@mattjala mattjala requested a review from brtnfld September 26, 2025 16:29
Copy link
Collaborator

@brtnfld brtnfld left a comment

Choose a reason for hiding this comment

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

Critical Issues: None Identified

No major bugs, security vulnerabilities, or breaking changes were found. The code appears to be defensive and follows HDF5's error handling patterns.

Code Quality Issues: Medium Priority

  1. Inconsistent Function Signature: The write_dset function signature change from 6 to 7 parameters (adding separate dataset and memory datatypes) lacks documentation explaining the
    rationale:
    // Old: write_dset(loc_id, rank, dims, dset_name, tid, buf)
    // New: write_dset(loc_id, rank, dims, dset_name, tid_dset, tid_mem, buf)
  2. Missing Error Handling: In CMakeTests.cmake:1014, the complex VOL connector test generation logic lacks proper error handling for file operations and path manipulations.
  3. Code Duplication: The ADD_H5_TEST macro modifications introduce repeated logic for handling different VOL connectors that could be abstracted into helper functions.
  4. Missing Function Documentation: The new gent_tvms() function lacks documentation explaining its purpose (generating VAX F64 datatype test files).

Enhancement Opportunities: Low Priority

  1. Performance Optimization: The CMake test generation could be optimized by caching file system operations and reducing redundant path calculations.
  2. Maintainability Improvements:
    - Extract the path separator logic into a dedicated CMake function
    - Add inline comments explaining the VOL connector test matrix approach
    - Consider using CMake generator expressions for cleaner conditional logic
  3. Test Coverage: The PR focuses on h5dump VOL testing but doesn't include corresponding tests for error conditions or edge cases specific to different VOL connectors.
  4. Configuration Management: The test configuration could benefit from a centralized VOL connector configuration file rather than inline conditional logic.

Specific Technical Recommendations:

  1. Line CMakeTests.cmake:1014: Consider extracting the complex path manipulation into a helper function:
    function(normalize_path_separators input_path output_var)
    # Implementation here
    endfunction()
  2. File h5dumpgentest.c: Add documentation for gent_tvms():
    /*
  • Function: gent_tvms
  • Purpose: Generate test file with VAX F64 datatype for VOL connector testing
  • Return: Success: 0, Failure: -1
    */
  1. File CMakeTests.cmake: Add error checking for file operations:
    if(NOT EXISTS "${source_file}")
    message(FATAL_ERROR "Required test file ${source_file} not found")
    endif()

For example, Recommendaton 1

brtnfld
brtnfld previously approved these changes Sep 26, 2025
#define FILE95 "tcomplex.h5"
#define FILE96 "tcomplex_be.h5"
#endif
#define FILE97 "tbfloat16.h5"
Copy link
Collaborator

Choose a reason for hiding this comment

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

While it can be annoying to resolve git conflicts around, we should either keep the original naming/numbering scheme here, or convert the existing macros to new names, along with all the associated function-specific value macros. Otherwise, it's easier for the file name and other macros to go out of sync.

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 HDFG-internal Internally coded for use by the HDF Group
Projects
Status: Scheduled/On-Deck
Development

Successfully merging this pull request may close these issues.

3 participants