Register MEPHIT CTest smoke checks - #36
Conversation
There was a problem hiding this comment.
Review verdict: Approve
Summary: PR #36 enables CTest in MEPHIT, registers two smoke tests (mephit_cli_help and mephit_requires_config), and suppresses dependency test registration by scoping BUILD_TESTING OFF before add_subdirectory(libneo).
Findings:
- minor
CMakeLists.txt:222-227—mephit_requires_configrunsmephit_test.xwith no args, relying onerror stop 'expected path to config file as first parameter'(src/mephit_test.f90) to produce a non-zero exit soWILL_FAIL TRUEpasses. This is sound, but the test only exercises the argument-check path (no I/O happens before theerror stop), so it is purely a smoke check. Acceptable given the PR's stated "smoke checks" scope, but worth noting it provides no coverage of the actual config-reading or HDF5 paths. - minor
.github/workflows/main.yml— CI job is named "Build and test" but only runsmake;make test(cd build && ctest) is never invoked, so neither new test is exercised in CI. The tests only run locally viamake test. Not introduced by this PR (pre-existing CI gap), but it means there is no CI evidence that the two tests actually pass; the correctness rests on manual testing. Consider adding amake teststep to the workflow. - minor
cmake/Util.cmake:29—set(BUILD_TESTING OFF)is a function-scope normal variable shadowing the cache var; it correctly suppresses libneo'sif(BUILD_TESTING)test registration for theadd_subdirectorythat follows. This is the standard idiom for fetched dependencies, and the comment makes the intent clear. No change needed.
The mephit_cli_help test is well-formed: mephit.bash is configured into ${PROJECT_BINARY_DIR}/scripts/ at configure time (CMakeLists.txt:211, @ONLY), the --help branch calls mephit_help which prints "README.md", and PASS_REGULAR_EXPRESSION "README.md" matches. The set(BUILD_TESTING OFF) fix correctly prevents the include(CTest) at the top level from leaking BUILD_TESTING ON into the libneo subdirectory, which would otherwise register unbuilt/irrelevant libneo tests into MEPHIT's CTest set. Both tests are safe (no file side effects) and fast.
Verdict: Approve — the diff is minimal, correct, and well-scoped; the BUILD_TESTING scoping fix is a known-good pattern, and the smoke tests are appropriate. The CI gap is pre-existing and not introduced here.
Summary
Register a nonempty MEPHIT CTest inventory and keep tests from fetched
dependencies out of the parent test suite.
The two self-contained checks exercise public process boundaries:
mephit_cli_helprequires the installed helper script's help path to exitsuccessfully and direct the user to the README;
mephit_requires_configrequiresmephit_test.xto reject invocationwithout its mandatory configuration file.
find_or_fetchtemporarily disablesBUILD_TESTINGwhile adding libneo as asubdirectory. MEPHIT retains its own
BUILD_TESTINGsetting, but it no longerregisters libneo tests whose executables and external fixtures are intentionally
excluded from the consumer build.
This closes #35's false-green condition. It does not claim a numerical MEPHIT
regression suite; numerical fixture coverage remains future test work.
Build and semantic invariants
runtime configuration are unchanged.
targets.
BUILD_TESTING=OFFstill disables all MEPHIT tests.discretizations, convergence criteria, CGS units, boundary conditions, ABI,
and memory layout are unchanged.
Verification
Failing before this change:
After initially enabling parent CTest but before isolating dependency tests,
fo testregistered 97 tests and failed on unbuilt libneo executables. Thisguards the dependency boundary as well as the empty-suite case.
Passing after both changes: