Refactor h5test.c, testframe.c and testpar.h testing frameworks #4891

jhendersonHDF · 2024-09-27T17:48:39Z

Added new testframe.h header to document testing framework functions and split them away from h5test.h and from test programs that don't integrate with the testframe.c testing framework

Added new test setup callback to testframe.c testing framework

Added parameters to AddTest() to specify size of test parameters so they can be copied for later use

Enabled HDF5 error stacks in testframe.c framework by default and added some error stack suppressions to some testhdf5 tests

Added new maxthreads option to testframe.c framework to allow specifying the maximum number of threads a multi-threaded test can use

Moved TestExpress functionality out of testframe.c and into more general h5test.c for wider use by tests through getter and setter

Updated some tests to not mix and match functionality between h5test.c/h and testframe.c/h

Moved some functionality from testphdf5.h into testpar.h for parallel tests that aren't part of testphdf5

Added new parallel test library that contains common shared functionality for parallel tests (similar to h5test library)

jhendersonHDF · 2024-09-27T17:50:31Z

c++/test/dsets.cpp

@@ -1492,9 +1492,6 @@ test_dset()
    catch (Exception &E) {
        test_report(nerrors, H5std_string(" Dataset"));
    }
-
-    // Clean up data file
-    cleanup_dsets();


No need to explicitly call this here since the testing framework will do this now

jhendersonHDF · 2024-09-27T17:51:29Z

c++/test/dsets.cpp

 {
-    HDremove(FILE1.c_str());
-    HDremove(FILE_ACCPLIST.c_str());
+    if (GetTestCleanup()) {


This is a similar pattern replicated everywhere. The testing framework always calls the cleanup callback for a test now as the callback can do more than just cleanup temp. testing files. GetTestCleanup() just surrounds the part where the testing files are cleaned up.

jhendersonHDF · 2024-09-27T17:52:44Z

hl/test/test_packet_vlen.c

@@ -861,9 +861,11 @@ verify_attribute(hid_t fid, const char *table_name, const char *attr_name)

    /* Verify values read in */
    for (ii = 0; ii < ATTR_DIM; ii++)
-        if (attr_data[ii] != read_data[ii])
-            TestErrPrintf("%d: attribute data different: attr_data[%d]=%d, read_data[%d]=%d\n", __LINE__, ii,


TestErrPrintf was converted to fprintf(stderr, ...) for a few test files that weren't really integrated with the testframe.c framework.

jhendersonHDF · 2024-09-27T17:53:49Z

test/dsets.c

@@ -990,14 +990,16 @@ test_compact_io(hid_t fapl)

            /* Verify the dataset's layout and fill message versions */
            if (fp->shared->low_bound == H5F_LIBVER_EARLIEST) {
-                VERIFY(dsetp->shared->layout.version, H5O_LAYOUT_VERSION_DEFAULT, "layout_ver_bounds");


Replaced usages of the VERIFY macro with equivalent logic for some test files that weren't really integrated with the testframe.c framework

jhendersonHDF · 2024-09-27T17:54:31Z

test/dsets.c

@@ -16165,8 +16168,7 @@ main(void)
        goto error;
    printf("All dataset tests passed.\n");
 #ifdef H5_HAVE_FILTER_SZIP
-    if (GetTestCleanup())


This test never changes the test cleanup status and wasn't really integrated with the testframe.c framework, so the call to GetTestCleanup() wasn't useful.

jhendersonHDF · 2024-09-27T17:55:02Z

test/earray.c

@@ -2302,8 +2302,6 @@ main(void)
    /* Reset library */
    h5_test_init();
    fapl = h5_fileaccess();
-    if (TestExpress > 0)


This test doesn't do anything with the value of TestExpress

jhendersonHDF · 2024-09-27T17:55:33Z

test/enc_dec_plist.c

@@ -191,9 +191,6 @@ main(void)
                     "Testing ENCODE/DECODE with file version bounds: (%s, %s):", low_string, high_string);
            puts(msg);

-            if (VERBOSE_MED)


This test never used the testframe.c framework and never changed the test verbosity, so checking for VERBOSE_MED had no effect.

jhendersonHDF · 2024-09-27T17:55:47Z

test/enc_dec_plist_cross_platform.c

@@ -26,9 +26,6 @@ static int test_plists(const char *filename_prefix);
 int
 main(void)
 {
-    if (VERBOSE_MED)


Same comment here as above

jhendersonHDF · 2024-09-27T17:56:01Z

test/farray.c

@@ -1633,8 +1633,6 @@ main(void)
    /* Reset library */
    h5_test_init();
    fapl = h5_fileaccess();
-    if (TestExpress > 0)


This test doesn't do anything with the value of TestExpress

jhendersonHDF · 2024-09-27T17:57:18Z

test/h5test.c

-size_t   n_tests_failed_g  = 0;
-size_t   n_tests_skipped_g = 0;
-uint64_t vol_cap_flags_g   = H5VL_CAP_FLAG_NONE;
+static int TestExpress_g     = -1; /* Whether to expedite testing. -1 means not set yet. */


TestExpress functionality was moved back out of the testframe.c framework for more widespread usage in the future, but the global variable is once again hidden behind get/set functions so that tests don't have direct access to it.

jhendersonHDF · 2024-09-27T17:59:44Z

test/h5test.c

+uint64_t   vol_cap_flags_g   = H5VL_CAP_FLAG_NONE;
+
+/* Whether h5_cleanup should clean up temporary testing files */
+static bool do_test_file_cleanup_g = true;


While this duplicates the GetTestCleanup() functionality from testframe.c into h5test.c, it prevents conflating the two functionalities. h5test.c's usage was also less structured (the application had to call h5_fixname to influence the status), so it seemed best to leave them separate for now until a potential unified testing framework can come together in the future.

jhendersonHDF · 2024-09-27T18:00:02Z

test/h5test.c

- */
-H5_ATTR_FORMAT(printf, 1, 2)
-int
-print_func(const char *format, ...)


No longer needed at this point

jhendersonHDF · 2024-09-27T18:00:30Z

test/h5test.h

@@ -24,77 +24,6 @@
 #include "H5private.h"
 #include "H5Eprivate.h"

-/*


Nearly all of the below code was moved into testframe.h

jhendersonHDF · 2024-09-27T18:01:15Z

test/stab.c

@@ -1466,10 +1466,9 @@ main(void)
    puts("All symbol table tests passed.");

    /* Cleanup */
-    if (GetTestCleanup()) {


This test wasn't integrated with testframe.c and never changed the cleanup status; GetTestCleanup() calls weren't useful here

jhendersonHDF · 2024-09-27T18:12:44Z

test/testhdf5.h

        TestErrPrintf("*** UNEXPECTED RESULT from %s at line %4d in %s\n", where, (int)__LINE__, __FILE__);  \
    } while (0)

-/* definitions for command strings */
-#define VERBOSITY_STR "Verbosity"


These macros aren't used anywhere

jhendersonHDF · 2024-09-27T18:15:34Z

testpar/CMakeLists.txt

Added a new parallel testing library in testpar/testpar.c that contains shared functionality among parallel tests. Only 2 functions are in it currently, but it separates parallel tests with a main() function from testframe.c-style tests like testphdf5.

jhendersonHDF · 2024-09-27T18:18:55Z

testpar/t_2Gio.c

This file had a lot of functions copied into it from testphdf5 without having made any changes to those functions and without using them in this file, so they were removed. This file also had included testphdf5.h just for some of the macros and support functions, which were mostly moved into the parallel testing library.

jhendersonHDF · 2024-09-27T18:20:29Z

testpar/t_2Gio.c

@@ -1348,6 +1266,22 @@ dataset_writeAll(void)
    /* Dataset5: point selection in File - Hyperslab selection in Memory*/
    /* create a file dataspace independently */
    point_set(start, count, stride, block, num_points, coords, OUT_OF_ORDER);
+    if (VERBOSE_MED) {


This previously existed in point_set(), but that function was moved into the parallel testing library where VERBOSE_MED isn't available (since some tests don't integrate with the testframe.c framework). So this output section was moved into individual tests that do have access to VERBOSE_MED.

All of these replicated blocks would really benefit from a refactor into a subroutine.

Will look into pulling that back out into a separate function that isn't tied to VERBOSE_MED

jhendersonHDF · 2024-09-27T18:23:14Z

testpar/t_prestart.c

@@ -115,8 +122,6 @@ main(int argc, char **argv)
    if (data_array)
        free(data_array);

-    nerrors += GetTestNumErrs();


This test never used testframe.c functionality, so GetTestNumErrs() isn't useful here

jhendersonHDF · 2024-09-27T18:25:11Z

testpar/t_shapesame.c

@@ -4319,8 +4282,6 @@ main(int argc, char **argv)
        return -1;
    }

-    mpi_rank_framework_g = mpi_rank;


Instead of relying on parallel tests to set this global variable for the testframe.c framework somewhere in main, it gets passed to TestInit() now instead

mattjala · 2024-09-27T20:31:55Z

test/testframe.h

+
+/**
+ * --------------------------------------------------------------------------
+ * \ingroup H5TEST


Would it make more sense to have the doxygen group for these routines be called something like H5TESTFRAME?

No particular opinion on the group name, especially since we don't currently have any testing documentation and this documentation won't get parsed by doxygen yet anyway until we decide to start doing that. H5TEST was just a generic name to keep all testing documentation together, but having a separate group for this framework would work just as well.

test/testhdf5.c

test/tskiplist.c

test/testframe.c

mattjala · 2024-09-30T16:55:35Z

test/testhdf5.h

 #include "h5test.h"

+/* Include testing framework functionality */
+#include "testframe.h"


Testframe currently includes h5test, so including them both here is redundant. I'm guessing the goal is to eventually separate the two?

Separate or ideally unify. In the meantime, both headers are included here without assuming what each header includes. testframe.h mostly just includes h5test.h as a convenience since library developers have generally assumed they can just include h5test.h and get everything needed. This is moving a bit more toward a test either including testframe.h or h5test.h, depending on the functionality needed, until tests are maybe made consistent at some point in the future. The general rule with these changes is the testhdf5-style tests can access functionality from both testframe.h and h5test.h, but not the other way around.

qkoziol · 2024-10-01T16:45:09Z

Looking at this now...

jhendersonHDF · 2024-10-01T17:25:39Z

Rebased off of develop

Added new testframe.h header to document testing framework functions and split them away from h5test.h and from test programs that don't integrate with the testframe.c testing framework Added new test setup callback to testframe.c testing framework Added parameters to AddTest() to specify size of test parameters so they can be copied for later use Enabled HDF5 error stacks in testframe.c framework by default and added some error stack suppressions to some testhdf5 tests Added new maxthreads option to testframe.c framework to allow specifying the maximum number of threads a multi-threaded test can use Moved TestExpress functionality out of testframe.c and into more general h5test.c for wider use by tests through getter and setter Updated some tests to not mix and match functionality between h5test.c/h and testframe.c/h Moved some functionality from testphdf5.h into testpar.h for parallel tests that aren't part of testphdf5 Added new parallel test library that contains common shared functionality for parallel tests (similar to h5test library)

qkoziol · 2024-10-01T18:13:46Z

testpar/t_2Gio.c

-
-    /* Reduce the number of metadata cache slots, so that there are cache
-     * collisions during the raw data I/O on the chunked dataset.  This stresses
-     * the metadata cache and tests for cache bugs. -QAK


Why was this removed?

My comment above may not have showed up inline, but every single test function that was removed from this file wasn't being tested in the first place. They were exact or near-exact duplicates of the same functions from testphdf5's t_dset.c and they weren't actually being run by the t_2Gio.c testing; they simply existed in this file for no reason. Since t_2Gio.c previously included testphdf5.h, these functions matched the prototypes declared there for t_dset.c's tests and caused confusion for some code editors.

qkoziol · 2024-10-01T18:15:34Z

testpar/t_2Gio.c

-    mem_dataspace = H5Screate_simple(MAX_RANK, block, NULL);
-    VRFY((mem_dataspace >= 0), "");
-
-    /* Extend its current dim sizes before writing */


Also, why was this test removed?

qkoziol · 2024-10-01T18:15:58Z

testpar/t_2Gio.c

- */
-
-void
-extend_writeInd2(void)


Same here - why was this test removed?

qkoziol · 2024-10-01T18:17:09Z

testpar/t_2Gio.c

-    H5Eset_auto2(H5E_DEFAULT, old_func, old_client_data);
-    H5Sclose(file_dataspace);
-
-    /* Read dataset1 using BYROW pattern */


Same here - why removed?

qkoziol · 2024-10-01T18:17:35Z

testpar/t_2Gio.c

- */
-
-void
-extend_writeAll(void)


Same here - removed?

qkoziol · 2024-10-01T18:18:03Z

testpar/t_2Gio.c

-     * collisions during the raw data I/O on the chunked dataset.  This stresses
-     * the metadata cache and tests for cache bugs. -QAK
-     */
-    {


Same here - removed?

qkoziol · 2024-10-01T18:18:17Z

testpar/t_2Gio.c

-     * Define the dimensions of the overall datasets and create them.
-     * ------------------------------------------------------------- */
-
-    /* set up dataset storage chunk sizes and creation property list */


Same here - removed?

qkoziol · 2024-10-01T18:18:59Z

testpar/t_2Gio.c

-        dataset_print(start, block, data_array1);
-    }
-
-    /* create a memory dataspace independently */


So many tests removed, I'll stop commenting :-)

qkoziol

Generally speaking, this looks like a solid improvement to the testing infrastructure, particularly the TLC invested in improving the parallel tests. Just concerned about some of the tests that were removed.

…roup#4891) Added new testframe.h header to document testing framework functions and split them away from h5test.h and from test programs that don't integrate with the testframe.c testing framework Added new test setup callback to testframe.c testing framework Added parameters to AddTest() to specify size of test parameters so they can be copied for later use Enabled HDF5 error stacks in testframe.c framework by default and added some error stack suppressions to some testhdf5 tests Added new maxthreads option to testframe.c framework to allow specifying the maximum number of threads a multi-threaded test can use Moved TestExpress functionality out of testframe.c and into more general h5test.c for wider use by tests through getter and setter Updated some tests to not mix and match functionality between h5test.c/h and testframe.c/h Moved some functionality from testphdf5.h into testpar.h for parallel tests that aren't part of testphdf5 Added new parallel test library that contains common shared functionality for parallel tests (similar to h5test library)

Added new testframe.h header to document testing framework functions and split them away from h5test.h and from test programs that don't integrate with the testframe.c testing framework Added new test setup callback to testframe.c testing framework Added parameters to AddTest() to specify size of test parameters so they can be copied for later use Enabled HDF5 error stacks in testframe.c framework by default and added some error stack suppressions to some testhdf5 tests Added new maxthreads option to testframe.c framework to allow specifying the maximum number of threads a multi-threaded test can use Moved TestExpress functionality out of testframe.c and into more general h5test.c for wider use by tests through getter and setter Updated some tests to not mix and match functionality between h5test.c/h and testframe.c/h Moved some functionality from testphdf5.h into testpar.h for parallel tests that aren't part of testphdf5 Added new parallel test library that contains common shared functionality for parallel tests (similar to h5test library)

jhendersonHDF requested review from lrknox, derobins, byrnHDF, fortnern, qkoziol, vchoi-hdfgroup, bmribler, glennsong09, mattjala and brtnfld as code owners September 27, 2024 17:48

jhendersonHDF commented Sep 27, 2024

View reviewed changes

mattjala reviewed Sep 27, 2024

View reviewed changes

test/testhdf5.c Show resolved Hide resolved

mattjala reviewed Sep 30, 2024

View reviewed changes

test/tskiplist.c Show resolved Hide resolved

mattjala reviewed Sep 30, 2024

View reviewed changes

test/testframe.c Show resolved Hide resolved

mattjala reviewed Sep 30, 2024

View reviewed changes

mattjala approved these changes Oct 1, 2024

View reviewed changes

jhendersonHDF force-pushed the feature/test_framework_refactor branch from 2f16023 to ba4fd07 Compare October 1, 2024 17:25

qkoziol reviewed Oct 1, 2024

View reviewed changes

testpar/t_2Gio.c

*/

void

extend_writeInd2(void)

Copy link

Contributor

qkoziol Oct 1, 2024

Choose a reason for hiding this comment

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

Same here - why was this test removed?

qkoziol reviewed Oct 1, 2024

View reviewed changes

testpar/t_2Gio.c

*/

void

extend_writeAll(void)

Copy link

Contributor

qkoziol Oct 1, 2024

Choose a reason for hiding this comment

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

Same here - removed?

qkoziol reviewed Oct 1, 2024

View reviewed changes

lrknox approved these changes Oct 1, 2024

View reviewed changes

lrknox merged commit 34d6ef5 into HDFGroup:develop Oct 1, 2024
56 checks passed

jhendersonHDF mentioned this pull request Oct 4, 2024

Minor refactoring of testframe.c testing framework #4930

Merged

Refactor h5test.c, testframe.c and testpar.h testing frameworks #4891

Refactor h5test.c, testframe.c and testpar.h testing frameworks #4891

Conversation

jhendersonHDF commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhendersonHDF Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qkoziol commented Oct 1, 2024

jhendersonHDF commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qkoziol left a comment

Choose a reason for hiding this comment

jhendersonHDF Sep 27, 2024 •

edited

Loading