From a6855d6f0ecd7239a85a3d7d2ee2a5c959953059 Mon Sep 17 00:00:00 2001 From: Nick Newson Date: Thu, 27 Aug 2026 22:31:35 +0100 Subject: [PATCH] Run the release half of the suite, and keep it running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rejection tests guarded by `#ifdef NDEBUG` — the release behaviour of every writer that asserts in Dev and returns `false` under NDEBUG — have never run. Every preset here builds Dev, so those bodies compiled to nothing on every machine and in every job that has ever checked them: the cases ran, passed, and asserted precisely nothing. Death tests are not the fix; they prove the Dev assertion fires and say nothing about the half that ships. Tag those cases `[release-contract]` (18 of them) and add a Linux-only CI job that builds a genuine Release tree and runs that selection. Linux alone because the behaviour does not vary by platform, and the tagged cases alone because the whole Release suite would drag in the optimisation-sensitive physics goldens. Two mechanisms keep the job honest, since a job that checks nothing looks exactly like a job that finds no fault. Catch2 exits non-zero when a spec matches nothing, so a renamed tag fails rather than passing quietly; and tests/release_contract.cpp holds a hidden ([.]) sentinel case inside the same selection that fails unless NDEBUG is defined, so a Dev build cannot pass by mistake. Hidden is what keeps it out of ctest and tests-full, which are default runs. And the tag itself is now enforced: release_contract_guard fails on a conditional case that is untagged (no check at all, and silent), on a tagged case with no conditional, and if the sentinel or the last tag disappears. Without it the convention lived only in prose, which is the same gap one layer up. Nine mutations tested. The vcpkg-release preset builds into build-release/ while pointing VCPKG_INSTALLED_DIR back at build/vcpkg_installed, so the job reuses the ports the Dev tree and the CI cache already hold. The job sources tools/ci/ci-stages.sh rather than restating its commands, and run-local-ci.sh gains the stage. --- .github/workflows/ci.yml | 56 ++++++++ CLAUDE.md | 49 +++++-- CMakeLists.txt | 15 +++ CMakePresets.json | 18 +++ README.md | 15 ++- cmake/check_release_contract.cmake | 140 ++++++++++++++++++++ docs/onboarding.md | 25 ++++ docs/review-order.md | 5 +- docs/roadmap.md | 31 ----- tests/graphics/test_shadow_diagnostics.cpp | 4 +- tests/graphics/test_shadow_lod_resolver.cpp | 10 +- tests/graphics/test_shadow_render_view.cpp | 29 ++-- tests/graphics/test_vdpm.cpp | 2 +- tests/release_contract.cpp | 41 ++++++ tools/ci/ci-stages.sh | 41 +++++- tools/ci/container-run.sh | 6 + tools/ci/run-local-ci.sh | 14 +- tools/ci/run-local-macos.sh | 5 +- 18 files changed, 437 insertions(+), 69 deletions(-) create mode 100644 cmake/check_release_contract.cmake create mode 100644 tests/release_contract.cpp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cc40b203..173674b2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -77,6 +77,62 @@ jobs: - name: clang-tidy run: cmake --build build --target run-clang-tidy + # -------- Release contract (Linux only) -------- + # + # Every other job builds `Dev`, so the suite's `#ifdef NDEBUG` bodies compile to nothing: the + # cases run, pass, and assert precisely nothing. This job is the only place they are real code. + # Linux alone, because what they assert — what a writer returns once NDEBUG removes its + # assertion — does not vary by platform. + # + # It runs the SHARED stage body rather than restating the commands, so the Docker replica + # (`tools/ci/run-local-ci.sh release-contract`) and this job cannot drift. + + release-contract: + name: release contract (Linux) + runs-on: ubuntu-latest + env: + VCPKG_ROOT: ${{ github.workspace }}/vcpkg + steps: + - uses: actions/checkout@v4 + + - name: Install system packages + run: | + sudo apt-get update + # See the clang-tidy job above for why autotools are here: glfw3 3.5.1's `pthread-stubs` + # dependency is autotools-only on Linux. + sudo apt-get install -y ninja-build cmake libvulkan-dev \ + xorg-dev libxinerama-dev libxcursor-dev libglu1-mesa-dev pkg-config \ + autoconf autoconf-archive automake libtool + + - name: Checkout vcpkg + uses: actions/checkout@v4 + with: + repository: microsoft/vcpkg + path: vcpkg + + - name: Bootstrap vcpkg + run: ./vcpkg/bootstrap-vcpkg.sh + + # The same cache path as the Dev jobs, which is the point: the vcpkg-release preset points + # VCPKG_INSTALLED_DIR back at build/vcpkg_installed, so this job reuses those ports instead + # of building a second copy of every one of them under build-release/. + - name: Cache vcpkg packages + uses: actions/cache@v4 + with: + path: | + build/vcpkg_installed + vcpkg/downloads + vcpkg/buildtrees + vcpkg/packages + key: ${{ runner.os }}-vcpkg-${{ hashFiles('vcpkg.json', 'vcpkg-configuration.json') }} + restore-keys: | + ${{ runner.os }}-vcpkg- + + - name: Release contract + run: | + . tools/ci/ci-stages.sh + ci_release_contract + # -------- Per-platform build + test (each validates its own determinism golden) -------- build-test-linux: diff --git a/CLAUDE.md b/CLAUDE.md index d2e50abe..d0d70104 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -177,23 +177,48 @@ Reference class: `include/fire_engine/graphics/image.hpp`. Catch2 (v3, `Catch2::Catch2WithMain`). Single binary `test_fire_engine`. CTest registers `test_fire_engine` as the fast `~[slow]` entry, so plain `ctest` and `ctest --preset fast` -stay fast. The `tests-full` target runs the all-tags Catch2 binary plus the four build-time guards — -graphics-layer includes, shared shader blocks, the shadow bias law, and the shared GPU limits; from -the source root use `cmake --build --preset full`. Test files mirror +stay fast. The `tests-full` target runs the all-tags Catch2 binary plus the six build-time guards — +graphics-layer includes, shared shader blocks, the shadow bias law, the shared GPU limits, the +per-view shadow matrix, and the `[release-contract]` tags; from the source root use +`cmake --build --preset full`. Test files mirror source paths. Shared helpers/traits live in `tests/support/`. Test assets in `tests/assets/` → copied to `build/test_assets/`. Graphics-layer tests run without a GPU (opaque handles). -**CI has four parallel jobs** (GitHub Actions, all `FIRE_ENGINE_WARNINGS_AS_ERRORS=ON`): `clang-format` -+ `clang-tidy` (platform-independent lint, run once on Ubuntu) and `build-test-linux` + -`build-test-macos` (build + `tests-full` on Ubuntu and macOS/arm64). Each build job validates *its* -platform's determinism golden. The build/test/lint stage bodies are shared across the Docker replica -and the native macOS replica via `tools/ci/ci-stages.sh` — edit stages there, not in each script. - -**Local CI parity.** `tools/ci/run-local-ci.sh [format|configure|build|tidy|test|all|shell]` -reproduces the **Linux** checks (Ubuntu 24.04) in Docker; `tools/ci/run-local-macos.sh -[format|configure|build|tidy|test|all]` runs the same stages **natively on macOS** (no container — +**CI has five parallel jobs** (GitHub Actions, all `FIRE_ENGINE_WARNINGS_AS_ERRORS=ON`): `clang-format` ++ `clang-tidy` (platform-independent lint, run once on Ubuntu), `build-test-linux` + +`build-test-macos` (build + `tests-full` on Ubuntu and macOS/arm64), and `release-contract` (below). +Each build job validates *its* platform's determinism golden. The build/test/lint stage bodies are +shared across the Docker replica and the native macOS replica via `tools/ci/ci-stages.sh` — edit +stages there, not in each script, and the GitHub `release-contract` job sources that file rather +than restating its commands. + +**A test whose body is `#ifdef NDEBUG` is tagged `[release-contract]`.** Every preset here builds +`Dev`, so those bodies — the release half of a writer that asserts in Dev and returns `false` under +`NDEBUG` — compile to nothing: the cases run, pass, and assert nothing at all. The Linux-only +`release-contract` job is the only place they are real code (`cmake --preset vcpkg-release`, build +`test_fire_engine`, run `test_fire_engine "[release-contract]"`; locally, +`tools/ci/run-local-ci.sh release-contract`). Two things keep the job honest, and both must survive +any edit to it: Catch2 exits non-zero when a spec matches nothing, so a renamed tag fails rather +than passing quietly; and `tests/release_contract.cpp` holds a HIDDEN (`[.]`) sentinel case in the +same selection that fails unless `NDEBUG` is defined, so the job cannot pass by building `Dev` by +mistake. The `[.]` is what keeps it out of the Dev suites — `ctest` (`~[slow]`) and `tests-full` (no +filter) are both default runs, which exclude hidden cases, while an explicit tag selection includes +them. **Only tag a case that actually has an `#ifdef NDEBUG` in it**: the tag means "this asserts +something a Dev build cannot see". Both halves of that rule are enforced by the +`release_contract_guard` CTest case (`cmake/check_release_contract.cmake`), which fails on a +conditional case that is untagged — the silent failure, since nothing else would ever run it as real +code — on a tagged case with no conditional, on a missing sentinel, and on the tag vanishing +altogether; so the convention no longer depends on the next person knowing it. The `vcpkg-release` +preset builds into `build-release/` and points `VCPKG_INSTALLED_DIR` back at +`build/vcpkg_installed`, so it reuses the ports the Dev tree and the CI cache already hold instead +of building a second copy of every one. + +**Local CI parity.** `tools/ci/run-local-ci.sh [format|configure|build|tidy|test|release-contract|all|shell]` +reproduces the **Linux** checks (Ubuntu 24.04) in Docker — its `all` includes the Release contract; +`tools/ci/run-local-macos.sh [format|configure|build|tidy|test|all]` runs the same stages +**natively on macOS**, minus that one (it is platform-independent, so one job proves it) (no container — uses your existing vcpkg + C++ toolchain, installs nothing; Vulkan/GLFW/glslc all come from vcpkg). The Docker runner copies the working tree into volumes (host artifacts untouched), defaults to `linux/amd64` to match CI (`DOCKER_PLATFORM=linux/arm64` is faster but off-platform). Run the relevant one before committing diff --git a/CMakeLists.txt b/CMakeLists.txt index 3b4082c9..42ae2927 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -384,6 +384,9 @@ endif() enable_testing() add_executable(test_fire_engine + # The build-configuration sentinel for the [release-contract] gate — it mirrors no source + # file, because its subject is how this binary was compiled. + tests/release_contract.cpp tests/collision/test_aabb.cpp tests/collision/test_geometry.cpp tests/collision/test_sweep_and_prune_broad_phase.cpp @@ -527,6 +530,7 @@ add_custom_target(tests-full COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R shadow_bias_guard COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R gpu_limits_guard COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R shadow_matrix_guard + COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R release_contract_guard DEPENDS test_fire_engine COMMENT "Running the full test suite, including [slow] tests" ) @@ -582,6 +586,17 @@ add_test( -P ${PROJECT_SOURCE_DIR}/cmake/check_shadow_matrix.cmake ) +# Every NDEBUG-conditional test case carries `[release-contract]`, which is the only thing that +# selects it. An untagged one is not a weaker check but no check at all — every preset here builds +# Dev, so its guarded body compiles to nothing and it passes each suite while asserting nothing — +# and forgetting the tag is silent by construction, which is the gap this closes. +add_test( + NAME release_contract_guard + COMMAND ${CMAKE_COMMAND} + -DTESTS_DIR=${PROJECT_SOURCE_DIR}/tests + -P ${PROJECT_SOURCE_DIR}/cmake/check_release_contract.cmake +) + find_program(CLANG_TIDY_EXE NAMES clang-tidy) if(CLANG_TIDY_EXE) get_target_property(FIRE_ENGINE_TIDY_SOURCES fireengine SOURCES) diff --git a/CMakePresets.json b/CMakePresets.json index 3984f36f..fd7313f8 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -10,6 +10,17 @@ "CMAKE_EXPORT_COMPILE_COMMANDS": "ON", "CMAKE_TOOLCHAIN_FILE": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake" } + }, + { + "name": "vcpkg-release", + "inherits": "vcpkg", + "displayName": "Release — the [release-contract] gate", + "description": "A genuine Release build (NDEBUG defined), which is the only configuration in which the suite's #ifdef NDEBUG bodies are real code. VCPKG_INSTALLED_DIR deliberately points back at build/vcpkg_installed: vcpkg would otherwise install a second copy of every port under build-release/, which no CI cache populates and which takes as long as the ports themselves. Its own binaryDir so it never disturbs the Dev tree that ctest, tests-full and compile_commands.json all live in.", + "binaryDir": "${sourceDir}/build-release", + "cacheVariables": { + "CMAKE_BUILD_TYPE": "Release", + "VCPKG_INSTALLED_DIR": "${sourceDir}/build/vcpkg_installed" + } } ], "buildPresets": [ @@ -23,6 +34,13 @@ "targets": [ "tests-full" ] + }, + { + "name": "release-contract", + "configurePreset": "vcpkg-release", + "targets": [ + "test_fire_engine" + ] } ], "testPresets": [ diff --git a/README.md b/README.md index e1e9048c..f25248f4 100644 --- a/README.md +++ b/README.md @@ -310,7 +310,9 @@ Or from the build directory: ctest --test-dir build --output-on-failure ``` -Run the full Catch2 suite, including `[slow]` tests, plus the graphics-layer include guard: +Run the full Catch2 suite, including `[slow]` tests, plus the build-time guards (graphics-layer +includes, shared shader blocks, the shadow bias law, the shared GPU limits, the per-view shadow +matrix, and the `[release-contract]` tags): ```bash cmake --build --preset full @@ -340,13 +342,19 @@ cmake --build build --target run-clang-tidy # if clang-tidy is installed `cmake --build build --target run-clang-tidy --parallel ` or `CMAKE_BUILD_PARALLEL_LEVEL` to cap local CPU/memory use. -CI (GitHub Actions, all `FIRE_ENGINE_WARNINGS_AS_ERRORS=ON`) runs four parallel jobs: +CI (GitHub Actions, all `FIRE_ENGINE_WARNINGS_AS_ERRORS=ON`) runs five parallel jobs: - **`clang-format`** and **`clang-tidy`** — platform-independent lint gates, run once (Ubuntu). - **`build-test-linux`** — build + `tests-full` on Ubuntu (validates the **Linux/x86_64** determinism golden). - **`build-test-macos`** — build + `tests-full` on macOS/arm64 (validates the **macOS/arm64** golden). Like Linux, it gets Vulkan + GLFW + `glslc` from vcpkg — the runner only adds `ninja`. +- **`release-contract`** — the only job that builds `Release`. Every other job builds `Dev`, so the + suite's `#ifdef NDEBUG` bodies (what a writer returns once its assertion is compiled away) + vanish; this one builds `test_fire_engine` from the `vcpkg-release` preset and runs + `test_fire_engine "[release-contract]"`. Ubuntu only — that behaviour does not vary by platform — + and deliberately not the whole Release suite, which would drag in the optimisation-sensitive + physics goldens. Each platform's `Determinism.GoldenHash` golden is now enforced by its own job — see [`docs/collision.md`](docs/collision.md) and CLAUDE.md § Testing. @@ -360,7 +368,8 @@ tools/ci/run-local-macos.sh all # macOS, native (no container) The **Docker** runner copies the working tree into an Ubuntu 24.04 container, keeps Linux build/vcpkg state in Docker volumes, and accepts `format`, `configure`, `build`, `tidy`, `test`, -`all`, or `shell` to isolate a stage. It defaults to `linux/amd64` to match GitHub Actions; set +`release-contract`, `all`, or `shell` to isolate a stage (`all` includes the Release contract on +Linux; the macOS runner has no such stage, by design). It defaults to `linux/amd64` to match GitHub Actions; set `DOCKER_PLATFORM=linux/arm64` for a faster native Apple Silicon check. The **native macOS** runner takes the same stages and runs them directly on your host toolchain (it installs nothing — Vulkan, GLFW, and `glslc` all come from vcpkg, so it just needs your existing vcpkg + compiler + `ninja`). diff --git a/cmake/check_release_contract.cmake b/cmake/check_release_contract.cmake new file mode 100644 index 00000000..d978155d --- /dev/null +++ b/cmake/check_release_contract.cmake @@ -0,0 +1,140 @@ +# Guard: a test case whose body is conditional on NDEBUG carries the `[release-contract]` tag, and +# nothing else does. +# +# The tag is what the Linux `release-contract` job selects, and it is the ONLY thing that selects +# it. An untagged `#ifdef NDEBUG` case is therefore not a weaker check, it is no check at all: every +# preset here builds `Dev`, so its guarded body compiles to nothing in every suite that runs it, and +# the case passes everywhere while asserting nothing anywhere. That is precisely the failure the tag +# was introduced to end, and it comes back silently the first time someone adds a conditional case +# without knowing the convention — which is the normal case, since the convention is invisible at +# the point where it matters. +# +# The other direction is checked too, and is not merely tidiness. `[release-contract]` means "this +# case asserts something a Dev build cannot see". A case carrying the tag without a conditional body +# runs identically in both configurations, so it pads the job's selection with work every other +# suite already covers and makes a green Release run look broader than it is. +# +# COMMENTS ARE STRIPPED FIRST, both forms. This file's subject is `#ifdef NDEBUG`, and so is the +# prose of nearly every case it guards: an unstripped scan is satisfied by a case that merely talks +# about release behaviour and — worse — by one whose real conditional has been commented out. +# +# Invoked as a CTest case; needs TESTS_DIR. + +if(NOT DEFINED TESTS_DIR) + message(FATAL_ERROR "TESTS_DIR must be set (path to tests/)") +endif() + +include("${CMAKE_CURRENT_LIST_DIR}/strip_glsl_comments.cmake") + +set(offenders "") +set(tag "[release-contract]") +set(sentinel_file "release_contract.cpp") +set(sentinel_seen FALSE) +set(tagged_count 0) + +file(GLOB_RECURSE test_sources "${TESTS_DIR}/*.cpp") +if(NOT test_sources) + message(FATAL_ERROR + "release contract guard: no .cpp files under ${TESTS_DIR} — the sweep found nothing to check, which is not a pass") +endif() + +foreach(source IN LISTS test_sources) + file(RELATIVE_PATH relative "${TESTS_DIR}" "${source}") + file(READ "${source}" raw) + strip_glsl_comments("${raw}" code) + + # Split the file into cases. CMake regexes cannot report a match POSITION, so the macro name + # becomes the list separator and the text is split on it: element 1 is whatever precedes the first + # case (includes, helpers) and is dropped; every later element is one case — its argument list + # followed by its body, up to the start of the next case. + # + # Semicolons and backslashes go first, and both matter: `;` is CMake's list separator, so a case + # containing one would be scanned in fragments; a trailing `\` escapes the separator itself. + # Neither character can join two identifiers, so replacing them with a space cannot manufacture or + # destroy a match below. + string(REPLACE "\\" " " scan "${code}") + string(REPLACE ";" " " scan "${scan}") + string(REPLACE "TEST_CASE(" ";" scan "${scan}") + + set(index 0) + foreach(case IN LISTS scan) + math(EXPR index "${index} + 1") + if(index EQUAL 1) + continue() + endif() + + # Argument list and body are separated at the first line that STARTS with `{`, not at the first + # `)`: 36 case names in this suite contain a parenthesis and one contains a brace, so a + # punctuation-based split would read part of a name as body (and then miss its tag). Allman + # braces make the line-initial `{` reliable, and they are themselves CI-gated by clang-format. + string(FIND "${case}" "\n{" brace) + if(brace LESS 0) + list(APPEND offenders + "${relative}: a TEST_CASE( has no body brace on its own line — the guard cannot tell its tags from its body") + continue() + endif() + string(SUBSTRING "${case}" 0 ${brace} arguments) + string(LENGTH "${case}" case_length) + math(EXPR body_start "${brace} + 2") + math(EXPR body_length "${case_length} - ${body_start}") + string(SUBSTRING "${case}" ${body_start} ${body_length} body) + + # The case's NAME, for the report — the first string literal, by Catch2's signature. + set(name "") + if(arguments MATCHES "\"([^\"]*)\"") + set(name "${CMAKE_MATCH_1}") + endif() + + string(FIND "${arguments}" "${tag}" tag_position) + if(tag_position GREATER_EQUAL 0) + set(tagged TRUE) + math(EXPR tagged_count "${tagged_count} + 1") + else() + set(tagged FALSE) + endif() + + # A preprocessor conditional ON NDEBUG, in any spelling. `#if !defined(NDEBUG)` counts: what + # matters is that the case's behaviour differs between the two configurations, not which half is + # which. + if(body MATCHES "#[ \t]*(ifdef|ifndef|if)[^\n]*NDEBUG") + set(conditional TRUE) + else() + set(conditional FALSE) + endif() + + if(conditional AND NOT tagged) + list(APPEND offenders + "${relative}: \"${name}\" has an NDEBUG-conditional body but is not tagged ${tag} — nothing builds it as real code, so it asserts nothing in any configuration") + elseif(tagged AND NOT conditional) + list(APPEND offenders + "${relative}: \"${name}\" is tagged ${tag} but has no NDEBUG conditional — the tag means 'this asserts something a Dev build cannot see', and every other suite already covers this case") + endif() + + # The sentinel: hidden, tagged, conditional. Its `[.]` is what keeps it out of the Dev suites, + # and removing the `[.]` fails them loudly — but removing the CASE is silent, and leaves the job + # unable to tell a Release build from a Dev one again. + if(relative STREQUAL "${sentinel_file}" AND tagged AND conditional AND arguments MATCHES "\\[\\.\\]") + set(sentinel_seen TRUE) + endif() + endforeach() +endforeach() + +if(NOT sentinel_seen) + list(APPEND offenders + "tests/${sentinel_file} has no hidden ([.]) ${tag} case with an NDEBUG conditional — that case is what fails the job when the binary was not built with NDEBUG, and without it a Dev build passes the selection silently") +endif() + +# A selection of nothing is not a green run. Catch2 catches that at job time (it exits non-zero when +# a spec matches no tests), but this fails in the build that removed the last tag, where the cause is +# still in front of whoever caused it. +if(tagged_count EQUAL 0) + list(APPEND offenders "no test case carries ${tag} — the Linux release-contract job now selects nothing") +endif() + +if(offenders) + string(REPLACE ";" "\n " report "${offenders}") + message(FATAL_ERROR "release contract guard failed:\n ${report}") +endif() + +message(STATUS + "release contract guard: ${tagged_count} case(s) tagged ${tag}, each with an NDEBUG-conditional body, plus the hidden sentinel") diff --git a/docs/onboarding.md b/docs/onboarding.md index dc1d8555..d6387e3d 100644 --- a/docs/onboarding.md +++ b/docs/onboarding.md @@ -1357,6 +1357,31 @@ cmake --build --preset full # all Catch2 tests + layering gua cmake --build build --target tests-full # build-dir equivalent ``` +**Some tests do not run in any of the above, and that is deliberate.** A case whose body is +`#ifdef NDEBUG` asserts RELEASE behaviour — what a writer returns once `NDEBUG` has compiled its +assertion away, which is the half that ships. Every preset here builds `Dev`, so those bodies are +empty: the cases run, pass, and assert nothing. They are tagged `[release-contract]` and run by one +Linux CI job, reproducible locally as: + +```bash +cmake --preset vcpkg-release # Release into build-release/, shared vcpkg tree +cmake --build --preset release-contract # test_fire_engine only +./build-release/test_fire_engine "[release-contract]" +tools/ci/run-local-ci.sh release-contract # or the whole thing in Docker +``` + +The selection contains a HIDDEN sentinel case (`tests/release_contract.cpp`) that fails unless +`NDEBUG` is defined, so running the tag against a Dev build reports the mistake instead of passing +vacuously. Hidden (`[.]`) is also why the sentinel never fires in the Dev suites above: they are +default runs, which exclude hidden cases. + +When you add a case with an `#ifdef NDEBUG` body, tag it — and `release_contract_guard` (part of +`tests-full`) fails if you don't. An untagged conditional case is not a weaker check but no check at +all: nothing selects it in Release, so its guarded body never becomes real code anywhere, and it +passes every suite while asserting nothing. The guard also rejects the tag on a case with no +conditional (it would run identically in both configurations, padding the job's selection with work +the Dev suites already do), and fails if the sentinel or the tag itself disappears. + Graphics-layer tests run headless because the layer only stores opaque handles — keep it that way so the suite stays GPU-free. Test files mirror their source path (`src/foo/bar.cpp` → `tests/foo/test_bar.cpp`). Shared Catch2 helpers and compile-time test traits live in diff --git a/docs/review-order.md b/docs/review-order.md index ee6c2105..8192b832 100644 --- a/docs/review-order.md +++ b/docs/review-order.md @@ -17,9 +17,9 @@ Read these first when a change touches build configuration, CI, or local tooling | File | Pay attention to | |---|---| | `CMakePresets.json` | The `vcpkg` preset pins Apple Clang on macOS, selects `Dev`, and exports `compile_commands.json` for `clangd`. | -| `CMakeLists.txt` | `Dev` is the default build type (`-O2 -g`, no `NDEBUG`); `FIRE_ENGINE_WARNINGS_AS_ERRORS` is CI-only by default; CTest registers `test_fire_engine` (`~[slow]`) so plain CTest stays fast; `tests-full` runs the all-tags Catch2 binary plus the layering, shared-shader-block, shadow-bias and GPU-limits guards; `shaders/` is on the C++ include path (`BUILD_INTERFACE`, PUBLIC) because the public header `graphics/gpu_limits.hpp` includes `shaders/gpu_limits.glsl` — the dual-language limits file; `run-clang-tidy` appears only when `clang-tidy` is installed. **Read the include-order block near the top before touching dependency includes**: `CMAKE_NO_SYSTEM_FROM_IMPORTED` + `include_directories(BEFORE …)` force vcpkg's headers to arrive as `-I`, because clang searches `/usr/local/include` ahead of every `-isystem` path — with a Vulkan SDK installed there, our TUs compiled against ITS vulkan-hpp while includes resolved relative to a vcpkg header got vcpkg's, and the first RAII call aborted on a header-version assert with no bad C++ behind it (real: SDK 1.4.357 beside the pinned 1.4.335). The guard checks both `VCPKG_INSTALLED_DIR` spellings and hard-errors if the directory is missing, since a silent miss restores the mixed-header build. `SHADER_INCLUDES` lists the shared GLSL includes (`gpu_limits.glsl`, `light_ubo.glsl`, `material.glsl`, `shadow_push.glsl`, `shadow_depth.glsl`, `self_shadow_second.glsl`) so editing one rebuilds every shader — a stale `.spv` against a changed block is the layout bug the include exists to prevent. Consequence: third-party headers are no longer warning-suppressed, so a finding gets a narrow `#pragma` at the include site (`src/graphics/frame_capture.cpp`), never a weakened flag. | +| `CMakeLists.txt` | `Dev` is the default build type (`-O2 -g`, no `NDEBUG`); `FIRE_ENGINE_WARNINGS_AS_ERRORS` is CI-only by default; CTest registers `test_fire_engine` (`~[slow]`) so plain CTest stays fast; `tests-full` runs the all-tags Catch2 binary plus the layering, shared-shader-block, shadow-bias, GPU-limits, shadow-matrix and release-contract guards. Both of those are DEFAULT Catch2 runs, so neither sees a hidden (`[.]`) case — which is what keeps the `[release-contract]` sentinel (`tests/release_contract.cpp`, registered first in the test target) out of the Dev suites while an explicit tag selection picks it up; `shaders/` is on the C++ include path (`BUILD_INTERFACE`, PUBLIC) because the public header `graphics/gpu_limits.hpp` includes `shaders/gpu_limits.glsl` — the dual-language limits file; `run-clang-tidy` appears only when `clang-tidy` is installed. **Read the include-order block near the top before touching dependency includes**: `CMAKE_NO_SYSTEM_FROM_IMPORTED` + `include_directories(BEFORE …)` force vcpkg's headers to arrive as `-I`, because clang searches `/usr/local/include` ahead of every `-isystem` path — with a Vulkan SDK installed there, our TUs compiled against ITS vulkan-hpp while includes resolved relative to a vcpkg header got vcpkg's, and the first RAII call aborted on a header-version assert with no bad C++ behind it (real: SDK 1.4.357 beside the pinned 1.4.335). The guard checks both `VCPKG_INSTALLED_DIR` spellings and hard-errors if the directory is missing, since a silent miss restores the mixed-header build. `SHADER_INCLUDES` lists the shared GLSL includes (`gpu_limits.glsl`, `light_ubo.glsl`, `material.glsl`, `shadow_push.glsl`, `shadow_depth.glsl`, `self_shadow_second.glsl`) so editing one rebuilds every shader — a stale `.spv` against a changed block is the layout bug the include exists to prevent. Consequence: third-party headers are no longer warning-suppressed, so a finding gets a narrow `#pragma` at the include site (`src/graphics/frame_capture.cpp`), never a weakened flag. | | `.clang-format` / `.clang-tidy` | Formatting is CI-gated. Tidy is the first-pass static-analysis config for engine `src/` plus `include/fire_engine/`; disabled checks are documented inline. | -| `.github/workflows/ci.yml` | Builds with vcpkg + warnings-as-errors, runs `run-clang-tidy`, runs the full `tests-full` target, and has a separate clang-format dry-run job. | +| `.github/workflows/ci.yml` + `tools/ci/ci-stages.sh` | Five parallel jobs: a clang-format dry-run, `run-clang-tidy`, `tests-full` on Ubuntu and on macOS/arm64 (each validating its own determinism golden), and `release-contract` — the only job that builds `Release`, which is the only configuration where the suite's `#ifdef NDEBUG` bodies are real code. The stage BODIES live in `ci-stages.sh` and are shared with both local replicas, so a stage edited in one place cannot drift from the others; the `release-contract` job sources that file rather than restating its commands, and `all` includes the stage on Linux only (`CI_RELEASE_CONTRACT`). Read `ci_release_contract` together with the `vcpkg-release` preset: it builds `test_fire_engine` alone into `build-release/` while pointing `VCPKG_INSTALLED_DIR` back at `build/vcpkg_installed`, which is what lets the CI cache serve both trees. | | `tools/assetgen/geometry.py` | Primitive builders, all returning the same `(positions, normals, indices)` triple: box (24 verts, per-face normals), tetrahedron, UV sphere, flat-shaded mesh-from-triangles, and `combine_geometry` for compounds. Plus the shared vector helpers. Everything is authored at true size with node scale left at 1. | | `tools/assetgen/quaternions.py` | glTF `[x, y, z, w]` order throughout — the easiest thing to get wrong when hand-authoring. `look_at_quat` builds a camera orientation (glTF cameras look down −Z); `quat_from_to` is the "aim this at that" helper. | | `tools/assetgen/scene.py` | The `Scene` assembler: one self-contained `.gltf` with the binary embedded as a base64 data URI (no sidecar `.bin`, no textures). Note the two layers — `node`/`box`/`sphere` are generic and pass `extras` through verbatim, while `box_body`/`sphere_body`/`compound_body`/`static_mesh_body`/`static_floor` are physics wrappers that inject `extras.Physics` and default the collider to match the mesh. `generator` is a constructor argument, not a hard-coded string. `write_gltf`'s 2-space-plus-newline formatting is part of the contract: the build regenerates the committed assets, so a formatting change is churn in every one. | @@ -211,6 +211,7 @@ Read these first when a change touches build configuration, CI, or local tooling | `render/transmission.hpp` + `transmission.cpp` | **High-attention.** `KHR_materials_transmission` off the captured `sceneColor`. The `shader.frag` split (post-fix): clear/frosted glass does screen-space refraction (roughness-blurred by the sceneColor mip chain); a thin-walled surface that is **also emissive** (a self-lit paper lamp shade) instead scatters to a view-independent irradiance tint — so a bright bulb behind it doesn't beam a camera-tracking blob. Discriminator is the **emissive factor**, NOT thickness. Plus back-face normal flip. Its forward recorder shares the main recorder's descriptor-order invariant: after a pipeline transition, push set 0 before binding allocated sets 1/2 through the same layout. | | `render/debug_draw.hpp` + `debug_draw.cpp` | Physics debug wireframes. `PhysicsDebugData` (AABBs + `ClothCollider` shapes + `DebugContact`s + free-form `DebugLine`s `queryLines` (query probe + the ragdoll joint RGB gizmo) + `DebugLabel`s `jointLabels` (drawn as projected ImGui text by the overlay, not line geometry), all Vulkan-free) → CPU-built line list in a per-frame mapped `Vertex` buffer (`Resources::createMappedVertexBuffers`) → line-list pipeline (`Pipeline::debugLineConfig`, `PipelineConfig::topology` + `dynamicDepthTest`) drawn into HDR after particles. Brackets the HDR target ShaderReadOnly↔ColorAttachment + depth ReadOnly→Attachment. Physics side: `PhysicsWorld::debugColliderBounds()` / `debugContacts()` (captured in `step()` pre-resolve). | | `tests/support/vdpm.hpp` | Shared VDPM front validators — `foldoverCount` / `coverageFailures`, templated on any front exposing `forest()` + `active(v)`, so the sequential `ActiveFront` and parallel `ParallelFront` repairs are judged by the **same first-principles yardstick** (recomputed from the mesh, never the repair under test). Coverage keeps its geometry independent but shares the one runtime tuning knob (`detail::kMinCoverageScreenAreaPx`, viewport-derived) so the "worth-fixing" threshold tracks the repair's; checks only **projectable** coverage (near-plane straddles go to the separate path). | +| `tests/release_contract.cpp` + `cmake/check_release_contract.cmake` | The `[release-contract]` gate's two halves, and neither is a test of engine behaviour. The **sentinel** is one HIDDEN (`[.]`) case that fails unless `NDEBUG` is defined: it rides in the same tag selection the Linux job runs, so a job that built `Dev` by mistake fails instead of reporting a green run over bodies that compiled to nothing. Hidden is load-bearing — `ctest` and `tests-full` are default runs and skip it, an explicit tag selection does not. The **guard** enforces the tag in both directions across `tests/`: a case with an NDEBUG conditional must carry the tag (untagged, it is not a weaker check but no check at all — nothing ever builds it as real code), and a tagged case must have a conditional (otherwise it pads the selection with what the Dev suites already run). It also fails if the sentinel or the last tag disappears. It splits cases at the line-initial `{` rather than the first `)`, because 36 case names here contain a parenthesis and one contains a brace; comments are stripped first, since the prose it reads is *about* `#ifdef NDEBUG`. Nine mutations tested. | | `tests/physics/test_physics_determinism.cpp` + `tests/support/state_hash.hpp` | Determinism harness: FNV-1a body-state hash; ReplayIsBitIdentical / FreeFallMatchesClosedForm / GoldenHash / **BroadphasesAgree** (same scene through the tree + an injected SAP → identical hash). The GoldenHash constant is a behaviour tripwire — update it intentionally when the solver math changes. | | `render/ssao.hpp` + `ssao.cpp` | **High-attention.** SSAO + contact shadows. Runs after the depth prepass: borrows the shared scene depth (attachment → read-only → attachment), reconstructs view position+normal from depth alone (analytic unprojection from `proj`, no normal G-buffer), writes R8G8 (R = hemisphere-kernel AO, G = sun-direction contact-shadow ray-march; `ssao.frag`), then a **depth-aware bilateral blur** (`ssao_blur.frag`, view-space-Z edge-stop) into a second target. Always runs (disabled = intensity 0). Forward set 1 binding 13 samples the blurred target. The depth prepass itself is `Pipeline::depthPrepassConfig` + `Renderer::recordDepthPrepass` (reuses `shader.vert` w/ `invariant gl_Position`; forward loads depth `LESS_OR_EQUAL`). | | `render/taa.hpp` + `taa.cpp` | **High-attention.** Temporal AA subsystem. Owns the RG16F velocity target (written by the forward/transmission passes as a 2nd colour attachment), two ping-pong history HDR targets, and the resolve pass (`taa.frag`): reproject history along velocity → 3×3 neighbourhood clamp → blend → blit into the offscreen HDR target. `historyWritten_` guards the first frame after (re)create. Sub-pixel jitter lives in `Renderer::drawFrame`; motion vectors are jitter-free. | diff --git a/docs/roadmap.md b/docs/roadmap.md index abd72392..ef723862 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -96,9 +96,6 @@ review's priority order: enables — every view is still `Recorded`, which is what let the restructure be verified as decision-identical. Then #15 below, which is the same mechanism. - **Sequenced after arc 4** (see below) — the reuse stage adds conditional release behaviour, and the - job that exercises it should be in place first. - **The gate for that stage is agreed, and it is NOT the byte-identical row dump** — that gate worked precisely because nothing changed, and reuse changes what the recorder does. The claim to prove is: *on the second identical frame, every active CACHEABLE view is reused and every @@ -196,34 +193,6 @@ Further tiers of this review are expected to follow the [`review-order.md`](revi --- -## Arc 4 — Release-contract CI job - -**Trigger: after `shadow-static-cascade-cache` lands, and BEFORE arc 2 #4's residency/reuse work.** Its own branch. The order is deliberate: the reuse stage adds more conditional release behaviour to a set of checks nothing currently executes, so the job that proves those checks run should exist first. - -Rejection tests guarded by `#ifdef NDEBUG` — the release behaviour of every writer that asserts in -Dev and returns `false` under `NDEBUG` — **never run**. Both presets build `Dev`, locally and in CI, -so those blocks compile to nothing on every machine that has ever checked them. The gap was found -while adding `setPointLight`'s new range / one-light validation: a one-off local Release build was -needed to prove the new checks fire at all. - -**Death tests are not the fix.** They prove the Dev assertion fires; they say nothing about whether -the `NDEBUG` fallback returns `false` (or throws) correctly, which is the half that ships. - -The job: - -- Tag the conditional cases `[release-contract]`. -- Configure a genuine Release build with warnings-as-errors. -- Build the real library and test executable. -- Run `test_fire_engine "[release-contract]"` explicitly. -- Include a **sentinel assertion that `NDEBUG` is defined**, so a misconfigured job cannot pass by - selecting zero relevant cases. - -**Linux only, and deliberately not the whole suite.** These rejection semantics are -platform-independent, and running all tests under Release would mix this contract with the -optimisation-sensitive physics goldens. - ---- - ## Parked & revisit — trigger-based Not a backlog. Each item was investigated, has data behind the decision, and is picked up only on diff --git a/tests/graphics/test_shadow_diagnostics.cpp b/tests/graphics/test_shadow_diagnostics.cpp index d640b34b..e3573a3a 100644 --- a/tests/graphics/test_shadow_diagnostics.cpp +++ b/tests/graphics/test_shadow_diagnostics.cpp @@ -264,7 +264,7 @@ TEST_CASE("a focused view distinguishes 'ran and drew nothing' from 'never ran'" CHECK_FALSE(stats.focused(absent).found()); } -TEST_CASE("one diagnostic row belongs to one logical view", "[ShadowDiagnostics]") +TEST_CASE("one diagnostic row belongs to one logical view", "[ShadowDiagnostics][release-contract]") { // Slots are reused across frames, but WITHIN a frame a row is one view's counters. Two // identities landing on one slot would sum two views' draws, triangles and level distributions @@ -575,7 +575,7 @@ TEST_CASE("claiming a view and rasterising a layer are separate facts", "[Shadow } TEST_CASE("a raster pass cannot be counted for an unclaimed or mismatched row", - "[ShadowDiagnostics]") + "[ShadowDiagnostics][release-contract]") { #ifdef NDEBUG // Two refusals. GPU work attributed to NO view carries a cost with nothing to name it; work diff --git a/tests/graphics/test_shadow_lod_resolver.cpp b/tests/graphics/test_shadow_lod_resolver.cpp index 0480f64d..fe018089 100644 --- a/tests/graphics/test_shadow_lod_resolver.cpp +++ b/tests/graphics/test_shadow_lod_resolver.cpp @@ -289,7 +289,8 @@ TEST_CASE("ShadowLodResolver.AGenerationChangeMissesTheOldHistory", "[ShadowLodR kNoPreviousShadowLod); } -TEST_CASE("ShadowLodResolver.AnInvalidKeyNeverEntersEitherStore", "[ShadowLodResolver]") +TEST_CASE("ShadowLodResolver.AnInvalidKeyNeverEntersEitherStore", + "[ShadowLodResolver][release-contract]") { // A caster with no identity still has to draw — leaving a hole in a shadow map would be worse // than the producer bug — but it must not be cached (it would collide with every other @@ -402,7 +403,7 @@ TEST_CASE("ShadowLodResolver.AnInvalidViewStillDraws", "[ShadowLodResolver]") } TEST_CASE("ShadowLodResolver.AMalformedCoarserLevelFallsBackInsteadOfBindingNothing", - "[ShadowLodResolver]") + "[ShadowLodResolver][release-contract]") { // The selector reasons about ERRORS; nothing upstream promises the level's CARRIERS. A half- // failed build can leave a coarser level with a null buffer or a zero count, and selecting it @@ -563,7 +564,8 @@ TEST_CASE("ShadowLodResolver.ProvenanceIsPerFamilyEvenWhenTheResolutionIsShared" CHECK(resolver.contentResolution(ShadowViewGroup::Cascade, key) == nullptr); } -TEST_CASE("ShadowLodResolver.MarkingAnUnresolvedCasterDrawnChangesNothing", "[ShadowLodResolver]") +TEST_CASE("ShadowLodResolver.MarkingAnUnresolvedCasterDrawnChangesNothing", + "[ShadowLodResolver][release-contract]") { // Provenance is a FIELD of a decision, not a record of its own: marking a caster the resolver // never resolved would manufacture attribution for a level nobody chose. (The shadow pass @@ -869,7 +871,7 @@ TEST_CASE("ShadowLodResolver.ADeformableCasterCannotSelectBelowFullDetail", "[Sh } TEST_CASE("ShadowLodResolver.DeformationOutranksTheChainButNotTheUserOrAProducerBug", - "[ShadowLodResolver]") + "[ShadowLodResolver][release-contract]") { const auto lods = chain(); ShadowRenderViewSet views = populatedViews(); diff --git a/tests/graphics/test_shadow_render_view.cpp b/tests/graphics/test_shadow_render_view.cpp index 1969d965..10834a71 100644 --- a/tests/graphics/test_shadow_render_view.cpp +++ b/tests/graphics/test_shadow_render_view.cpp @@ -127,7 +127,8 @@ TEST_CASE("ShadowRenderViewSet.EachWriterStampsItsOwnSlotsIdentity", "[ShadowRen CHECK(face->logicalId() == ShadowLogicalViewId::point(light, 4)); } -TEST_CASE("ShadowRenderViewSet.WritersRejectTheWrongProjectionKind", "[ShadowRenderView]") +TEST_CASE("ShadowRenderViewSet.WritersRejectTheWrongProjectionKind", + "[ShadowRenderView][release-contract]") { // A perspective descriptor in a cascade slot (or an orthographic one on a point face) is not // merely wrong, it is meaningless — and would be read back as authoritative. Dev asserts; these @@ -181,7 +182,8 @@ TEST_CASE("ShadowRenderViewSet.PointFacesCarryTheLightTheirDepthIsMeasuredAgains CHECK_FALSE(views.find(ShadowViewGroup::Spot, 0)->pointLightDepth().has_value()); } -TEST_CASE("ShadowRenderViewSet.APointCubeNeedsOneLightAndAUsableRange", "[ShadowRenderView]") +TEST_CASE("ShadowRenderViewSet.APointCubeNeedsOneLightAndAUsableRange", + "[ShadowRenderView][release-contract]") { // Both halves of the stored ratio are checked as strictly as the matrices. A zero or non-finite // range makes every texel of all six faces meaningless; six faces about DIFFERENT positions @@ -209,7 +211,8 @@ TEST_CASE("ShadowRenderViewSet.APointCubeNeedsOneLightAndAUsableRange", "[Shadow #endif } -TEST_CASE("ShadowRenderViewSet.WritersRejectAnUnkeyableIdentity", "[ShadowRenderView]") +TEST_CASE("ShadowRenderViewSet.WritersRejectAnUnkeyableIdentity", + "[ShadowRenderView][release-contract]") { // An engaged entry must always be keyable: hysteresis keys on the identity, so an invalid one // would leave the view rendering but its history unreachable. @@ -252,7 +255,7 @@ TEST_CASE("ShadowRenderViewSet.AbsentMeansInactiveAndEngagedInvalidIsDifferent", } TEST_CASE("ShadowRenderViewSet.OutOfRangeAccessIsNullAndOutOfRangeWritesAreDropped", - "[ShadowRenderView]") + "[ShadowRenderView][release-contract]") { // Never clamped into a neighbouring valid slot: that would bill one view's matrix to another // and render wrongly instead of failing. @@ -332,7 +335,8 @@ TEST_CASE("ShadowRenderViewSet.WorldOnlyFollowsACascadeRefitAfterEnabling", "[Sh CHECK(worldOnly->projection().worldUnitsPerTexel() == 0.5f); } -TEST_CASE("ShadowRenderViewSet.WorldOnlyCannotOutliveOrPrecedeItsCascade", "[ShadowRenderView]") +TEST_CASE("ShadowRenderViewSet.WorldOnlyCannotOutliveOrPrecedeItsCascade", + "[ShadowRenderView][release-contract]") { #ifdef NDEBUG ShadowRenderViewSet views; @@ -378,7 +382,8 @@ TEST_CASE("ShadowRenderViewSet.PointFacesOccupyDistinctFlatSlotsWithDistinctForw CHECK_FALSE(views.active(ShadowViewGroup::Point, shadowPointViewSlot(lightSlot + 1, 0))); } -TEST_CASE("ShadowRenderViewSet.PointLightSlotIsValidatedBeforeFlattening", "[ShadowRenderView]") +TEST_CASE("ShadowRenderViewSet.PointLightSlotIsValidatedBeforeFlattening", + "[ShadowRenderView][release-contract]") { // `lightSlot * kCubeFaceCount + face` is unsigned arithmetic, so a large enough slot WRAPS back // into the valid range: with an even face count, the top bit times 6 is 0 modulo the word. @@ -411,7 +416,8 @@ TEST_CASE("ShadowRenderViewSet.PointLightSlotIsValidatedBeforeFlattening", "[Sha #endif } -TEST_CASE("ShadowRenderViewSet.ANonFiniteRenderMatrixIsNotAView", "[ShadowRenderView]") +TEST_CASE("ShadowRenderViewSet.ANonFiniteRenderMatrixIsNotAView", + "[ShadowRenderView][release-contract]") { // The asymmetry that matters. An invalid PROJECTION is a reportable state: the view rasterises // and selection says InvalidView. A non-finite MATRIX is not a view at all — it is what the @@ -447,7 +453,8 @@ TEST_CASE("ShadowRenderViewSet.ANonFiniteRenderMatrixIsNotAView", "[ShadowRender #endif } -TEST_CASE("ShadowRenderViewSet.ARejectedReplacementClearsTheSlotItAddressed", "[ShadowRenderView]") +TEST_CASE("ShadowRenderViewSet.ARejectedReplacementClearsTheSlotItAddressed", + "[ShadowRenderView][release-contract]") { // Rejecting a write is only half the job. A producer that ATTEMPTED to describe this view and // failed has invalidated whatever was there: keeping the previous entry would rasterise the @@ -609,7 +616,8 @@ TEST_CASE("ShadowViewMetrics.ADegenerateFitPacksNoConversionRatherThanAnInfinity CHECK(ShadowViewMetrics::pointLight(0.004f, 0.0f).packed()[1] == Catch::Approx(0.0f)); } -TEST_CASE("ShadowRenderViewSet.WritersRejectMetricsOfTheWrongKind", "[ShadowRenderView]") +TEST_CASE("ShadowRenderViewSet.WritersRejectMetricsOfTheWrongKind", + "[ShadowRenderView][release-contract]") { // The three packings share a shape, so a mismatch is not caught by anything downstream: the // receiver would simply read a spot's near plane as a cascade's depth conversion and produce a @@ -653,7 +661,8 @@ TEST_CASE("ShadowRenderViewSet.APointCubeCarriesOneMetricForTheWholeLight", "[Sh } } -TEST_CASE("ShadowRenderViewSet.ARejectedCubeLeavesNoFaceBehind", "[ShadowRenderView]") +TEST_CASE("ShadowRenderViewSet.ARejectedCubeLeavesNoFaceBehind", + "[ShadowRenderView][release-contract]") { // "All six faces or none" is now the writer's contract rather than a comment in the renderer. A // five-face cube is not a usable caster: the missing face would sample whatever the previous diff --git a/tests/graphics/test_vdpm.cpp b/tests/graphics/test_vdpm.cpp index 258be62c..0afffffe 100644 --- a/tests/graphics/test_vdpm.cpp +++ b/tests/graphics/test_vdpm.cpp @@ -1502,7 +1502,7 @@ TEST_CASE("shadowDeviationForCut: takes the maximum over the EXACT prefix, not t } TEST_CASE("shadowDeviationForCut: a negative or NaN radius in the prefix yields infinity", - "[MeshSimplifier]") + "[MeshSimplifier][release-contract]") { // A deviation is a magnitude, so these are invalid data, not cheap collapses. std::max would // swallow both (NaN loses every comparison; a negative simply loses), leaving a plausible small diff --git a/tests/release_contract.cpp b/tests/release_contract.cpp new file mode 100644 index 00000000..6b46d39b --- /dev/null +++ b/tests/release_contract.cpp @@ -0,0 +1,41 @@ +#include + +// THE RELEASE CONTRACT, AND THE SENTINEL THAT PROVES IT WAS ACTUALLY CHECKED. +// +// A number of this suite's cases assert RELEASE behaviour: what a writer that asserts in a Dev +// build returns once `NDEBUG` compiles that assertion away. `ShadowRenderViewSet`'s writers are the +// clearest family — Dev stops at the assertion inside the writer, while under `NDEBUG` the writer +// must return `false` and clear the slot it addressed, which is the half that ships. Those bodies +// sit behind `#ifdef NDEBUG` and are tagged `[release-contract]`. +// +// Every preset in this repository builds `Dev`. Locally, in the Docker replica, and in every +// GitHub job before this one, those bodies therefore compiled to NOTHING — the cases ran, passed, +// and asserted precisely nothing, for as long as they have existed. That is what the Linux +// `release-contract` job exists to fix. +// +// This file is the sentinel, and it is here because a job that checks nothing looks exactly like a +// job that checks everything and finds no fault. Two failure modes are covered, by two different +// mechanisms: +// +// 1. THE SELECTION MATCHED NOTHING — a renamed or mistyped tag. Catch2 already exits non-zero when +// a test spec matches no tests, so this needs no help from us. +// 2. THE BUILD WAS NOT A RELEASE BUILD. That one is silent: the filter matches the tagged cases, +// they run, their guarded halves are empty, and the job reports success while proving nothing. +// The case below fails outright in that situation, so the job cannot pass by accident. +// +// HIDDEN (`[.]`) on purpose. The Dev suites — `ctest` (`~[slow]`) and `tests-full` (no filter) — +// are both default runs, and Catch2 excludes hidden cases from those, so this does not fail the +// builds it is designed to detect. Explicitly selecting `[release-contract]`, which is the only +// thing the release job does, runs it. Do not remove the `[.]`, and do not give the tag to a case +// that has no `#ifdef NDEBUG` in it: the tag's meaning is "this case asserts something a Dev build +// cannot see". +TEST_CASE("the release contract is only checked where NDEBUG is defined", "[.][release-contract]") +{ +#ifdef NDEBUG + SUCCEED("NDEBUG is defined — the guarded bodies in this selection are real code"); +#else + FAIL("[release-contract] was selected from a build WITHOUT NDEBUG: every guarded body in this " + "selection compiled to nothing, so the run proves nothing. Configure the vcpkg-release " + "preset (or any Release build) and re-run."); +#endif +} diff --git a/tools/ci/ci-stages.sh b/tools/ci/ci-stages.sh index a3ed0484..c640c537 100644 --- a/tools/ci/ci-stages.sh +++ b/tools/ci/ci-stages.sh @@ -8,9 +8,15 @@ # Tunables via the environment: # CLANG_FORMAT clang-format binary (clang-format-22 in the container; clang-format on macOS) # WARNINGS_AS_ERRORS ON/OFF for the configure step (default ON — CI parity) +# CI_RELEASE_CONTRACT 1 to include the Release-contract stage in `all` (Linux only — see below) : "${CLANG_FORMAT:=clang-format}" : "${WARNINGS_AS_ERRORS:=ON}" +# LINUX ONLY, and deliberately so. The `[release-contract]` cases assert what a writer returns once +# NDEBUG compiles its assertion away, which is platform-independent behaviour — one job proves it, +# and a second would only spend runner minutes. The Linux runner sets this; the macOS one leaves it +# at 0, so `all` means "every gate this platform owns" on both. +: "${CI_RELEASE_CONTRACT:=0}" ci_print_versions() { @@ -51,6 +57,31 @@ ci_test() cmake --build build --target tests-full } +# The RELEASE CONTRACT: the suite's `#ifdef NDEBUG` bodies, in the only configuration where they are +# real code. +# +# Every other stage here builds `Dev`, so those bodies compile to nothing — the cases run, pass, and +# assert precisely nothing. This stage exists because that is indistinguishable from a clean run. +# +# A GENUINE Release build, not a flag bolted onto the Dev tree: the behaviour under test belongs to +# the LIBRARY (a writer that asserts in Dev must return false under NDEBUG), so the library has to +# be the one compiled with NDEBUG. `vcpkg-release` puts it in its own binaryDir while sharing +# build/vcpkg_installed — see the preset's own description. +# +# Only `test_fire_engine` is built. It already pulls in `fireengine` and the shaders; the +# application adds no contract coverage. And only the tagged cases run: the whole Release suite +# would drag in the optimisation-sensitive physics goldens, which are a different question with +# different failure modes. +ci_release_contract() +{ + cmake --preset vcpkg-release -DFIRE_ENGINE_WARNINGS_AS_ERRORS="${WARNINGS_AS_ERRORS}" + cmake --build --preset release-contract + # Catch2 exits non-zero when a spec matches nothing, so a renamed tag fails here rather than + # passing quietly; the hidden sentinel case inside the selection fails if this binary was not + # built with NDEBUG. Between them, a green run means the contract was actually checked. + ./build-release/test_fire_engine "[release-contract]" +} + # Run one named stage, composing prerequisites the same way CI does. ci_run_stage() { @@ -60,7 +91,15 @@ ci_run_stage() build) ci_configure && ci_build ;; tidy) ci_configure && ci_build && ci_tidy ;; test) ci_configure && ci_build && ci_test ;; - all) ci_format && ci_configure && ci_build && ci_tidy && ci_test ;; + release-contract) ci_release_contract ;; + all) + ci_format && ci_configure && ci_build && ci_tidy && ci_test || return $? + # Appended rather than folded into the chain above: `all` means "every gate this + # platform owns", and the Release contract is one Linux job by design. + if [ "${CI_RELEASE_CONTRACT}" = "1" ]; then + ci_release_contract + fi + ;; *) echo "unknown CI stage: $1" >&2 return 2 diff --git a/tools/ci/container-run.sh b/tools/ci/container-run.sh index a7542985..4d3606db 100755 --- a/tools/ci/container-run.sh +++ b/tools/ci/container-run.sh @@ -10,9 +10,15 @@ mode="${1:-all}" sync_source() { + # Every build tree is excluded, and for two reasons at once: a host CMakeCache names host + # ABSOLUTE paths, so copying one in makes the container's configure fail outright ("the source + # /work/fireEngine/CMakeLists.txt does not match the source /Users/... used to generate + # cache"); and each of these is a mounted volume, which `--delete` would otherwise empty on + # every run. rsync -a --delete \ --exclude /.git \ --exclude /build \ + --exclude /build-release \ --exclude /vcpkg \ --exclude /vcpkg_installed \ /repo/ /work/fireEngine/ diff --git a/tools/ci/run-local-ci.sh b/tools/ci/run-local-ci.sh index 965b7458..162aa86b 100755 --- a/tools/ci/run-local-ci.sh +++ b/tools/ci/run-local-ci.sh @@ -3,17 +3,22 @@ set -euo pipefail usage() { cat <<'EOF' -Usage: tools/ci/run-local-ci.sh [format|configure|build|tidy|test|all|shell] +Usage: tools/ci/run-local-ci.sh [format|configure|build|tidy|test|release-contract|all|shell] Runs the GitHub Actions Linux checks inside Docker. The repository is copied into the container before each run; Docker volumes hold the Linux build tree and vcpkg checkout/cache so host build artifacts are left alone. + +`release-contract` builds a genuine Release tree and runs the [release-contract] +cases — the suite's #ifdef NDEBUG bodies, which every other stage compiles away. +`all` includes it; the macOS runner deliberately does not (the contract is +platform-independent, so one job proves it). EOF } mode="${1:-all}" case "${mode}" in - format|configure|build|tidy|test|all|shell) ;; + format|configure|build|tidy|test|release-contract|all|shell) ;; -h|--help|help) usage exit 0 @@ -29,6 +34,9 @@ platform="${DOCKER_PLATFORM:-linux/amd64}" platform_tag="${platform//\//-}" image="fireengine-local-ci:ubuntu-24.04-${platform_tag}" build_volume="fireengine-local-ci-build-${platform_tag}" +# The Release-contract tree gets its own volume for the same reason the Dev one has one: without it +# every `release-contract` run recompiles all ~270 translation units inside an emulated container. +release_build_volume="fireengine-local-ci-build-release-${platform_tag}" vcpkg_volume="fireengine-local-ci-vcpkg-${platform_tag}" build_parallel_level="${CMAKE_BUILD_PARALLEL_LEVEL:-2}" docker_run_flags=(--rm) @@ -50,9 +58,11 @@ docker run "${docker_run_flags[@]}" \ --platform "${platform}" \ --volume "${repo_root}:/repo:ro" \ --volume "${build_volume}:/work/fireEngine/build" \ + --volume "${release_build_volume}:/work/fireEngine/build-release" \ --volume "${vcpkg_volume}:/cache/vcpkg" \ --env VCPKG_ROOT=/cache/vcpkg \ --env VCPKG_DISABLE_METRICS=1 \ + --env CI_RELEASE_CONTRACT=1 \ --env CMAKE_BUILD_PARALLEL_LEVEL="${build_parallel_level}" \ --workdir /work/fireEngine \ "${image}" \ diff --git a/tools/ci/run-local-macos.sh b/tools/ci/run-local-macos.sh index 2ad368be..ec726841 100755 --- a/tools/ci/run-local-macos.sh +++ b/tools/ci/run-local-macos.sh @@ -7,7 +7,10 @@ set -euo pipefail # MoltenVK, glfw, a shader compiler (glslc or glslangValidator), and vcpkg via VCPKG_ROOT. It does # NOT install anything. # -# Stages mirror run-local-ci.sh: format | configure | build | tidy | test | all. +# Stages mirror run-local-ci.sh: format | configure | build | tidy | test | all. The one exception +# is `release-contract`, which is Linux-only ON PURPOSE — those cases assert what a writer returns +# once NDEBUG removes its assertion, which does not vary by platform, so a second job would spend +# runner minutes to re-prove it. `all` here therefore stays "every gate macOS owns". # # Note: clang-tidy on macOS uses Apple's clang-tidy, which can differ from the Linux one the CI / # Docker replica runs — treat the macos `tidy` stage as advisory; Linux is the source of truth.