Unit test for template shape model with bare C++ Combine classes #1131
Conversation
WalkthroughAdds test infrastructure for validating statistical model fit workflows, including a new CombineHarvester data card template, a parameterized GoogleTest suite (testCreateNLL.cxx) comparing fit backends, updated CMake test configuration, LCG environment adjustment, and CI workflow modifications for conditional dependency installation. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as TEST_P<br/>BackendConsistency
participant Fixture as CreateNLLTest<br/>SetUp/TearDown
participant Config as Load Config<br/>(Workspace, PDF, Data)
participant BaselineFit as Run Baseline<br/>Fit (Backend)
participant CandidateFit as Run Candidate<br/>Fit (CPU/Codegen)
participant Compare as Compare<br/>Results
participant Assert as Assert<br/>Pass/Fail
Test->>Fixture: runComparison()<br/>(analyticBarlowBeeston toggle)
Fixture->>Config: Open ROOT file &<br/>fetch workspace, ModelConfig, PDF, data
Config->>BaselineFit: Execute fit<br/>(backend=combine)
BaselineFit->>BaselineFit: Build NLL,<br/>CascadeMinimizer,<br/>compute Hessian
BaselineFit-->>Compare: FitSummary<br/>(POI, params, errors)
Config->>CandidateFit: Execute fit<br/>(backend=CPU or Codegen)
CandidateFit->>CandidateFit: Build NLL,<br/>CascadeMinimizer,<br/>compute Hessian
CandidateFit-->>Compare: FitSummary<br/>(POI, params, errors)
Compare->>Compare: compareFits()<br/>check prop_*<br/>parameter errors<br/>within tolerance
Compare->>Assert: Pass if match<br/>or expectFailure set
Assert-->>Test: Result logged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
env_lcg.sh (1)
1-1: Add shebang/strict mode, robust quoting, and path checks; allow LCG release override.Prevents subtle env breakage and satisfies ShellCheck SC2148. Also clarifies the dev3/dev4 comment.
Apply:
+#!/usr/bin/env bash +set -euo pipefail + -LCG_RELEASE=dev3 # or dev3 for ROOT master (dev4 is the latest ROOT stable branch) +# LCG release: override via environment (dev3 often tracks ROOT master; dev4 is ROOT stable). +LCG_RELEASE="${LCG_RELEASE:-dev3}" LCG_PATH=/cvmfs/sft.cern.ch/lcg/views/$LCG_RELEASE/latest/x86_64-el9-gcc13-opt -source $LCG_PATH/setup.sh -source $LCG_PATH/bin/thisroot.sh +if [[ -f "${LCG_PATH}/setup.sh" ]]; then + # shellcheck source=/dev/null + source "${LCG_PATH}/setup.sh" +else + echo "ERROR: ${LCG_PATH}/setup.sh not found." >&2 + exit 1 +fi +if [[ -f "${LCG_PATH}/bin/thisroot.sh" ]]; then + # shellcheck source=/dev/null + source "${LCG_PATH}/bin/thisroot.sh" +fi -export PATH=$PWD/install/bin:$PATH -export LD_LIBRARY_PATH=$PWD/install/lib:$LD_LIBRARY_PATH +export PATH="${PWD}/install/bin${PATH:+:${PATH}}" +export LD_LIBRARY_PATH="${PWD}/install/lib${LD_LIBRARY_PATH:+:${LD_LIBRARY_PATH}}"Based on static analysis hints.
Also applies to: 2-2, 4-5, 7-8
data/ci/Makefile (1)
5-8: Add ROOT rpath and an explicit ‘all’ target to reduce env coupling.Ensures the binary runs without relying on an externally sourced ROOT env and placates make linters.
ROOTCFLAGS := $(shell root-config --cflags) ROOTLIBS := $(shell root-config --libs) ROOFITLIBS := -lRooFit -lRooFitCore -lRooStats -lHistFactory +ROOTLIBDIR := $(shell root-config --libdir) @@ LIBS := -L$(COMBINE_LIBDIR) -lHiggsAnalysisCombinedLimit -RPATH := -Wl,-rpath,$(abspath $(COMBINE_LIBDIR)) +RPATH := -Wl,-rpath,$(abspath $(COMBINE_LIBDIR)) -Wl,-rpath,$(ROOTLIBDIR) @@ -$(TARGET): $(SOURCES) +.PHONY: all +all: $(TARGET) + +$(TARGET): $(SOURCES) $(CXX) $(CXXFLAGS) $(ROOTCFLAGS) $(INCLUDES) $< -o $@ $(ROOTLIBS) $(ROOFITLIBS) $(LIBS) $(RPATH) @@ -.PHONY: clean +.PHONY: clean clean: rm -f $(TARGET)Also applies to: 13-16, 20-21, 23-25
test/CMakeLists.txt (1)
41-47: Source the LCG/ROOT env for the build step to avoid relying on CI-global state.
makeusesroot-config; ensure it’s on PATH. Prefer sourcingenv_lcg.shin a bash login shell.ROOT_ADD_TEST(template_analysis-exampleCreateNLL - COMMAND ${CMAKE_COMMAND} -E env + COMMAND ${CMAKE_COMMAND} -E env LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}:$ENV{LD_LIBRARY_PATH} COMBINE_SRCDIR=${CMAKE_SOURCE_DIR} COMBINE_LIBDIR=${CMAKE_BINARY_DIR} - /bin/sh -c "make && ./exampleCreateNLL template-analysis_shape_autoMCStats.root" + bash -lc "source ${CMAKE_SOURCE_DIR}/env_lcg.sh && make && ./exampleCreateNLL template-analysis_shape_autoMCStats.root"Optional: assert runtime success text.
ROOT_ADD_TEST(template_analysis-exampleCreateNLL @@ FIXTURES_REQUIRED template_analysis_workspace + PASS_REGULAR_EXPRESSION "Finished building and minimising the NLL from workspace" )Please confirm CI already sources an equivalent env if you prefer not to touch the test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
data/ci/Makefile(1 hunks)data/ci/exampleCreateNLL.cxx(1 hunks)data/ci/template-analysis_shape_autoMCStats.txt(1 hunks)env_lcg.sh(1 hunks)test/CMakeLists.txt(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
data/ci/Makefile
[warning] 23-23: Missing required phony target "all"
(minphony)
[warning] 23-23: Missing required phony target "test"
(minphony)
🪛 Shellcheck (0.11.0)
env_lcg.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: CMSSW_14_2_0_pre2_ROOT632 - ROOT 6.32.06
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.32.2)
🔇 Additional comments (1)
data/ci/template-analysis_shape_autoMCStats.txt (1)
16-16: Confirm intent: -1 rates for ggH/bbH are taken from shapes.Combine will read yields from
htt_tt_9_13TeV/{ggH120,bbH120}when rate is -1. Just double-check this matches the workspace content inhtt_input.root.
maxgalli
force-pushed
the
unit_test_dev_ad
branch
from
92df19f to
2996b50
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
env_lcg.sh (1)
1-8: Implement shell hardening with path availability check; test in actual CI environment.The path verification failed in the sandbox environment (expected—CVMFS is not available), but the core recommendations are sound shell script best practices. The shebang, safety flags, variable quoting, and path guard should be implemented regardless. Verify that the LCG path is accessible in your actual CI environment where this script will run.
+#!/usr/bin/env bash +set -euo pipefail -LCG_RELEASE=dev3 # or dev3 for ROOT master (dev4 is the latest ROOT stable branch) +LCG_RELEASE=${LCG_RELEASE:-dev3} # dev3 = ROOT master, dev4 = latest ROOT stable -LCG_PATH=/cvmfs/sft.cern.ch/lcg/views/$LCG_RELEASE/latest/x86_64-el9-gcc13-opt +LCG_PATH="/cvmfs/sft.cern.ch/lcg/views/${LCG_RELEASE}/latest/x86_64-el9-gcc13-opt" + +if [[ ! -d "${LCG_PATH}" ]]; then + echo "ERROR: LCG path not found: ${LCG_PATH}" >&2 + exit 1 +fi - -source $LCG_PATH/setup.sh -source $LCG_PATH/bin/thisroot.sh +source "${LCG_PATH}/setup.sh" +source "${LCG_PATH}/bin/thisroot.sh" -export PATH=$PWD/install/bin:$PATH -export LD_LIBRARY_PATH=$PWD/install/lib:$LD_LIBRARY_PATH +export PATH="${PWD}/install/bin:${PATH}" +export LD_LIBRARY_PATH="${PWD}/install/lib:${LD_LIBRARY_PATH:-}"
♻️ Duplicate comments (1)
test/exampleCreateNLL.cxx (1)
78-80: Fix ownership: don’t unique_ptr-manage a workspace-owned object.ws.genobj("discreteParams") returns a workspace-owned pointer; wrapping in unique_ptr risks double-delete.
- std::unique_ptr<RooArgSet> discreteParameters{ - dynamic_cast<RooArgSet *>(ws.genobj("discreteParams"))}; + RooArgSet* discreteParameters = dynamic_cast<RooArgSet*>(ws.genobj("discreteParams"));
🧹 Nitpick comments (1)
data/ci/Makefile (1)
23-25: Add standard phony and default target.Improves developer UX and satisfies checkmake warnings.
-.PHONY: clean +.PHONY: all clean +all: $(TARGET) clean: rm -f $(TARGET)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
data/ci/Makefile(1 hunks)data/ci/template-analysis_shape_autoMCStats.txt(1 hunks)env_lcg.sh(1 hunks)test/CMakeLists.txt(1 hunks)test/exampleCreateNLL.cxx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- data/ci/template-analysis_shape_autoMCStats.txt
🧰 Additional context used
🪛 checkmake (0.2.2)
data/ci/Makefile
[warning] 23-23: Missing required phony target "all"
(minphony)
[warning] 23-23: Missing required phony target "test"
(minphony)
🪛 Shellcheck (0.11.0)
env_lcg.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.10, root6.26.4)
🔇 Additional comments (1)
test/CMakeLists.txt (1)
46-48: ****The review claims that the test name in
set_propertyuses a hyphen where it should use an underscore (gtest-template-analysis-exampleCreateNLLvs.gtest-template_analysis-exampleCreateNLL). However, based on codebase inspection:
ROOT_ADD_GTEST(template_analysis-exampleCreateNLL ...)at line 42 uses an underscore in the input- The
set_propertyreference at line 46 uses a hyphenThe
ROOT_ADD_GTESTmacro (from ROOT framework) is not defined in this repository. Without access to its implementation or the ability to execute tests to verify the actual generated test name, I cannot conclusively determine whether:
- The macro transforms underscores to hyphens (making the current code correct), or
- The macro preserves underscores (making the current code incorrect)
Please manually verify the actual test name by building and running
ctest -Nor by checking ROOT framework documentation forROOT_ADD_GTESTbehavior regarding underscore/hyphen transformation. Also confirm whether the proposed additions forWORKING_DIRECTORYandENVIRONMENTare appropriate for this test.
| for (RooAbsArg *arg : *poi) { | ||
| auto *var = dynamic_cast<RooRealVar *>(arg); | ||
| if (!var) | ||
| continue; | ||
| cfg.parametersOfInterest.add(*var); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use utils::iter(...) for RooArgSet/List iteration (portable across ROOT versions).
Direct range‑for over RooArgSet/RooArgList may not compile across LCG/ROOT variants; Combine provides utils::iter for this.
- if (const RooArgSet *poi = mc.GetParametersOfInterest()) {
- for (RooAbsArg *arg : *poi) {
+ if (const RooArgSet *poi = mc.GetParametersOfInterest()) {
+ for (RooAbsArg *arg : utils::iter(*poi)) {
...
}
}
@@
- if (const RooArgSet *nuis = mc.GetNuisanceParameters()) {
- for (RooAbsArg *arg : *nuis) {
+ if (const RooArgSet *nuis = mc.GetNuisanceParameters()) {
+ for (RooAbsArg *arg : utils::iter(*nuis)) {
...
}
}
@@
- for (RooAbsArg *arg : allVars) {
+ for (RooAbsArg *arg : utils::iter(allVars)) {
...
}
@@
- if (discreteParameters) {
- for (RooAbsArg *arg : *discreteParameters) {
+ if (discreteParameters) {
+ for (RooAbsArg *arg : utils::iter(*discreteParameters)) {
...
}
} else if (runtimedef::get("ADD_DISCRETE_FALLBACK")) {
- RooArgSet categories(ws.allCats());
- for (RooAbsArg *arg : categories) {
+ RooArgSet categories(ws.allCats());
+ for (RooAbsArg *arg : utils::iter(categories)) {
...
}
}
@@
- for (RooAbsArg *arg : clients) {
+ for (RooAbsArg *arg : utils::iter(clients)) {
...
- std::unique_ptr<RooArgSet> pdfParams{multi->getParameters((const RooArgSet *)nullptr)};
- for (RooAbsArg *a : *pdfParams) {
+ std::unique_ptr<RooArgSet> pdfParams{multi->getParameters((const RooArgSet*)nullptr)};
+ for (RooAbsArg *a : utils::iter(*pdfParams)) {
...
}
}
@@
- for (RooAbsArg *arg : *poi) {
+ for (RooAbsArg *arg : utils::iter(*poi)) {
...
}
@@
- for (RooAbsArg *arg : *poi) {
+ for (RooAbsArg *arg : utils::iter(*poi)) {
...
}Also applies to: 55-62, 66-72, 81-89, 91-100, 106-122, 195-200, 243-265
🤖 Prompt for AI Agents
In test/exampleCreateNLL.cxx around lines 45-51 (and similarly for ranges 55-62,
66-72, 81-89, 91-100, 106-122, 195-200, 243-265), the code uses direct range-for
iteration over RooArgSet/RooArgList which is not portable across ROOT/LCG
variants; replace each range-for with utils::iter(...) from Combine to iterate
the RooArgSet/RooArgList, and update the loop body to iterate over the returned
iterable (preserving the dynamic_cast to RooRealVar and the existing
continue/add logic) so compilation works across ROOT versions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
test/exampleCreateNLL.cxx (5)
81-82: Fix ownership bug: don't unique_ptr-manage a workspace-owned object.
ws.genobj("discreteParams")returns a workspace-owned pointer. Wrapping it inunique_ptrwill cause a double-delete when the unique_ptr destructor runs and the workspace is destroyed.Apply this diff:
- std::unique_ptr<RooArgSet> discreteParameters{ - dynamic_cast<RooArgSet *>(ws.genobj("discreteParams"))}; + RooArgSet *discreteParameters = dynamic_cast<RooArgSet *>(ws.genobj("discreteParams"));Note: The later
std::unique_ptr<RooArgSet> pdfParams{multi->getParameters(nullptr)}at line 116 is correct sincegetParametersreturns a caller-owned object.
48-54: Use utils::iter(...) for RooArgSet/RooArgList iteration.Direct range-for over RooArgSet/RooArgList may not compile across different ROOT/LCG versions. Combine provides
utils::iter()for portable iteration.Apply this pattern throughout the function:
- for (RooAbsArg *arg : *poi) { + for (RooAbsArg *arg : utils::iter(*poi)) {This applies to all loops over:
*poi(line 48)*nuis(line 58)allVars(line 69)*discreteParameters(line 85)categories(line 94)clients(line 109)*pdfParams(line 117)Also applies to: 58-64, 69-75, 85-91, 94-102, 109-124
140-143: Use utils::iter(...) for RooArgSet iteration.Direct range-for over
poiSetmay not compile across different ROOT/LCG versions.Apply this pattern:
- for (RooAbsArg *arg : poiSet) { + for (RooAbsArg *arg : utils::iter(poiSet)) {Also applies to: 177-182
218-223: Promote fatal conditions to ASSERT_ to avoid null dereferences.*Using
std::cerrand commented-out returns allows the test to continue with null pointers, risking segfaults. Replace with GTest fatal assertions to stop immediately on failure.Apply these changes:
auto *workspace = dynamic_cast<RooWorkspace *>(file->Get(workspaceName.c_str())); - if (!workspace) { - std::cerr << "ERROR: workspace '" << workspaceName << "' not found in '" << fileName << "'.\n"; - file->ls(); - //return 2; - } + ASSERT_NE(workspace, nullptr) << "workspace '" << workspaceName << "' not found in '" << fileName << "'";auto *modelConfig = dynamic_cast<RooStats::ModelConfig *>(workspace->genobj(modelConfigName.c_str())); - if (!modelConfig) { - std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" - << workspaceName << "'.\n"; - //return 2; - } + ASSERT_NE(modelConfig, nullptr) << "ModelConfig '" << modelConfigName << "' not found in workspace '" << workspaceName << "'";RooAbsPdf *pdf = modelConfig->GetPdf(); - if (!pdf) { - std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' does not define a pdf.\n"; - //return 2; - } + ASSERT_NE(pdf, nullptr) << "ModelConfig '" << modelConfigName << "' does not define a pdf";RooAbsData *data = workspace->data(dataName.c_str()); - if (!data) { - std::cerr << "ERROR: dataset '" << dataName << "' not found in workspace '" << workspaceName - << "'.\n"; - //return 2; - } + ASSERT_NE(data, nullptr) << "dataset '" << dataName << "' not found in workspace '" << workspaceName << "'";const RooArgSet *poi = modelConfig->GetParametersOfInterest(); - if (!poi || poi->getSize() == 0) { - std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n"; - //return 2; - } + ASSERT_TRUE(poi && poi->getSize() > 0) << "ModelConfig '" << modelConfigName << "' has no parameters of interest";Also applies to: 229-235, 237-241, 243-248, 254-257
258-262: Use utils::iter(...) for RooArgSet iteration.Direct range-for over
*poimay not compile across different ROOT/LCG versions.- for (RooAbsArg *arg : *poi) { + for (RooAbsArg *arg : utils::iter(*poi)) { auto *var = dynamic_cast<RooRealVar *>(arg); if (var) var->setConstant(false); }
🧹 Nitpick comments (1)
test/exampleCreateNLL.cxx (1)
116-116: Consider removing the cast to nullptr.The cast
(const RooArgSet *)nullptris unnecessary;getParameters(nullptr)orgetParameters(static_cast<const RooArgSet*>(nullptr))would be clearer.- std::unique_ptr<RooArgSet> pdfParams{multi->getParameters((const RooArgSet *)nullptr)}; + std::unique_ptr<RooArgSet> pdfParams{multi->getParameters(nullptr)};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/exampleCreateNLL.cxx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: LCG_106 - ROOT 6.32.02
🔇 Additional comments (5)
test/exampleCreateNLL.cxx (5)
1-31: LGTM: Includes are comprehensive and appropriate.All necessary headers for ROOT, Combine, and GTest are present.
34-40: LGTM: Helper function correctly checks and loads snapshots.The null and existence checks are appropriate before loading.
127-131: LGTM: Struct definition is clean and well-designed.Using NaN as the default for
minNllis a good practice for detecting uninitialized values.
158-161: LGTM: Proper error handling with GTest assertions.Using
ADD_FAILURE()with early return viastd::nulloptis the correct pattern for non-fatal test failures that prevent further execution.Also applies to: 165-168
264-283: LGTM: Test execution and comparison logic are sound.The dual-backend fit comparison with both absolute and relative tolerance checks is a robust approach for validating NLL backends.
| std::unique_ptr<TFile> file(TFile::Open(fileName.c_str(), "READ")); | ||
| EXPECT_FALSE(!file || file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n"; |
There was a problem hiding this comment.
Use ASSERT for critical file opening check.
The file must be open for the test to proceed. Using EXPECT_FALSE allows the test to continue even if the file open fails, which will cause crashes later. Use ASSERT_FALSE or ASSERT_TRUE to stop immediately on failure.
Apply this diff:
- EXPECT_FALSE(!file || file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n";
+ ASSERT_FALSE(!file || file->IsZombie()) << "Failed to open input file '" << fileName << "'";Or more idiomatically:
- std::unique_ptr<TFile> file(TFile::Open(fileName.c_str(), "READ"));
- EXPECT_FALSE(!file || file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n";
+ std::unique_ptr<TFile> file(TFile::Open(fileName.c_str(), "READ"));
+ ASSERT_TRUE(file && !file->IsZombie()) << "Failed to open input file '" << fileName << "'";🤖 Prompt for AI Agents
In test/exampleCreateNLL.cxx around lines 215-216, the test uses EXPECT_FALSE to
check file open which allows execution to continue on failure; replace the
EXPECT_FALSE check with an assertion (e.g., ASSERT_FALSE(!file ||
file->IsZombie()) or ASSERT_TRUE(file && !file->IsZombie())) so the test aborts
immediately if the input file cannot be opened, preserving the existing
diagnostic message.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
test/exampleCreateNLL.cxx (6)
48-122: Useutils::iter(...)for portable RooArgSet/RooArgList iteration.Direct range-for over RooArgSet/RooArgList may fail across ROOT/LCG variants. Replace with
utils::iter(...)at lines 48, 58, 69, 83, 92, 107, and 115.
143-146: Useutils::iter(...)for portable RooArgSet iteration.Replace direct range-for with
utils::iter(poiSet)for portability across ROOT versions.Apply this diff:
- for (RooAbsArg *arg : poiSet) { + for (RooAbsArg *arg : utils::iter(poiSet)) { if (auto *var = dynamic_cast<RooRealVar *>(arg)) var->setConstant(false); }
191-196: Useutils::iter(...)for portable RooArgSet iteration.Replace direct range-for with
utils::iter(poiSet)for portability across ROOT versions.Apply this diff:
- for (RooAbsArg *arg : poiSet) { + for (RooAbsArg *arg : utils::iter(poiSet)) { auto *poiVar = dynamic_cast<RooRealVar *>(arg); if (!poiVar) continue;
287-288: UseASSERT_FALSEfor critical file opening check.The file must be open for the test to proceed.
EXPECT_FALSEallows the test to continue on failure, causing crashes later.Apply this diff:
- EXPECT_FALSE(!file || file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n"; + ASSERT_FALSE(!file || file->IsZombie()) << "Failed to open input file '" << fileName << "'";
290-329: Replacestd::cerrprints withASSERT_*to avoid null dereferences.Printing errors and continuing risks segfaults. Use GTest fatal assertions to bail out immediately.
Apply this diff to replace error handling with assertions:
auto *workspace = dynamic_cast<RooWorkspace *>(file->Get(workspaceName.c_str())); - if (!workspace) { - std::cerr << "ERROR: workspace '" << workspaceName << "' not found in '" << fileName << "'.\n"; - file->ls(); - //return 2; - } + ASSERT_NE(workspace, nullptr) << "workspace '" << workspaceName << "' not found in '" << fileName << "'"; // ... later ... auto *modelConfig = dynamic_cast<RooStats::ModelConfig *>(workspace->genobj(modelConfigName.c_str())); - if (!modelConfig) { - std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" - << workspaceName << "'.\n"; - //return 2; - } + ASSERT_NE(modelConfig, nullptr) << "ModelConfig '" << modelConfigName << "' not found in workspace '" << workspaceName << "'"; RooAbsPdf *pdf = modelConfig->GetPdf(); - if (!pdf) { - std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' does not define a pdf.\n"; - //return 2; - } + ASSERT_NE(pdf, nullptr) << "ModelConfig '" << modelConfigName << "' does not define a pdf"; RooAbsData *data = workspace->data(dataName.c_str()); - if (!data) { - std::cerr << "ERROR: dataset '" << dataName << "' not found in workspace '" << workspaceName - << "'.\n"; - //return 2; - } + ASSERT_NE(data, nullptr) << "dataset '" << dataName << "' not found in workspace '" << workspaceName << "'"; // ... later ... const RooArgSet *poi = modelConfig->GetParametersOfInterest(); - if (!poi || poi->getSize() == 0) { - std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n"; - //return 2; - } + ASSERT_TRUE(poi && poi->getSize() > 0) << "ModelConfig '" << modelConfigName << "' has no parameters of interest";
330-334: Useutils::iter(...)for portable RooArgSet iteration.Replace direct range-for with
utils::iter(*poi)for portability across ROOT versions.Apply this diff:
- for (RooAbsArg *arg : *poi) { + for (RooAbsArg *arg : utils::iter(*poi)) { auto *var = dynamic_cast<RooRealVar *>(arg); if (var) var->setConstant(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/exampleCreateNLL.cxx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: dev3/latest - ROOT LCG master
| const RooArgSet &poiSet, | ||
| const std::string &backend, | ||
| std::string *error = nullptr) { | ||
| std:: cout << "Running fit with backend " << backend << std::endl; |
There was a problem hiding this comment.
Fix typo: extra space in std:: cout.
Apply this diff:
- std:: cout << "Running fit with backend " << backend << std::endl;
+ std::cout << "Running fit with backend " << backend << std::endl;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std:: cout << "Running fit with backend " << backend << std::endl; | |
| std::cout << "Running fit with backend " << backend << std::endl; |
🤖 Prompt for AI Agents
In test/exampleCreateNLL.cxx around line 141, remove the extra space in the
stream namespace usage; replace "std:: cout" with "std::cout" so the output
statement uses the correct identifier and compiles cleanly.
78de27e to
f35fa4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
test/testCreateNLL.cxx (5)
42-124: Use utils::iter(...) for portable RooArgSet/RooArgList iteration.Direct range-for iteration over RooArgSet/RooArgList is not portable across ROOT/LCG variants. Multiple locations in this function need to be updated:
- Lines 48, 58, 69, 83, 92, 107, 115
The past review provided detailed fixes for these exact issues. Please apply the suggested changes to use
utils::iter()wrapper.
141-141: Fix typo: extra space in std::cout.Apply this diff:
- std:: cout << "Running fit with backend " << backend << std::endl; + std::cout << "Running fit with backend " << backend << std::endl;
143-146: Use utils::iter(...) for RooArgSet iteration.Lines 143 and 191 iterate directly over
poiSetwhich is a RooArgSet. Useutils::iter(poiSet)for portability.Apply this diff:
- for (RooAbsArg *arg : poiSet) { + for (RooAbsArg *arg : utils::iter(poiSet)) { if (auto *var = dynamic_cast<RooRealVar *>(arg)) var->setConstant(false); } @@ - for (RooAbsArg *arg : poiSet) { + for (RooAbsArg *arg : utils::iter(poiSet)) { auto *poiVar = dynamic_cast<RooRealVar *>(arg);Also applies to: 191-196
287-288: Use ASSERT for critical file opening check.The file must be open for the test to proceed.
EXPECT_FALSEallows continuation even if the file fails to open, causing crashes later. The past review flagged this exact issue.Apply this diff:
- EXPECT_FALSE(!file || file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n"; + ASSERT_FALSE(!file || file->IsZombie()) << "Failed to open input file '" << fileName << "'";Or more idiomatically:
- EXPECT_FALSE(!file || file->IsZombie()) << "ERROR: failed to open input file '" << fileName << "'.\n"; + ASSERT_TRUE(file && !file->IsZombie()) << "Failed to open input file '" << fileName << "'";
290-329: Promote fatal conditions to ASSERT_ to avoid null dereferences.*Multiple locations still use
std::cerrwith commented-out returns instead of GTest assertions, risking null pointer dereferences. The past review provided specific fixes for lines 290-295, 301-307, 309-313, 315-320, and 326-329.Additionally, line 330 uses direct range-for over
*poiwithoututils::iter().Apply the past review's suggested changes to replace all
std::cerrchecks withASSERT_NEorASSERT_TRUE, and wrap the POI iteration:- auto *workspace = dynamic_cast<RooWorkspace *>(file->Get(workspaceName.c_str())); - if (!workspace) { - std::cerr << "ERROR: workspace '" << workspaceName << "' not found in '" << fileName << "'.\n"; - file->ls(); - //return 2; - } + auto *workspace = dynamic_cast<RooWorkspace *>(file->Get(workspaceName.c_str())); + ASSERT_NE(workspace, nullptr) << "workspace '" << workspaceName << "' not found in '" << fileName << "'"; @@ - if (!modelConfig) { - std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' not found in workspace '" - << workspaceName << "'.\n"; - //return 2; - } + ASSERT_NE(modelConfig, nullptr) << "ModelConfig '" << modelConfigName << "' not found"; @@ - if (!poi || poi->getSize() == 0) { - std::cerr << "ERROR: ModelConfig '" << modelConfigName << "' has no parameters of interest.\n"; - //return 2; - } + ASSERT_TRUE(poi && poi->getSize() > 0) << "ModelConfig has no parameters of interest"; @@ - for (RooAbsArg *arg : *poi) { + for (RooAbsArg *arg : utils::iter(*poi)) {(Similar changes for pdf and data checks as detailed in the past review.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
env_lcg.sh(1 hunks)test/CMakeLists.txt(2 hunks)test/testCreateNLL.cxx(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
env_lcg.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: Compile (py3.10, root6.32.2)
🔇 Additional comments (9)
test/CMakeLists.txt (2)
59-68: LGTM!The test configuration correctly sets up the template analysis workspace fixture by:
- Converting the datacard to a ROOT workspace
- Copying required input files to the build directory
- Configuring the library path for Combine classes
- Establishing the fixture for dependent tests
70-77: LGTM!The test is properly configured with:
- Correct source file reference (testCreateNLL.cxx)
- Appropriate library linkage (HiggsAnalysisCombinedLimit)
- Proper fixture dependency to ensure workspace availability
test/testCreateNLL.cxx (6)
1-32: LGTM!The includes are comprehensive and appropriate for a unit test that exercises Combine's NLL creation and fitting infrastructure.
219-240: LGTM!The comparison logic correctly:
- Filters for Barlow-Beeston parameters (prop_ prefix)
- Uses relative tolerance scaling to handle different parameter magnitudes
- Provides clear diagnostic messages on mismatch
244-261: LGTM!The test fixture properly:
- Saves and restores the runtime flag state
- Configures the analytic Barlow-Beeston mode per test parameter
- Ensures clean test isolation
262-281: LGTM!The runtime configuration correctly mirrors the combine binary defaults, enabling optimizations and specialized NLL implementations needed for the template analysis workflow.
336-367: LGTM!The test execution correctly:
- Runs baseline fit with 'combine' backend
- Conditionally tests 'cpu' or 'codegen' backend based on configuration
- Handles expected failures appropriately with GTEST_SKIP or ADD_FAILURE
- Compares fit results with proper tolerance
373-391: LGTM!The test parameterization provides comprehensive coverage:
- CPU and Codegen backends
- Analytic Barlow-Beeston on/off
- Expected failure flags for known codegen limitations
- Clear test naming via AnalyticParamName function
This effectively exercises the backend consistency matrix for template shape models.
env_lcg.sh (1)
1-1: LCG dev3 release confirmed valid for ROOT master testing.dev3 includes ROOT HEAD (development HEAD), confirming alignment with the intent to test against ROOT master. The switch from dev4 to dev3 is appropriate for your use case.
Note: Direct CVMFS path verification cannot be completed in this environment. To confirm CI and developer access, test the path directly in your CI pipeline or local environment:
ls -la /cvmfs/sft.cern.ch/lcg/views/dev3/latest/x86_64-el9-gcc13-opt/Consider documenting this LCG release dependency in the PR description or README for team reference.
| function(COMBINE_ADD_GTEST test_suite) | ||
| cmake_parse_arguments(ARG | ||
| "" | ||
| "TIMEOUT;FAILREGEX" | ||
| "COPY_TO_BUILDDIR;LIBRARIES;LABELS;INCLUDE_DIRS;ENVIRONMENT" ${ARGN}) | ||
|
|
||
| ROOT_GET_SOURCES(source_files . ${ARG_UNPARSED_ARGUMENTS}) | ||
| ROOT_EXECUTABLE(${test_suite} ${source_files} LIBRARIES ${ARG_LIBRARIES}) | ||
| target_link_libraries(${test_suite} PRIVATE GTest::gtest GTest::gmock GTest::gtest_main GTest::gmock_main) | ||
| target_include_directories(${test_suite} PRIVATE ${CMAKE_CURRENT_BINARY_DIR}) | ||
| if (ARG_INCLUDE_DIRS) | ||
| target_include_directories(${test_suite} PRIVATE ${ARG_INCLUDE_DIRS}) | ||
| endif(ARG_INCLUDE_DIRS) | ||
|
|
||
| ROOT_PATH_TO_STRING(name_with_path ${test_suite} PATH_SEPARATOR_REPLACEMENT "-") | ||
| string(REPLACE "-test-" "-" clean_name_with_path ${name_with_path}) | ||
| ROOT_ADD_TEST( | ||
| gtest${clean_name_with_path} | ||
| COMMAND ${test_suite} ${extra_command} | ||
| WORKING_DIR ${CMAKE_CURRENT_BINARY_DIR} | ||
| COPY_TO_BUILDDIR "${ARG_COPY_TO_BUILDDIR}" | ||
| ${willfail} | ||
| TIMEOUT "${ARG_TIMEOUT}" | ||
| LABELS "${ARG_LABELS}" | ||
| FAILREGEX "${ARG_FAILREGEX}" | ||
| ENVIRONMENT "${ARG_ENVIRONMENT}" | ||
| ) | ||
| endfunction() |
There was a problem hiding this comment.
Verify undefined CMake variables in COMBINE_ADD_GTEST.
The function references ${extra_command} (line 19) and ${willfail} (line 22) which are never defined or parsed from arguments. These will expand to empty strings, which may be intentional but looks like incomplete implementation or copy-paste remnants.
If these are intentional placeholders, remove them for clarity. If they should be configurable, add them to the cmake_parse_arguments call:
function(COMBINE_ADD_GTEST test_suite)
cmake_parse_arguments(ARG
- ""
+ "WILL_FAIL"
- "TIMEOUT;FAILREGEX"
+ "TIMEOUT;FAILREGEX;EXTRA_COMMAND"
"COPY_TO_BUILDDIR;LIBRARIES;LABELS;INCLUDE_DIRS;ENVIRONMENT" ${ARGN})Then use ${ARG_EXTRA_COMMAND} and handle ${ARG_WILL_FAIL} appropriately in the ROOT_ADD_TEST call.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/CMakeLists.txt lines 1-28 the function uses undefined variables
${extra_command} and ${willfail}; add EXTRA_COMMAND and WILLFAIL to the
cmake_parse_arguments call (e.g. add WILLFAIL to the second argument list and
EXTRA_COMMAND to the third/multi-valued list) so they are parsed as
ARG_EXTRA_COMMAND and ARG_WILLFAIL, replace ${extra_command} with
${ARG_EXTRA_COMMAND}, and add a small conditional before ROOT_ADD_TEST:
if(ARG_WILLFAIL) set(willfail WILLFAIL) else unset(willfail) endif so you still
pass ${willfail} to ROOT_ADD_TEST when requested.
test/CMakeLists.txt
Outdated
| ROOT_EXECUTABLE(${test_suite} ${source_files} LIBRARIES ${ARG_LIBRARIES}) | ||
| target_link_libraries(${test_suite} PRIVATE GTest::gtest GTest::gmock GTest::gtest_main GTest::gmock_main) |
There was a problem hiding this comment.
| ROOT_EXECUTABLE(${test_suite} ${source_files} LIBRARIES ${ARG_LIBRARIES}) | |
| target_link_libraries(${test_suite} PRIVATE GTest::gtest GTest::gmock GTest::gtest_main GTest::gmock_main) | |
| ROOT_EXECUTABLE(${test_suite} ${source_files} | |
| LIBRARIES ${ARG_LIBRARIES} GTest::gtest GTest::gmock GTest::gtest_main GTest::gmock_main | |
| ) |
@maxgalli, I think this is the way to fix the build error on older ROOT versions
maxgalli
force-pushed
the
unit_test_dev_ad
branch
from
f35fa4d to
b3bf7bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/CMakeLists.txt (1)
1-27: Fix undefined variables in COMBINE_ADD_GTEST function.The function references
${extra_command}(line 18) and${willfail}(line 21), but these are never defined in thecmake_parse_argumentscall (lines 2–5). They expand to empty strings, indicating incomplete implementation. Either remove them if unused, or add them to the argument parser if they should be configurable.Option 1: Remove if intentionally unused
function(COMBINE_ADD_GTEST test_suite) cmake_parse_arguments(ARG "" "TIMEOUT;FAILREGEX" "COPY_TO_BUILDDIR;LIBRARIES;LABELS;INCLUDE_DIRS;ENVIRONMENT" ${ARGN}) ROOT_GET_SOURCES(source_files . ${ARG_UNPARSED_ARGUMENTS}) ROOT_EXECUTABLE(${test_suite} ${source_files} LIBRARIES ${ARG_LIBRARIES} GTest::gtest GTest::gmock GTest::gtest_main GTest::gmock_main) target_include_directories(${test_suite} PRIVATE ${CMAKE_CURRENT_BINARY_DIR}) if (ARG_INCLUDE_DIRS) target_include_directories(${test_suite} PRIVATE ${ARG_INCLUDE_DIRS}) endif(ARG_INCLUDE_DIRS) ROOT_PATH_TO_STRING(name_with_path ${test_suite} PATH_SEPARATOR_REPLACEMENT "-") string(REPLACE "-test-" "-" clean_name_with_path ${name_with_path}) ROOT_ADD_TEST( gtest${clean_name_with_path} - COMMAND ${test_suite} ${extra_command} + COMMAND ${test_suite} WORKING_DIR ${CMAKE_CURRENT_BINARY_DIR} COPY_TO_BUILDDIR "${ARG_COPY_TO_BUILDDIR}" - ${willfail} TIMEOUT "${ARG_TIMEOUT}" LABELS "${ARG_LABELS}" FAILREGEX "${ARG_FAILREGEX}" ENVIRONMENT "${ARG_ENVIRONMENT}" ) endfunction()Option 2: Add them as configurable parameters
function(COMBINE_ADD_GTEST test_suite) cmake_parse_arguments(ARG - "" - "TIMEOUT;FAILREGEX" + "WILL_FAIL" + "TIMEOUT;FAILREGEX;EXTRA_COMMAND" "COPY_TO_BUILDDIR;LIBRARIES;LABELS;INCLUDE_DIRS;ENVIRONMENT" ${ARGN}) ROOT_GET_SOURCES(source_files . ${ARG_UNPARSED_ARGUMENTS}) ROOT_EXECUTABLE(${test_suite} ${source_files} LIBRARIES ${ARG_LIBRARIES} GTest::gtest GTest::gmock GTest::gtest_main GTest::gmock_main) target_include_directories(${test_suite} PRIVATE ${CMAKE_CURRENT_BINARY_DIR}) if (ARG_INCLUDE_DIRS) target_include_directories(${test_suite} PRIVATE ${ARG_INCLUDE_DIRS}) endif(ARG_INCLUDE_DIRS) ROOT_PATH_TO_STRING(name_with_path ${test_suite} PATH_SEPARATOR_REPLACEMENT "-") string(REPLACE "-test-" "-" clean_name_with_path ${name_with_path}) + if(ARG_WILL_FAIL) + set(will_fail WILLFAIL) + endif() ROOT_ADD_TEST( gtest${clean_name_with_path} - COMMAND ${test_suite} ${extra_command} + COMMAND ${test_suite} ${ARG_EXTRA_COMMAND} WORKING_DIR ${CMAKE_CURRENT_BINARY_DIR} COPY_TO_BUILDDIR "${ARG_COPY_TO_BUILDDIR}" - ${willfail} + ${will_fail} TIMEOUT "${ARG_TIMEOUT}" LABELS "${ARG_LABELS}" FAILREGEX "${ARG_FAILREGEX}" ENVIRONMENT "${ARG_ENVIRONMENT}" ) endfunction()
🧹 Nitpick comments (1)
env_lcg.sh (1)
1-8: Add shebang directive.The shell script lacks a shebang line, which static analysis tools flag as ambiguous. Add
#!/bin/bashat the top to specify the target shell explicitly.+#!/bin/bash + LCG_RELEASE=dev3 # or dev3 for ROOT master (dev4 is the latest ROOT stable branch)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
data/ci/template-analysis_shape_autoMCStats.txt(1 hunks)env_lcg.sh(1 hunks)test/CMakeLists.txt(2 hunks)test/testCreateNLL.cxx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- data/ci/template-analysis_shape_autoMCStats.txt
- test/testCreateNLL.cxx
🧰 Additional context used
🪛 Shellcheck (0.11.0)
env_lcg.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
🔇 Additional comments (3)
env_lcg.sh (1)
1-1: Clarify the LCG_RELEASE downgrade and update comment.The comment appears contradictory: it says "for ROOT master" yet references "dev4 is the latest ROOT stable branch." The change downgrades from
dev4todev3, which seems like a regression if dev4 is stable. Clarify the intent and update the comment accordingly.Is this intentional? If dev3 is required for test compatibility, please explain in a comment. If dev4 should be retained, revert this change.
test/CMakeLists.txt (2)
8-8: Verify ROOT_EXECUTABLE syntax compatibility.The
ROOT_EXECUTABLEcall (line 8) uses a specific LIBRARIES syntax that may not be compatible with older ROOT versions. A past review suggested an alternative form for compatibility.Please confirm that this syntax works with all targeted ROOT versions. If compatibility issues arise, apply this alternative:
ROOT_EXECUTABLE(${test_suite} ${source_files} - LIBRARIES ${ARG_LIBRARIES} GTest::gtest GTest::gmock GTest::gtest_main GTest::gmock_main) + LIBRARIES ${ARG_LIBRARIES} GTest::gtest GTest::gmock GTest::gtest_main GTest::gmock_main + )Or, if the issue persists, consider using
target_link_librariesseparately after the executable is created.
58-76: Template-analysis test configuration looks good.The new template_analysis tests properly set up test fixtures and dependencies: the text2workspace step (lines 58–67) creates the workspace fixture, which is correctly required by the testCreateNLL test via
FIXTURES_REQUIRED(lines 74–76). Data files are properly copied, and environment variables are configured to enable Combine classes to be loaded.
maxgalli
force-pushed
the
unit_test_dev_ad
branch
3 times, most recently
from
948012a to
3cd51dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
🔇 Additional comments (2)
Makefile (2)
34-36: Variable initialization for system libxml2 detection is well-structured.The use of
firstwordwithwildcardfor LIBXML2_SYSTEM detection and the empty initialization of PREPEND_LIBS provide a clean way to conditionally compose library paths. The updated LIBS variable correctly chains these components.
40-49: I need to verify the linker flag best practices. Let me search for information about how linkers handle direct.sopaths and therpath-linkflag.Let me search for more context about how LIBXML2_SYSTEM and LIBS are used in the actual build:
Conditional block correctly handles system libxml2, but verify runtime library resolution.
The direct
.sopath on line 47 is a valid linker flag. However, the -rpath-link option is only effective at link time, while the actual runtime library search depends on whether the linked library has a SONAME set or whether the library is found via standard search paths.For this approach to work reliably at runtime, the system libxml2 must be in standard library directories (
/usr/lib,/lib64, etc.) or a runtime search path must be embedded. If the system libxml2 is in a non-standard location without a SONAME, add an explicit-Wl,-rpathflag (distinct from-rpath-link) or confirm that standard CentOS library paths are guaranteed in your deployment environments.
maxgalli
force-pushed
the
unit_test_dev_ad
branch
2 times, most recently
from
7d95af4 to
707711d
Compare
maxgalli
force-pushed
the
unit_test_dev_ad
branch
from
707711d to
78f04d9
Compare
2 participants
In the context of the integration of AD in Combine, we add this test that bypasses the Combine command line and creates+minimizes a likelihood from a simplified version of this model.
Summary by CodeRabbit
Tests
Chores