Skip to content

Register MEPHIT CTest smoke checks - #36

Open
krystophny wants to merge 1 commit into
mainfrom
fix/ctest-registration
Open

Register MEPHIT CTest smoke checks#36
krystophny wants to merge 1 commit into
mainfrom
fix/ctest-registration

Conversation

@krystophny

Copy link
Copy Markdown
Member

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_help requires the installed helper script's help path to exit
    successfully and direct the user to the README;
  • mephit_requires_config requires mephit_test.x to reject invocation
    without its mandatory configuration file.

find_or_fetch temporarily disables BUILD_TESTING while adding libneo as a
subdirectory. MEPHIT retains its own BUILD_TESTING setting, but it no longer
registers 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

  • MEPHIT sources, compiler flags, linked libraries, executable interfaces, and
    runtime configuration are unchanged.
  • libneo remains fetched at the caller-selected ref and linked through the same
    targets.
  • BUILD_TESTING=OFF still disables all MEPHIT tests.
  • The change affects test registration only; numerical algorithms,
    discretizations, convergence criteria, CGS units, boundary conditions, ABI,
    and memory layout are unchanged.

Verification

Failing before this change:

$ make test LIBNEO_REF=e451e0a42f5228d85b0479de40aa35db8b48239d
cd build && ctest
No test configuration file found!
$ echo $?
0

$ fo test
Tests: 0 passed (0.00s)

After initially enabling parent CTest but before isolating dependency tests,
fo test registered 97 tests and failed on unbuilt libneo executables. This
guards the dependency boundary as well as the empty-suite case.

Passing after both changes:

$ fo
Static: OK (115 modules, 115 changed, 115 affected)
Build: OK
Tests: OK
Lint: OK
All stages passed

$ fo test
Tests: 2 passed (0.2s)

$ make test LIBNEO_REF=e451e0a42f5228d85b0479de40aa35db8b48239d
1/2 Test #1: mephit_cli_help ........... Passed
2/2 Test #2: mephit_requires_config .... Passed
100% tests passed, 0 tests failed out of 2

@krystophny
krystophny marked this pull request as ready for review July 13, 2026 13:55

@slopqueue slopqueue Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

  1. minor CMakeLists.txt:222-227mephit_requires_config runs mephit_test.x with no args, relying on error stop 'expected path to config file as first parameter' (src/mephit_test.f90) to produce a non-zero exit so WILL_FAIL TRUE passes. This is sound, but the test only exercises the argument-check path (no I/O happens before the error 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.
  2. minor .github/workflows/main.yml — CI job is named "Build and test" but only runs make; make test (cd build && ctest) is never invoked, so neither new test is exercised in CI. The tests only run locally via make 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 a make test step to the workflow.
  3. minor cmake/Util.cmake:29set(BUILD_TESTING OFF) is a function-scope normal variable shadowing the cache var; it correctly suppresses libneo's if(BUILD_TESTING) test registration for the add_subdirectory that 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test target exits successfully without running tests

1 participant