Skip to content

unittests: start introducing unit tests in Idefix (currently for the lookupTable) - #419

Open
svalat wants to merge 22 commits into
idefix-code:developfrom
svalat:feature/introduce-gtest-usage-valid-lookup-table
Open

svalat wants to merge 22 commits into
idefix-code:developfrom
svalat:feature/introduce-gtest-usage-valid-lookup-table

Conversation

@svalat

@svalat svalat commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

This PR introduces the usage of GoogleTest in the Idefix sources via the first use case about the LookupTable.

In the build system

GoogleTest is automatically downloaded by the CMake script in version 1.18.0 via the function GoogleTestDownloadAndLink() implemented in cmake/HandleDeps.cmake.

Due to the limitation discussed in next part I cannot safely use the system GoogleTest because I don't know the version on which I fall on form the sources.

The core functions of Idefix has been moved into libidefix-core to be linked to idefix as for the new unittests exacultable.

GTest limitation with KOKKOS_LAMBDA

When building a unit test defined via TEST(..., ...) with a KOKKOS_LAMDA inside we cannot compile with
NVCC because it complains about the fact the the function generated by TEST() is private.

A fix has been proposed to Google in 2022 but they rejected it due to non standard
behavior of the compiler : google/googletest#4104

I currently override inside Idefix the GTEST_TEST_ macro applying the private -> public change but to be safe
it makes the code sticked to a specific version of GoogleTest.

With this fix everything is working as long as we #include "gtest-idfx.hpp" in each test file.

Idefix & MPI & Kokkos init

A new main() has been defined in gtest-idfx.cpp to be used to launch the unit tests with Kokkos and MPI initialized.

Extra assertions

When we will progress in unit testing Idefix we will need to validation some IdefixXDArrays. I already defined some dedicated macros
to get a simple semantic reusing all the matchers provided by
GoogleTest & GoogleMock.

#include "gtest-idfx.hpp"

//check all 5 static
ASSERT_THAT_1D_ARRAY(a, testing::Eq, 6);

//compare both array
ASSERT_THAT_1D_ARRAYS(a, testing::Eq, b);

//compare with near
ASSERT_THAT_1D_ARRAYS_ARGS(a, testing::DoubleNear, b, 0.001);

There is a full example of use in src/test/testArrays.hpp.

First ported tests

The LoopkupTable test has been translated to GoogeTest and is first real
test in Idefix.

Building and launching

You need to activate -DIdefix_GTEST=ON to build the unit tests.

# configure
mkdir build-unittests
cd build-unittests
cmake .. -DIdefix_GTEST=ON -DIdefix_PROBLEM_DIR=../test/MHD/AxisFluxTube/

# build
make

# run all
./unittest

Intégration in github actions

I patched the github action scripts so they first build & run the unittests
for each mode (CUDA / Intel / gcc).

It currently run first the unit tests then run the integration tests the time
to validate. I will revert that behavior before doing the final merge.

TODO

  • Revert the unittest priority in github action before merging.

svalat added 22 commits October 5, 2026 16:30
… download GoogleTest if not found on the system
@svalat svalat added the enhancement New feature or request label Oct 30, 2026

@glesur glesur left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Already addressed in this branch: the UnitTests job in .github/workflows/idefix-ci-jobs.yml was inserted as the first job; it is now moved back to the end of the jobs: list, restoring the previous ordering (per the PR TODO).

Change requests

  1. cmake/ReplaceIdefixSource.cmake:16 — broken for non-main.cpp replacements since the core split. The match/uniqueness check queries get_target_property(fullsource idefix SOURCES), but core sources now live in idefix-core (the idefix target only holds main.cpp + the problem setup.cpp). Any non-main.cpp replacement now trips the spurious SEND_ERROR "no source file match". Fix: compute _target_name first and use get_target_property(fullsource ${_target_name} SOURCES). (Only test/skeleton uses this today, with main.cpp, which is why CI is green.)
  2. CMake minimum vs. the GTEST path. CMakeLists.txt:19 declares cmake_minimum_required(VERSION 3.16), but the GTEST path uses cmake_policy(SET CMP0135 NEW) (cmake/HandleDeps.cmake:21, requires CMake ≥ 3.27 — hard configure error on older versions) and cmake --fresh (scripts/ci/run-unit-tests:87, requires ≥ 3.24). Please guard with if(POLICY CMP0135) / drop --fresh, or raise the minimum when Idefix_GTEST=ON.
  3. doc/source/programmingguide.rst:733 — stale pointer. "Usage examples are provided in test/utils/lookupTable" — that directory is deleted by this PR. Point to src/utils/tests/testLookupTable.cpp instead.

Nits

  • CMakeLists.txt:34: stray ) in the Idefix_GTEST option help string.
  • CMakeLists.txt:116: optional modernization — add_test(NAME unittests COMMAND unittests).
  • cmake/HandleDeps.cmake:16,20: typos — "verison" → "version"; "# it not, download it internally" → "# if not found on the system, download it internally".
  • Wrong Source file header paths: src/gtest-idfx.hpp:4 and src/gtest-idfx.cpp:4 say src/utils/gtest-idfx.hpp (files live in src/); src/tests/testKokkos.cpp:4 and src/tests/testArrays.cpp:4 say .hpp (they are .cpp); src/utils/tests/testLookupTable.cpp:4 says src/utils/testLookupTable.hpp; src/utils/tests/data/gendata.py:4 still says test/utils/lookupTable/testmelib.py after the move/rename.
  • src/gtest-idfx.hpp:388: "GEST_TEST_" → "GTEST_TEST_"; doc comments say "patcher matcher" (×12) and "recieve" (×6); src/gtest-idfx.cpp says "environnement" (×3).
  • src/utils/tests/testLookupTable.cpp: leftover commented-out //idfx::cout debug lines (200, 222, 235, 249, 302, 332, 502, 509, 547, 569) — remove them, or keep deliberately as debugging aids.

@@ -16,6 +16,12 @@ function(replace_idefix_source _old_source _new_source)
get_target_property(fullsource idefix SOURCES)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since the idefix-core split, this query looks at the wrong target: idefix no longer holds the core sources (only main.cpp + problem setup.cpp). Any non-main.cpp replacement now hits the spurious SEND_ERROR "no source file match" below. Fix: move the _target_name computation (lines 19-23) above this line and use get_target_property(fullsource ${_target_name} SOURCES).

Comment thread cmake/HandleDeps.cmake
set(gtest_version "1.18.0")

# it not, download it internally to the project
cmake_policy(SET CMP0135 NEW)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cmake_minimum_required(VERSION 3.16) in the root CMakeLists.txt, but CMP0135 requires CMake >= 3.27 and hard-errors at configure time on older versions. Please guard with if(POLICY CMP0135) ... endif() or raise the minimum when Idefix_GTEST=ON.

Comment thread cmake/HandleDeps.cmake
######################################################################################

function(GoogleTestDownloadAndLink)
# CAUTION: if you change this verison, you should also check if you need to adapt

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo: "verison" -> "version". (Line 20: "# it not, download it internally" -> "# if not found on the system, download it internally".)

Comment thread scripts/ci/run-unit-tests
display_and_run mkdir -p build-unittests

# configure via cmake
display_and_run cmake . -B ./build-unittests -DIdefix_GTEST=ON -DIdefix_PROBLEM_DIR=$PWD/test/MHD/AxisFluxTube/ $(gen_cmake_options "$@") --fresh

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--fresh requires CMake >= 3.24, but the project declares cmake_minimum_required(VERSION 3.16). Consider dropping --fresh (the build dir is already created above) or documenting the requirement.

Comment thread CMakeLists.txt
option(Idefix_MPI "enable Message Passing Interface parallelisation" OFF)
option(Idefix_HIGH_ORDER_FARGO "Force Fargo to use a PPM reconstruction scheme" OFF)
option(Idefix_DEBUG "Enable Idefix debug features (makes the code very slow)" OFF)
option(Idefix_GTEST "Enable building unittests with GoogleTest framework)" OFF)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Stray ) at the end of the option help string: "Enable building unittests with GoogleTest framework)".

Comment thread CMakeLists.txt
if (Idefix_GTEST)
add_executable(unittests)
target_link_libraries(unittests GTest::gtest GTest::gmock)
add_test(unittests unittests)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Optional: modern signature add_test(NAME unittests COMMAND unittests) instead of the positional form.

Comment thread src/gtest-idfx.hpp
// ***********************************************************************************
// Idefix MHD astrophysical code
//
// Source file src/utils/gtest-idfx.hpp

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Header says Source file src/utils/gtest-idfx.hpp but the file lives in src/. Same in src/gtest-idfx.cpp:4. Also line 388: "GEST_TEST_" -> "GTEST_TEST_"; "patcher matcher"/"recieve" wording in the matcher doc comments.

int dataIdx[2];
csv1D.GetNeighboursHost(xq, xN, dataN);
csv1D.GetNeighboursIndxHost(xq, idx, dataIdx);
//idfx::cout << "1D: x=" << xN[0] << "," << xN[1] << " data=" << dataN[0] << "," << dataN[1]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Leftover commented-out //idfx::cout debug lines throughout this file (200, 222, 235, 249, 302, 332, 502, 509, 547, 569) - remove them or keep deliberately. Header line 4 also has a wrong path (src/utils/testLookupTable.hpp).

Comment thread cmake/ReplaceIdefixSource.cmake Resolved
Comment thread cmake/HandleDeps.cmake Resolved
Comment thread cmake/HandleDeps.cmake Resolved
Comment thread scripts/ci/run-unit-tests Resolved
Comment thread CMakeLists.txt Resolved
Comment thread CMakeLists.txt Resolved
Comment thread src/gtest-idfx.hpp Resolved
Comment thread src/utils/tests/testLookupTable.cpp Resolved
@glesur

glesur commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Duplicated review (made offline), so marked as resolved the second one :-)

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants