build(cmake): honor BUILD_TESTING and run simg4ox via ctest#378
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns Simphony’s test setup with standard CMake/CTest workflows by making test subdirectories conditional on BUILD_TESTING and registering the existing simg4ox integration check as a CTest test.
Changes:
- Gate all
*/testssubdirectories (and new top-leveltests/) behindif(BUILD_TESTING)while keeping CTest integration present. - Register
Integration.simg4oxviactest, using a dedicated build-tree working directory and environment-driven wrapper script execution. - Relocate the A/B comparison script from
tests/intooptiphy/ana/and update the wrapper accordingly; remove duplicate CI invocation of the same integration check.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Introduces a BUILD_TESTING default and gates test subdirectories; keeps CTest included. |
tests/CMakeLists.txt |
Adds a CTest-registered integration test (Integration.simg4ox) that drives test_simg4ox.sh. |
tests/test_simg4ox.sh |
Hardens the script (pipefail, env setup) and points to the relocated compare_ab.py. |
tests/compare_ab.py |
Removes the old test-local A/B comparison script (moved). |
optiphy/ana/compare_ab.py |
Adds the relocated A/B comparison script with improved CLI/error handling structure. |
.github/workflows/build-pull-request.yaml |
Removes the redundant direct execution of tests/test_simg4ox.sh in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Use CMake's standard testing option, but avoid enabling this project's tests | ||
| # by default when Simphony is pulled in as a subproject. | ||
| if(NOT DEFINED BUILD_TESTING) | ||
| set(BUILD_TESTING ${PROJECT_IS_TOP_LEVEL} CACHE BOOL "Build the testing tree.") | ||
| endif() |
| # Keep CTest included even when BUILD_TESTING=OFF so external drivers can run | ||
| # ctest and observe an empty test set rather than a missing configuration. | ||
| include(CTest) |
| @@ -0,0 +1,20 @@ | |||
| find_package(Python3 REQUIRED COMPONENTS Interpreter) | |||
| find_program(BASH_EXECUTABLE bash REQUIRED) | |||
![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: cf4881deaa
ℹ️ 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".
| # Use CMake's standard testing option, but avoid enabling this project's tests | ||
| # by default when Simphony is pulled in as a subproject. | ||
| if(NOT DEFINED BUILD_TESTING) | ||
| set(BUILD_TESTING ${PROJECT_IS_TOP_LEVEL} CACHE BOOL "Build the testing tree.") |
There was a problem hiding this comment.
cmake_minimum_required still allows CMake 3.18, but PROJECT_IS_TOP_LEVEL is only available starting in CMake 3.21 (as shown by cmake --help-variable PROJECT_IS_TOP_LEVEL). In top-level builds with CMake 3.18–3.20 and no explicit -DBUILD_TESTING, this expands empty and caches BUILD_TESTING as false, so the new if(BUILD_TESTING) block skips all test subdirectories and ctest won't discover the existing tests or Integration.simg4ox. Please either use a fallback such as comparing CMAKE_PROJECT_NAME and PROJECT_NAME, or raise the minimum CMake version.
Useful? React with 👍 / 👎.
plexoos
force-pushed
the
ctest-simg4ox
branch
2 times, most recently
from
4e5bf1d to
e0a896d
Compare
| find_package(Python3 REQUIRED COMPONENTS Interpreter) | ||
| find_program(BASH_EXECUTABLE bash REQUIRED) |
There was a problem hiding this comment.
The official CMake 3.18 docs include [REQUIRED] in the find_program() signature and say it stops processing with an error if nothing is found.
- make Simphony follow CMake's standard `BUILD_TESTING` behavior - only add `sysrap/tests`, `CSG/tests`, `qudarap/tests`, `CSGOptiX/tests`, `u4/tests`, and `g4cx/tests` when testing is enabled - keep `include(CTest)` unconditional so external drivers can still run `ctest` and see an empty test set when `BUILD_TESTING=OFF` The top-level build was including all test subdirectories unconditionally, so `BUILD_TESTING=OFF` did not fully disable test configuration. This change makes testing behavior predictable for normal CMake builds and for packaging workflows such as Spack, where external drivers may configure with testing disabled and/or invoke `ctest` during build or install validation.
- add a top-level tests CMake target and register Integration.simg4ox - move compare_ab.py to optiphy/ana and remove the old tests copy - simplify the simg4ox wrapper to run from the ctest working directory - drop the duplicate standalone simg4ox invocation from PR CI
This branch aligns Simphony's testing setup with standard CMake/CTest behavior and moves the
existing
simg4oxintegration check underctest.The main goals are:
BUILD_TESTINGin the project treectestctestin a conventional waytest_simg4ox.shMotivation
Before this change, test subdirectories were added unconditionally, which made the project less
friendly to standard CMake testing flows and to package-manager driven builds.
In addition,
simg4oxwas validated via a standalone shell script instead of a registered CTesttest. That meant CI and external tooling had to know about the script separately instead of relying
on
ctestas the single test entry point.What Changed
Top-level CMake testing behavior
BUILD_TESTINGusing the standard CMake patternBUILD_TESTINGtoPROJECT_IS_TOP_LEVELinclude(CTest)even whenBUILD_TESTING=OFF*/testssubdirectories behindif(BUILD_TESTING)This gives the expected behavior in both cases:
Keeping
CTestincluded even withBUILD_TESTING=OFFmeans external drivers can still runctestand see an empty test set rather than missing test configuration.
simg4oxCTest integrationIntegration.simg4oxsimg4oxfrom CTest using$<TARGET_FILE:simg4ox>This turns the existing
tests/test_simg4ox.shflow into a normal CTest integration test instead ofan external ad hoc command.
A/B comparison script cleanup
tests/compare_ab.pyto optiphy/ana/compare_ab.pytests/compare_ab.pyGeant4-version-dependent mismatch indices
Placing it under
optiphy/anamakes it easier to treat as a reusable analysis/helper script ratherthan a one-off test-local file.
CI cleanup
tests/test_simg4ox.shinvocation from.github/workflows/build-pull-request.yamlctest --test-dir "$SIMPHONY_BUILD" --output-on-failurestep to runIntegration.simg4oxThis avoids running the same test twice in PR CI.