test(sysrap): convert stree_load_test to ctest and harden GDML-fixture coverage#405
test(sysrap): convert stree_load_test to ctest and harden GDML-fixture coverage#405plexoos wants to merge 3 commits into
Conversation
- build stree_load_test with CMake and register it in ctest - construct stree directly from a checked-in GDML fixture via U4 - remove the old bash runner and update related references
Restore support for ridx_type 'N' in stree::pick_lvid_ordinal_node so callers can query the full nds vector again. This aligns the public selector with the rest of stree, which still uses 'N' for full structural-node searches, and prevents TEST=ALL style sweeps from aborting when they exercise the existing full-node path.
Make stree_load_test degrade gracefully when it runs against the small default GDML fixture instead of a JUNO-scale geometry. - skip soname-dependent probes when the requested solids are absent - treat missing MOI and TMPFOLD context as skippable test setup, not hard failures - clamp small-vector AABB scans to valid bounds - skip factor dumps when the current geometry has no factors This keeps TEST=ALL useful as a portability sweep while preserving the richer branches for geometries that provide the expected names and structure.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7f308fe7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| foreach(SRC ${STREE_GDML_TEST_SOURCE}) | ||
| get_filename_component(TGT ${SRC} NAME_WE) | ||
| add_executable(${TGT} ${SRC}) | ||
| target_compile_definitions(${TGT} PRIVATE STREE_LOAD_TEST_DEFAULT_GDML="${CMAKE_SOURCE_DIR}/tests/geom/sphere_leak.gdml") |
There was a problem hiding this comment.
When Simphony is included from another project with add_subdirectory() and that parent enables tests, this points at the parent's tests/geom/sphere_leak.gdml rather than Simphony's fixture; CMake documents CMAKE_SOURCE_DIR as the top-level source tree. In that context the new ctest target is compiled with a non-existent default GDML path, so U4GDML::Read returns null and the default desc run later dereferences st. Use this project's source directory (for example PROJECT_SOURCE_DIR here) or a path relative to CMAKE_CURRENT_SOURCE_DIR instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR turns sysrap/tests/stree_load_test.cc into a ctest-managed executable that can build a stree directly from a checked-in GDML fixture (via U4/Geant4), and hardens multiple “broad inspection” code paths so they don’t abort on the small default fixture geometry.
Changes:
- Register
stree_load_testas a standard CMake/ctest target and compile in a default GDML fixture path. - Update
stree_load_testto constructstreefrom GDML in-process (and add multiple guards/skips for fixture-limited geometries and env-dependent modes). - Restore
'N'support instree::pick_lvid_ordinal_node(...)to enable full-node lookups.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sysrap/tests/stree_load_test.cc | Converts to a ctest-friendly executable that loads stree from a GDML fixture and adds multiple safety guards/skips. |
| sysrap/tests/CMakeLists.txt | Adds a dedicated ctest target for stree_load_test with a compiled-in default GDML fixture path and U4 linkage. |
| sysrap/stree.h | Restores/extends pick_lvid_ordinal_node to accept 'N' for full nds searches. |
| CSG/CSGImport.h | Removes a large header comment block (documentation cleanup). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| stree_load_test t; | ||
| OPTICKS_LOG(argc, argv); | ||
|
|
||
| stree_load_test t(argc, argv); | ||
| return t.main(); |
| const glm::tmat4x4<double>& gtd = st->gtd[nidx] ; | ||
| std::cout << "gtd\n" << stra<double>::Desc(m2w) << "\n" ; |
Turn
sysrap/tests/stree_load_test.ccinto a regular CMake/ctest target that buildsstreedirectly from a checked-inGDML fixture, and harden its broad inspection modes so they run safely on the small default geometry.
Why
stree_load_testwas previously a developer-only helper with shell-script setup and external geometry assumptions. Thatmade it hard to run in CI and brittle when exercised against a minimal fixture.
This change makes the test self-contained, ctest-friendly, and more robust across geometries.
What Changed
stree_load_testas a normal CMake/ctest target insysrap/tests/CMakeLists.txtstreein-process from a checked-in GDML fixture viaU4GDMLandU4Treetests/geom/sphere_leak.gdmlas the default fixture geometry'N'support instree::pick_lvid_ordinal_node(...)so full-node lookups continue to work with existing callersTEST=ALLand related inspection paths to skip geometry-specific probes when the default fixture does not provide the expected solid namesResult
stree_load_testcan now be run directly under ctest without external shell setup, andTEST=ALLno longer aborts onthe default small GDML fixture.
Validation
ctest --test-dir build -R SysRapTest.stree_load_testTEST=ALL ctest --test-dir build -R SysRapTest.stree_load_test -V