[HS3] [RF] Add HS3 conformance test suite integration#22567
Conversation
Test Results 22 files 22 suites 3d 12h 19m 21s ⏱️ For more details on these failures, see this check. Results for commit b02d03b. ♻️ This comment has been updated with latest results. |
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Thank you for this awesome initiative! I really like the idea, but have some change requests about the implementation.
| execute_process(COMMAND ${GIT_EXECUTABLE} clone https://github.com/Phmonski/HS3TestSuite | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}) |
There was a problem hiding this comment.
The CMake configuration should not put new files in the ${CMAKE_CURRENT_SOURCE_DIR}. The source directory has to stay clean on build artifacts.
You need to make sure that test inputs are somewhere in the build directory.
| set(hs3testsuite_dir ${CMAKE_CURRENT_SOURCE_DIR}/HS3TestSuite) | ||
| if(NOT EXISTS ${hs3testsuite_dir}) | ||
| find_package(Git QUIET REQUIRED) | ||
| execute_process(COMMAND ${GIT_EXECUTABLE} clone https://github.com/Phmonski/HS3TestSuite |
There was a problem hiding this comment.
We should not clone the master branch of a repository during configuration, because this makes the build and test non-deterministic. There is a big risk that if that repo is accidentally updated with problematic code, the ROOT CI will break.
Please pin this to a specific commit hash.
| option(minimal "Enable only required options by default" OFF) | ||
| option(rootbench "Build rootbench if rootbench exists in root or if it is a sibling directory (implies testing=ON)" OFF) | ||
| option(roottest "Build roottest (implies testing=ON)" OFF) | ||
| option(hs3testsuite "Setup and use the HS3 conformance test suite (implies testing=ON)" OFF) |
There was a problem hiding this comment.
| option(hs3testsuite "Setup and use the HS3 conformance test suite (implies testing=ON)" OFF) | |
| option(test_roofit_hs3testsuite "Setup and use the HS3 conformance test suite (implies testing=ON)" OFF) |
Following the precedent of the test_distrdf_dask and test_distrdf_pyspark examples. Otherwise, it's a bit unclear which part of ROOT this flag affects for someone not familiar with HS3.
|
|
||
| #---roottest option implies testing | ||
| if(roottest OR rootbench) | ||
| if(roottest OR rootbench OR hs3testsuite) |
There was a problem hiding this comment.
Build options that imply or implicitly require other options are making the configuration more difficult to understand. It would be clearer just to error out the configuration if hs3testsuite=ON without testing=ON, with a clear message that this is not a meaningful configuration because the HS3 test suite required the testing infrastructure.
| ROOT_ADD_GTEST(testRooFitHS3 testRooFitHS3.cxx LIBRARIES RooFitCore RooFit RooFitHS3) | ||
| ROOT_ADD_GTEST(testHS3SimultaneousFit testHS3SimultaneousFit.cxx LIBRARIES RooFitCore RooFit RooFitHS3 RooStats) | ||
|
|
||
| if(pyroot AND hs3testsuite) |
There was a problem hiding this comment.
You don't document anywhere that pyroot=OFF secretly disables the HS3 tests. That would be confusing to a user who wants to build with the HS3 tests but without PyROOT.
Again, I think it's better here to error out the configuration early if pyroot=OFF and hs3testsuite=ON with a clear message.
| option(minimal "Enable only required options by default" OFF) | ||
| option(rootbench "Build rootbench if rootbench exists in root or if it is a sibling directory (implies testing=ON)" OFF) | ||
| option(roottest "Build roottest (implies testing=ON)" OFF) | ||
| option(hs3testsuite "Setup and use the HS3 conformance test suite (implies testing=ON)" OFF) |
There was a problem hiding this comment.
The option documentation should state that it requires also an internet connection, similar to what other build options already do.
| set(hs3testsuite_dir ${CMAKE_CURRENT_SOURCE_DIR}/HS3TestSuite) | ||
| if(NOT EXISTS ${hs3testsuite_dir}) | ||
| find_package(Git QUIET REQUIRED) | ||
| execute_process(COMMAND ${GIT_EXECUTABLE} clone https://github.com/Phmonski/HS3TestSuite |
There was a problem hiding this comment.
Can this repo be moved to https://github.com/hep-statistics-serialization-standard to make it more "official"?
2 participants
This Pull request:
Changes or fixes:
Adds a new hs3testsuite CMake option that clones the HS3TestSuite (https://github.com/Phmonski/HS3TestSuite) repository and runs the ROOT HS3 conformance checks, ensuring RooFit's JSON serialization stays compatible with the evolving schema.
Checklist: