diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d5c8b93cbe..1786759aa0 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -218,7 +218,8 @@ On every arch except STM32WL and bare nRF52832 (`WARM_NODE_COUNT > 0`), a node e - **Write:** `getOrCreateMeshNode`'s eviction and `demoteOldestHotNodesToWarm` (the over-cap boot migration) call `warmStore.absorb(num, last_heard, key)` _before_ the node leaves the header. - **Read-back:** `getOrCreateMeshNode` calls `warmStore.take()` to rehydrate `last_heard` + key when a warm node is re-admitted; `copyPublicKey()` falls back to the warm tier so the PKI send path finds keys for evicted peers. - **Persistence:** nRF52840 uses a 12 KB raw-flash record-ring at `0xEA000` (below LittleFS; append + replay + compact-on-rotate, link-guarded by `nrf52840_s140_v7.ld` and `extra_scripts/nrf52_warm_region.py`). Everywhere else: a `/prefs/warm.dat` snapshot flushed by `saveIfDirty()` on the node-DB save cadence. -- **Tunables** (`mesh-pb-constants.h`): `WARM_NODE_COUNT` (per-arch; `0` disables the tier) and `MAX_NUM_NODES` (hot cap - 120 on nRF52840/generic ESP32 to fit the 28 KB LittleFS; ESP32-S3 keeps its flash-scaled 100/200/250, portduino 250). Verbose migration/self-care tracing routes through `LOG_MIGRATION`, gated by `MESHTASTIC_NODEDB_MIGRATION_VERBOSE`. +- **Tunables** (`mesh-pb-constants.h`): `WARM_NODE_COUNT` (per-arch; `0` disables the tier) and `MAX_NUM_NODES` (hot cap - 120 on nRF52840/generic ESP32 to fit the 28 KB LittleFS; ESP32-S3 picks 100/200/250 at boot from its flash size). Verbose migration/self-care tracing routes through `LOG_MIGRATION`, gated by `MESHTASTIC_NODEDB_MIGRATION_VERBOSE`. +- **`MAX_NUM_NODES` on native is not in that header and is not a constant.** `variants/native/portduino{,-buildroot}/variant.h` define it as `portduino_config.MaxNodes` - resolved at **runtime**, default **200**, overridable per-host with `General: MaxNodes` in the portduino YAML. `variant.h` is reached first, so the `ARCH_PORTDUINO` branch in `mesh-pb-constants.h` never fires; it is now `#error`-guarded rather than holding a plausible-looking `250`. Reading 250 there yields a protected-node cap of 248 when the real one is 198 (`numProtectedNodes() < MAX_NUM_NODES - 2`), which has already produced one wrong diagnosis. The separate 250 in `NodeDB::getMaxNodesAllocatedSize()` is `NODEDB_MIGRATION_LOAD_CEILING`, a decode allowance for files from larger-cap firmware - not a cap. ### Satellite caps @@ -312,7 +313,7 @@ firmware/ │ └── native/ # Linux/Portduino variants ├── protobufs/ # Protocol buffer definitions ├── boards/ # Custom PlatformIO board definitions -├── test/ # Unit tests (12 test suites) +├── test/ # Native unit-test suites (count: test/native-suite-count) └── bin/ # Build and utility scripts ``` @@ -662,9 +663,10 @@ Most workflows can be triggered manually via `workflow_dispatch` for testing. ### Native unit tests (C++) -Unit tests in `test/` directory. The canonical suite count is in `test/native-suite-count` and is cross-checked on every full run. Current suites: +Unit tests in `test/` directory. The canonical suite count is in `test/native-suite-count`, cross-checked against `test/test_*` on every full run and by the `suite-count-check` CI job. **Never state the count as a literal anywhere else** - point at that file. The list below is a partial description of what suites cover, not an inventory: -- `test_admin_radio/` - LoRa region/config validation and AdminModule dispatch +- `test_admin_radio/` - LoRa region/config validation, AdminModule dispatch, node-DB metadata saves +- `test_fscommon_getfiles/` - bounded file-manifest walk (cap, depth, truncation reporting) - `test_atak/` - ATAK integration - `test_crypto/` - Cryptography - `test_default/` - Default configuration @@ -691,21 +693,31 @@ Unit tests in `test/` directory. The canonical suite count is in `test/native-su - `test_utf8/` - UTF-8 utilities - `test_warm_store/` - Warm-tier node store -**Preferred run command - `bin/run-tests.sh`** (uses the `coverage` env with ASan/LSan sanitizers; emits a machine-readable verdict on the final line; update `test/native-suite-count` when adding or removing suites): +**Preferred run command - `bin/run-tests.sh`** (defaults to the `coverage` env; emits a machine-readable verdict on the final line; update `test/native-suite-count` when adding or removing suites): ```bash ./bin/run-tests.sh # all suites ./bin/run-tests.sh -f test_traffic_management # single suite (yields FILTERED, not GREEN) ``` +**The harness is Linux-only, and rejects anything else.** `bin/run-tests.sh` needs bash 4+ and GNU coreutils/find (`find -printf`, `md5sum`, `-executable`), so it exits 2 on a non-Linux `uname` rather than degrade quietly - a state check that silently mis-hashes a sandbox still prints a verdict, and that verdict would be worthless. The `native-macos` PlatformIO env is a **build** target for `meshtasticd`, not a test host. On macOS or Windows use `./bin/test-native-docker.sh`. + +**Sanitizer coverage is per env, and only one env has any.** `coverage` (the default) adds gcov + ASan/LSan on top of `native`. **`native` itself has none** - verified, zero ASan symbols in the built binary. A `-e native` run is _not_ sanitized, so do not reason from "run-tests.sh uses ASan" when you passed `-e native`. + +**A signal name from the runner is not a crash.** `exit(UNITY_END())` returns the failure count, and PlatformIO's native runner renders a non-zero exit code as a POSIX signal - 4 failures prints `Program received signal SIGILL`, 5 prints `SIGTRAP`, and the suite is reported `[ERRORED]` instead of `[FAILED]`. Check the exit code against the failure count before theorising about memory bugs; confirm any real crash under a debugger. + +**Suite order is randomisable.** `./bin/run-tests.sh --shuffle` runs suites in a seeded random order; `--seed ` replays one. The seed defaults to the commit SHA (deterministic per commit, varied across commits), is printed at the start and on the `RESULT:` line, and the full order is printed on failure. CI shuffles its area order the same way, seeded from `GITHUB_SHA`. A single green seed is not evidence of order independence. + +**`-f` is not a gate.** A filtered run can pass while a full run fails, because filtering removes the suites that _create_ the state a later suite trips over. Iterate with `-f`; gate on a full run. + Exit codes and verdicts (exact counts will vary; examples below are illustrative): -| Exit | Verdict | Meaning | -| ---- | ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| 0 | `GREEN` | All canonical suites ran, all passed, no ignored test cases | -| 1 | `RED` | At least one failure, build error, or sanitizer fault | -| 2 | `AMBER` | All that ran passed, but something was lost: a suite silently went missing on a full run, individual test cases were skipped (`TEST_IGNORE`), or `test/native-suite-count` disagrees with the `test/` directory count | -| 3 | `FILTERED` | A `-f` run completed cleanly; suites outside the filter were intentionally not run | +| Exit | Verdict | Meaning | +| ---- | ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| 0 | `GREEN` | All canonical suites ran, all passed, no ignored test cases | +| 1 | `RED` | At least one failure, build error, or sanitizer fault | +| 2 | `AMBER` | All that ran passed, but something was lost or unexplained: a suite silently went missing on a full run, individual test cases were skipped (`TEST_IGNORE`), `test/native-suite-count` disagrees with the `test/` directory count, or a suite left behind shared state it does not declare | +| 3 | `FILTERED` | A `-f` run completed cleanly; suites outside the filter were intentionally not run | Examples - exact counts will vary by suite count and env: @@ -745,6 +757,30 @@ Simulation testing: `bin/test-simulator.sh` Quick entry point for new test modules: `test/README.md` (native unit-test authoring guide, skeleton, pitfalls, and setup checklist). +### Shared state: every suite gets a clean sandbox + +Each suite runs inside its own scratch `$HOME` (`bin/pio-test-isolate.sh`, wired in per env as `test_testing_command`, so a bare `pio test` and CI get it too). **State never crosses a suite boundary.** Mutation _inside_ a suite is free; carrying state _out_ of one is impossible by construction, not by policy. + +The state in question lives in `~/.portduino/default/prefs/` - `nodes.proto`, `config.proto`, `channels.proto`, `module.proto`, `device.proto`, `warm.dat`, `transmit_history.dat`. `NodeDB`'s constructor calls `loadFromDisk()`, so any suite that constructs one reads it, and several `NodeDB` paths (`removeNodeByNum()`, `resetNodes()`, `nodeDBSelfCare()`, and the constructor when the file is absent) write it without being asked. + +Two orthogonal axes: **PASS/FAIL x CLEAN/DIRTY**. + +- **CLEAN** - nothing changed, or everything that changed is declared. +- **DIRTY** - an undeclared path changed. Graded **AMBER**: with isolation in place it means "undeclared", not "dangerous". +- **MISSING** - a declared write did not happen. A warning only; it catches persistence that silently stopped working. + +Declare deliberate writes in **`test/state-manifest.tsv`** - one central file, `` / `` / ``, with the reason mandatory and reviewed on change. Central so every opt-out is visible in one diffable list; per-suite files hide growth. `run-tests.sh` prints how many suites declare non-default handling on every run. + +| Flag | Meaning | +| ----------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| _(no entry)_ | the default: fresh state in, contents discarded out | +| `writes=` | files this suite mutates on purpose; matched on the path relative to the sandbox `$HOME` or just the basename | +| `state=per-suite` | state persists across this suite's own test cases (persistence round-trips, migration ladders). Only the suite boundary is checked; the default is per-test, which names the exact test that dirtied things | + +No flag grants cross-suite carry. A suite that needs another suite's output needs an explicit fixture, not inheritance. + +`./bin/run-tests.sh --write-manifest` prints the entries a run would need, for a human to paste and justify - it never applies them, and neither does CI. `bin/test-state-check.sh` is the checker's own self-test: fixtures asserting CLEAN / CLEAN / DIRTY / MISSING, plus the before-empty assertion. + ### Hardware-in-the-loop tests ([meshtastic-mcp](https://github.com/meshtastic/meshtastic-mcp)) Separate pytest suite that exercises real USB-connected Meshtastic devices. It now lives in the standalone [meshtastic-mcp](https://github.com/meshtastic/meshtastic-mcp) repo, run against a firmware checkout via `MESHTASTIC_FIRMWARE_ROOT`. See the **MCP Server & Hardware Test Harness** section below for invocation, tier layout, and agent usage rules. diff --git a/.github/node-id-format-allowlist.txt b/.github/node-id-format-allowlist.txt new file mode 100644 index 0000000000..ec0830ca76 --- /dev/null +++ b/.github/node-id-format-allowlist.txt @@ -0,0 +1,15 @@ +# Exception list for bin/lint-node-id-format.sh (trunk linter: node-id-format). +# +# Format: [:] +# - "" exempts the whole file +# - ":" exempts one call site +# - blank lines and # comments are ignored; the reason column is mandatory +# +# Intentionally empty. The known pre-existing sites - PacketHistory.cpp, NodeInfoModule.cpp +# and PositionModule.cpp - are deliberately NOT listed, so trunk surfaces them the next time +# someone edits those files and they get cleaned up in the change that was already touching +# them. The rule emits "note", trunk's only non-blocking level, so this costs a notice rather +# than a red PR. +# +# Add an entry only for a value the linter has misread as an ID - a CRC, a register, a hash - +# and say which, so the next reader can tell a real exemption from a deferred cleanup. diff --git a/.github/workflows/test_native.yml b/.github/workflows/test_native.yml index 2c677a5440..2688a07b93 100644 --- a/.github/workflows/test_native.yml +++ b/.github/workflows/test_native.yml @@ -2,6 +2,16 @@ name: Run Tests on Native platform on: workflow_call: + inputs: + suite_order_seed: + description: >- + Seed for shuffling the test-area order. Empty (the default) means: fixed declared order on + pull_request, so a contributor's PR never turns red because of an order they did not + choose; commit-SHA-derived elsewhere. Set a number to force that exact order anywhere - + that is how you replay a shuffled failure. + type: string + required: false + default: "" workflow_dispatch: permissions: {} @@ -201,6 +211,11 @@ jobs: - name: Run tests one area at a time shell: bash + # Both values reach the script through env: rather than ${{ }} inside run:, so nothing from + # the event payload is ever spliced into the shell text. + env: + SUITE_ORDER_SEED: ${{ inputs.suite_order_seed }} + EVENT_NAME: ${{ github.event_name }} run: | set -uo pipefail # One runner, no matrix, no concurrency. Group the test_* suites by area and run each @@ -236,6 +251,41 @@ jobs: for rule in "${area_rules[@]}"; do run_order+=("${rule%%:*}"); done run_order+=("misc") + # Area order. The rule order above is an accident of how the areas were written, and + # running it fixed forever means order dependence between areas is never observed - but + # randomising it on a contributor's PR would turn their run red for an order they did not + # choose, which is how a randomisation gets reverted instead of the coupling fixed. + # + # So: pull_request keeps the fixed declared order. Everywhere else (push, schedule) the + # order is shuffled, seeded from the commit SHA - deterministic per commit, replayable, + # attributable, and it never blocks someone else's PR. An explicit seed input overrides + # both, which is how you replay a specific failing order anywhere. + # + # Intra-area order stays PlatformIO's: filters select suites, they do not order them + # (list_test_names() walks test/ with os.walk()), so controlling it needs one invocation + # per suite. bin/run-tests.sh --shuffle does exactly that locally. + seed_input="${SUITE_ORDER_SEED:-}" + if [ -n "$seed_input" ]; then + seed="$seed_input" + echo "area order: shuffled with explicitly supplied seed $seed" + elif [ "${EVENT_NAME:-}" = "pull_request" ]; then + seed="" + echo "area order: fixed declared order (pull_request) - ${run_order[*]}" + echo " to exercise a different order, re-run this workflow with a suite_order_seed input" + else + seed=$((16#${GITHUB_SHA:0:8})) + echo "area order: shuffled with seed $seed (from ${GITHUB_SHA:0:8})" + fi + + if [ -n "$seed" ]; then + # Same shuffle_suites() bin/run-tests.sh uses, so the replay hint below is true by + # construction rather than by two copies happening to agree. + source bin/lib/shuffle.sh + mapfile -t run_order < <(shuffle_suites "$seed" "${run_order[@]}") + echo "area order: ${run_order[*]}" + echo " replay locally: ./bin/run-tests.sh --shuffle --seed $seed" + fi + fail=0 for a in "${run_order[@]}"; do [ -n "${group[$a]:-}" ] || continue diff --git a/.trunk/trunk.yaml b/.trunk/trunk.yaml index 741a51e584..7b45c5f830 100644 --- a/.trunk/trunk.yaml +++ b/.trunk/trunk.yaml @@ -66,9 +66,37 @@ lint: run: ${workspace}/bin/lint-ifdef-complexity.sh ${target} success_codes: [0] read_output_from: stdout + # Flags node/packet IDs logged as bare %08x instead of the 0x%08x convention in + # src/mesh/RadioInterface.cpp. Emits "note", trunk's only non-blocking level, so it + # advises without gating - including on the known pre-existing sites, which are left + # unlisted so they get cleaned up by whoever next edits those files. + - name: node-id-format + files: [cpp-sources] + commands: + - name: lint + output: regex + parse_regex: (?P.+):(?P\d+):(?P\d+):(?P\w+):(?P.+):(?P[a-z-]+) + run: ${workspace}/bin/lint-node-id-format.sh ${target} + success_codes: [0] + read_output_from: stdout + # Flags a UNITY_END() not wrapped in exit(). A bare one ends the reporting, not the suite: + # the runtime keeps calling loop(), so the process never exits, its sandbox is deleted + # underneath it, and its .gcda and LeakSanitizer report never flush. Emits "note" because the + # enforcing half is bin/pio-test-isolate.sh, which catches an actual survivor at run time. + - name: unity-exit + files: [cpp-sources] + commands: + - name: lint + output: regex + parse_regex: (?P.+):(?P\d+):(?P\d+):(?P\w+):(?P.+):(?P[a-z-]+) + run: ${workspace}/bin/lint-unity-exit.sh ${target} + success_codes: [0] + read_output_from: stdout enabled: - ascii-dash@SYSTEM - too-many-defined@SYSTEM + - node-id-format@SYSTEM + - unity-exit@SYSTEM - checkov@3.3.8 - renovate@44.2.3 - prettier@3.9.6 diff --git a/AGENTS.md b/AGENTS.md index 9423c51692..9dc3fa22e6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -107,7 +107,16 @@ Sequence these; don't parallelize on the same port. 4. On failure, open the run's `tests/report.html` → `Meshtastic debug` section for the firmware log tail + device state dump 5. Iterate -### Debugging a flaky test +### Debugging a native unit-test failure + +1. **Run the full suite before believing a filtered one.** `-f` is not a gate: it removes the suites that _create_ the shared state a later suite trips over. +2. **A signal name is not a crash.** `exit(UNITY_END())` returns the failure count and PlatformIO renders it as a signal (4 -> `SIGILL`, 5 -> `SIGTRAP`), reporting `[ERRORED]`. Match it against the failure count first. +3. **Check the CLEAN/DIRTY axis.** Each suite runs in its own scratch `$HOME`; deliberate writes are declared in `test/state-manifest.tsv`. A DIRTY verdict names the suite and the undeclared path, and the kept sandbox under `.pio/test-state//` is a replayable reproduction. +4. **Sanitizers are per env** - `coverage` has ASan/LSan, `native` has none. Don't reason from ASan on a `-e native` run. +5. **Reproduce a shuffled order.** `--shuffle` prints its seed and puts it on the `RESULT:` line; `--seed ` replays that exact order. One green seed proves nothing about order independence. +6. **Exit 2 with "Linux-only" is the host, not the tests.** The harness needs bash 4+ and GNU coreutils/find and rejects any other `uname` rather than degrade quietly. `native-macos` is a build target, not a test host; elsewhere use `./bin/test-native-docker.sh`. + +### Debugging a flaky hardware test 1. `/repro [count]` - re-runs the test N times, diffs firmware logs between passes and failures 2. If the first attempt always fails and the rest pass, that's a state-leak pattern → suggest `--force-bake` or a clean device state, don't chase the first failure @@ -122,7 +131,7 @@ Sequence these; don't parallelize on the same port. | `src/modules/` | Feature modules; `Telemetry/Sensor/` has 50+ I2C sensor drivers | | `variants/` | 200+ hardware variant definitions (`variant.h` + `platformio.ini` per board) | | `protobufs/` | `.proto` definitions; regenerate with `bin/regen-protos.sh` | -| `test/` | Firmware unit tests (19 suites; `./bin/run-tests.sh` preferred, falls back to `pio test -e native`) | +| `test/` | Firmware unit tests (count: `test/native-suite-count`; `./bin/run-tests.sh` preferred, falls back to `pio test -e native`) | | [meshtastic-mcp](https://github.com/meshtastic/meshtastic-mcp) | Standalone MCP server + tiered pytest hardware harness (`unit/`, `mesh/`, `telemetry/`, `monitor/`, `recovery/`, `ui/`, `fleet/`, `admin/`, `provisioning/`) - registered here via `.mcp.json` | | `.github/prompts/` | Copilot prompt bodies (firmware scaffolding: new module / sensor / variant) | | `.github/copilot-instructions.md` | **Primary agent instructions - read this** | diff --git a/bin/lib/shuffle.sh b/bin/lib/shuffle.sh new file mode 100644 index 0000000000..89ddb97dc5 --- /dev/null +++ b/bin/lib/shuffle.sh @@ -0,0 +1,29 @@ +# shellcheck shell=bash +# +# The seeded shuffle shared by bin/run-tests.sh and .github/workflows/test_native.yml. Sourced by +# both so there is exactly one implementation; nothing here executes on its own. +# +# This has to live in one place. The workflow prints "replay locally: ./bin/run-tests.sh --shuffle +# --seed $seed" after a CI shuffle, and that instruction is only true while CI and the local script +# produce the same permutation for a seed. Two copies of the algorithm cannot be relied on to stay +# byte-identical, and the way they'd announce their drift is a replay that quietly reproduces a +# different order than the one that failed. + +# Deterministic Fisher-Yates over a MINSTD generator rather than awk's rand(), whose sequence +# differs between gawk and mawk - a seed that does not reproduce the same order on another machine +# is not a seed. +shuffle_suites() { + local seed="$1" + shift + # Nothing in, nothing out. `printf '%s\n'` with no arguments still writes one empty line, and the + # callers read this through mapfile - so an empty suite list would arrive as a suite named "". + (($#)) || return 0 + printf '%s\n' "$@" | awk -v seed="$seed" ' + function rnd() { s = (s * 16807) % 2147483647; return s / 2147483647 } + BEGIN { s = seed % 2147483647; if (s <= 0) s += 2147483646 } + { a[NR] = $0 } + END { + for (i = NR; i > 1; i--) { j = int(rnd() * i) + 1; t = a[i]; a[i] = a[j]; a[j] = t } + for (i = 1; i <= NR; i++) print a[i] + }' +} diff --git a/bin/lib/test-state.sh b/bin/lib/test-state.sh new file mode 100644 index 0000000000..a0c6244555 --- /dev/null +++ b/bin/lib/test-state.sh @@ -0,0 +1,169 @@ +# shellcheck shell=bash +# +# Shared helpers for the native test harness's shared-state check. Sourced by +# bin/pio-test-isolate.sh (which enforces it per suite) and bin/test-state-check.sh (which proves +# the checker itself still works). Nothing here executes on its own. +# +# Linux-only, like the rest of the native harness: this uses GNU coreutils behaviour (`find -printf`, +# md5sum) rather than carrying a per-host fallback. bin/run-tests.sh states and enforces that. +# +# The check answers one question: did this suite change any file it did not declare? It deliberately +# does NOT compare file *contents* against a baseline. Content baselines over protobuf bytes are +# snapshot tests - add a field to NodeInfoLite and every recorded hash in the repo churns, which is +# how snapshot suites turn into an --update-all ritual and then into noise. Hashes are used only to +# answer the boolean "did this change?"; what gets declared and reviewed is the set of paths. + +STATE_MANIFEST_DEFAULT="test/state-manifest.tsv" + +# Files the native binaries persist under $HOME. Listed for documentation and for the +# --write-manifest hint; the scan itself is unfiltered, so a suite writing somewhere unexpected is +# still caught. +# shellcheck disable=SC2034 # referenced by callers and by the docs +STATE_KNOWN_FILES="nodes.proto config.proto channels.proto module.proto device.proto warm.dat transmit_history.dat" + +# Guard the guard: refuse to run a suite against a sandbox that is not empty. If isolation ever +# leaks, the after-diff measures against the wrong baseline and the whole check reports CLEAN while +# meaning nothing - so before-empty is as load-bearing as after-diff. Returns non-zero and explains +# itself rather than carrying on. +state_assert_empty() { + local dir="$1" + if [[ -n $(find "$dir" -mindepth 1 -print -quit 2>/dev/null) ]]; then + echo "test-state: sandbox $dir is not empty before the suite ran - isolation is broken" >&2 + return 1 + fi + return 0 +} + +# Fingerprint every file under $1 as " ", sorted. Empty output for an empty or +# missing tree. Output is fed to comm/diff, so the sort order has to be stable across calls. +# +# GNU `find -printf` and md5sum(1), deliberately: this harness is Linux-only and bin/run-tests.sh +# refuses to start anywhere else, so there is no host here that needs a BSD fallback. +state_fingerprint() { + local root="$1" + [[ -d $root ]] || return 0 + ( + cd "$root" || return 0 + find . -type f -printf '%P\n' 2>/dev/null | LC_ALL=C sort | while IFS= read -r rel; do + printf '%s %s\n' "$rel" "$(md5sum -- "$rel" 2>/dev/null | cut -d' ' -f1)" + done + ) +} + +# Processes still running inside the suite's sandbox $HOME. Prints one PID per line. +# +# A suite that ends on a bare UNITY_END() does not stop: setup() returns, the runtime keeps calling +# loop(), and PlatformIO - which reports a suite from its Unity output, not from process exit - +# moves on with the binary still resident. Nothing else notices, and the damage is quiet: the +# sandbox gets deleted under a live process, so the after-fingerprint below describes what the suite +# had written when we stopped looking rather than what it left behind, and .gcda plus LeakSanitizer +# both flush from atexit handlers that never run. +# +# Matching on the environment rather than on a remembered PID is deliberate: the sandbox HOME is +# mktemp-unique per suite, so this identifies survivors whatever their parentage - a fork, a +# grandchild, a process already reparented to init - none of which a $! comparison would catch. +# Scoped to this user's processes: /proc//environ is unreadable for anyone else's anyway, and +# the narrower sweep costs ~270ms against ~460ms for all of /proc. +state_find_survivors() { + local home="$1" pid + [[ -n $home ]] || return 0 + for pid in $(ps -u "$(id -u)" -o pid= 2>/dev/null); do + [[ $pid == "$$" ]] && continue + if tr '\0' '\n' <"/proc/$pid/environ" 2>/dev/null | grep -qxF "HOME=$home"; then + printf '%s\n' "$pid" + fi + done +} + +# Paths present in the "after" fingerprint ($2) that are absent or different in "before" ($1). +# Prints one relative path per line. +state_changed_paths() { + local before="$1" after="$2" + LC_ALL=C comm -13 <(LC_ALL=C sort "$before") <(LC_ALL=C sort "$after") | awk '{print $1}' | LC_ALL=C sort -u +} + +# Read a suite's flag string out of the manifest. Empty when the suite has no entry, which is the +# default and means "isolated": fresh state in, contents discarded out. +# +# Manifest format - TSV, three columns, the same shape as an allowlist entry: the thing, what it is +# allowed to do, and why. The reason column is mandatory and is what a reviewer reads. +# +# test_nodedb_blockedstate=per-suite writes=nodes.protosaturates the DB to test the cap +state_manifest_flags() { + local suite="$1" manifest="${2:-$STATE_MANIFEST_DEFAULT}" + [[ -f $manifest ]] || return 0 + awk -F'\t' -v s="$suite" '!/^[[:space:]]*#/ && $1 == s { print $2; exit }' "$manifest" +} + +# Pull one flag's value out of a flag string: state_flag_value "writes" "state=per-suite writes=a,b" +state_flag_value() { + local key="$1" flags="$2" f + for f in $flags; do + [[ $f == "$key="* ]] && { + printf '%s' "${f#"$key="}" + return 0 + } + done + return 0 +} + +# Does a changed path match a declared write? A declaration matches either the full path relative to +# the scratch HOME or just the basename, because the useful name for these is the basename +# (`nodes.proto`) and nobody should have to write .portduino/default/prefs/ in front of it. +state_path_declared() { + local path="$1" declared="$2" entry + IFS=',' read -ra _entries <<<"$declared" + for entry in "${_entries[@]}"; do + [[ -z $entry ]] && continue + [[ $path == "$entry" || ${path##*/} == "$entry" ]] && return 0 + done + return 1 +} + +# Classify a suite's leftovers. Prints "\t" where verdict is one of: +# +# CLEAN nothing changed, or everything that changed was declared +# DIRTY at least one undeclared path changed - the finding this whole check exists for +# MISSING every changed path was declared, but a declared path did NOT change +# +# MISSING is reported separately rather than folded into DIRTY because it catches the opposite bug: +# persistence that silently stopped happening. That is a real class here - the TAK config bug +# upstream was a has_ flag never being set, so the save wrote nothing and no test noticed. It starts +# as a warning because some declared writes are legitimately conditional. +state_classify() { + local changed="$1" declared="$2" + local undeclared=() missing=() path entry found + + while IFS= read -r path; do + [[ -z $path ]] && continue + if ! state_path_declared "$path" "$declared"; then + undeclared+=("$path") + fi + done <<<"$changed" + + # Ask state_path_declared() in the other direction rather than matching by hand: one rule for + # "does this path match this declaration", so the two directions cannot drift apart. Matching + # an entry as a regex would also let a metacharacter in a manifest name (`.`, `+`) match a file + # that is not the declared one. + IFS=',' read -ra _declared <<<"$declared" + for entry in "${_declared[@]}"; do + [[ -z $entry ]] && continue + found=1 + while IFS= read -r path; do + [[ -z $path ]] && continue + if state_path_declared "$path" "$entry"; then + found=0 + break + fi + done <<<"$changed" + ((found)) && missing+=("$entry") + done + + if ((${#undeclared[@]} > 0)); then + printf 'DIRTY\tundeclared: %s\n' "${undeclared[*]}" + elif ((${#missing[@]} > 0)); then + printf 'MISSING\tdeclared but unwritten: %s\n' "${missing[*]}" + else + printf 'CLEAN\t\n' + fi +} diff --git a/bin/lint-node-id-format.sh b/bin/lint-node-id-format.sh new file mode 100755 index 0000000000..395dbfc01b --- /dev/null +++ b/bin/lint-node-id-format.sh @@ -0,0 +1,102 @@ +#!/usr/bin/env bash +# lint-node-id-format.sh - flag node/packet IDs logged as bare %08x instead of 0x%08x. +# +# src/mesh/RadioInterface.cpp states the convention: node IDs and packet IDs are +# formatted 0x%08x in logs and !%08x in user-facing display. A bare %08x still +# prints the right digits, so nothing breaks - it just makes the value hard to +# grep for and easy to misread as decimal. +# +# Emitted at "note" on purpose: this is a consistency rule, not a correctness one, so +# it should never be the thing that fails someone's review. Note is trunk's only +# non-blocking level - "warning" and "info" both exit non-zero and would gate CI. +# +# Only flags a %08x whose statement also mentions an ID-shaped argument +# (->num, .from, nodeNum, getFrom(), ...). A 32-bit hex value that is not an ID - +# a CRC, a register, a hash - is none of this rule's business. +# +# Emits one line per finding in the format +# ::::: +# which trunk parses via parse_regex. Always exits 0; findings go to stdout. + +set -uo pipefail + +ALLOWLIST=".github/node-id-format-allowlist.txt" + +for target in "$@"; do + [[ -f $target ]] || continue + + # Path is reported relative to the workspace so allowlist entries stay portable. + rel="${target#"$PWD"/}" + + awk -v path="$rel" -v allowlist="$ALLOWLIST" ' + BEGIN { + LINE_CAP = 12 # give up accumulating a statement after this many lines + + # Allowlist entries are "" (whole file) or ":", followed by + # whitespace and a mandatory reason. Blank lines and # comments are ignored. + while ((getline line < allowlist) > 0) { + sub(/#.*/, "", line) + gsub(/^[ \t]+|[ \t]+$/, "", line) + if (line == "") continue + split(line, f, /[ \t]+/) + skip[f[1]] = 1 + } + close(allowlist) + if (path in skip) exempt_file = 1 + } + + # Two independent signals, either of which marks the value as an ID: an ID-shaped + # argument, or the message text naming one. The second catches the common + # LOG_INFO("node %08x", n) shape, where the argument alone is indistinguishable + # from a CRC. Deliberately a allowlist of shapes rather than "any variable" - a + # false positive here costs more than a miss. + function looks_like_id(s) { + return (s ~ /(->|\.)(num|from|to|id|dest|sender|relay_node|next_hop)[^A-Za-z0-9_]/) || + (s ~ /[Nn]ode[Nn]um/) || (s ~ /nodeId/) || (s ~ /getFrom[ \t]*\(/) || + (s ~ /[^A-Za-z0-9_]sender[^A-Za-z0-9_]/) || + (s ~ /[Nn]ode/) || (s ~ /[Pp]acket/) || (s ~ /[Ss]ender/) || (s ~ /[Rr]elay/) + } + + # True when the text still contains a %08x after every correctly-prefixed + # 0x%08x has been removed - i.e. at least one occurrence is bare. + function has_bare_hex(s, t) { + t = s + gsub(/0[xX]%08[xX]/, "", t) + gsub(/![ \t]*%08[xX]/, "", t) # !%08x is the user-facing display form, also fine + return (t ~ /%08[xX]/) + } + + { + if (exempt_file) next + + # Accumulate a logical LOG_ statement; these routinely wrap across lines. + if (!in_stmt && $0 ~ /LOG_[A-Z]+[ \t]*\(/) { + in_stmt = 1; stmt = $0; start = NR; hit_line = 0; hit_col = 0 + } else if (in_stmt) { + stmt = stmt " " $0 + } else { + next + } + + # Remember the first line carrying a bare %08x, for a useful caret position. + if (!hit_line && has_bare_hex($0)) { hit_line = NR; hit_col = index($0, "%08") } + + # End of statement: any line closing the call. Matched anywhere on the line, not + # just at EOL, so `LOG_INFO(...); }` terminates too - if it did not, the + # accumulator would run to EOF and silently swallow every later finding in the + # file. LINE_CAP is the same backstop for a close paren we never see at all. + if ($0 ~ /\)[ \t]*;/ || NR - start >= LINE_CAP) { + if (has_bare_hex(stmt) && looks_like_id(stmt)) { + if (!hit_line) { hit_line = start; hit_col = 1 } + if ((path ":" hit_line) in skip) { in_stmt = 0; next } + printf "%s:%d:%d:%s:%s:%s\n", path, hit_line, (hit_col ? hit_col : 1), "note", + "node/packet ID logged as bare %08x - use 0x%08x (see src/mesh/RadioInterface.cpp)", + "node-id-format" + } + in_stmt = 0 + } + } + ' "$target" +done + +exit 0 diff --git a/bin/lint-unity-exit.sh b/bin/lint-unity-exit.sh new file mode 100755 index 0000000000..94887886a4 --- /dev/null +++ b/bin/lint-unity-exit.sh @@ -0,0 +1,166 @@ +#!/usr/bin/env bash +# lint-unity-exit.sh - flag a UNITY_END() that is not wrapped in exit(). +# +# A bare UNITY_END() ends the *reporting*, not the suite: setup() returns, the runtime goes on +# calling loop(), and the process runs forever. PlatformIO does not notice - it reads the Unity +# summary off stdout, reports the suite PASSED and moves on - so the run is green while the binary +# is still resident. The costs are invisible by construction: the per-suite sandbox is deleted +# under a live process, and .gcda coverage plus LeakSanitizer's report are both flushed by atexit +# handlers, so a suite that never exits contributes no coverage and gets no leak check. +# +# The rule is per occurrence, not per file. test/test_serial/SerialModule.cpp had a correct +# exit(UNITY_END()) in its ESP32 branch and bare ones in both #else branches; a "does this file +# call exit() anywhere" check passes it. The empty branch of a feature or architecture guard is +# the easiest one to get wrong, because it looks like there is nothing to clean up. +# +# Statement-aware, like bin/lint-node-id-format.sh and for the same reason: judging one physical +# line at a time reports `exit(\n UNITY_END());` as bare, and reports the interior lines of a +# /* ... */ block comment that happens to mention the macro. A note-level rule that cries wolf +# gets ignored, and then the real finding goes with it. +# +# bin/test-lint-unity-exit.sh is this rule's self-test. It exists because the scanner has now been +# wrong twice: every false positive and false negative found in review is pinned there as a +# fixture, so the next rewrite has to keep them all passing. +# +# Not handled: raw string literals (R"(...)"). There are none under test/, and delimiter tracking +# for a case that does not occur would be untested code guarding untested code. +# +# Emitted at "note" - trunk's only non-blocking level - because the enforcing half of this pair is +# bin/pio-test-isolate.sh, which detects an actual survivor at run time and grades it AMBER. This +# is the author-time advice that stops it being written in the first place. +# +# Emits one line per finding in the format +# ::::: +# which trunk parses via parse_regex. Always exits 0; findings go to stdout. + +set -uo pipefail + +for target in "$@"; do + [[ -f $target ]] || continue + + # Path is reported relative to the workspace so findings are clickable from the repo root. + rel="${target#"$PWD"/}" + + # Only test sources declare a suite's lifecycle. Unity's own headers and any production file + # mentioning the macro are none of this rule's business. + [[ $rel == test/* ]] || continue + + awk -v path="$rel" ' + # Return the line with comments and string/char literals removed, carrying /* ... */ state + # across lines. A character-level scan, not layered regexes: regexes cannot tokenise C++ and + # each attempt was wrong differently - a /* inside a string literal flipped comment state and + # hid real calls, a greedy .* swallowed the code between two comments on one line, and + # UNITY_END() inside a string read as code. Literals collapse to a space rather than vanishing, + # so a token cannot be glued to its neighbour. + # Also fills colmap[], mapping each position in the returned string back to its column in the + # raw line. Without it a caret cannot be placed: removing a comment or collapsing a literal + # shifts every later column, and counting occurrences in the raw line does not help either - + # TEST_MESSAGE("... UNITY_END() ..."); UNITY_END(); has two in the raw text and one in the code. + function strip_noncode(s, out, i, n, c, two, q) { + n = length(s); i = 1; out = "" + delete colmap + while (i <= n) { + if (in_block) { + if (substr(s, i, 2) == "*/") { in_block = 0; i += 2 } else { i++ } + continue + } + two = substr(s, i, 2) + if (two == "//") return out # rest of the line is a comment + if (two == "/*") { in_block = 1; i += 2; continue } + c = substr(s, i, 1) + if (c == "\"" || c == "'"'"'") { # skip a whole literal, honouring backslash escapes + q = c + out = out " "; colmap[length(out)] = i + i++ + while (i <= n) { + c = substr(s, i, 1) + if (c == "\\") { i += 2; continue } + i++ + if (c == q) break + } + continue + } + out = out c; colmap[length(out)] = i; i++ + } + return out + } + + # Is this one occurrence wrapped in a form that terminates the process? Two count: + # + # exit(UNITY_END()) the documented one + # int rc = UNITY_END() capture-then-exit, used by test_packet_signing to restore globals + # between the summary and the exit + # + # Judged per occurrence by looking back through whitespace, not by stripping forms out of the + # whole statement. A line carrying both - exit(UNITY_END()); UNITY_END(); - must report the bare + # call at the bare column, rather than once at whichever came first. + # + # Both forms are token-bounded. `exit` must be a whole identifier, so myexit(UNITY_END()) is + # still reported; the assignment must be a plain `=`, so `==`, `!=`, `<=`, `>=` and `+=` are not + # mistaken for a capture. `return UNITY_END()` is deliberately NOT accepted - it only terminates + # from main(), there is no main() under test/, and from a helper it just returns a count. + # + # Where the rule gives ground: capturing the value and then never exiting would leak and is not + # flagged. That is rarer than the bare call, and flagging a correct idiom would push someone to + # "fix" working code. + function is_wrapped(s, at, j, c, tail) { + j = at - 1 + while (j >= 1 && substr(s, j, 1) ~ /[ \t]/) j-- # skip space before the macro + if (j < 1) return 0 + + # exit ( UNITY_END - `exit` must be a whole identifier, so myexit( does not qualify + if (substr(s, j, 1) == "(") { + j-- + while (j >= 1 && substr(s, j, 1) ~ /[ \t]/) j-- + if (j >= 4 && substr(s, j - 3, 4) == "exit" && + (j - 4 < 1 || substr(s, j - 4, 1) !~ /[A-Za-z0-9_]/)) return 1 + return 0 + } + + # = UNITY_END - a plain assignment is capture-then-exit; ==, !=, <=, >=, += are not + if (substr(s, j, 1) == "=") { + c = (j - 1 >= 1) ? substr(s, j - 1, 1) : " " + tail = (j + 1 <= length(s)) ? substr(s, j + 1, 1) : " " + if (c ~ /[-+*\/%&|^!<>=]/ || tail == "=") return 0 + return 1 + } + + return 0 + } + + BEGIN { LINE_CAP = 12 } # give up accumulating a statement after this many lines + + { + code = strip_noncode($0) + + # Record every occurrence on this line with the position it has in the accumulated + # statement, plus its real line and column, so each can be judged and reported separately. + if (stmt == "") { start = NR; nhits = 0 } + base = length(stmt) + 1 # the leading space added below shifts everything by one + stmt = stmt " " code + + off = 0 + while ((p = index(substr(code, off + 1), "UNITY_END")) > 0) { + off += p + nhits++ + hit_at[nhits] = base + off # index within stmt + hit_line[nhits] = NR + hit_col[nhits] = colmap[off] + } + + # End of statement. The cap is the backstop for a semicolon we never see, so one unclosed + # call cannot swallow every later finding in the file. + if (code ~ /;/ || NR - start >= LINE_CAP) { + for (k = 1; k <= nhits; k++) + if (!is_wrapped(stmt, hit_at[k])) + printf "%s:%d:%d:%s:%s:%s\n", path, hit_line[k], hit_col[k], "note", + "bare UNITY_END() leaves the process running - use exit(UNITY_END()) (see test/README.md)", + "unity-exit" + stmt = "" + nhits = 0 + } + } + ' "$target" +done + +exit 0 diff --git a/bin/pio-test-isolate.sh b/bin/pio-test-isolate.sh new file mode 100755 index 0000000000..bd58c73eb2 --- /dev/null +++ b/bin/pio-test-isolate.sh @@ -0,0 +1,133 @@ +#!/usr/bin/env bash +# PlatformIO `test_testing_command` wrapper - runs one native test suite in its own scratch $HOME +# and reports what it left behind. Registered per-env in variants/native/portduino/platformio.ini, +# so it applies to a bare `pio test` and to CI, not only to bin/run-tests.sh. +# +# Every native suite that constructs a NodeDB loads and saves ~/.portduino/default/prefs/, and +# nothing cleared it between suites, so state leaked suite -> suite within a run and run -> run +# after it. Per-*run* isolation is not enough: the leak is generated within a single run, so the +# boundary has to be per suite. +# +# Contract: run "$@" unchanged, exit with its exit code. PlatformIO's own pass/fail is untouched - +# everything else here is reporting. +# +# Escape hatch: MESHTASTIC_TEST_NO_ISOLATION=1 runs the binary bare, for when you need the real +# $HOME (e.g. reproducing against a live prefs directory). + +set -uo pipefail + +if [[ ${MESHTASTIC_TEST_NO_ISOLATION:-0} == 1 ]]; then + exec "$@" +fi + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +ROOT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" +# shellcheck source=bin/lib/test-state.sh +source "$SCRIPT_DIR/lib/test-state.sh" + +STATE_ROOT="${MESHTASTIC_TEST_STATE_DIR:-$ROOT_DIR/.pio/test-state}" +MANIFEST="${MESHTASTIC_TEST_STATE_MANIFEST:-$ROOT_DIR/$STATE_MANIFEST_DEFAULT}" +SUMMARY="${MESHTASTIC_TEST_STATE_SUMMARY:-$STATE_ROOT/summary.tsv}" + +if ! mkdir -p "$STATE_ROOT" 2>/dev/null; then + echo "pio-test-isolate: cannot create $STATE_ROOT - running without isolation" >&2 + exec "$@" +fi + +SCRATCH="$(mktemp -d "$STATE_ROOT/suite.XXXXXX")" || exec "$@" +SUITE_HOME="$SCRATCH/home" +LOG="$SCRATCH/output.log" +REPORT="$SCRATCH/per-test.tsv" +mkdir -p "$SUITE_HOME" + +state_assert_empty "$SUITE_HOME" || exit 1 + +BEFORE="$SCRATCH/before.fp" +state_fingerprint "$SUITE_HOME" >"$BEFORE" + +# HOME points at the sandbox; PLATFORMIO_CORE_DIR is pinned to the real one so nothing re-downloads +# a toolchain into a directory we are about to delete. (Overriding HOME around `pio` itself is what +# breaks its own ~/.platformio/penv/bin/pio lookup - doing it here, around the already-built binary, +# sidesteps that entirely.) +REAL_HOME="$HOME" +HOME="$SUITE_HOME" \ + PLATFORMIO_CORE_DIR="${PLATFORMIO_CORE_DIR:-$REAL_HOME/.platformio}" \ + MESHTASTIC_TEST_STATE_REPORT="$REPORT" \ + "$@" 2>&1 | tee "$LOG" +RC=${PIPESTATUS[0]} + +# Survivors, before anything else looks at the sandbox: reap them first so the after-fingerprint is +# taken against a tree nobody is still writing to, and so a run cannot leave processes accumulating +# on the host. SIGTERM, then SIGKILL for anything that ignores it. Reported on the summary line as a +# fourth outcome - it is not a filesystem verdict, and folding it into DIRTY would lose the reason. +SURVIVORS="$(state_find_survivors "$SUITE_HOME" | tr '\n' ' ')" +SURVIVORS="${SURVIVORS% }" +if [[ -n $SURVIVORS ]]; then + # shellcheck disable=SC2086 # deliberate word splitting: SURVIVORS is a PID list + kill $SURVIVORS 2>/dev/null + sleep 0.2 + STILL="$(state_find_survivors "$SUITE_HOME" | tr '\n' ' ')" + # shellcheck disable=SC2086 # as above + [[ -n ${STILL// /} ]] && kill -9 $STILL 2>/dev/null + echo "pio-test-isolate: survivor(s) still running after the suite finished: $SURVIVORS (killed)" >&2 +fi + +# The suite name is not passed to a test_testing_command, so recover it from the output: every Unity +# result line carries the suite's source path. Fall back to the per-test report, which records it +# from __FILE__, and finally to the scratch dir name. +SUITE="$(grep -oE 'test/test_[a-z0-9_]+/' "$LOG" 2>/dev/null | head -1 | sed -E 's#test/(test_[a-z0-9_]+)/#\1#')" +if [[ -z $SUITE && -f $REPORT ]]; then + SUITE="$(awk -F'\t' 'NR==1 {print $1}' "$REPORT")" +fi +[[ -z $SUITE ]] && SUITE="unknown-$(basename "$SCRATCH")" + +AFTER="$SCRATCH/after.fp" +state_fingerprint "$SUITE_HOME" >"$AFTER" +CHANGED="$(state_changed_paths "$BEFORE" "$AFTER")" + +FLAGS="$(state_manifest_flags "$SUITE" "$MANIFEST")" +DECLARED="$(state_flag_value writes "$FLAGS")" +GRANULARITY="$(state_flag_value state "$FLAGS")" +[[ -z $GRANULARITY ]] && GRANULARITY="per-test" + +IFS=$'\t' read -r VERDICT DETAIL <<<"$(state_classify "$CHANGED" "$DECLARED")" + +# Per-test attribution, when the suite has not declared that it carries state across its own test +# cases. For a state=per-suite suite every test after the first would be flagged by design - that +# carry *is* the declared behaviour - so only the suite boundary is meaningful there. +PER_TEST_DETAIL="" +if [[ $GRANULARITY == "per-test" && -f $REPORT ]]; then + awk -F'\t' -v d="$DECLARED" ' + BEGIN { n = split(d, a, ","); } + { + path = $4; base = path; sub(/^.*\//, "", base); + for (i = 1; i <= n; i++) if (a[i] == path || a[i] == base) next; + print $2 " -> " base; + }' "$REPORT" | LC_ALL=C sort -u >"$SCRATCH/per-test-undeclared.txt" + # Keep the summary line readable; the full attribution stays in the sandbox's per-test.tsv. + PER_TEST_COUNT=$(wc -l <"$SCRATCH/per-test-undeclared.txt") + # Not `paste -sd'; '`: with -s, paste cycles through a multi-char delimiter one character per + # join, so five paths render as "a;b c;d e" rather than "a; b; c; d; e". + PER_TEST_DETAIL="$(head -5 "$SCRATCH/per-test-undeclared.txt" | awk '{printf "%s%s", (NR > 1 ? "; " : ""), $0} END {print ""}')" + ((PER_TEST_COUNT > 5)) && PER_TEST_DETAIL="$PER_TEST_DETAIL; +$((PER_TEST_COUNT - 5)) more" +fi + +STATUS=$([[ $RC -eq 0 ]] && echo PASS || echo FAIL) +mkdir -p "$(dirname "$SUMMARY")" 2>/dev/null +printf '%s\t%s\t%s\t%s\t%s\t%s\n' "$SUITE" "$STATUS" "$VERDICT" "${DETAIL-}" "${PER_TEST_DETAIL-}" \ + "${SURVIVORS-}" >>"$SUMMARY" + +# Keep the sandbox when there is something to look at: on a failure it plus the built binary is a +# complete, replayable reproduction, and on a DIRTY verdict the leftovers *are* the bug report. A +# clean pass leaves nothing behind. +KEEP="${MESHTASTIC_TEST_KEEP_STATE:-0}" +if [[ $RC -ne 0 || $VERDICT != CLEAN || -n ${SURVIVORS-} || $KEEP == 1 ]]; then + DEST="$STATE_ROOT/$SUITE" + rm -rf "$DEST" 2>/dev/null + mv "$SCRATCH" "$DEST" 2>/dev/null || DEST="$SCRATCH" + echo "pio-test-isolate: $SUITE $STATUS/$VERDICT - state and log kept at $DEST" >&2 +else + rm -rf "$SCRATCH" +fi + +exit "$RC" diff --git a/bin/run-tests.sh b/bin/run-tests.sh index de824ad822..1c5109282b 100755 --- a/bin/run-tests.sh +++ b/bin/run-tests.sh @@ -2,7 +2,7 @@ # Run native PlatformIO unit tests and emit a single, unambiguous verdict. # # Why this exists: PlatformIO reports failures three different ways ([FAILED], :FAIL:, -# [ERRORED]) and an all-pass run prints "N succeeded" with NO "0 failed" clause — so naive +# [ERRORED]) and an all-pass run prints "N succeeded" with NO "0 failed" clause - so naive # greps produce false greens (see .notes/test-passfail-filter.md). This script encodes the # correct logic once, and cross-checks the number of suites that actually ran against the # canonical set in test/ so a suite silently going missing shows up as AMBER, not green. @@ -12,37 +12,85 @@ # ./bin/run-tests.sh -f test_utf8 # run one suite (yields FILTERED, not GREEN) # ./bin/run-tests.sh -e native # override env (default: coverage) # ./bin/run-tests.sh --quiet # only print the final RESULT line +# ./bin/run-tests.sh --write-manifest # print the test/state-manifest.tsv entries this run +# # would need, for a human to paste and justify +# ./bin/run-tests.sh --keep-state # keep every suite's sandbox, not just the interesting ones +# ./bin/run-tests.sh --shuffle # randomise suite order (seed from HEAD; printed) +# ./bin/run-tests.sh --seed 12345 # replay an exact order (implies --shuffle) # # Exit codes: 0 = GREEN, 1 = RED, 2 = AMBER, 3 = FILTERED. # +# HOST. This is a Linux tool: bash 4+ (mapfile), GNU coreutils and GNU find (`-printf`, md5sum, +# `-executable`). That is a deliberate choice, not an oversight - the alternative is a second, +# untested code path per host, and a state check that silently degrades is worse than one that does +# not run. It is enforced below rather than left to be discovered. macOS and Windows are supported as +# *build* targets by CI, not as hosts for this harness; run it in a container there, via +# ./bin/test-native-docker.sh. +# +# -f IS NOT A GATE. A filtered run can pass while a full run fails: filtering removes the suites +# that create the shared state a later suite trips over. Use -f to iterate; gate on a full run. +# # Verdicts: -# GREEN — all canonical suites ran, all passed, no ignored test cases. -# AMBER — all that ran passed, but something was lost: a suite silently went missing on a -# full run, or individual test cases were skipped (Unity TEST_IGNORE / :IGNORE:). -# FILTERED — a -f run completed cleanly; suites not in the filter were intentionally skipped. +# GREEN - all canonical suites ran, all passed, no ignored test cases, no undeclared leftovers. +# AMBER - all that ran passed, but something was lost or unexplained: a suite silently went +# missing on a full run, individual test cases were skipped (Unity TEST_IGNORE / +# :IGNORE:), or a suite left behind shared state it does not declare in +# test/state-manifest.tsv. +# FILTERED - a -f run completed cleanly; suites not in the filter were intentionally skipped. # Use this when iterating on a single suite; it is not a quality signal. -# RED — at least one failure, build error, or sanitizer fault. +# RED - at least one failure, build error, or sanitizer fault. +# +# Two orthogonal axes: PASS/FAIL × CLEAN/DIRTY. Each suite runs in its own scratch $HOME +# (bin/pio-test-isolate.sh), so leftovers are harmless; DIRTY means "undeclared", not "dangerous". +# +# ORDER. PlatformIO chooses suite order itself - list_test_names() walks test/ with os.walk() and +# filters only *select*, they do not order - so --shuffle runs one `pio test -f ` invocation +# per suite in the chosen order. That costs about 4.7s per suite in extra pio startup. The seed is +# printed on every shuffled run and derived from HEAD by default: deterministic for a given commit, +# varied across commits, so a red is reproducible and attributable rather than flaky. A single green +# seed is not evidence of order independence; vary it. +# +# Sanitizers, per env - this trips people up: `coverage` (the default here) has ASan/LSan; +# `native` has NONE. Verified: zero ASan symbols in the native binary. `-e native` runs are not +# sanitized, whatever the coverage wording elsewhere implies. # # The final line is machine-readable, e.g.: # RESULT: GREEN N/N suites passed -# RESULT: AMBER N/M suites ran (missing: test_radio test_serial) — all that ran passed +# RESULT: AMBER N/M suites ran (missing: test_radio test_serial) - all that ran passed # RESULT: AMBER 3 test case(s) ignored -# RESULT: FILTERED 1/N suites ran (not run: …) — filtered: test_utf8 +# RESULT: FILTERED 1/N suites ran (not run: …) - filtered: test_utf8 # RESULT: RED test_traffic_management: 1 failed (or: build/crash error) -# RESULT: RED sanitizer fault — SUMMARY: AddressSanitizer: 1272 byte(s) leaked (tests may have -# all passed; the coverage build aborts at exit on an ASan/LSan fault — often shown only +# RESULT: RED sanitizer fault - SUMMARY: AddressSanitizer: 1272 byte(s) leaked (tests may have +# all passed; the coverage build aborts at exit on an ASan/LSan fault - often shown only # as [ERRORED]/SIGHUP. The script names it and points at running the binary bare.) set -uo pipefail +# Refuse to start off Linux rather than fail somewhere in the middle. This harness is a Linux tool by +# choice (see the HOST note in the header); on a BSD userland it would not fail cleanly, it would +# mis-hash the sandbox, mis-read a suite list and report a verdict that looks real. +if [[ $(uname -s) != Linux ]]; then + echo "run-tests.sh is Linux-only (bash 4+, GNU coreutils, GNU find); this host is $(uname -s)." >&2 + echo "Run the suite in a container instead: ./bin/test-native-docker.sh" >&2 + exit 2 +fi + SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" ROOT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" -cd "$ROOT_DIR" +cd "$ROOT_DIR" || exit 1 ENV="coverage" FILTER="" QUIET=false +WRITE_MANIFEST=false +KEEP_STATE=false +SHUFFLE=false +SEED="" PASSTHRU=() +# Same passthrough args minus the -f pair. The shuffled loop supplies its own -f per suite, but +# must still forward everything else the user gave (-v, -vvv, ...) - otherwise a shuffled run +# builds with those flags and then runs without them. +EXTRA_ARGS=() while [[ $# -gt 0 ]]; do case "$1" in @@ -59,8 +107,26 @@ while [[ $# -gt 0 ]]; do QUIET=true shift ;; + --write-manifest) + WRITE_MANIFEST=true + shift + ;; + --keep-state) + KEEP_STATE=true + shift + ;; + --shuffle) + SHUFFLE=true + shift + ;; + --seed) + SEED="$2" + SHUFFLE=true + shift 2 + ;; *) PASSTHRU+=("$1") + EXTRA_ARGS+=("$1") shift ;; esac @@ -74,16 +140,35 @@ if [[ ! -x $PIO ]] && ! command -v "$PIO" >/dev/null 2>&1; then fi LOG="$(mktemp -t meshtest.XXXXXX.log)" +# Build output stays out of $LOG on purpose: the outcome regexes below match "error:" and +# "[ERRORED]", so a compiler diagnostic in the same file would read as a test failure. +BUILD_LOG="$(mktemp -t meshtest-build.XXXXXX.log)" MARKER="" PROGRESS_PID="" -trap 'rm -f "$LOG" "${MARKER:-}"; [[ -n ${PROGRESS_PID:-} ]] && kill "$PROGRESS_PID" 2>/dev/null' EXIT +trap 'rm -f "$LOG" "$BUILD_LOG" "${MARKER:-}"; [[ -n ${PROGRESS_PID:-} ]] && kill "$PROGRESS_PID" 2>/dev/null' EXIT + +# --- Shared-state reporting --------------------------------------------------- +# bin/pio-test-isolate.sh (wired in as test_testing_command) gives every suite its own scratch +# $HOME and appends one line per suite here: suite, PASS/FAIL, CLEAN/DIRTY/MISSING, detail. The +# wrapper enforces isolation on its own - a bare `pio test` gets it too - so all this section does +# is collect and grade. Start from an empty summary so a stale one cannot be read as this run's. +# shellcheck source=bin/lib/test-state.sh +source "$SCRIPT_DIR/lib/test-state.sh" +STATE_DIR="$ROOT_DIR/.pio/test-state" +STATE_SUMMARY="$STATE_DIR/summary.tsv" +rm -rf "$STATE_DIR" +mkdir -p "$STATE_DIR" +export MESHTASTIC_TEST_STATE_DIR="$STATE_DIR" +export MESHTASTIC_TEST_STATE_SUMMARY="$STATE_SUMMARY" +$KEEP_STATE && export MESHTASTIC_TEST_KEEP_STATE=1 +$WRITE_MANIFEST && export MESHTASTIC_TEST_KEEP_STATE=1 # Canonical suite set = the directories in test/. This is the source of truth for # "what should run"; a filtered run only expects its filtered suite. mapfile -t ALL_SUITES < <(find test -maxdepth 1 -type d -name 'test_*' -printf '%f\n' | sort) EXPECTED_COUNT=${#ALL_SUITES[@]} -# Canonical suite count — the registered total, maintained in test/native-suite-count. +# Canonical suite count - the registered total, maintained in test/native-suite-count. # Update that file whenever a test suite is added or removed. CANONICAL_COUNT_FILE="test/native-suite-count" if [[ -f $CANONICAL_COUNT_FILE ]]; then @@ -98,7 +183,7 @@ fi BASELINE_FILE=".pio/build/${ENV}/.runtests-objcount" # Progress trail file (gitignored build dir). ALWAYS written so a backgrounded/piped run can be -# checked mid-build with `tail -f` — that's the whole point: don't fly blind on a 20-min rebuild. +# checked mid-build with `tail -f` - that's the whole point: don't fly blind on a 20-min rebuild. PROGRESS_FILE=".pio/build/${ENV}/.runtests-progress" # --- Progress heartbeat ------------------------------------------------------ @@ -114,16 +199,16 @@ progress_monitor() { el=$((now - start)) if grep -q 'Testing\.\.\.' "$LOG" 2>/dev/null; then ran=$(grep -cE "${ENV}:test_[a-z0-9_]+ \[(PASSED|FAILED|ERRORED)\]" "$LOG" 2>/dev/null) - line=$(printf '[test] %s/%s suites done — %dm%02ds' "$ran" "$testtotal" $((el / 60)) $((el % 60))) + line=$(printf '[test] %s/%s suites done - %dm%02ds' "$ran" "$testtotal" $((el / 60)) $((el % 60))) else done=$(find ".pio/build/${ENV}" -name '*.o' -newer "$marker" 2>/dev/null | wc -l) if ((objtotal > 0 && done > 0)); then eta=$((objtotal > done ? (objtotal - done) * el / done : 0)) - line=$(printf '[build] %d/%d objs — %dm%02ds — ETA ~%dm%02ds' \ + line=$(printf '[build] %d/%d objs - %dm%02ds - ETA ~%dm%02ds' \ "$done" "$objtotal" $((el / 60)) $((el % 60)) $((eta / 60)) $((eta % 60))) else - # done==0 (incremental: nothing to rebuild yet) or no cached baseline — no ETA yet. - line=$(printf '[build] %d objs compiled — %dm%02ds' "$done" $((el / 60)) $((el % 60))) + # done==0 (incremental: nothing to rebuild yet) or no cached baseline - no ETA yet. + line=$(printf '[build] %d objs compiled - %dm%02ds' "$done" $((el / 60)) $((el % 60))) fi fi printf '%s\n' "$line" >>"$pfile" 2>/dev/null # file trail (always) @@ -133,9 +218,12 @@ progress_monitor() { } # Launch the heartbeat for every run. It writes the progress file unconditionally; the live tty -# line only when interactive AND --quiet (where pio's own output is hidden — otherwise pio's +# line only when interactive AND --quiet (where pio's own output is hidden - otherwise pio's # streamed compile lines already show progress and a \r line would just fight them). mkdir -p ".pio/build/${ENV}" 2>/dev/null || true +# Clear last run's failure logs: a green run must not leave a red one's log lying around looking +# current. +rm -f ".pio/build/${ENV}/build-failure.log" ".pio/build/${ENV}/test-failure.log" 2>/dev/null || true : >"$PROGRESS_FILE" 2>/dev/null || true MARKER="$(mktemp -t meshtest-mark.XXXXXX)" TOTTY=0 @@ -149,30 +237,96 @@ if ! $QUIET; then fi echo "progress: tail -f $PROGRESS_FILE" >&2 if [[ ! -t 1 ]] && ! $QUIET; then - echo "hint: stdout is a pipe — build errors appear at the top of output and may be lost; use --quiet to get just the RESULT line" >&2 + echo "hint: stdout is a pipe - build errors appear at the top of output and may be lost; use --quiet to get just the RESULT line" >&2 +fi + +# shuffle_suites() lives in lib/ because the CI workflow runs the same permutation; see the header +# of that file for why a second copy cannot be allowed to exist. +# shellcheck source=bin/lib/shuffle.sh +source "$SCRIPT_DIR/lib/shuffle.sh" + +RUN_ORDER=() +if $SHUFFLE; then + # Seed from HEAD when not given: same order for a given commit (so a PR's red is replayable and + # attributable to its diff), different orders as the project moves. + if [[ -z $SEED ]]; then + SEED=$((16#$(git rev-parse --short=8 HEAD 2>/dev/null || echo 0))) + fi + if [[ -n $FILTER ]]; then + mapfile -t RUN_ORDER < <(shuffle_suites "$SEED" "$FILTER") + else + mapfile -t RUN_ORDER < <(shuffle_suites "$SEED" "${ALL_SUITES[@]}") + fi + echo "suite order: shuffled with --seed $SEED (${#RUN_ORDER[@]} suites)" +fi + +# Build every test program before running any of them, the way .github/workflows/test_native.yml +# does. Fused build+run makes whichever suite PlatformIO's directory walk reaches first absorb the +# whole src compile and report it as its own duration - that is how a 35s suite once reported 13 +# minutes, and it hides the build cost from every timing the summary prints. +BUILD_SECS=0 +build_started=$SECONDS +if $QUIET; then + "$PIO" test -e "$ENV" "${PASSTHRU[@]}" --without-testing >"$BUILD_LOG" 2>&1 + BUILD_RC=$? +else + "$PIO" test -e "$ENV" "${PASSTHRU[@]}" --without-testing 2>&1 | tee "$BUILD_LOG" + BUILD_RC=${PIPESTATUS[0]} +fi +BUILD_SECS=$((SECONDS - build_started)) +if ((BUILD_RC != 0)); then + # The grep below shows the first few diagnostics; the first error: is usually a cascade from + # something further up, so keep the whole log rather than only what fits on screen. + BUILD_FAIL_LOG=".pio/build/${ENV}/build-failure.log" + cp "$BUILD_LOG" "$BUILD_FAIL_LOG" 2>/dev/null || BUILD_FAIL_LOG="" + echo "" + echo "RED - build failed before any suite ran:" + grep -nE 'error:|undefined reference|\[ERRORED\]' "$BUILD_LOG" | head -5 | sed 's/^/ /' + [[ -n $BUILD_FAIL_LOG ]] && echo " -> full build output: $BUILD_FAIL_LOG" + echo "RESULT: RED build failed in ${BUILD_SECS}s (no suites ran)" + exit 1 +fi +if ! $QUIET; then + echo "build: ${BUILD_SECS}s (shared by every suite; suite durations below exclude it)" fi # Run pio, tee to log. PIPESTATUS[0] is pio's real exit (NOT tee's). -if $QUIET; then - "$PIO" test -e "$ENV" "${PASSTHRU[@]}" >"$LOG" 2>&1 +PIO_RC=0 +if $SHUFFLE; then + # One invocation per suite: PlatformIO orders by its own directory walk, so this is the only way + # to control it. Output is appended to the one $LOG the verdict logic already parses. + : >"$LOG" + for suite in "${RUN_ORDER[@]}"; do + if $QUIET; then + "$PIO" test -e "$ENV" -f "$suite" "${EXTRA_ARGS[@]}" --without-building >>"$LOG" 2>&1 + rc=$? + else + "$PIO" test -e "$ENV" -f "$suite" "${EXTRA_ARGS[@]}" --without-building 2>&1 | tee -a "$LOG" + rc=${PIPESTATUS[0]} + fi + ((rc != 0)) && PIO_RC=$rc + done +elif $QUIET; then + "$PIO" test -e "$ENV" "${PASSTHRU[@]}" --without-building >"$LOG" 2>&1 + PIO_RC=$? else - "$PIO" test -e "$ENV" "${PASSTHRU[@]}" 2>&1 | tee "$LOG" + "$PIO" test -e "$ENV" "${PASSTHRU[@]}" --without-building 2>&1 | tee "$LOG" + PIO_RC=${PIPESTATUS[0]} fi -PIO_RC=${PIPESTATUS[0]} # Stop the heartbeat, clear its line, and cache this build's object total for next time. if [[ -n $PROGRESS_PID ]]; then kill "$PROGRESS_PID" 2>/dev/null wait "$PROGRESS_PID" 2>/dev/null PROGRESS_PID="" - # Clear the live line only if we were writing one — opening /dev/tty when there is none is + # Clear the live line only if we were writing one - opening /dev/tty when there is none is # itself a redirect-open error the trailing 2>/dev/null cannot suppress. [[ $TOTTY == 1 ]] && printf '\r\033[K' >/dev/tty 2>/dev/null fi [[ -d ".pio/build/${ENV}" ]] && find ".pio/build/${ENV}" -name '*.o' 2>/dev/null | wc -l >"$BASELINE_FILE" 2>/dev/null || true # --- Outcome detection ------------------------------------------------------- -# The SAME outcome is spelled differently depending on which layer emitted the line — this is +# The SAME outcome is spelled differently depending on which layer emitted the line - this is # the trap that produces false greens (grepping ":PASS" misses pio's "[PASSED]", grepping # "[FAILED]" misses Unity's ":FAIL:"). So every regex below matches BOTH spellings: # pass: Unity per-assertion ":PASS" | pio per-suite "[PASSED]" | summary "N succeeded" @@ -184,7 +338,7 @@ FAIL_RE=':FAIL\b|\[FAILED\]|\[ERRORED\]|[1-9][0-9]* failed|[0-9]+ Tests [1-9][0- # the per-test/per-suite tokens OR a success summary line. PASS_RE=':PASS\b|\[PASSED\]|test cases: *[0-9]+ succeeded|[0-9]+ Tests 0 Failures' # Sanitizer (ASan/LSan/UBSan/TSan) fault signatures. The coverage build is sanitizer-instrumented -# and aborts NON-ZERO at exit on a fault — most often a LeakSanitizer leak — AFTER every test has +# and aborts NON-ZERO at exit on a fault - most often a LeakSanitizer leak - AFTER every test has # already printed [PASSED]. pio then reports [ERRORED]/SIGHUP with no :FAIL: anywhere, so it # masquerades as a phantom "N-1 of N succeeded". See .notes/test-passfail-filter.md. # Match only real FAULT lines, never the benign "AddressSanitizer: failed to intercept '...'" @@ -203,35 +357,91 @@ RAN_COUNT=${#RAN_SUITES[@]} mapfile -t SKIPPED_SUITES < <(grep -oE "${ENV}:test_[a-z0-9_]+.*\bSKIPPED\b" "$LOG" | grep -oE "test_[a-z0-9_]+" | sort -u) +# Keep the whole-run log, which the EXIT trap would otherwise delete. This is the cross-suite view +# - order, pio-level output, what ran before the failure; bin/pio-test-isolate.sh separately keeps +# the failing suite's own sandbox and log under .pio/test-state//. +preserve_run_log() { + local dest=".pio/build/${ENV}/test-failure.log" + cp "$LOG" "$dest" 2>/dev/null && echo " -> full run output: $dest" +} + +# PlatformIO prints one "N test cases: ... succeeded in T" line per invocation. A shuffled run is one +# invocation per suite appending to the same $LOG, so taking the last line would report whatever the +# LAST suite did - a failure in suite 3 printed under suite 44's "0 failed". Sum the lines instead. +# One line in (the unshuffled case) is passed through verbatim, so the familiar output is unchanged. +summarise_test_cases() { + # The patterns are strings, not /regex/ literals: awk evaluates a regex literal passed as a + # function argument as `$0 ~ /re/`, so the callee would receive 0 or 1 rather than a pattern. + awk ' + function num(s, pat, m) { + if (!match(s, pat)) return 0 + m = substr(s, RSTART, RLENGTH); gsub(/[^0-9]/, "", m); return m + 0 + } + /test cases:/ { + last = $0; n++ + cases += num($0, "[0-9]+ test cases") + failed += num($0, "[0-9]+ failed") + skipped += num($0, "[0-9]+ skipped") + passed += num($0, "[0-9]+ succeeded") + } + END { + if (n == 0) exit + if (n == 1) { print " " last; exit } + printf " %d test cases: ", cases + if (failed) printf "%d failed, ", failed + if (skipped) printf "%d skipped, ", skipped + printf "%d succeeded, summed over %d suite invocations\n", passed, n + }' "$1" +} + verdict_red() { local detail bin + # The order IS the diagnostic for an order-dependent failure; without it a shuffled red is + # unreadable. + if $SHUFFLE; then + echo "" + echo "suite order (--seed $SEED):" + printf '%s\n' "${RUN_ORDER[@]}" | nl -ba | sed 's/^/ /' + fi detail="$(grep -nE '\[FAILED\]|:FAIL:|\[ERRORED\]' "$LOG" | head -3 | sed 's/^/ /')" echo "" - echo "RED — failures detected:" + echo "RED - failures detected:" [[ -n $detail ]] && echo "$detail" - grep -E 'test cases:' "$LOG" | tail -1 | sed 's/^/ /' + summarise_test_cases "$LOG" + preserve_run_log # Path to the test binary for the "run it bare" hint. For native/coverage the test program is # the env executable (e.g. .pio/build/coverage/meshtasticd), NOT a file named 'program'. bin="$(find ".pio/build/${ENV}" -maxdepth 1 -type f -executable ! -name '*.so' 2>/dev/null | head -1)" [[ -z $bin ]] && bin=".pio/build/${ENV}/ (build it first: $PIO test -e ${ENV} ${FILTER:+-f $FILTER} --without-testing)" + # A signal name from this runner is almost never a crash. `exit(UNITY_END())` returns the + # FAILURE COUNT, and PlatformIO's native runner renders a non-zero exit code as a POSIX signal: + # 4 failures -> "Program received signal SIGILL", 5 -> SIGTRAP, and the suite is reported + # [ERRORED] rather than [FAILED]. That is pure noise, and it cost hours of hunting a memory bug + # that did not exist. Say so before anyone theorises. + if grep -qE 'Program received signal SIG' "$LOG"; then + echo " -> the signal name above is Unity's exit code, not a crash: exit(UNITY_END()) returns the" + echo " failure count and the runner renders it as a signal number (4 -> SIGILL, 5 -> SIGTRAP)." + echo " Match it against the failure count before assuming a fault; confirm any real crash in gdb." + fi + # Sanitizer fault (ASan/LSan/UBSan/TSan): name the real cause instead of "build/crash error". if grep -qE "$SAN_RE" "$LOG"; then grep -nE "$SAN_RE" "$LOG" | head -4 | sed 's/^/ /' echo " -> sanitizer fault: if every test above is PASS, this is an exit-time abort, not a failed assertion." echo " -> read the full report by running the binary BARE (gdb hides it via ptrace): ./$bin 2>&1 | tail -40" - echo "RESULT: RED sanitizer fault — $(grep -ohE 'SUMMARY: [A-Za-z]+Sanitizer:.*' "$LOG" | tail -1 || echo 'see report above')" + echo "RESULT: RED sanitizer fault - $(grep -ohE 'SUMMARY: [A-Za-z]+Sanitizer:.*' "$LOG" | tail -1 || echo 'see report above')" exit 1 fi # All tests passed but the process still aborted at EXIT (ERRORED/SIGHUP/SIGABRT) and the # sanitizer report was swallowed by the runner (often surfaced only as SIGHUP). Almost always a - # sanitizer fault — point at how to surface it rather than calling it a generic crash. + # sanitizer fault - point at how to surface it rather than calling it a generic crash. if grep -qE "$PASS_RE" "$LOG" && grep -qE '\[ERRORED\]|SIGHUP|SIGABRT' "$LOG" && ! grep -qE ':FAIL\b|\[FAILED\]' "$LOG"; then - echo " -> all tests passed but the process aborted at EXIT — likely an ASan/LSan fault whose report" + echo " -> all tests passed but the process aborted at EXIT - likely an ASan/LSan fault whose report" echo " the runner swallowed (commonly shown as SIGHUP). Run the binary BARE to see it: ./$bin 2>&1 | tail -40" - echo "RESULT: RED exit-time abort (tests passed; likely sanitizer — see hint above)" + echo "RESULT: RED exit-time abort (tests passed; likely sanitizer - see hint above)" exit 1 fi @@ -245,33 +455,90 @@ if [[ $PIO_RC -ne 0 ]] || grep -qE "$FAIL_RE" "$LOG"; then fi if ! grep -qE "$PASS_RE" "$LOG"; then echo "" - echo "RESULT: RED no success summary found (build error / no tests ran?) — see log" + # This path never runs verdict_red, and if the build died before any suite started there is no + # per-suite sandbox either - so without preserving here, "see log" points at nothing. + preserve_run_log + echo "RESULT: RED no success summary found (build error / no tests ran?)" exit 1 fi -# Canonical-count rating suffix — appended to every verdict line so the result is always +# Canonical-count rating suffix - appended to every verdict line so the result is always # rated against the registered total, not just the directory count. # If the two counts diverge (suite added/removed without updating native-suite-count), that # is itself surfaced as AMBER before we reach any verdict. canonical_rating() { + local rating="" if [[ -n $CANONICAL_COUNT ]]; then - echo "[canonical: ${RAN_COUNT}/${CANONICAL_COUNT}]" + rating="[canonical: ${RAN_COUNT}/${CANONICAL_COUNT}]" fi + # Carry the seed into the machine-readable line so a verdict is always replayable from it alone. + $SHUFFLE && rating="$rating [seed: $SEED]" + echo "$rating" } -# AMBER: directory count disagrees with native-suite-count — file needs updating. +# AMBER: directory count disagrees with native-suite-count - file needs updating. if [[ -n $CANONICAL_COUNT && $EXPECTED_COUNT -ne $CANONICAL_COUNT ]]; then echo "" if [[ $EXPECTED_COUNT -gt $CANONICAL_COUNT ]]; then - echo "RESULT: AMBER test/ has $EXPECTED_COUNT suite directories but native-suite-count says $CANONICAL_COUNT — update test/native-suite-count after registering new suites" + echo "RESULT: AMBER test/ has $EXPECTED_COUNT suite directories but native-suite-count says $CANONICAL_COUNT - update test/native-suite-count after registering new suites" else - echo "RESULT: AMBER test/ has $EXPECTED_COUNT suite directories but native-suite-count says $CANONICAL_COUNT — update test/native-suite-count after removing suites" + echo "RESULT: AMBER test/ has $EXPECTED_COUNT suite directories but native-suite-count says $CANONICAL_COUNT - update test/native-suite-count after removing suites" fi exit 2 fi +# --- Shared-state axis -------------------------------------------------------- +# Read what the per-suite wrapper recorded. Reported after the count checks so a structural problem +# still wins, and before the pass/fail verdict lines so the state summary always prints. +DIRTY_SUITES=() +MISSING_SUITES=() +SURVIVOR_SUITES=() +if [[ -f $STATE_SUMMARY ]]; then + mapfile -t DIRTY_SUITES < <(awk -F'\t' '$3 == "DIRTY" { print $1 " (" $4 ")" }' "$STATE_SUMMARY") + mapfile -t MISSING_SUITES < <(awk -F'\t' '$3 == "MISSING" { print $1 " (" $4 ")" }' "$STATE_SUMMARY") + mapfile -t SURVIVOR_SUITES < <(awk -F'\t' '$6 != "" { print $1 " (pid " $6 ")" }' "$STATE_SUMMARY") +fi + +# Print the opt-out count on every run, so the number creeping upward is visible without anyone +# auditing test/state-manifest.tsv on purpose. +DECLARED_COUNT=0 +if [[ -f $ROOT_DIR/$STATE_MANIFEST_DEFAULT ]]; then + DECLARED_COUNT=$(grep -cvE '^[[:space:]]*(#|$)' "$ROOT_DIR/$STATE_MANIFEST_DEFAULT" || true) +fi +if ! $QUIET; then + echo "" + echo "shared state: $DECLARED_COUNT suite(s) declare non-default state handling (test/state-manifest.tsv)" +fi + +# --write-manifest: propose, never apply. An auto-accepted baseline is the same rot as an +# auto-updated snapshot, so this prints lines for a human to paste AND justify - the reason column +# is the point, and only a person can write it. +if $WRITE_MANIFEST; then + echo "" + echo "Proposed test/state-manifest.tsv entries from this run (paste and replace ):" + if [[ -f $STATE_SUMMARY ]]; then + awk -F'\t' '$3 == "DIRTY" { + detail = $4; sub(/^undeclared: /, "", detail); + n = split(detail, paths, " "); out = ""; + for (i = 1; i <= n; i++) { base = paths[i]; sub(/^.*\//, "", base); out = out (i > 1 ? "," : "") base } + printf "%s\twrites=%s\t\n", $1, out + }' "$STATE_SUMMARY" | sort -u | sed 's/^/ /' + fi + echo "" + echo " Sandboxes kept under $STATE_DIR// - the leftovers themselves are the evidence." +fi + +# MISSING is a warning, never a verdict: a declared write that did not happen catches silently +# broken persistence (the upstream TAK config bug was a has_ flag never set, so the save wrote +# nothing and no test noticed), but some declared writes are legitimately conditional. +if ((${#MISSING_SUITES[@]} > 0)) && ! $QUIET; then + echo "" + echo "warning: declared writes that did not happen - check for silently broken persistence:" + printf ' %s\n' "${MISSING_SUITES[@]}" +fi + # AMBER: individual test cases were skipped (Unity TEST_IGNORE → :IGNORE: in output). -# Applies to both full and filtered runs — a skipped test case is a lost signal either way. +# Applies to both full and filtered runs - a skipped test case is a lost signal either way. mapfile -t IGNORED_TESTS < <(grep -oE '[^:]+:[0-9]+:[^:]+:IGNORE:.*' "$LOG" 2>/dev/null | sed 's/:IGNORE:.*//' | sort -u) IGNORED_COUNT=${#IGNORED_TESTS[@]} if [[ $IGNORED_COUNT -gt 0 ]]; then @@ -283,7 +550,7 @@ if [[ $IGNORED_COUNT -gt 0 ]]; then exit 2 fi -# AMBER: full run only — a canonical suite neither ran NOR was explicitly skipped (silently missing). +# AMBER: full run only - a canonical suite neither ran NOR was explicitly skipped (silently missing). ACCOUNTED_COUNT=$((RAN_COUNT + ${#SKIPPED_SUITES[@]})) if [[ -z $FILTER && $ACCOUNTED_COUNT -lt $EXPECTED_COUNT ]]; then missing=() @@ -291,7 +558,37 @@ if [[ -z $FILTER && $ACCOUNTED_COUNT -lt $EXPECTED_COUNT ]]; then printf '%s\n' "${RAN_SUITES[@]}" "${SKIPPED_SUITES[@]}" | grep -qx "$s" || missing+=("$s") done echo "" - echo "RESULT: AMBER ${RAN_COUNT}/${EXPECTED_COUNT} suites ran (missing: ${missing[*]}) — all that ran passed $(canonical_rating)" + echo "RESULT: AMBER ${RAN_COUNT}/${EXPECTED_COUNT} suites ran (missing: ${missing[*]}) - all that ran passed $(canonical_rating)" + exit 2 +fi + +# AMBER: a suite mutated shared state it does not declare. Per-suite isolation means this is no +# longer dangerous - nothing survives the suite boundary - so it is graded AMBER rather than RED: +# it means "undeclared", not "broken". Applies to filtered runs too, because a suite writing state +# nobody declared is a finding whether or not its neighbours ran. +if ((${#DIRTY_SUITES[@]} > 0)); then + echo "" + printf ' %s\n' "${DIRTY_SUITES[@]}" + echo "" + echo " -> declare these in test/state-manifest.tsv with a reason, or stop the write." + echo " -> ./bin/run-tests.sh --write-manifest prints the entries to paste." + echo "RESULT: AMBER ${#DIRTY_SUITES[@]} suite(s) left undeclared shared state $(canonical_rating)" + exit 2 +fi + +# AMBER: a suite was still running after PlatformIO reported it. A bare UNITY_END() ends the +# reporting, not the process - the runtime goes on calling loop() - so the suite passes, the run goes +# green, and the binary stays resident. The wrapper has already killed it, but the consequences do +# not undo: its CLEAN/DIRTY verdict was measured against a tree it may still have been writing to, +# and .gcda plus LeakSanitizer both flush from atexit handlers that never ran, so the suite silently +# contributed no coverage and got no leak check. AMBER, not RED - the tests themselves did pass. +if ((${#SURVIVOR_SUITES[@]} > 0)); then + echo "" + printf ' %s\n' "${SURVIVOR_SUITES[@]}" + echo "" + echo " -> end every setup() branch with exit(UNITY_END()), not a bare UNITY_END()." + echo " -> ./bin/lint-unity-exit.sh test/**/*.cpp finds the sites; see test/README.md." + echo "RESULT: AMBER ${#SURVIVOR_SUITES[@]} suite(s) still running after the suite finished $(canonical_rating)" exit 2 fi @@ -302,10 +599,10 @@ if [[ -n $FILTER ]]; then for s in "${ALL_SUITES[@]}"; do printf '%s\n' "${RAN_SUITES[@]}" "${SKIPPED_SUITES[@]}" | grep -qx "$s" || not_run+=("$s") done - echo "RESULT: FILTERED ${RAN_COUNT}/${EXPECTED_COUNT} suites ran (not run: ${not_run[*]}) — filtered: $FILTER $(canonical_rating)" + echo "RESULT: FILTERED ${RAN_COUNT}/${EXPECTED_COUNT} suites ran (not run: ${not_run[*]}) - filtered: $FILTER $(canonical_rating)" exit 3 fi -# GREEN: all canonical suites ran, all passed, no ignored test cases. -echo "RESULT: GREEN ${RAN_COUNT}/${EXPECTED_COUNT} suites passed $(canonical_rating)" +# GREEN: all canonical suites ran, all passed, no ignored test cases, nothing undeclared left behind. +echo "RESULT: GREEN ${RAN_COUNT}/${EXPECTED_COUNT} suites passed, all CLEAN $(canonical_rating)" exit 0 diff --git a/bin/test-lint-unity-exit.sh b/bin/test-lint-unity-exit.sh new file mode 100755 index 0000000000..7c1dac30d7 --- /dev/null +++ b/bin/test-lint-unity-exit.sh @@ -0,0 +1,101 @@ +#!/usr/bin/env bash +# Self-test for bin/lint-unity-exit.sh. +# +# This exists because the scanner has been wrong twice in review, both times in a way that looked +# fine by inspection: layered regexes cannot tokenise C++, so a `/*` inside a string literal flipped +# comment state, a greedy `.*` swallowed code between two comments, `myexit(...)` matched the `exit` +# exemption as a substring, and `==` matched the assignment exemption. Every one of those is pinned +# below as a fixture, so the next rewrite has to keep them all passing. +# +# Each fixture is a snippet of C++ plus the exact diagnostics it must produce, as a comma-separated +# list of : - empty for none. Asserting the locations rather than just "did it say +# anything" is what catches a rule that reports the right number of findings in the wrong places, or +# that collapses two findings on one line into one. +# +# Not a Unity suite and not counted in test/native-suite-count - same arrangement as +# bin/test-state-check.sh, and for the same reason: it asserts the behaviour of a process. +# +# Usage: ./bin/test-lint-unity-exit.sh (exit 0 = all fixtures behaved) + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +ROOT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" +cd "$ROOT_DIR" || exit 1 + +WORK="$(mktemp -d -t meshlintunity.XXXXXX)" +trap 'rm -rf "$WORK"' EXIT +mkdir -p "$WORK/test/probe" + +PASSES=0 +FAILURES=0 + +# expect ":[,:...]"