Skip to content

[HS3] [RF] Add HS3 conformance test suite integration #22567

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.

Sign up for GitHub

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
stalbrec wants to merge 2 commits into
base: master
Choose a base branch
from
+65 −1
Open

[HS3] [RF] Add HS3 conformance test suite integration #22567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12e7039
[HS3] [RF] Adding test to pull and run external HS3TestSuite
stalbrec Jun 9, 2026
b02d03b
fix linting (import sorting)
stalbrec Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/root-ci-config/buildconfig/global.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ roofit_multiprocess=OFF
root7=ON
rootbench=OFF
roottest=ON
hs3testsuite=ON
runtime_cxxmodules=ON
shadowpw=OFF
shared=ON
Expand Down
3 changes: 2 additions & 1 deletion cmake/modules/RootBuildOptions.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ option(gminimal "Enable only required options by default, but include X11/Cocoa"
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)

@guitargeek guitargeek Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

@guitargeek guitargeek Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The option documentation should state that it requires also an internet connection, similar to what other build options already do.

option(testing "Enable testing with CTest" OFF)
option(asan "Build ROOT with address sanitizer instrumentation" OFF)

Expand Down Expand Up @@ -329,7 +330,7 @@ endif()
ROOT_APPLY_OPTIONS()

#---roottest option implies testing
if(roottest OR rootbench)
if(roottest OR rootbench OR hs3testsuite)

@guitargeek guitargeek Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

set(testing ON CACHE BOOL "" FORCE)
endif()

Expand Down
13 changes: 13 additions & 0 deletions roofit/hs3/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,15 @@
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)

@guitargeek guitargeek Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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

@guitargeek guitargeek Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@guitargeek guitargeek Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this repo be moved to https://github.com/hep-statistics-serialization-standard to make it more "official"?

WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
Comment on lines +8 to +9

@guitargeek guitargeek Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

endif()

if(EXISTS ${hs3testsuite_dir})
ROOT_ADD_PYUNITTEST(hs3-suite test_hs3_suite.py PYTHON_DEPS jsonschema ENVIRONMENT HS3TESTSUITE_ROOT=${hs3testsuite_dir})
endif()
endif()
49 changes: 49 additions & 0 deletions roofit/hs3/test/test_hs3_suite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import os
import sys
import unittest
from pathlib import Path

hs3_root_dir = os.environ.get("HS3TESTSUITE_ROOT")
if hs3_root_dir:
sys.path.insert(0, hs3_root_dir)

try:
from hs3suite.runner import run_suite

except ImportError as e:
TestHS3Suite = type(
"TestHS3Suite",
(unittest.TestCase,),
dict(test_dependencies=lambda self, e=e: self.fail(f"Missing dependency: {e}")),
)
else:
hs3_root_path = Path(hs3_root_dir)
try:
results = run_suite(hs3_root_path, "roofit")
except Exception as e:

def test(self, e=e):
self.fail(f"HS3TestSuite failed to run: {e}")

test.__name__ = "test_hs3testsuite_execution"
TestHS3Suite = type("TestHS3Suite", (unittest.TestCase,), {test.__name__: test})
else:
namespace = dict(__module__=__name__)
for r in results:

def _make(result):
def test(self):
if result.status == "failed":
self.fail(result.message)
return

test.__name__ = f"test_{result.test_id}__{result.check_id}"
test.__doc__ = f"{result.test_id}::{result.check_id}"
return test

func = _make(r)
namespace[func.__name__] = func
TestHS3Suite = type("TestHS3Suite", (unittest.TestCase,), namespace)

if __name__ == "__main__":
unittest.main()
Loading