diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2d1cd4985..54112f4a7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2262,8 +2262,49 @@ jobs: # # Compiler caching (sccache/fastcache-cc) is still not set up here. That # step compiles 67 objects; a real build of this configure would compile - # the database's 703, so the caching steps would cost more setup than + # the database's 737, so the caching steps would cost more setup than # they could save. + # + # MORPH_BUILD_BANK_GUI is the last of the two flags "every optional + # feature is ON here" was not actually true about (morph#651). It + # defaults OFF (CMakeLists.txt), gates `add_subdirectory(gui)` in + # examples/bank/CMakeLists.txt, and the bank GUI's *tests* are gated on + # it too -- bank_gui_tests only exists `if(TARGET bank_gui_lib)`, which + # is that option's target. Eleven tracked sources therefore had no + # compile command in this job's database and were dropped, unanalysed, + # by the filter below: nine under examples/bank/gui/ and two under + # examples/bank/tests/gui/. + # + # Measured locally on 4563aff3, clang 22.1.8, Qt 6, this job's own flag + # set, cold build directory each time: + # + # without BANK_GUI: 703 entries, 695 distinct in-workspace sources, + # 270 under examples/; 63 of 473 tracked C/C++ + # sources absent from the database; configure 63.7s + # with BANK_GUI: 737 entries, 727 distinct in-workspace sources, + # 281 under examples/; 52 absent; configure 64.4s + # + # The delta is exactly the eleven files, and the configure cost is under + # a second -- an `add_subdirectory` of Qt Quick targets nothing here ever + # builds. Both of the filter step's floors (600 in-workspace, 200 under + # examples/) move further from their limit, not closer. + # + # This needs nothing new installed: linux-all-features already builds + # this option on the same runner image with the same + # `jurplel/install-qt-action@v4 modules: qtwebsockets` install and the + # same apt set (morph#604 is why it is there), so the GUI's + # Qml/Quick/QuickControls2 requirement is already known to be met. + # + # What it does *not* do is clear the eleven files' existing findings. + # Measured on the same configure, whole-file, with this step's own + # `-extra-arg` pair: 97 findings inside the eleven sources themselves + # (44 cppcoreguidelines-pro-bounds-avoid-unchecked-container-access, 23 + # performance-unnecessary-value-param, 9 misc-const-correctness, 8 + # readability-identifier-length, 13 others), across ten of them -- + # BankController.cpp is clean. clang-tidy-diff only reports on *changed* + # lines, so none of that goes red until someone edits one of these files + # on a line that carries a finding. That is morph#656, filed rather than + # folded in here; morph#646 is the precedent for keeping the two apart. - name: Configure (generates compile_commands.json over every optional feature) run: | cmake --preset clang-debug \ @@ -2276,6 +2317,7 @@ jobs: -DMORPH_BUILD_FUZZERS=ON \ -DMORPH_BUILD_LADDER=ON \ -DMORPH_BUILD_BANK_EXAMPLE=ON \ + -DMORPH_BUILD_BANK_GUI=ON \ -DCMAKE_C_COMPILER=clang-${{ env.CLANG_VERSION }} \ -DCMAKE_CXX_COMPILER=clang++-${{ env.CLANG_VERSION }} @@ -2511,9 +2553,12 @@ jobs: # symlinked checkout, a path this script fails to normalise the same # way) trips it too instead of silently skipping everything. # - # Measured with this job's own configure flags and clang 22: 703 - # entries naming 695 distinct in-workspace sources, 270 of them under - # examples/. CI measured 690/276 at morph#481's revision. Both floors + # Measured with this job's own configure flags and clang 22: 737 + # entries naming 727 distinct in-workspace sources, 281 of them under + # examples/ (703/695/270 before morph#651 added + # -DMORPH_BUILD_BANK_GUI=ON to the Configure step above -- that flag + # moves both numbers away from their floor, never towards it). + # CI measured 690/276 at morph#481's revision. Both floors # sit under both pairs with room for ordinary churn, and the # regression they exist to catch takes examples/ to 16, not to 199. MIN_ENTRIES = 600 diff --git a/.github/workflows/spec-sync.yml b/.github/workflows/spec-sync.yml index 2bcc612ea..e5119b779 100644 --- a/.github/workflows/spec-sync.yml +++ b/.github/workflows/spec-sync.yml @@ -92,4 +92,21 @@ jobs: # of paths, which is what lets scripts/test_check_spec_sync.sh drive # it against the commits it must reject rather than asserting it # works. - printf '%s\n' "$changed" | bash scripts/check_spec_sync.sh + # + # --allow-empty only when this step has *observed* the emptiness + # (morph#655). An empty path list is what a broken producer hands + # that script, and answering "OK, nothing to check" to it is a green + # tick for a check that read nothing -- so the script now refuses + # empty input unless the caller states it meant it. This step is + # entitled to state it: `set -euo pipefail` above means the + # `git diff` either produced this list or aborted the step, so an + # empty `$changed` here is a real net-zero diff (a pull request whose + # commits cancel out) and not a failure upstream. Passing the flag + # unconditionally would hand that entitlement back, and it is not the + # ordinary path anyway: every other run goes through the `else`. + if [ -z "$changed" ]; then + echo "The diff against the base is empty -- git diff succeeded and named no files." + printf '%s\n' "$changed" | bash scripts/check_spec_sync.sh --allow-empty + else + printf '%s\n' "$changed" | bash scripts/check_spec_sync.sh + fi diff --git a/examples/TESTING.md b/examples/TESTING.md index 2c5c4da7b..227768bc2 100644 --- a/examples/TESTING.md +++ b/examples/TESTING.md @@ -626,7 +626,15 @@ root `CMakeLists.txt` — don't repeat that eight times): too. There is deliberately **no** `examples/.clang-tidy`: one file there would cover every rung's `src/`, `include/` and `gui_lib/` as well, where a hand-written `a < b < c` is a real defect — `check_rung_filters.sh` fails if - one appears. + one appears. It also fails if any `.cpp` in one of those directories is not + a Catch2 translation unit: clang-tidy resolves configuration from the TU's + path, so a library source living beside the tests gets a suppression argued + as Catch2 idiom without being Catch2 at all. `morph_ladder_testkit`'s own + two TUs are in `examples/common/testkit_src/` rather than beside their + headers for exactly that reason. The headers themselves stay in + `testkit/` and are listed as sources of `morph_ladder_testkit`, which is + the only thing that keeps AUTOMOC seeing `fault_proxy.hpp`'s `Q_OBJECT` + once the same-basename pairing is gone. - `examples/common/` declares exactly three consumable targets: `morph_ladder_testkit` (morph + Catch2 + Qt), `morph_ladder_gui` (STATIC, `Qt6::Core` only, **no Catch2**, **no `Qt6::WebSockets`** — presenter rule diff --git a/examples/bank/CMakeLists.txt b/examples/bank/CMakeLists.txt index a4f84b94d..ae75f7090 100644 --- a/examples/bank/CMakeLists.txt +++ b/examples/bank/CMakeLists.txt @@ -219,7 +219,13 @@ if(MORPH_BUILD_TESTS) # while the audit itself needs Qt6::Core and nothing more. # This is the same translation unit, not a copy of it, so the # audit still has exactly one implementation. - ${PROJECT_SOURCE_DIR}/examples/common/testkit/qml_surface.cpp + # + # testkit_src/, not testkit/: the TU sits one directory up + # from its own header so that testkit/.clang-tidy's Catch2 + # suppression cannot reach it (morph#652). The + # ${PROJECT_SOURCE_DIR}/examples/common include directory + # below is what resolves its . + ${PROJECT_SOURCE_DIR}/examples/common/testkit_src/qml_surface.cpp ) target_include_directories(bank_gui_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/tests diff --git a/examples/common/CMakeLists.txt b/examples/common/CMakeLists.txt index 2458cca91..d996eabeb 100644 --- a/examples/common/CMakeLists.txt +++ b/examples/common/CMakeLists.txt @@ -181,9 +181,47 @@ endif() # regex/QMetaObject walker with no reason to be inlined into every rung's test # TU, and it deliberately stays out of apply_warnings()' reach by living in # this target (see the note below on Lightweight). +# +# Those two TUs live in testkit_src/ rather than beside their headers in +# testkit/, and that is the whole of what morph#652 fixed. clang-tidy resolves +# its configuration from the path of the translation unit it is analysing, and +# testkit/.clang-tidy subtracts bugprone-chained-comparison on the argument +# that the finding is Catch2's REQUIRE expansion rather than a defect. That +# argument is true of the twenty-one Catch2 sources in that directory and says +# nothing about a library TU with no REQUIRE in it -- yet while these two sat +# there, the suppression covered them on the same terms, and the file's own +# prose claimed it could not ("a hand-written `a < b < c` in the code under +# test is still reported"). One directory up the suppression cannot reach +# them, so the claim is true by construction instead of by assertion. The +# headers stay in testkit/ because every consumer includes them as +# ; only the two TUs moved. +# +# Which is why the two headers are listed as sources below, and they are not +# there to be compiled. AUTOMOC finds a Q_OBJECT header two ways: next to the +# .cpp of the same basename, or listed among the target's own sources. Moving +# the TUs out of testkit/ broke the first, fault_proxy.hpp declares a QObject, +# and nothing generated moc_fault_proxy.cpp any more -- a failure that is +# invisible at compile time and that the static library archives happily. It +# surfaces only at the first link that needs the vtable: +# +# libmorph_ladder_testkit.a(fault_proxy.cpp.o): in function +# `morph::ladder::testkit::FaultProxy::FaultProxy(QUrl, QObject*)': +# undefined reference to `vtable for morph::ladder::testkit::FaultProxy' +# undefined reference to `...::FaultProxy::staticMetaObject' +# undefined reference to `typeinfo for ...::FaultProxy' +# +# Header entries are not compiled; they only join the AUTOMOC scan. This is +# the same remedy, for the same reason, that cmake/morph_add_rung.cmake +# already applies to every rung -- its _lib_headers glob exists because a +# rung splits include//app/app.hpp from src/app/app.cpp, and +# pastebin::app::App is where this repository last hit it. Both headers are +# listed rather than only the one carrying a Q_OBJECT today, so the move is +# moc-neutral by construction instead of by which header happens to need moc. add_library(morph_ladder_testkit STATIC - testkit/fault_proxy.cpp - testkit/qml_surface.cpp + testkit_src/fault_proxy.cpp + testkit_src/qml_surface.cpp + testkit/fault_proxy.hpp + testkit/qml_surface.hpp ) add_library(morph::ladder_testkit ALIAS morph_ladder_testkit) target_include_directories(morph_ladder_testkit PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) diff --git a/examples/common/testkit/.clang-tidy b/examples/common/testkit/.clang-tidy index 1bbb5028c..759365510 100644 --- a/examples/common/testkit/.clang-tidy +++ b/examples/common/testkit/.clang-tidy @@ -24,6 +24,41 @@ # keeps every other check the repository-root .clang-tidy enables: exactly one # entry is subtracted, and only here. # +# ── What "only here" is worth, and how it is kept true (morph#652) ─────────── +# +# That sentence was false for two years' worth of commits, in the way this +# repository finds hardest to notice: the directory held the testkit *library* +# as well as the testkit's tests. `fault_proxy.cpp` and `qml_surface.cpp` -- +# the two translation units of `morph_ladder_testkit`, neither containing a +# single REQUIRE -- resolved their configuration from this file, so the check +# was off for them on the strength of an argument about Catch2 that did not +# apply to them at all. "The code under test" was in the directory with the +# test, which is exactly the case the paragraph above says cannot happen. +# +# They now live in `examples/common/testkit_src/`, one directory up and +# outside this file's reach, so the claim holds by construction. It is not +# left to hold by assertion either: scripts/check_rung_filters.sh fails if any +# .cpp in a directory carrying this suppression is not a Catch2 translation +# unit -- no TEST_CASE, no SCENARIO, no catch2/ include -- which is the shape +# a second library source arriving here would have. That check is the reason +# the claim above can be read as a fact rather than an intention. +# +# What is *not* narrowed, and cannot be: the reusable headers beside the tests +# (backend_rig.hpp, pump.hpp, db_fixture.hpp, ...) are library code too, and a +# header reached from a TU under this directory is analysed with that TU's +# configuration, not its own (morph#632 measured the same effect for +# tests/.clang-tidy: 333 findings inside include/morph/** hidden across 25 +# headers). So a chained comparison written by hand in one of those headers is +# unreported whenever the reaching TU is one of this directory's tests -- and +# since every other consumer is a rung's test binary, reaching them from an +# examples//tests/ that carries its own copy of this same one entry, in +# practice it is unreported from everywhere. That is the residue, stated +# rather than narrowed, for morph#632's reason: a suppression whose reach is +# written down is auditable, and one whose reach is guessed is not. It is +# bounded by what the check is -- `clang-diagnostic-parentheses`, which this +# file does not subtract, reports the same `a < b < c` shape independently +# and was observed doing so on the probe morph#652 was measured with. +# # One copy per test directory rather than one shared file, because no directory # contains every example's tests and nothing else. scripts/check_rung_filters.sh # fails if a test directory on the application ladder has no such file, so a new diff --git a/examples/common/testkit/fault_proxy.cpp b/examples/common/testkit_src/fault_proxy.cpp similarity index 100% rename from examples/common/testkit/fault_proxy.cpp rename to examples/common/testkit_src/fault_proxy.cpp diff --git a/examples/common/testkit/qml_surface.cpp b/examples/common/testkit_src/qml_surface.cpp similarity index 99% rename from examples/common/testkit/qml_surface.cpp rename to examples/common/testkit_src/qml_surface.cpp index bb4d8b45f..3d14b7cd1 100644 --- a/examples/common/testkit/qml_surface.cpp +++ b/examples/common/testkit_src/qml_surface.cpp @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 -#include "qml_surface.hpp" +#include "testkit/qml_surface.hpp" #include #include diff --git a/examples/kanban/tests/test_kanban_qml_surface.cpp b/examples/kanban/tests/test_kanban_qml_surface.cpp index ba7b6cedc..d948e3cce 100644 --- a/examples/kanban/tests/test_kanban_qml_surface.cpp +++ b/examples/kanban/tests/test_kanban_qml_surface.cpp @@ -156,7 +156,7 @@ TEST_CASE("Every kanban bridge exposes exactly the surface gui/qml binds, and no // on each banner's `visible` is the probe; the `visible ? ... : // ""` ternary on each `text` is the use). The audit treats a // property's NOTIFY signal as covered by reading the property -- - // testkit/qml_surface.cpp's signal sweep does model this -- so + // testkit_src/qml_surface.cpp's signal sweep does model this -- so // the signal needs no exemption here, and claiming one would // misdescribe a guard that is already doing its job. // * OFF: neither property is compiled in, so no property read can cover diff --git a/examples/ledger/tests/test_ledger_qml_surface.cpp b/examples/ledger/tests/test_ledger_qml_surface.cpp index f1a7ce48e..10a79950f 100644 --- a/examples/ledger/tests/test_ledger_qml_surface.cpp +++ b/examples/ledger/tests/test_ledger_qml_surface.cpp @@ -79,7 +79,7 @@ TEST_CASE("Every ledger bridge exposes exactly the surface gui/qml binds, and no // * `busy` now gates a `BusyIndicator` in `LedgerView.qml`, // `BudgetView.qml` and `RulesView.qml`. Its `busyChanged` NOTIFY needs // no exemption of its own: the audit treats a property's NOTIFY as - // covered by reading the property (testkit/qml_surface.cpp's signal + // covered by reading the property (testkit_src/qml_surface.cpp's signal // sweep), and that is exactly what these three views now do. // * `ledgerBridge.refresh` is a "Refresh" button; `undoTransaction` is a // journal-id field plus an "Undo" button, both in `LedgerView.qml`. diff --git a/scripts/check_rung_filters.sh b/scripts/check_rung_filters.sh index ef1660392..3d8a40056 100644 --- a/scripts/check_rung_filters.sh +++ b/scripts/check_rung_filters.sh @@ -253,15 +253,33 @@ shopt -u nullglob # test file written afterwards failed the gate on 14 findings, every one of # them a REQUIRE. # -# Two conditions, not one, and the second is the load-bearing one. The file -# must subtract the check -- and it must carry `InheritParentConfig: true`, -# because without that key clang-tidy *replaces* the parent configuration -# instead of extending it: a directory holding `Checks: -# '-bugprone-chained-comparison'` alone runs with no checks enabled whatsoever -# and reports green while linting nothing. A suppression that silences more -# than the false positive is worse than the finding it hides, so it is checked -# here rather than left to review. +# Three conditions, not one, and the second and third are the load-bearing +# ones. The file must subtract the check -- and it must carry +# `InheritParentConfig: true`, because without that key clang-tidy *replaces* +# the parent configuration instead of extending it: a directory holding +# `Checks: '-bugprone-chained-comparison'` alone runs with no checks enabled +# whatsoever and reports green while linting nothing. A suppression that +# silences more than the false positive is worse than the finding it hides, so +# it is checked here rather than left to review. +# +# The third condition is morph#652: every .cpp the suppression reaches must +# actually be a Catch2 translation unit. clang-tidy resolves configuration +# from the TU's path, so a non-Catch2 source in one of these directories gets +# the suppression on the strength of an argument -- "this finding is Catch2's +# REQUIRE expansion" -- that is not true of it. That is not hypothetical: +# examples/common/testkit/ held `fault_proxy.cpp` and `qml_surface.cpp`, the +# two translation units of the morph_ladder_testkit *library*, neither with a +# REQUIRE anywhere in it, while that directory's .clang-tidy told every reader +# the suppression could not reach the code under test. They now live in +# examples/common/testkit_src/, and this condition is what stops the next one +# arriving unnoticed. +# +# "Catch2 translation unit" is read from the source rather than from any build +# file, because no build file is available to the jobs that run this gate: a +# TEST_CASE/SCENARIO macro, or an include of catch2/, or CATCH_CONFIG_* for a +# Catch2 main such as testkit_main.cpp. A library TU has none of the three. readonly tidy_false_positive="bugprone-chained-comparison" +readonly catch2_marker='TEST_CASE|SCENARIO|CATCH_CONFIG|catch2/' check_test_dir_tidy_config() { local dir="$1" why="$2" @@ -287,6 +305,44 @@ check_test_dir_tidy_config() { return fi note "${dir}/.clang-tidy subtracts ${tidy_false_positive} and inherits every other check" + + # morph#652. Recursive: the suppression reaches every subdirectory too + # (examples/bank/tests/gui/ is the live case), and clang-tidy walks up + # from the TU, so a nested source is governed exactly as a top-level one + # is. + local -a sources=() + local source + while IFS= read -r source; do + [ -n "$source" ] || continue + sources+=("$source") + done < <(find "${repo_root}/${dir}" -type f -name '*.cpp' | sort) + + checks=$((checks + 1)) + if [ "${#sources[@]}" -eq 0 ]; then + fail "${dir}/ carries the ${tidy_false_positive} suppression but holds no .cpp at all -- either the suppression is pointing at the wrong directory, or this scan has stopped seeing the tree. Both leave the check below examining nothing while reporting green." + return + fi + + # One grep over the whole list rather than one per file: `-L` prints the + # files that did *not* match, which is exactly the set wanted here, and a + # per-file loop costs a process per source for no extra information. + local -a non_catch2=() + while IFS= read -r source; do + [ -n "$source" ] || continue + non_catch2+=("${source#"${repo_root}/"}") + done < <(grep -LE -- "$catch2_marker" "${sources[@]}") + + if [ "${#non_catch2[@]}" -ne 0 ]; then + fail "${dir}/.clang-tidy subtracts ${tidy_false_positive} as *Catch2 idiom*, but ${#non_catch2[@]} of the ${#sources[@]} .cpp it governs contain no Catch2 at all: +$(printf ' %s\n' "${non_catch2[@]}") + clang-tidy resolves configuration from the translation unit's path, so each + of these is analysed with the check off on the strength of an argument + about REQUIRE that does not apply to it -- morph#652. Move the source out + of this directory (examples/common/testkit_src/ is the precedent), or, if + it really is a test, give it the Catch2 include or macro that says so." + return + fi + note "all ${#sources[@]} .cpp under ${dir}/ are Catch2 translation units, so the suppression's own justification covers every file it reaches" } while IFS= read -r rung; do diff --git a/scripts/check_spec_sync.sh b/scripts/check_spec_sync.sh index 41a2a5a54..3d8673b8f 100755 --- a/scripts/check_spec_sync.sh +++ b/scripts/check_spec_sync.sh @@ -1,5 +1,6 @@ #!/usr/bin/env bash # Usage: git diff --name-only HEAD | bash scripts/check_spec_sync.sh +# bash scripts/check_spec_sync.sh --allow-empty # silence must be asked for # # The header <-> spec sync gate .github/workflows/spec-sync.yml enforces: a # change to a header sub-domain must come with a change to the docs that @@ -34,8 +35,47 @@ # part that stops this recurring -- every directory under include/morph/ must # appear either in that table or in the exempt list below. A new sub-domain is # now a failure that names itself, instead of a silent exemption. +# +# ── Why empty input is an error unless asked for (morph#655) ──────────────── +# +# This gate reads a list of paths from stdin, so the question "what happens +# when the list is empty?" is the question "what happens when whatever +# produced it broke?". It used to answer `Spec sync OK: the change touches no +# files.` and exit 0. The sentence is honest -- it claims nothing was checked, +# rather than claiming a check passed -- but nobody reads the sentence, and a +# broken upstream then produces a green tick. +# +# Measured on 4563aff3, before the flag below existed. CI's own invocation was +# never exposed: .github/workflows/spec-sync.yml assigns the diff to a +# variable under `set -euo pipefail` and pipes a `printf`, so a failing +# `git diff` aborts the step with status 128 before this script is reached. +# The documented manual invocation, in a shell with neither -e nor pipefail -- +# an interactive one, or a pre-push hook -- is exposed exactly as filed: +# +# $ git diff --name-only HEAD | bash scripts/check_spec_sync.sh +# fatal: bad object +# Spec sync OK: the change touches no files. +# pipeline exit=0 +# +# So the fix is a flag rather than a hard error: "this change touches no +# headers" is a real and common outcome the gate must still pass, and the +# whole point is to make the empty case something a caller states rather than +# something it falls into. Without --allow-empty, empty input is a failure. set -euo pipefail +allow_empty=0 +for argument in "$@"; do + case "$argument" in + --allow-empty) allow_empty=1 ;; + *) + printf 'error: unrecognised argument: %s\n' "$argument" >&2 + printf 'usage: git diff --name-only HEAD | %s [--allow-empty]\n' \ + "${BASH_SOURCE[0]}" >&2 + exit 2 + ;; + esac +done + cd "$(git rev-parse --show-toplevel)" # ── The table: ||... ─────────────────── @@ -81,8 +121,17 @@ exempt_subdomains="detail qt" changed="$(cat)" if [ -z "${changed//[[:space:]]/}" ]; then - echo "Spec sync OK: the change touches no files." - exit 0 + if [ "${allow_empty}" -eq 1 ]; then + echo "Spec sync OK: the change touches no files (--allow-empty given)." + exit 0 + fi + echo "::error::spec-sync: the path list on stdin is empty, and --allow-empty was not given." + echo " This gate checks a list of changed paths. An empty list is indistinguishable" + echo " from a broken producer -- a failed \`git diff\`, a mistyped base ref, a" + echo " pipeline in a shell without \`set -o pipefail\` -- and answering OK to that" + echo " is a green tick for a check that read nothing (morph#655)." + echo " If the emptiness is genuine and expected, say so: pass --allow-empty." + exit 1 fi fail=0 diff --git a/scripts/coverage.sh b/scripts/coverage.sh index a906fbc9f..7d8e53b73 100644 --- a/scripts/coverage.sh +++ b/scripts/coverage.sh @@ -234,7 +234,7 @@ for _rung in "${RUNG_TEST_EXES[@]}"; do done done -# examples/common/testkit/ mixes real, reusable test-support headers/.cpp +# examples/common/testkit/ mixes real, reusable test-support headers # (backend_rig.hpp, db_fixture.hpp, strand_interleaver.hpp, ...) with actual # Catch2 test files (test_event_poller.cpp, test_presenter.cpp, ...) in the # same directory — unlike include/morph and examples/pastebin's SOURCES @@ -246,6 +246,10 @@ done # project has repeatedly had to hand-verify file by file. Test files # genuinely are not part of what examples/IMPLEMENTATION.md rule 5's 100% # bar means to hold to that standard — only the real testkit/GUI code is. +# The library's own two TUs are not in that directory at all any more: they +# live in examples/common/testkit_src/ so that testkit/.clang-tidy's Catch2 +# suppression cannot reach them (morph#652). They stay measured -- SOURCES +# names examples/common, and this regex matches only testkit/test_*.cpp. IGNORE_REGEX='.*/testkit/test_[^/]+\.cpp$' ${LLVM_PROFDATA} merge -sparse "${PROFILES[@]}" -o "$MERGED" diff --git a/scripts/test_check_rung_filters.sh b/scripts/test_check_rung_filters.sh index ecf7d3ea0..208925eeb 100644 --- a/scripts/test_check_rung_filters.sh +++ b/scripts/test_check_rung_filters.sh @@ -47,11 +47,28 @@ make_tree() { # the same "whatever the checker reads" rule as everything above -- the # directory has to exist for the check to fire at all, since a rung with no # tests/ yet is deliberately skipped. + # + # The .cpp files under each of those directories come too, because check 5 + # now reads them: every source the suppression reaches must be a Catch2 + # translation unit (morph#652). Copying the real ones rather than + # synthesising stand-ins keeps this self-test driving the gate against + # what the repository actually contains -- a synthetic corpus would go on + # passing after the real tree had grown a library source in one of these + # directories, which is the exact failure morph#652 records. + # Whole directory, in one `cp -r`, rather than the .clang-tidy alone: the + # sources beside it are read too now (check 5's third condition -- every + # .cpp the suppression reaches must be a Catch2 translation unit, + # morph#652). The real files rather than synthesised stand-ins, so this + # self-test keeps driving the gate against what the repository actually + # contains; a synthetic corpus would go on passing after the real tree had + # grown a library source in one of these directories, which is the exact + # failure morph#652 records. 2.3MB all told, and per-file copying here + # cost six thousand processes a run. for tidy in "${repo_root}"/examples/*/tests/.clang-tidy "${repo_root}"/examples/common/testkit/.clang-tidy; do [ -f "$tidy" ] || continue tidy_dir="$(dirname "${tidy#"${repo_root}/"}")" - mkdir -p "$dest/$tidy_dir" - cp "$tidy" "$dest/$tidy_dir/" + mkdir -p "$dest/$(dirname "$tidy_dir")" + cp -r "${repo_root}/${tidy_dir}" "$dest/${tidy_dir}" done } @@ -191,6 +208,33 @@ expect_caught "a tests/.clang-tidy that replaces the root config instead of exte "edit examples/lims/tests/.clang-tidy -e '/^InheritParentConfig:/d'" \ "has no 'InheritParentConfig: true'" +# morph#652's own defect, reintroduced: a library translation unit sharing a +# directory with the Catch2 tests, so the suppression covers it on the +# strength of an argument about REQUIRE that is not true of it. This is not a +# hypothetical shape -- examples/common/testkit/ held morph_ladder_testkit's +# two TUs for as long as that library existed, while the .clang-tidy beside +# them told every reader the suppression could not reach the code under test. +expect_caught "a non-Catch2 library source sharing the shared testkit's directory" \ + "printf '// SPDX-License-Identifier: Apache-2.0\\nnamespace morph { int probe() { return 0; } }\\n' > examples/common/testkit/probe_library_tu.cpp" \ + "examples/common/testkit/probe_library_tu.cpp" + +# The same shape one directory down, because the suppression reaches there +# too: clang-tidy walks up from the translation unit, so a nested source is +# governed exactly as a top-level one is. examples/bank/tests/gui/ is the live +# case this generalises. +expect_caught "a non-Catch2 library source in a subdirectory of a suppressed test directory" \ + "mkdir -p examples/bank/tests/gui && printf '// SPDX-License-Identifier: Apache-2.0\\nnamespace bank { int probe() { return 0; } }\\n' > examples/bank/tests/gui/probe_library_tu.cpp" \ + "examples/bank/tests/gui/probe_library_tu.cpp" + +# Vacuity guard on the scan that check 5's third condition runs. A directory +# carrying the suppression and holding no .cpp at all means either the +# suppression is pointing somewhere it does not belong, or this scan has +# stopped seeing the tree -- and a scan that examines nothing reports "all +# Catch2" exactly as loudly as one that examined everything. +expect_caught "a suppressed test directory the .cpp scan finds nothing in" \ + "find examples/common/testkit -name '*.cpp' -delete" \ + "holds no .cpp at all" + # The one-file shortcut that would satisfy every check above while taking the # check off every rung's src/, include/ and gui_lib/ as well. expect_caught "a single examples/.clang-tidy standing in for the per-directory ones" \ diff --git a/scripts/test_check_spec_sync.sh b/scripts/test_check_spec_sync.sh index 84f384f28..18887c6f2 100755 --- a/scripts/test_check_spec_sync.sh +++ b/scripts/test_check_spec_sync.sh @@ -59,6 +59,35 @@ expect_accepted() { fi } +# The same two, for the cases that turn on an *argument* rather than on the +# path list. Separate helpers rather than an extra parameter on the two above, +# so that the fifteen existing call sites keep saying exactly what they say. +expect_rejected_with_args() { + local description="$1" paths="$2" expected="$3"; shift 3 + local output + if output="$(printf '%s\n' "$paths" | bash "$checker" "$@" 2>&1)"; then + fail "NOT caught: ${description} -- the gate passed an invocation it must reject" + return + fi + if printf '%s' "$output" | grep -qF "$expected"; then + note "rejected: ${description}" + else + fail "rejected for the WRONG reason: ${description} -- no diagnostic containing '${expected}':" + printf '%s\n' "$output" >&2 + fi +} + +expect_accepted_with_args() { + local description="$1" paths="$2"; shift 2 + local output + if output="$(printf '%s\n' "$paths" | bash "$checker" "$@" 2>&1)"; then + note "accepted: ${description}" + else + fail "FALSE POSITIVE: ${description} -- the gate rejected an invocation it must accept:" + printf '%s\n' "$output" >&2 + fi +} + # The live gap morph#560 reported, in its minimal form. expect_rejected "a morph::net header change with no spec change" \ "include/morph/net/detail/ws_frame.hpp" \ @@ -110,6 +139,49 @@ expect_accepted "a qt and detail change with no spec change" \ expect_accepted "a change touching no headers at all" \ "$(printf 'README.md\ntests/test_bridge.cpp')" +# ── Empty input: both paths pinned, because only one of them is obvious ───── +# +# morph#655. An empty path list is what a broken producer hands this gate -- a +# failed `git diff`, a mistyped base ref, a pipeline in a shell without +# `set -o pipefail`. It used to answer `Spec sync OK: the change touches no +# files.` and exit 0, which is honest and still a green tick for a check that +# read nothing. It is now an error unless the caller asks for it. +# +# Both directions are pinned here deliberately. Rejecting empty input is +# useless if --allow-empty stops working, because every caller that +# legitimately has nothing to check would then be wedged; and accepting it +# with the flag is useless if the flag is the only path anyone tests, because +# a gate that quietly went back to accepting silence would pass that test too. +expect_rejected "an empty path list with no --allow-empty" \ + "" \ + "the path list on stdin is empty, and --allow-empty was not given" + +expect_accepted_with_args "an empty path list with --allow-empty" \ + "" --allow-empty + +# Whitespace-only is the same case: `printf '%s\n' "$changed"` with an empty +# `$changed` is exactly what .github/workflows/spec-sync.yml pipes, so the +# rejection has to survive the newline that shape adds. +expect_rejected "a path list of nothing but whitespace" \ + "$(printf ' \n\t\n ')" \ + "the path list on stdin is empty, and --allow-empty was not given" + +# An unrecognised argument must not be silently ignored: a typo'd +# `--allow_empty` that the gate shrugged off would fail every legitimately +# empty invocation, and a typo'd anything-else would hide whatever the caller +# meant to ask for. +expect_rejected_with_args "an unrecognised argument" \ + "README.md" \ + "unrecognised argument: --allow_empty" \ + --allow_empty + +# The flag must not become a way to pass a real violation. It governs the +# empty case and nothing else. +expect_rejected_with_args "--allow-empty over a real violation" \ + "include/morph/net/detail/ws_frame.hpp" \ + "include/morph/net/** changed but none of its spec paths did" \ + --allow-empty + # ── Cases that need a mutated tree ────────────────────────────────────────── # # The checker resolves its own root with `git rev-parse` and reads the tree for