Unit tests for every propagation and flux model, and -Wall -Wextra

.github/workflows/macos.yml

@@ -5,7 +5,7 @@ on:
     branches:
       - brew
   pull_request:
-    branches: [ "master" ]  
+    branches: [ "master", "dev" ]
   workflow_dispatch:
 
 jobs:
@@ -26,6 +26,11 @@ jobs:
       - name: Check ForeFire version
         run: ./bin/forefire -v
 
+      # Also the check that the recorded rates of spread survive a different
+      # compiler and a different CPU.
+      - name: Run C++ unit tests
+        run: ctest --test-dir build --output-on-failure
+
       - name: Install Python test dependencies
         run: pip3 install --break-system-packages lxml xarray netCDF4
 

.github/workflows/main.yml

@@ -4,9 +4,9 @@ on:
   push:
     branches:
       - "master"
-      # - "dev"
+      - "dev"
   pull_request:
-    branches: [ "master" ]
+    branches: [ "master", "dev" ]
   workflow_dispatch:
 
 jobs:
@@ -43,6 +43,18 @@ jobs:
       - name: Check ForeFire version
         run: ./bin/forefire -v
 
+      # The unit tests are built by install-forefire.sh along with everything
+      # else (FOREFIRE_BUILD_TESTS defaults on outside wheel builds), so this
+      # only has to run them. They cover the propagation and flux models one
+      # call at a time, which is what runff below cannot reach: it exercises a
+      # single case through Rothermel.
+      - name: Run C++ unit tests
+        run: |
+          # The build step above runs under sudo, so build/ is root-owned and
+          # ctest cannot create build/Testing/Temporary as the runner user.
+          sudo chown -R "$(id -u):$(id -g)" build
+          ctest --test-dir build --output-on-failure
+
       - name: Add Build/Runtime Diagnostics
         run: |
           echo "--- ForeFire Linkage ---"

CMakeLists.txt

@@ -31,6 +31,7 @@ if(DEFINED SKBUILD)
     set(_ff_default_static ON)
     set(_ff_default_tools OFF)
     set(_ff_default_lfs OFF)
+    set(_ff_default_tests OFF)
 else()
     set(_ff_wheel_build OFF)
     set(_ff_default_mpi ON)
@@ -39,6 +40,7 @@ else()
     set(_ff_default_static OFF)
     set(_ff_default_tools ON)
     set(_ff_default_lfs ON)
+    set(_ff_default_tests ON)
 endif()
 
 option(FOREFIRE_ENABLE_MPI "Enable MPI coupling when MPI is available" ${_ff_default_mpi})
@@ -47,6 +49,9 @@ option(FOREFIRE_BUILD_PYTHON "Build the pyforefire Python extension module" ${_f
 option(FOREFIRE_STATIC_CORE "Build the ForeFire core as a static library" ${_ff_default_static})
 option(FOREFIRE_BUILD_TOOLS "Build the ANN_test helper executable" ${_ff_default_tools})
 option(FOREFIRE_CHECK_LFS "Run the Git LFS data integrity check at configure time" ${_ff_default_lfs})
+option(FOREFIRE_BUILD_TESTS "Build the C++ unit tests, registered with CTest" ${_ff_default_tests})
+option(FOREFIRE_ENABLE_WARNINGS "Compile ForeFire's own sources with -Wall -Wextra" ON)
+option(FOREFIRE_WARNINGS_AS_ERRORS "Fail the build on a compiler warning" OFF)
 
 # ----------------------------------
 # Set C++ Standard
@@ -154,7 +159,7 @@ if(DEFINED ENV{SRC_MESONH} AND DEFINED ENV{XYZ} AND DEFINED ENV{FF_STATIC})
             message(FATAL_ERROR "Required static library not found: ${lib}")
         endif()
     endforeach()
-    target_include_directories(forefire_netcdf INTERFACE "${NETCDF_HOME}/include")
+    target_include_directories(forefire_netcdf SYSTEM INTERFACE "${NETCDF_HOME}/include")
     target_link_libraries(forefire_netcdf INTERFACE
         ${NETCDF_LIB}
         ${NETCDF_CXX_LIB}
@@ -200,7 +205,9 @@ else()
 
     message(STATUS "NetCDF C:   ${NETCDF_LIBRARY} (headers: ${NETCDF_INCLUDE_DIR})")
     message(STATUS "NetCDF C++: ${NETCDF_CXX_LIBRARY} (headers: ${NETCDF_CXX_INCLUDE_DIR})")
-    target_include_directories(forefire_netcdf INTERFACE ${NETCDF_CXX_INCLUDE_DIR} ${NETCDF_INCLUDE_DIR})
+    # SYSTEM: the NetCDF C++4 headers trip -Wunused-variable, and that is not a
+    # warning anyone here can act on.
+    target_include_directories(forefire_netcdf SYSTEM INTERFACE ${NETCDF_CXX_INCLUDE_DIR} ${NETCDF_INCLUDE_DIR})
     target_link_libraries(forefire_netcdf INTERFACE ${NETCDF_CXX_LIBRARY} ${NETCDF_LIBRARY})
 endif()
 
@@ -222,6 +229,25 @@ else()
     endif()
 endif()
 
+# Warnings are a target property rather than a global flag on purpose: they
+# should apply to code this project writes, not to pybind11's headers or to
+# whatever a consumer compiles against the installed library.
+set(_ff_warning_flags "")
+if(FOREFIRE_ENABLE_WARNINGS)
+    if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang|AppleClang")
+        list(APPEND _ff_warning_flags -Wall -Wextra)
+    elseif(MSVC)
+        list(APPEND _ff_warning_flags /W4)
+    endif()
+endif()
+if(FOREFIRE_WARNINGS_AS_ERRORS AND _ff_warning_flags)
+    if(MSVC)
+        list(APPEND _ff_warning_flags /WX)
+    else()
+        list(APPEND _ff_warning_flags -Werror)
+    endif()
+endif()
+
 # ----------------------------------
 # Output Directories
 # ----------------------------------
@@ -262,6 +288,9 @@ target_include_directories(forefireL PUBLIC
 # its consumers the way the shared library does.
 find_package(Threads REQUIRED)
 target_link_libraries(forefireL PUBLIC forefire_netcdf Threads::Threads)
+if(_ff_warning_flags)
+    target_compile_options(forefireL PRIVATE ${_ff_warning_flags})
+endif()
 
 # ----------------------------------
 # Test Executable
@@ -276,6 +305,17 @@ endif()
 # ----------------------------------
 add_executable(forefire app/forefire/ForeFire.cpp app/forefire/AdvancedLineEditor.cpp)
 target_link_libraries(forefire PRIVATE forefireL)
+if(_ff_warning_flags)
+    target_compile_options(forefire PRIVATE ${_ff_warning_flags})
+endif()
+
+# ----------------------------------
+# Unit Tests
+# ----------------------------------
+if(FOREFIRE_BUILD_TESTS)
+    enable_testing()
+    add_subdirectory(tests/unit)
+endif()
 
 # ----------------------------------
 # Python Extension Module

TESTING.md

@@ -14,6 +14,22 @@ Install Python libraries via pip:
 pip3 install lxml xarray netCDF4
 ```
 
+## Running the Unit Tests
+
+The C++ unit tests exercise propagation and flux models one call at a time,
+without running a simulation. They are built alongside everything else and run
+through CTest:
+
+```bash
+cmake -S . -B build && cmake --build build -j
+ctest --test-dir build --output-on-failure
+```
+
+They need no Python and no test data. Configure with
+`-DFOREFIRE_BUILD_TESTS=OFF` to skip building them; wheel builds already do.
+
+`tests/unit/README.md` describes what they cover and how to add one.
+
 ## Running the Core Test (`runff`)
 
 The primary automated test, validated in our CI pipeline, is located in `tests/runff/`. This test verifies core simulation, save/reload functionality, and NetCDF/KML output generation against reference files.
@@ -36,6 +52,20 @@ The `ff-run.bash` script:
 
 The `tests/` directory contains other subdirectories (`mnh_*`, `python`, `runANN`) for potentially testing specific features like coupled simulations or Python bindings. A main `tests/run.bash` script exists but is not currently fully validated in CI. Refer to specific subdirectories for details if needed.
 
+## Compiler Warnings
+
+ForeFire's own sources compile with `-Wall -Wextra` by default. The warnings
+are not yet clean, so they are informational rather than fatal; two options
+control this:
+
+*   `-DFOREFIRE_ENABLE_WARNINGS=OFF` — build quietly.
+*   `-DFOREFIRE_WARNINGS_AS_ERRORS=ON` — fail the build on any warning. Useful
+    on a subset of files while clearing them; not yet usable repository-wide.
+
+The flags apply to `libforefireL`, the `forefire` executable and the unit
+tests. NetCDF's headers are included as system headers so their warnings do
+not appear.
+
 ## Contributing
 
 Please see `CONTRIBUTING.md` for guidelines on contributing to ForeFire, including adding new tests. Report any issues via the repository's issue tracker.

src/ForeFireModel.cpp

@@ -17,11 +17,17 @@ ForeFireModel::ForeFireModel(const int & mindex, DataBroker* db)
 	numProperties = 0;
 	numFuelProperties = 0;
 	fuelPropertiesTable = 0;
+	// Only the models that register at least one property allocate this, so
+	// without an explicit null the destructor of a model that registers none
+	// deletes whatever the member happened to be built over. Iso and
+	// heatFluxBasic are both in that group.
+	properties = 0;
 }
 
 ForeFireModel::~ForeFireModel() {
 	if ( properties != 0 ) delete [] properties;
-	if ( fuelPropertiesTable != 0 ) delete [] fuelPropertiesTable;
+	// DataBroker::extractFuelProperties allocates this with a scalar new.
+	if ( fuelPropertiesTable != 0 ) delete fuelPropertiesTable;
 }
 
 void ForeFireModel::setDataBroker(DataBroker* db){

tests/unit/CMakeLists.txt

@@ -0,0 +1,31 @@
+# ----------------------------------
+# C++ unit tests
+# ----------------------------------
+# These link the core library and exercise single models in isolation, which
+# is the half of the code tests/runff cannot reach: it runs one case through
+# one propagation model, so a regression in any of the other models is
+# invisible to it.
+
+add_executable(forefire_unit_tests
+    main.cpp
+    model_sandbox.cpp
+    test_flux_models.cpp
+    test_model_registry.cpp
+    test_propagation_models.cpp)
+
+target_link_libraries(forefire_unit_tests PRIVATE forefireL)
+target_include_directories(forefire_unit_tests PRIVATE
+    "${CMAKE_CURRENT_SOURCE_DIR}"
+    "${PROJECT_SOURCE_DIR}/third_party")
+
+if(_ff_warning_flags)
+    target_compile_options(forefire_unit_tests PRIVATE ${_ff_warning_flags})
+endif()
+
+# doctest groups its cases into suites; registering one CTest entry per suite
+# keeps `ctest` output useful without needing doctest's CMake integration.
+foreach(_suite "model registry" "propagation models" "flux models")
+    string(REPLACE " " "_" _suite_id "${_suite}")
+    add_test(NAME "unit.${_suite_id}"
+             COMMAND forefire_unit_tests --test-suite=${_suite} --no-skipped-summary)
+endforeach()

tests/unit/README.md

@@ -0,0 +1,120 @@
+# Unit tests
+
+C++ tests that exercise one model at a time, without running a simulation.
+
+`tests/runff` runs a whole case and diffs the output against reference files.
+That catches physics drift along the path it takes — but it takes one path,
+through one propagation model (`Rothermel`, set in `params.ff`). Nothing was
+watching the other eighteen propagation models or any of the sixteen flux
+models. These tests are for them.
+
+## Running them
+
+They build with everything else, and are registered with CTest:
+
+```bash
+cmake -S . -B build && cmake --build build -j
+ctest --test-dir build --output-on-failure
+```
+
+Or run the binary directly, which gives finer control:
+
+```bash
+./bin/forefire_unit_tests                          # everything
+./bin/forefire_unit_tests --test-suite="flux models"
+./bin/forefire_unit_tests --test-case="*Rothermel*"
+./bin/forefire_unit_tests --list-test-cases
+```
+
+`-DFOREFIRE_BUILD_TESTS=OFF` skips building them. Wheel builds default to off.
+
+The framework is [doctest](https://github.com/doctest/doctest) 2.5.3, vendored
+as a single header in `third_party/doctest/`. See that directory's README.
+
+## How a model gets tested
+
+`getSpeedForNode` splits in two: the DataBroker gathers properties out of the
+simulation into a `double*`, and then the model does arithmetic on that array.
+Only the second half is physics, and it needs nothing but the array — so
+`ModelSandbox` builds an empty `FireDomain` purely to instantiate models, and
+`Inputs` fills their property array by property *name* rather than by index.
+
+Addressing by name matters. A model's properties are numbered in the order its
+constructor calls `registerProperty`, so a test written against raw indices
+would keep passing after someone reorders that constructor, while silently
+testing a different quantity.
+
+Parameters are process-global — every `SimulationParameters` method reads and
+writes `GetInstance()` whatever instance it is called on — so a test setting
+`Iso.speed`, or `burningDuration` to zero, would otherwise change the result of
+whichever test ran next. `ModelSandbox` snapshots the parameter map on
+construction and puts it back on destruction.
+
+That restore is load-bearing rather than precautionary: with it removed, three
+of five `--order-by=rand` seeds fail. Running the suite under a few seeds is a
+cheap way to check it still holds:
+
+```bash
+./bin/forefire_unit_tests --order-by=rand --rand-seed=1337
+```
+
+## What the assertions mean
+
+Two kinds, and they are not equally trustworthy.
+
+**Invariants** — no spread without fuel, more wind never means less spread,
+a downslope is not an upslope, total released energy does not depend on how
+the time window is cut. These should hold whatever the implementation is, and
+they are the ones worth trusting.
+
+**Pinned values** — `recorded rates of spread for the standard fuel` is a
+record of what ForeFire produces today, at a 1e-5 relative tolerance so that
+`-march=native` and floating-point contraction differences between CI runners
+do not trip it. A pin moving means the model changed; that may well be
+intended, but it should be a decision rather than a surprise. The pins carry
+no claim of matching published values.
+
+## Things found while writing these, and not fixed here
+
+Two of them are why `test_model_registry.cpp` only destroys the models that
+register no properties.
+
+**Models are never destroyed in a normal run.** `FireDomain` keeps them in
+`propModelsTable` and `fluxModelsTable` and frees neither, so every model a
+simulation instantiates is leaked. That is why the two problems below have
+never been observed: the code that would trip them does not run.
+
+**The `properties` array is deleted twice.** Seventeen flux models and two
+propagation models delete `properties` in their own destructor, and
+`~ForeFireModel` deletes it again. Most of them also use scalar `delete` on an
+array allocated with `new[]`. So destroying any model that registers at least
+one property is a double free. Fixing it means removing the `delete` from each
+derived destructor and leaving it to the base class — nineteen files, worth
+doing as its own change.
+
+`~ForeFireModel` itself was fixed while writing these tests: it left
+`properties` uninitialised, so destroying a model that registers *no*
+properties — `Iso`, `heatFluxBasic` — deleted whatever the member happened to
+be built over. It also deleted `fuelPropertiesTable`, allocated with a scalar
+`new`, with `delete[]`.
+
+**`BalbiNov2011` responds non-physically to live fuel moisture at the values
+in the shipped fuel table.** `xsi` exceeds 1 for fuel 1 of
+`tests/runff/fuels.csv`, the flame temperature term goes negative, and `R00`
+raises it to the fourth power. Rate of spread therefore falls with rising live
+moisture up to about `Ml = 0.8` and then climbs again: 1.3e-3 m/s at
+`Ml = 0.5`, 1.9e-8 at `Ml = 0.8`, 8.2e-5 at `Ml = 1.0` — which is the value the
+table ships. `drier live fuel spreads faster, over the physical range` stops
+short of that inversion rather than asserting it is correct.
+
+## Adding a test
+
+Models needing an external resource cannot be covered here: `ANNPropagationModel`
+and `BMapLoggerForANNTraining` read a `.ffann` network in their constructor and
+abort when it is missing. `tests/runANN` covers those.
+
+For anything else, add its name to the list in `test_model_registry.cpp`. If
+`fillStandardConditions` knows every property it reads, it can also join
+`standardFuelModels()` in `test_propagation_models.cpp` and inherit the
+sweeps; otherwise teach `standardValues()` in `model_sandbox.cpp` the missing
+properties first.

tests/unit/main.cpp

@@ -0,0 +1,12 @@
+/**
+ * @file main.cpp
+ * @brief Entry point for the ForeFire unit tests.
+ * @copyright Copyright (C) 2025 ForeFire, Fire Team, SPE, CNRS/Universita di Corsica.
+ * @license This program is free software; See LICENSE file for details. (See LICENSE file).
+ *
+ * This translation unit exists only to generate doctest's main(). Keeping it
+ * on its own means the header is expanded once instead of in every test file.
+ */
+
+#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN
+#include "doctest/doctest.h"