Uh oh!
There was an error while loading. Please reload this page.
Unit tests for every propagation and flux model, and -Wall -Wextra - #156
Merged
Merged
Conversation
The constructor left `properties` uninitialised. Models that register at least one property overwrite it in registerProperty, but the six that register none never do -- Iso, heatFluxBasic, factorChemFlux, CraterVaporFluxModel, LavaSO2Flux and vaporFluxBasic. Destroying one of those passed whatever the member happened to be built over to delete[]. The same destructor also freed fuelPropertiesTable with delete[], while DataBroker::extractFuelProperties allocates it with a scalar new. Neither has been observed in a run, because FireDomain keeps its models in propModelsTable and fluxModelsTable and frees neither -- so nothing destroys a model today. Both become reachable as soon as anything does, including a test.
…l -Wextra tests/runff runs one case through one propagation model (Rothermel, set in params.ff). Nothing was watching the other sixteen propagation models or any of the sixteen flux models. These tests exercise one model at a time, without a simulation. getSpeedForNode splits in two -- the DataBroker gathers properties into a double*, then the model does arithmetic on that array -- and only the second half is physics. ModelSandbox builds an empty FireDomain purely to instantiate models; Inputs fills their property array by property name rather than by index, so reordering a constructor's registerProperty calls cannot silently change what a test measures. Two kinds of assertion, and the README says which to trust. Invariants -- no spread without fuel, more wind never means less spread, a downslope is not an upslope, released energy is independent of how the time window is cut -- hold whatever the implementation is. Pinned rates of spread merely record what ForeFire produces today, at 1e-5 relative tolerance so that -march=native and FP contraction differences between runners do not trip them. Parameters are process-global, so the sandbox snapshots the parameter map and restores it on destruction; without that, three of five --order-by=rand seeds fail. Also compiles ForeFire's own sources with -Wall -Wextra. The warnings are not clean yet, so they are informational: FOREFIRE_WARNINGS_AS_ERRORS turns them fatal for whoever is clearing a file, and FOREFIRE_ENABLE_WARNINGS=OFF turns them off. NetCDF's headers are marked SYSTEM so their own warnings stay out of the output. The framework is doctest 2.5.3, vendored as a single header under third_party/ so a build needs no network. Nothing there is compiled into libforefireL. FOREFIRE_BUILD_TESTS defaults on, except for wheel builds.
Both workflows only triggered on master, so nothing integrating into dev was tested. main.yml already carried a commented-out dev push trigger.
The Linux workflow builds with `sudo bash ./install-forefire.sh -y`, so build/ is root-owned and ctest cannot create build/Testing/Temporary as the runner user. It exited 8 without running a single test. macOS is unaffected: install-forefire-osx.sh runs unprivileged.
This was referenced Aug 12, 2026
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 12, 2026
antonio-leblanc pushed a commit
that referenced
this pull request
Aug 13, 2026
The CHANGELOG landed on master carries only #154 and #155 under [Unreleased], because that is all master has. This adds the ten pull requests merged into dev since: the unit suite, the moisture invariants, the sanitizer job, the HTTP characterisation tests, the threading reproduction, and the shared-state and double-free fixes. Two things went stale on dev while that work landed, both of them ours. The README's CMake option table stopped at six options; #156 and #180 added FOREFIRE_BUILD_TESTS, FOREFIRE_ENABLE_WARNINGS, FOREFIRE_WARNINGS_AS_ERRORS and FOREFIRE_SANITIZE. And CONTRIBUTING told contributors to run `cd tests && bash run.bash`, which does not reach the C++ unit tests at all; ctest does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unit tests for every propagation and flux model, and
-Wall -WextraTargets
dev.tests/runffruns one case, through one propagation model —Rothermel, set inparams.ff. Nothing was watching the other sixteen propagation models or any of the sixteen flux models. This adds tests that exercise one model at a time, without running a simulation, and turns on warning flags.21 test cases, 2082 assertions, ~0.07s. No new dependency: the framework is vendored.
How a model gets tested
getSpeedForNodesplits in two — the DataBroker gathers properties out of the simulation into adouble*, then the model does arithmetic on that array. Only the second half is physics, and it needs nothing but the array.So
ModelSandboxbuilds an emptyFireDomainpurely to instantiate models, andInputsfills 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 measuring a different quantity.Two kinds of assertion, 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 hold whatever the implementation is, and they are the ones worth trusting.
Pinned values —
recorded rates of spread for the standard fuelrecords what ForeFire produces today, at 1e-5 relative tolerance so that-march=nativeand floating-point contraction differences between runners do not trip them. A pin moving means a 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 — someone who knows the literature should confirm them, and that is a good reason to look attest_propagation_models.cppclosely.Compiler warnings
ForeFire's own sources now compile with
-Wall -Wextra(/W4on MSVC). The warnings are not clean yet — about 430 on Linux/GCC — so they are informational rather than fatal. Two options:-DFOREFIRE_WARNINGS_AS_ERRORS=ON— fatal, for whoever is clearing a file.-DFOREFIRE_ENABLE_WARNINGS=OFF— quiet.NetCDF's headers are now included as
SYSTEM, which stopsnetCDF::file_id defined but not usedappearing in every translation unit.One fix in
src/, in its own commitForeFireModel's constructor leftpropertiesuninitialised. The six models that register no properties —Iso,heatFluxBasic,factorChemFlux,CraterVaporFluxModel,LavaSO2Flux,vaporFluxBasic— never overwrite it, so destroying one passed whatever the member happened to be built over todelete[]. The same destructor freedfuelPropertiesTablewithdelete[]whileDataBroker::extractFuelPropertiesallocates it with a scalarnew.Neither has ever been observed, for the reason in the next section.
Three things found while writing these, not fixed here
Models are never destroyed.
FireDomainkeeps them inpropModelsTableandfluxModelsTableand frees neither, so every model a simulation instantiates is leaked — and the code that would trip the bug above never runs.propertiesis deleted twice. Seventeen flux models and two propagation models delete it in their own destructor, and~ForeFireModeldeletes it again; most also use scalardeleteon anew[]allocation. Destroying any model that registers at least one property is a double free. The fix touches nineteen files and deserves its own PR — which is whytest_model_registry.cpponly destroys the models that register none.BalbiNov2011responds non-physically to live fuel moisture at the values in the shipped fuel table.xsiexceeds 1 for fuel 1 oftests/runff/fuels.csv, the flame-temperature term goes negative, andR00raises it to the fourth power. Rate of spread therefore falls with rising live moisture up to aboutMl = 0.8and then climbs again: 1.3e-3 m/s atMl = 0.5, 1.9e-8 atMl = 0.8, 8.2e-5 atMl = 1.0— which is the value the table ships. The monotonicity test stops short of that inversion rather than asserting it is correct. This one is worth a maintainer's eye; I did not want to change physics in a test PR.Test isolation
Parameters are process-global — every
SimulationParametersmethod reads and writesGetInstance()whatever instance it is called on — so a test settingIso.speed, orburningDurationto zero, would change the result of whichever test ran next.ModelSandboxsnapshots the parameter map on construction and restores it on destruction.That restore is load-bearing, not precautionary: with it removed, three of five
--order-by=randseeds fail.tests/unit/README.mdrecords the check.CI
Both native workflows gained a
cteststep. They also now trigger on pull requests todev, which they did not before —main.ymlalready carried a commented-outdevpush trigger.The macOS run is the more useful of the two here: it is what checks the pinned rates of spread survive a different compiler and a different CPU.
Vendoring
doctest 2.5.3, single header, unmodified, under
third_party/with a README recording version, source URL and licence. Nothing there is compiled intolibforefireL—CMakeLists.txtglobssrc/*.cpponly, and the directory is on the include path of the test target alone. A build and a CI run need no network beyond cloning.FOREFIRE_BUILD_TESTSdefaults on, except for wheel builds, which stay unaffected.Verification
ctest3/3.--order-by=name|suite|fileand five random seeds.tests/runffstill passes — KML and NetCDF both within tolerance.-DFOREFIRE_BUILD_TESTS=OFFand-DFOREFIRE_ENABLE_WARNINGS=OFFboth build clean.Adding a test
tests/unit/README.mdcovers it. Models needing an external resource cannot be covered here —ANNPropagationModelandBMapLoggerForANNTrainingread a.ffannnetwork in their constructor and abort when it is missing;tests/runANNcovers those.