From bca7c0b480d77cb4208d93540a5bf7cbc6823027 Mon Sep 17 00:00:00 2001 From: Tom <116762865+NomDeTom@users.noreply.github.com> Date: Sun, 16 Aug 2026 11:34:02 +0000 Subject: [PATCH] Tom fiddles with the test suite - again (#11517) * test: make every suite run its own binary, and fail the run when it does not PlatformIO links every native test program to the one $BUILD_DIR/$PROGNAME path and attributes Unity output by text alone, never checking that the source file a case came from belongs to the suite it thinks it ran. Both harnesses had been split into a build pass (--without-testing) and a run pass (--without-building), and for a non-embedded platform the run pass never relinks - so all 57 suites executed whichever suite was linked last, each reporting PASSED under its own name. Introduced for CI in 4906f8a6 and for bin/run-tests.sh in de6b2319; both ran fused, and correctly, before that. Drop --without-building from both run passes. The --without-testing pass stays as a warm-up so no single suite absorbs the whole src compile in its reported duration; with the objects already cached the per-suite step is one test_main.cpp plus a link. Add bin/check-test-attribution.py, which grades the JUnit reports both harnesses already produce. It fails on a test case whose source file lies outside the suite that reported it, and on a suite that was asked to run and produced no cases at all. Wired in three places: bin/run-tests.sh as a RED verdict ahead of the softer ones, per area in CI so a mismatch names its area, and once over the merged report so an area that never executed cannot hide. Suite ownership is matched on whole path segments, so test_mesh does not claim test_mesh_module, and the -f pattern is resolved against the canonical set rather than taken as a literal suite name. * fix(test): pin simradio off for the packet-signing PKI cases [env:coverage] passes -s to the test binary (74e6723ad, #8251), which sets portduino_config.force_simradio. wouldEncryptWithPKC() lists !force_simradio among its preconditions, so perhapsEncode() takes the channel-crypto branch, returns NONE and leaves pki_encrypted false - failing test_B11_normal_unicast_still_uses_pki and test_B12_licensed_receiver_does_not_decrypt_pki, both of which assert the production PKI path. [env:native] passes no such flag, which is the whole of the long-standing "passes under native, fails under coverage" split; it was never gcov, ASan or a host. Save and clear the flag in setUp, restore it in tearDown, so the suite asserts the encode path it is named for under either env's invocation. Same binary, pristine $HOME: 77 tests 0 failures with -s and without, where before -s gave 2 failures. Whether the unit-test binary should run with -s at all is a separate question - it means CI exercises the simradio configuration for every suite - and is left alone here. * fix(router): drive the admin-key fallback budget from the injectable clock The budget is 8 tokens refilling one per 250ms of wall clock, and test_admin_key_fallback_is_rate_limited drains it with eight PKI decodes before asserting the ninth is refused. That gives the drain loop 31ms per iteration, each of which generates a keypair and does three X25519 operations under gcov and ASan. This box runs them in ~4ms; a GitHub runner takes ~38ms, so a token refills mid-drain and the packet the test expects to be blocked decodes. Measured from both runs' own log timestamps, 9.5x apart. Read the bucket through Time::getMillis() instead of millis(), and have the test set and advance the virtual clock rather than sleeping. The subtraction was already wrap-correct, so the deadline guard is unaffected. Restores the clock in tearDown so the rest of the suite is untouched, and drops ~3s of real sleeping from the run. * test: declare the event-channel suites' shared state Both construct a NodeDB, whose constructor persists a default set into an empty prefs directory, so each writes the five prefs protos. Neither was declared, because until suites started running their own binaries nothing had ever observed them writing anything. * test: add a repeat runner for order-independent flakes A single green run says nothing about a real-time race or a slow-host margin: the rate-limit budget above passes here with 7x headroom and still fails on a CI runner. Run one suite N times against a fresh scratch $HOME each time, optionally against CPU contention, and print a flake rate. Failing runs keep their log and their sandbox; passing runs leave nothing. Simradio is taken from the env's own test_testing_command, so a stress run reproduces the real invocation rather than inventing a third one. * fix(test): keep a native test run off the host's radio bin/pio-test-isolate.sh sandboxes $HOME, but portduinoSetup() looks for config in ./config.yaml and /etc/meshtasticd/config.yaml - the second absolute, so no $HOME sandbox can hide it. On a machine running meshtasticd that config selects the real LoRa module and the run continues into GPIO and SPI setup, so ./bin/run-tests.sh -e native would drive the developer's own radio without saying so. -e native is also the faster of the two, and the one reached for when iterating. [env:coverage] already passes -s, which short-circuits ahead of the config search and returns before hardware init. Pass it for [env:native] too. That closes the hazard and, incidentally, makes the two envs invoke the binary identically - they did not, which is the whole of the long-standing "green locally, red in CI" split. * test: run every suite with PKC on, and assert it stays that way force_simradio does two unrelated jobs. It keeps portduinoSetup() off the host's hardware, which every test run wants, and it makes wouldEncryptWithPKC() return false, which no test run wants: the encode path under test then falls back to channel crypto and any case asserting PKI fails, or worse, passes while asserting the wrong thing. Three suites had each worked this out separately and cleared the flag themselves - test_admin_session_repro's comment describes the mechanism exactly. Clear it once in initializeTestEnvironment() instead. By then portduinoSetup() has already skipped the config search and chosen the simulated radio, and it never reconsults the flag, so clearing it cannot bring hardware back; the only remaining readers are the PKC gate and an exit_simulator intercept no test can reach. The per-suite copy added to test_packet_signing for B11/B12 goes away with it. Two asserts, because both invariants were true only by inspection: - No listening sockets. main.cpp's setup()/loop() are compiled out under PIO_UNIT_TESTING, so the phone API, MQTT and the web server never start - but nothing checked. A suite that pulled in a service binding a port would open one on the developer's machine for the length of the run. - force_simradio still clear, before every test rather than once per suite, since a case that restores a struct it snapshotted earlier puts it back and silently disables PKC for everything after it. Named per test, so the report points at the case after the culprit. Both exit rather than TEST_FAIL: they run outside a Unity test frame, and silently repairing either one would leave the suite that broke it passing. Verified by disabling the clear and watching the guard fire on the first case instead of reporting two quiet failures. * test: let the repeat runner vary suite order too Repeating one binary finds races and slow-host margins; it cannot find state that leaks from one suite into the next, because only one suite runs. --shuffle drives run-tests.sh --seed with a fresh seed each iteration and reports which seeds went red, so the shuffle already in the harness yields a flake rate rather than a single sample. Seeds are printed and replayable. * fix(test): baseline the environment from whichever runs first Clearing force_simradio in initializeTestEnvironment() missed the suites that never call it. test_atak is one, and it also pulls in TestUtil.h, so it got the per-test assert without ever getting the baseline and aborted on its first case - caught by CI, which is what the assert is for. test_geocoord_distance, test_meshpacket_serializer and test_utf8 skip the init too, but include no TestUtil.h at all, so nothing reached them either way. Move the clear and the socket check into baselineEnvironment(), called from initializeTestEnvironment() or from the first RUN_TEST, whichever comes first. Suites that initialise are still asserted from their first case; the rest are baselined at case one and asserted from case two. Print the violation on stdout as well as stderr: bin/run-tests.sh filters the program's stderr, so locally the message vanished and the run reported "exit-time abort (likely sanitizer)" - the exit code read as a signal number again, with no sign of the real reason. * test: drop the per-suite simradio exceptions Three suites had each found that force_simradio disables PKC and cleared it themselves. initializeTestEnvironment() now clears it once for every suite, so all six sites are dead code - along with the PortduinoGlue.h include each pulled in for it. test_event_channel_router's is the one worth removing rather than leaving: it snapshotted the flag into SavedGlobals and restored it at teardown, which is exactly the shape the per-test assert exists to catch. Harmless while the snapshot reads false, and a silent PKC-off for every later case if that ever changed. The three suites pass unchanged: 54 cases, attribution clean. * test: tell a deliberate harness abort from a sanitizer fault A guard in TestUtil.cpp that aborts on purpose - a listening socket, or force_simradio put back - exits non-zero with no sanitizer report, so it fell through to the exit-time-abort heuristic and was announced as "RED exit-time abort (tests passed; likely sanitizer)". That is the same trap as the phantom SIGILL two checks above: a verdict line naming a cause it has not established, sending the reader after a memory bug that does not exist. It cost hours in the original investigation and it cost the first read of a test_atak failure today. Match the FATAL line the guards print on stdout for exactly this purpose, and report the reason they gave instead of guessing. * test: say why three suites omit TestUtil.h They are pure-function - no NodeDB, no router, no sockets, no PKC - so the harness-wide guards in TestUtil.h would assert conditions they cannot reach, and initializeTestEnvironment()'s RTC and OSThread setup would pull in portduino globals they otherwise never touch. Suite-level state cleanliness still applies: bin/pio-test-isolate.sh fingerprints the sandbox from outside and wraps every suite regardless. Recorded at the top of each so the omission reads as a decision rather than an oversight - it looked like the latter when the socket and simradio asserts landed. * test(traffic): give every case a primary channel resetTrafficConfig() zeroed channelFile and left channels_count at 0, so the 66 cases that do not install a channel themselves ran against a device with none. Every router lookup then hit Channels::getByIndex()'s out-of-range branch and logged, which is 12106 of the suite's 20088 ERROR lines and tests nothing - a real device always has a primary channel, and no case here asserts channels-unset behaviour. Install the well-known primary the suite already builds for its precision cases. All 85 pass unchanged, and the suite's ERROR output drops to 7985, the remainder being decode failures from test_tm_fuzz_nodenum_blitz's malformed payloads. * test: budget each suite's LOG_ERROR output A suite can pass while emitting six figures of ERROR, which buries a real failure and trains everyone to skim. Count them per suite and grade the count as a second axis, alongside the CLEAN/DIRTY verdict already computed from the same captured log. Declared in the same manifest, as a RANGE rather than a ceiling, because for a fuzz suite the floor is the half that matters: test_fuzz_decode logging ~100k rejections is the suite working, and the same suite logging none means it stopped feeding malformed input while every case still passes. Bounds are wide on purpose - they catch a path that has stopped running, not a drift of a few hundred lines. Undeclared suites get 100, which 50 of 57 already meet. AMBER, not RED. Three log sites - mesh-pb-constants.cpp:28, Channels.cpp:356, MQTT.cpp:92 - account for nearly all the remaining volume, and landing this red before they are demoted would buy exemptions rather than fixes. * test: canary the attribution check, and run the state self-test in CI check-test-attribution.py guards against the false green, and nothing guarded the guard. A checker that has quietly stopped matching looks exactly like a codebase with no problem, which is how the original went unnoticed for three weeks of green runs. The canary reproduces the failure deliberately - two suites run with --without-building, so PlatformIO does not relink and both execute the same leftover binary - and requires the checker to catch it. It also fails if the reproduction stops reproducing: if PlatformIO ever relinks per suite under that flag, the reason both harnesses stopped passing it no longer holds, and the harness should be revisited rather than left on a stale assumption. bin/test-state-check.sh already existed with fixtures asserting CLEAN/CLEAN/DIRTY/MISSING and had never run in CI. Wire it in too - the shared-state checker had the same blind spot, and somebody had already written the test for it. * fix(ci): run the attribution canary where it cannot clobber the daemon The canary relinks $BUILD_DIR/$PROGNAME, and in simulator-tests that replaced the daemon binary with a test suite. The integration test then started it and waited for a listening socket, which a test binary never opens - by assertion, since initializeTestEnvironment() now fails a suite that holds one - so the step sat until its 20s timeout and the job exited 124. The canary itself had already passed. Move it to platformio-tests, where the binary is per-suite already and nothing downstream needs the daemon, and place it after the coverage capture so its extra runs stay out of the numbers. The shared-state self-test stays in simulator-tests; it touches no binary. Fitting failure mode for this branch: one shared program path, two consumers, and the second one silently getting the first one's build. * fix(ci): silence the XXE rule on the attribution checker semgrep blocks xml.etree.ElementTree.parse as XXE-prone. The input here is the JUnit report PlatformIO wrote moments earlier in the same run, and anything able to plant a hostile report is already executing its own code in that job, so parsing it defused changes nothing it could do. defusedxml is in the tree but only under bin/bump_metainfo with its own requirements, and pulling it onto this path would add an install step to every native test job for no reachable threat. Suppressed with a reason at the call site, the same shape as the subprocess-shell-true suppression in extra_scripts/nrf54l15_linker.py. * fix(test): address the review findings on the harness guards Two were real defects rather than style: - state_count_errors() returned "0\n0" for a log with no ERROR lines, because grep -c prints 0 and *then* exits 1, so the `|| printf 0` fallback appended a second one. The classifier threw a syntax error on it. Dormant only because every suite currently emits at least one ERROR line; the planned log-level demotions would have driven most suites to zero and tripped it everywhere, looking like the demotions broke the harness. - check-test-attribution.py returned OK for a report whose cases carry no `file` attribute. It cannot prove ownership in that state, so a changed JUnit format would have restored the exact false green it exists to catch. Now its own finding, listed and fatal. The rest: keep the sandbox when an error budget is breached, since that is the one outcome whose evidence was being deleted; reject a missing or non-numeric option value in stress-suite.sh instead of running an empty loop and reporting 0/0 as a pass; exit on INT/TERM rather than cleaning up and carrying on; drive repetitions through pio-test-isolate.sh so a stress run exercises the real invocation; require the canary to see MISATTRIBUTED rather than any non-zero exit, so an unreadable report cannot read as a caught mismatch; and check for listening sockets before every test, since a listener would be opened by the code under test. resetAdminKeyFallbackBudget() is a new PIO_UNIT_TESTING hook, shaped like the neighbouring resetRoutingAuthEvaluationCount(). The refill stamp is only meaningful against the clock that produced it, so a suite switching timebases leaves a stamp from the other one and the next unsigned subtraction reads as a near-infinite gap - silently refilling the bucket. Also move the semgrep marker onto its own line: buried mid-sentence in a comment it was ignored, and the XXE finding stayed blocking. --- .github/workflows/test_native.yml | 61 +++++- bin/check-test-attribution.py | 165 ++++++++++++++ bin/lib/test-state.sh | 50 +++++ bin/pio-test-isolate.sh | 11 +- bin/run-tests.sh | 89 +++++++- bin/stress-suite.sh | 202 ++++++++++++++++++ bin/test-attribution-canary.sh | 82 +++++++ src/mesh/Router.cpp | 15 +- src/mesh/Router.h | 2 + test/README.md | 2 + test/TestUtil.cpp | 128 +++++++++++ test/TestUtil.h | 9 + test/state-manifest.tsv | 19 +- test/test_admin_session_repro/test_main.cpp | 12 -- test/test_event_channel_router/test_main.cpp | 15 -- test/test_geocoord_distance/test_main.cpp | 5 + .../test_serializer.cpp | 5 + test/test_pki_admin_fallback/test_main.cpp | 14 +- test/test_position_precision/test_main.cpp | 7 - test/test_traffic_management/test_main.cpp | 6 +- test/test_utf8/test_main.cpp | 5 + variants/native/portduino/platformio.ini | 7 + 22 files changed, 854 insertions(+), 57 deletions(-) create mode 100755 bin/check-test-attribution.py create mode 100755 bin/stress-suite.sh create mode 100755 bin/test-attribution-canary.sh diff --git a/.github/workflows/test_native.yml b/.github/workflows/test_native.yml index 04e6b3a23..2167e2956 100644 --- a/.github/workflows/test_native.yml +++ b/.github/workflows/test_native.yml @@ -195,6 +195,13 @@ jobs: timeout-minutes: 5 run: ./bin/test-config-check.sh .pio/build/coverage/meshtasticd + - name: Shared-state checker self-test + # Fixtures that write nothing / exactly what they declare / something undeclared / + # a declared write they never make, asserting CLEAN / CLEAN / DIRTY / MISSING. A + # checker that has silently stopped matching looks identical to a clean codebase. + timeout-minutes: 5 + run: ./bin/test-state-check.sh + - name: Integration test # Cap the whole step: if the simulator ever fails to exit (e.g. the # exit_simulator admin path regresses again) the job must fail fast, @@ -273,9 +280,12 @@ jobs: restore-keys: | pio-coverage-tests- - - name: Build test programs once - # One shared build of src + every test program. This is the single source build; gcov then - # accumulates coverage counts into this shared .pio/build/coverage/src as the chunks run. + - name: Warm the shared test build + # Compiles src + every test program once so no single area absorbs the whole src build in + # its reported duration; gcov then accumulates counts into this shared + # .pio/build/coverage/src as the areas run. NOT a substitute for building in the run step: + # PlatformIO links every test program to the one .pio/build/coverage/meshtasticd path, so a + # --without-building run executes whichever suite was linked last under every suite's name. run: platformio test -e coverage --without-testing - name: Save PlatformIO cache @@ -368,12 +378,21 @@ jobs: echo "::group::area $a (${group[$a]# })" # Capture platformio's real exit status (not grep's) via a log file, then show the log # with the noisy per-variant SKIPPED rows filtered out. - if ! platformio test -e coverage --without-building -v ${group[$a]# } \ + if ! platformio test -e coverage -v ${group[$a]# } \ --junit-output-path "testreport-$a.xml" > "area-$a.log" 2>&1; then fail=1 echo "::error::area $a had test failures" fi + # Suites outside this area are reported SKIPPED by design (PlatformIO lists every suite + # in the env and marks the unselected ones finished), so those rows are noise here. The + # attribution check below is what catches a suite that was selected and did not run. grep -v "[[:space:]]SKIPPED$" "area-$a.log" || true + # Per area, so a mismatch names the area it happened in rather than the whole run. + if ! ./bin/check-test-attribution.py --label "area $a" \ + --expect "${group[$a]# }" "testreport-$a.xml"; then + fail=1 + echo "::error::area $a ran suites that did not match their own test binaries" + fi echo "::endgroup::" done exit $fail @@ -398,6 +417,18 @@ jobs: ET.ElementTree(out).write('testreport.xml', encoding='utf-8', xml_declaration=True) PY + - name: Verify every suite ran its own tests + # Whole-run gate over the merged report: every test_* directory must appear with at least + # one test case, and every case must come from the suite that reported it. The per-area + # check above cannot see an area that never executed - this can. + if: always() # a suite going missing is the finding; do not hide it behind an earlier failure + shell: bash + run: | + set -euo pipefail + mapfile -t suites < <(find test -maxdepth 1 -type d -name 'test_*' -printf '%f\n' | sort) + ./bin/check-test-attribution.py --label "coverage (all areas)" \ + --expect "${suites[*]}" testreport.xml + - name: Capture coverage information if: always() # run this step even if previous step failed run: | @@ -405,9 +436,31 @@ jobs: lcov ${{ env.LCOV_CAPTURE_FLAGS }} --test-name tests --output-file coverage_tests.info sed -i -e "s#${PWD}#.#" coverage_tests.info # Make paths relative. + - name: Attribution canary + # Guards the guard above: runs two suites the broken way (--without-building, so PlatformIO + # does not relink and both execute the same leftover binary) and requires the checker to + # catch it. Fails if the checker regressed, or if the reproduction stops reproducing - in + # which case the reason both harnesses stopped passing that flag no longer holds. + # + # Lives in this job, not simulator-tests: it relinks $BUILD_DIR/$PROGNAME, and there that + # replaced the daemon binary with a test suite, so the integration test waited for a socket + # a test binary never opens. Here the binary is already per-suite and nothing later needs it. + timeout-minutes: 15 + run: ./bin/test-attribution-canary.sh -e coverage + - name: Event channel policy tests run: platformio test -e coverage-event-policy -v --junit-output-path event-policy-testreport.xml + - name: Verify the event-policy suites ran their own tests + # Expected set read through PlatformIO's own config parser, so it cannot drift from the + # env's test_filter the way a second hand-maintained list would. + run: | + set -euo pipefail + expect=$(python3 -c "from platformio.project.config import ProjectConfig; \ + print(' '.join(ProjectConfig().get('env:coverage-event-policy', 'test_filter', [])))") + ./bin/check-test-attribution.py --label coverage-event-policy \ + --expect "$expect" event-policy-testreport.xml + - name: Save test results if: always() # run this step even if previous step failed uses: actions/upload-artifact@v7 diff --git a/bin/check-test-attribution.py b/bin/check-test-attribution.py new file mode 100755 index 000000000..2d3d258e4 --- /dev/null +++ b/bin/check-test-attribution.py @@ -0,0 +1,165 @@ +#!/usr/bin/env python3 +"""Verify each PlatformIO JUnit report ran the suite it claims to have run. + +PlatformIO links every native test program to one path ($BUILD_DIR/$PROGNAME) and parses +Unity output textually, without checking that the reported source file belongs to the suite +it is running. Split a run into `--without-testing` then `--without-building` and every suite +executes whichever binary was linked last, all reporting PASSED. This reads the JUnit reports +that run already produces and fails on the two shapes that hides: + + MISATTRIBUTED - a test case whose source file lives outside the suite that reported it + EMPTY - a suite that was asked to run and produced no test cases at all + +Usage: + check-test-attribution.py [--expect "s1 s2"]... [--label TEXT] REPORT.xml... + +--expect names the suites the run was asked for (repeatable, whitespace- or `-f`-separated, +so a CI area string can be passed through verbatim). Omit it to check attribution only. +Exit: 0 clean, 1 findings, 2 bad usage / unreadable report. +""" + +import argparse +import glob +import sys +import xml.etree.ElementTree as ET + + +def parse_expect(values): + """Flatten repeated --expect values into a suite list, tolerating `-f suite` tokens.""" + suites = [] + for value in values or []: + for token in value.split(): + if token == "-f": + continue + suites.append(token.removeprefix("-f")) + return [s for s in suites if s] + + +def suite_of(testsuite_name): + """`coverage:test_foo` -> `test_foo`; a bare name is returned unchanged.""" + return testsuite_name.split(":", 1)[1] if ":" in testsuite_name else testsuite_name + + +def owns(suite, source_file): + """Report whether source_file sits inside the suite's own directory. + + Matched on a whole path segment so `test_mesh` does not claim `test_mesh_module`, and + with a leading separator so absolute and relative paths behave the same. + """ + normalized = "/" + source_file.replace("\\", "/").lstrip("/") + return f"/{suite}/" in normalized + + +def collect(paths): + """Map suite -> list of (case name, source file or None), merged across reports.""" + cases = {} + for path in paths: + try: + # The input is the JUnit report PlatformIO just wrote in this same run, not untrusted + # data, and defusedxml is not installed for this job. + # nosemgrep: python.lang.security.use-defused-xml-parse.use-defused-xml-parse + root = ET.parse(path).getroot() + except (ET.ParseError, OSError) as exc: + sys.stderr.write(f"check-test-attribution: cannot read {path}: {exc}\n") + sys.exit(2) + # PlatformIO nests under ; accept a bare too. + nodes = [root] if root.tag == "testsuite" else root.iter("testsuite") + for node in nodes: + suite = suite_of(node.get("name", "")) + if not suite: + continue + entries = cases.setdefault(suite, []) + for case in node.iter("testcase"): + entries.append((case.get("name", "?"), case.get("file"))) + return cases + + +def main(): + parser = argparse.ArgumentParser(add_help=True) + parser.add_argument("--expect", action="append", default=[]) + parser.add_argument("--label", default="") + parser.add_argument("reports", nargs="+") + args = parser.parse_args() + + # Expand globs ourselves: CI passes a pattern that may match nothing if a step was skipped, + # and a silent pass over zero reports is exactly the false green this script exists to stop. + paths = sorted({p for pattern in args.reports for p in glob.glob(pattern)}) + if not paths: + sys.stderr.write( + "check-test-attribution: no JUnit reports matched %s\n" + % " ".join(args.reports) + ) + return 2 + + cases = collect(paths) + expected = parse_expect(args.expect) + + misattributed = [] # (suite, case name, source file) + unsourced = [] # (suite, case name) + for suite, entries in sorted(cases.items()): + for name, source in entries: + if source is None: + unsourced.append((suite, name)) + elif not owns(suite, source): + misattributed.append((suite, name, source)) + + empty = [s for s in expected if not cases.get(s)] + + label = f" [{args.label}]" if args.label else "" + total = sum(len(v) for v in cases.values()) + print( + f"test attribution{label}: {len(paths)} report(s), " + f"{len([s for s, v in cases.items() if v])} suite(s) with cases, {total} case(s)" + ) + if unsourced: + print("") + print("UNSOURCED - these cases carry no source file, so ownership cannot be proved:") + for suite, name in unsourced[:20]: + print(f" {suite}: case '{name}'") + if len(unsourced) > 20: + print(f" ... +{len(unsourced) - 20} more") + print( + "A report without file attributes is not evidence that the suites ran their own" + ) + print( + "tests. Treat it as a finding rather than a pass: the JUnit format has changed, or" + ) + print("the runner emitted cases it could not attribute.") + + if misattributed: + print("") + print( + "MISATTRIBUTED - these suites reported test cases belonging to another suite." + ) + print( + "The run executed one suite's binary under another suite's name; the named" + ) + print( + "suites did NOT run. Check for --without-building in the test invocation." + ) + for suite, name, source in misattributed[:20]: + print(f" {suite}: case '{name}' came from {source}") + if len(misattributed) > 20: + print(f" ... +{len(misattributed) - 20} more") + + if empty: + print("") + print("EMPTY - these suites were asked to run and produced no test cases:") + for suite in empty: + print(f" {suite}") + + if misattributed or empty or unsourced: + print("") + print( + "RESULT: test attribution FAILED" + f"{label} ({len(misattributed)} misattributed, {len(empty)} empty," + f" {len(unsourced)} unsourced)" + ) + return 1 + + print(f"RESULT: test attribution OK{label}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/bin/lib/test-state.sh b/bin/lib/test-state.sh index a0c624455..ce77fdd2e 100644 --- a/bin/lib/test-state.sh +++ b/bin/lib/test-state.sh @@ -167,3 +167,53 @@ state_classify() { printf 'CLEAN\t\n' fi } + +# --- Error-line budget ------------------------------------------------------------------------- +# +# A second orthogonal axis, like CLEAN/DIRTY above: a suite can pass while emitting six figures of +# LOG_ERROR, which buries a real failure and trains everyone to skim. The budget is declared in the +# same manifest, as an `errors=` flag, and it is a RANGE rather than a ceiling - for a fuzz suite the +# floor is the load-bearing half. test_fuzz_decode logging ~100k rejections is it working; the same +# suite logging none means it stopped feeding malformed input, and every case would still pass. +# +# Undeclared suites get ERROR_BUDGET_DEFAULT. Declared forms: "N" (max), "MIN..MAX", "MIN.." (floor +# only). Everything is inclusive. +ERROR_BUDGET_DEFAULT=100 + +# Count LOG_ERROR lines in a suite's captured output. +state_count_errors() { + local log="$1" + [[ -f $log ]] || { + printf '0' + return 0 + } + # `|| true`, not `|| printf 0`: grep -c already prints 0 before exiting 1 on no match, so a + # fallback that prints appends a second line and the caller gets "0\n0" to do arithmetic on. + grep -cE '^ERROR +\|' "$log" 2>/dev/null || true +} + +# VERDICTDETAIL. WITHIN / OVER / UNDER, mirroring state_classify()'s shape. +state_classify_errors() { + local count="$1" declared="$2" min=0 max="$ERROR_BUDGET_DEFAULT" + + if [[ -n $declared ]]; then + if [[ $declared == *".."* ]]; then + min="${declared%%..*}" + max="${declared##*..}" + [[ -z $max ]] && max="" + else + max="$declared" + fi + fi + + if [[ -n $max ]] && ((count > max)); then + printf 'OVER\t%d error line(s), budget %s' "$count" "${declared:-$ERROR_BUDGET_DEFAULT}" + return 0 + fi + if ((count < min)); then + printf 'UNDER\t%d error line(s), expected at least %d - is it still exercising the path?' \ + "$count" "$min" + return 0 + fi + printf 'WITHIN\t%d' "$count" +} diff --git a/bin/pio-test-isolate.sh b/bin/pio-test-isolate.sh index bd58c73eb..bfc51cffd 100755 --- a/bin/pio-test-isolate.sh +++ b/bin/pio-test-isolate.sh @@ -92,6 +92,11 @@ GRANULARITY="$(state_flag_value state "$FLAGS")" IFS=$'\t' read -r VERDICT DETAIL <<<"$(state_classify "$CHANGED" "$DECLARED")" +# Error-line budget: same manifest, same declare-and-justify shape as the writes above. Counted from +# the captured log, so it costs nothing extra. +ERROR_COUNT="$(state_count_errors "$LOG")" +IFS=$'\t' read -r ERROR_VERDICT ERROR_DETAIL <<<"$(state_classify_errors "$ERROR_COUNT" "$(state_flag_value errors "$FLAGS")")" + # 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. @@ -114,14 +119,14 @@ 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" +printf '%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n' "$SUITE" "$STATUS" "$VERDICT" "${DETAIL-}" "${PER_TEST_DETAIL-}" \ + "${SURVIVORS-}" "${ERROR_VERDICT-}" "${ERROR_DETAIL-}" >>"$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 +if [[ $RC -ne 0 || $VERDICT != CLEAN || $ERROR_VERDICT != WITHIN || -n ${SURVIVORS-} || $KEEP == 1 ]]; then DEST="$STATE_ROOT/$SUITE" rm -rf "$DEST" 2>/dev/null mv "$SCRATCH" "$DEST" 2>/dev/null || DEST="$SCRATCH" diff --git a/bin/run-tests.sh b/bin/run-tests.sh index dcc3a705f..f454d5c8f 100755 --- a/bin/run-tests.sh +++ b/bin/run-tests.sh @@ -38,7 +38,8 @@ # 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, sanitizer fault, or a suite that reported +# another suite's test cases (bin/check-test-attribution.py). # # 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". @@ -59,6 +60,7 @@ # 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: RED test attribution failed - suites did not run their own tests # 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 @@ -163,6 +165,16 @@ export MESHTASTIC_TEST_STATE_SUMMARY="$STATE_SUMMARY" $KEEP_STATE && export MESHTASTIC_TEST_KEEP_STATE=1 $WRITE_MANIFEST && export MESHTASTIC_TEST_KEEP_STATE=1 +# --- Test attribution -------------------------------------------------------- +# PlatformIO parses Unity output textually and never checks that the source file a case came from +# belongs to the suite it thinks it ran, so one suite's binary running under another's name reads +# as a pass. The JUnit reports carry both halves (testsuite@name vs testcase@file), so collect them +# here and grade with bin/check-test-attribution.py below. Cleared first: a stale report from an +# earlier run would otherwise satisfy this run's expectations. +ATTRIB_DIR="$ROOT_DIR/.pio/test-attribution" +rm -rf "$ATTRIB_DIR" +mkdir -p "$ATTRIB_DIR" + # Canonical suite set = the directories in test/, detected on the fly. This is the sole 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) @@ -251,10 +263,15 @@ if $SHUFFLE; then 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 +# Warm the shared src objects before running any suite, 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. +# +# This is a WARM-UP ONLY: the run below must still build. PlatformIO links every test program to +# the one $BUILD_DIR/$PROGNAME path, so a `--without-building` run executes whichever suite was +# linked last - every suite, under its own name, all PASSED. The warm-up keeps the src compile out +# of the suite timings; the per-suite step is then just one test_main.cpp plus a link. BUILD_SECS=0 build_started=$SECONDS if $QUIET; then @@ -289,19 +306,23 @@ if $SHUFFLE; then : >"$LOG" for suite in "${RUN_ORDER[@]}"; do if $QUIET; then - "$PIO" test -e "$ENV" -f "$suite" "${EXTRA_ARGS[@]}" --without-building >>"$LOG" 2>&1 + "$PIO" test -e "$ENV" -f "$suite" "${EXTRA_ARGS[@]}" \ + --junit-output-path "$ATTRIB_DIR/$suite.xml" >>"$LOG" 2>&1 rc=$? else - "$PIO" test -e "$ENV" -f "$suite" "${EXTRA_ARGS[@]}" --without-building 2>&1 | tee -a "$LOG" + "$PIO" test -e "$ENV" -f "$suite" "${EXTRA_ARGS[@]}" \ + --junit-output-path "$ATTRIB_DIR/$suite.xml" 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" test -e "$ENV" "${PASSTHRU[@]}" \ + --junit-output-path "$ATTRIB_DIR/all.xml" >"$LOG" 2>&1 PIO_RC=$? else - "$PIO" test -e "$ENV" "${PASSTHRU[@]}" --without-building 2>&1 | tee "$LOG" + "$PIO" test -e "$ENV" "${PASSTHRU[@]}" \ + --junit-output-path "$ATTRIB_DIR/all.xml" 2>&1 | tee "$LOG" PIO_RC=${PIPESTATUS[0]} fi @@ -426,6 +447,18 @@ verdict_red() { exit 1 fi + # A guard in test/TestUtil.cpp aborting on purpose - a listening socket, or force_simradio put + # back. It prints FATAL on stdout precisely so this can be told apart from a fault: otherwise its + # exit(EXIT_FAILURE) lands in the heuristic below and is reported as a sanitizer abort that never + # happened, which is the same wrong-cause-in-the-verdict trap as the phantom signal above. + if grep -qE '^FATAL: ' "$LOG"; then + grep -E '^FATAL: ' "$LOG" | head -3 | sed 's/^/ /' + echo " -> a harness guard aborted the suite deliberately. Not a crash and not a sanitizer" + echo " fault; the reason is the FATAL line above, and the suite's sandbox has the full log." + echo "RESULT: RED harness guard - $(grep -m1 -oE '^FATAL: .*' "$LOG")" + 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. @@ -462,6 +495,34 @@ verdict_suffix() { echo "$rating" } +# --- Attribution axis --------------------------------------------------------- +# RED, and checked before every softer verdict: a suite that reported another suite's test cases +# did not run at all, so every count and state verdict below it is measuring the wrong thing. A +# filtered run expects only its own suite; a full run expects the canonical set. +# -f takes an fnmatch pattern, not necessarily a suite name, so resolve it against the canonical +# set rather than expecting a suite literally called "test_nodedb*". An unmatched pattern leaves +# the list empty, which checks attribution only - a filter that selects nothing is already RED +# above, for want of a pass summary. +ATTRIB_EXPECT="${ALL_SUITES[*]}" +if [[ -n $FILTER ]]; then + ATTRIB_EXPECT="" + for attrib_suite in "${ALL_SUITES[@]}"; do + # shellcheck disable=SC2053 # deliberate glob match: FILTER is a pattern, not a literal + [[ $attrib_suite == $FILTER ]] && ATTRIB_EXPECT+="$attrib_suite " + done +fi +ATTRIB_OUT="$("$SCRIPT_DIR/check-test-attribution.py" --expect "$ATTRIB_EXPECT" \ + --label "$ENV" "$ATTRIB_DIR"/*.xml 2>&1)" +ATTRIB_RC=$? +if ((ATTRIB_RC != 0)); then + echo "" + echo "$ATTRIB_OUT" | sed 's/^/ /' + preserve_run_log + echo "RESULT: RED test attribution failed - suites did not run their own tests $(verdict_suffix)" + exit 1 +fi +$QUIET || echo "$ATTRIB_OUT" | tail -1 + # --- 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. @@ -472,6 +533,7 @@ 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") + mapfile -t ERROR_BUDGET_SUITES < <(awk -F'\t' '$7 == "OVER" || $7 == "UNDER" { print $1 " " tolower($7) " budget: " $8 }' "$STATE_SUMMARY") fi # Print the opt-out count on every run, so the number creeping upward is visible without anyone @@ -551,6 +613,21 @@ if ((${#DIRTY_SUITES[@]} > 0)); then exit 2 fi +# AMBER: a suite spent its LOG_ERROR budget, or came in under a declared floor. Over budget buries a +# real failure in noise - three log sites account for nearly all of today's volume, and until those +# are demoted this stays AMBER rather than RED so it does not land red on day one and get switched +# off. Under a floor is the more interesting half: a fuzz suite that stops logging rejections has +# stopped feeding malformed input, and every one of its cases still passes. +if ((${#ERROR_BUDGET_SUITES[@]} > 0)); then + echo "" + printf ' %s\n' "${ERROR_BUDGET_SUITES[@]}" + echo "" + echo " -> over: demote the log line if the condition is expected, or declare errors= in" + echo " test/state-manifest.tsv with a reason. Under: check the suite still exercises the path." + echo "RESULT: AMBER ${#ERROR_BUDGET_SUITES[@]} suite(s) outside their error budget $(verdict_suffix)" + 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 diff --git a/bin/stress-suite.sh b/bin/stress-suite.sh new file mode 100755 index 000000000..ec63612b1 --- /dev/null +++ b/bin/stress-suite.sh @@ -0,0 +1,202 @@ +#!/usr/bin/env bash +# +# Run one native test suite repeatedly and report how often it fails. +# +# For order-independent flakes - a real-time race, a slow-host margin, an uninitialised read - a +# single green run proves nothing. This runs the same built binary N times and prints a flake rate, +# so "passes here" becomes a measurement instead of an anecdote. +# +# ./bin/stress-suite.sh test_pki_admin_fallback # 20 runs, coverage, as CI invokes it +# ./bin/stress-suite.sh -n 200 test_packet_signing # 200 runs +# ./bin/stress-suite.sh -e native -n 50 test_admin_radio # the other env's invocation +# ./bin/stress-suite.sh -l 8 -n 50 test_pki_admin_fallback # 8 spinners of CPU contention +# ./bin/stress-suite.sh --no-simradio -n 50 test_packet_signing +# ./bin/stress-suite.sh --shuffle -n 5 # whole suite set, a new order each time +# +# --shuffle is the other axis and takes no suite name: it drives bin/run-tests.sh --seed with a fresh +# seed per iteration, so suite ORDER varies. Use it for state that leaks suite -> suite; use the +# single-suite mode above for races and slow-host margins, which order cannot expose. Every seed is +# printed, and a red one is replayable with ./bin/run-tests.sh --seed . +# +# Each run gets a fresh scratch $HOME, so no run inherits another's prefs. Failing runs keep their +# log and their $HOME; passing runs leave nothing behind. +# +# Exit: 0 = every run passed, 1 = at least one failed, 2 = usage/build error. + +set -uo pipefail + +REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +ENV_NAME=coverage +RUNS=20 +LOAD=0 +SIMRADIO=auto +SHUFFLE=false +SUITE="" + +usage() { + sed -n '3,20p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//' + exit 2 +} + +# A missing or non-numeric value used to sail through and produce a loop that never ran, reporting +# "0/0 failed" as a pass. Reject it at parse time instead. +need_value() { + [[ -n ${2:-} && $2 != -* ]] || { + echo "$1 needs a value" >&2 + exit 2 + } +} +need_number() { + [[ $2 =~ ^[0-9]+$ ]] || { + echo "$1 needs a number, got '$2'" >&2 + exit 2 + } +} + +while [[ $# -gt 0 ]]; do + case "$1" in + -e | --environment) + need_value "$1" "${2:-}" + ENV_NAME="$2" + shift 2 + ;; + -n | --runs) + need_value "$1" "${2:-}" + need_number "$1" "$2" + RUNS="$2" + shift 2 + ;; + -l | --load) + need_value "$1" "${2:-}" + need_number "$1" "$2" + LOAD="$2" + shift 2 + ;; + --shuffle) + SHUFFLE=true + shift + ;; + --simradio) + SIMRADIO=yes + shift + ;; + --no-simradio) + SIMRADIO=no + shift + ;; + -h | --help) usage ;; + -*) + echo "unknown option: $1" >&2 + usage + ;; + *) + SUITE="$1" + shift + ;; + esac +done + +if $SHUFFLE; then + [[ -z $SUITE ]] || { + echo "--shuffle varies suite order across the whole set; drop the suite name" >&2 + exit 2 + } + fails=0 + reds=() + echo "running the full suite set x$RUNS on $ENV_NAME, reshuffled each time" + for ((run = 1; run <= RUNS; run++)); do + # Seeds from /dev/urandom, printed and recorded: an order you cannot replay is not evidence. + seed=$((RANDOM * 32768 + RANDOM)) + log="$REPO/.pio/build/$ENV_NAME/stress-shuffle.$seed.log" + mkdir -p "$(dirname "$log")" + printf 'run %d/%d seed %s ... ' "$run" "$RUNS" "$seed" + if "$REPO/bin/run-tests.sh" -e "$ENV_NAME" --seed "$seed" >"$log" 2>&1; then + echo "GREEN" + rm -f "$log" + else + rc=$? + fails=$((fails + 1)) + reds+=("$seed") + echo "$(grep -m1 '^RESULT:' "$log" || echo "exit $rc") - log $log" + fi + done + echo "RESULT: $fails/$RUNS runs not green" + [[ ${#reds[@]} -gt 0 ]] && echo "replay: ./bin/run-tests.sh --seed ${reds[0]}" + [[ $fails -eq 0 ]] || exit 1 + exit 0 +fi + +[[ -n $SUITE ]] || usage + +# Mirror what the env's test_testing_command passes, so a stress run reproduces the real invocation +# rather than a third one of its own. [env:coverage] adds -s (simradio); [env:native] does not. +if [[ $SIMRADIO == auto ]]; then + # Read to the next [section] header, not a fixed window: -s is the last line of the command block. + if awk "/^\\[env:$ENV_NAME\\]/{f=1;next} /^\\[/{f=0} f" \ + "$REPO/variants/native/portduino/platformio.ini" | grep -qE '^[[:space:]]+-s[[:space:]]*$'; then + SIMRADIO=yes + else + SIMRADIO=no + fi +fi +ARGS=() +[[ $SIMRADIO == yes ]] && ARGS+=(-s) + +PIO="$REPO/.pio_env/bin/pio" +[[ -x $PIO ]] || PIO="$(command -v pio)" || { + echo "pio not found" >&2 + exit 2 +} + +BIN="$REPO/.pio/build/$ENV_NAME/meshtasticd" +echo "building $SUITE for $ENV_NAME ..." +"$PIO" test -e "$ENV_NAME" -f "$SUITE" --without-testing >/dev/null 2>&1 || { + echo "build failed - rerun without --without-testing to see why" >&2 + exit 2 +} +[[ -x $BIN ]] || { + echo "no binary at $BIN" >&2 + exit 2 +} + +LOADPIDS=() +cleanup() { + [[ ${#LOADPIDS[@]} -gt 0 ]] && kill "${LOADPIDS[@]}" 2>/dev/null + return 0 +} +# EXIT cleans up; INT/TERM must also stop, or the loop keeps launching runs after a ^C. +trap cleanup EXIT +trap 'cleanup; exit 130' INT +trap 'cleanup; exit 143' TERM + +if [[ $LOAD -gt 0 ]]; then + echo "starting $LOAD spinner(s) against $(nproc) cpu(s)" + for ((i = 0; i < LOAD; i++)); do + (while :; do :; done) & + LOADPIDS+=($!) + done +fi + +OUT="$REPO/.pio/build/$ENV_NAME/stress" +mkdir -p "$OUT" +fails=0 +echo "running $SUITE x$RUNS on $ENV_NAME (simradio=$SIMRADIO)" +for ((run = 1; run <= RUNS; run++)); do + scratch=$(mktemp -d) + log="$OUT/$SUITE.$run.log" + # Through pio-test-isolate.sh, not the bare binary: that is what test_testing_command runs, so + # a repetition here exercises the sandboxing, survivor reaping and state verdict too. + if MESHTASTIC_TEST_STATE_DIR="$scratch/state" "$REPO/bin/pio-test-isolate.sh" "$BIN" "${ARGS[@]}" >"$log" 2>&1; then + rm -rf "$scratch" "$log" + printf '.' + else + fails=$((fails + 1)) + printf '\nRUN %d FAILED - log %s - state %s\n' "$run" "$log" "$scratch" + grep -E ':(FAIL|IGNORE)' "$log" | head -5 + fi +done +printf '\n' + +pct=$((fails * 100 / RUNS)) +echo "RESULT: $fails/$RUNS failed (${pct}%)" +[[ $fails -eq 0 ]] || exit 1 diff --git a/bin/test-attribution-canary.sh b/bin/test-attribution-canary.sh new file mode 100755 index 000000000..cb873e6a8 --- /dev/null +++ b/bin/test-attribution-canary.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# Canary for bin/check-test-attribution.py: reproduce the false green on purpose and require the +# checker to catch it. +# +# The attribution check exists because both harnesses once ran every suite against whichever binary +# was linked last, so all 57 reported a pass while five test programs actually executed. A checker +# for that is only worth having if it still fires, and a checker that has quietly stopped firing +# looks exactly like a codebase with no problem. So: build two suites, run them the broken way +# (--without-building, which is what stops PlatformIO relinking on a non-embedded platform), and +# assert the checker reports a mismatch. +# +# It also fails if the reproduction stops reproducing - if PlatformIO ever relinks per suite under +# --without-building, the premise behind dropping that flag no longer holds and the harness should +# be revisited rather than left resting on a stale assumption. +# +# Not a Unity suite and not a test_* directory, so it stays outside the suite count run-tests.sh +# derives from test/ - same arrangement as bin/test-state-check.sh and bin/test-config-check.sh. +# +# Usage: ./bin/test-attribution-canary.sh [-e ] (default: coverage, as CI runs) + +set -uo pipefail + +REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$REPO" || exit 2 + +ENV_NAME=coverage +[[ ${1-} == "-e" ]] && ENV_NAME="$2" + +PIO="$REPO/.pio_env/bin/pio" +[[ -x $PIO ]] || PIO="$(command -v pio)" || { + echo "canary: pio not found" >&2 + exit 2 +} + +# Two suites whose cases cannot be confused: different source files, different counts. Both are +# small and neither touches shared state, so the canary costs a link rather than a rebuild. +A=test_utf8 +B=test_breakout +REPORT="$(mktemp -d)/canary.xml" + +echo "canary: building $A and $B for $ENV_NAME" +"$PIO" test -e "$ENV_NAME" -f "$A" -f "$B" --without-testing >/dev/null 2>&1 || { + echo "canary: build failed" >&2 + exit 2 +} + +echo "canary: running them the broken way (--without-building)" +"$PIO" test -e "$ENV_NAME" -f "$A" -f "$B" --without-building --junit-output-path "$REPORT" >/dev/null 2>&1 + +[[ -s $REPORT ]] || { + echo "canary: no JUnit report at $REPORT - cannot judge the checker" >&2 + exit 2 +} + +# The checker must FAIL here, and fail for the RIGHT reason. Exit 1 is a finding; exit 2 is bad +# usage or an unreadable report, which would let a broken canary read as a caught mismatch. +OUT="$(./bin/check-test-attribution.py --label "canary" "$REPORT" 2>&1)" +RC=$? +if [[ $RC -eq 2 ]]; then + echo "" + echo "CANARY INCONCLUSIVE: the checker could not read the report it was given (exit 2)." + echo "$OUT" + echo "Report kept at: $REPORT" + exit 2 +fi +if [[ $RC -eq 0 ]] || ! grep -q 'MISATTRIBUTED' <<<"$OUT"; then + echo "" + echo "CANARY FAILED: the attribution check passed a run that mis-attributes its cases." + echo "" + echo "Two suites were run with --without-building, so PlatformIO did not relink and both" + echo "executed the same leftover binary. check-test-attribution.py is supposed to catch exactly" + echo "that and it did not, which means the guard against the whole false-green class is dead." + echo "" + echo "Either the checker regressed, or PlatformIO now relinks per suite under --without-building" + echo "- in which case the reason bin/run-tests.sh and CI stopped passing that flag has changed," + echo "and the harness should be revisited rather than left on a stale assumption." + echo "Report kept at: $REPORT" + exit 1 +fi + +echo "canary: OK - the attribution check caught the deliberate mis-attribution" +rm -rf "$(dirname "$REPORT")" diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index e4f52c779..c382e8576 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -809,6 +809,18 @@ RoutingAuthVerdict passesRoutingAuthGate(meshtastic_MeshPacket *p) static uint32_t adminKeyFallbackTokens = ADMIN_KEY_FALLBACK_BURST; static uint32_t adminKeyFallbackRefillMs = 0; +#ifdef PIO_UNIT_TESTING +// The refill stamp is a timestamp, so it is only meaningful against the clock that produced it. +// A suite that swaps between the real and the virtual clock leaves a stamp from the other +// timebase, and the next unsigned subtraction reads as a near-infinite gap: the bucket silently +// refills to full. Re-stamp when the clock changes. +void resetAdminKeyFallbackBudget() +{ + adminKeyFallbackTokens = ADMIN_KEY_FALLBACK_BURST; + adminKeyFallbackRefillMs = Time::getMillis(); +} +#endif + static bool adminKeyFallbackAllowed() { bool haveAdminKey = false; @@ -821,7 +833,8 @@ static bool adminKeyFallbackAllowed() if (!haveAdminKey) return false; // nothing to try, so do not spend a token - uint32_t now = millis(); + // Injectable clock so the budget can be tested without sleeping, and without racing a slow host. + uint32_t now = Time::getMillis(); if (adminKeyFallbackRefillMs == 0) adminKeyFallbackRefillMs = now; uint32_t elapsed = now - adminKeyFallbackRefillMs; diff --git a/src/mesh/Router.h b/src/mesh/Router.h index eb1213de3..9882a4b9c 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -265,6 +265,8 @@ RoutingAuthVerdict passesRoutingAuthGate(meshtastic_MeshPacket *p); #ifdef PIO_UNIT_TESTING uint32_t routingAuthEvaluationCount(); void resetRoutingAuthEvaluationCount(); +/** Refill the admin-key fallback budget and re-stamp it against the clock in use right now. */ +void resetAdminKeyFallbackBudget(); #endif /** Return 0 for success or a Routing_Error code for failure diff --git a/test/README.md b/test/README.md index d1dbd804c..ceb819206 100644 --- a/test/README.md +++ b/test/README.md @@ -33,6 +33,8 @@ Randomisation costs one `pio` invocation per suite (about 4.7s each), because Pl > **Copilot interface note:** When running tests via the Copilot chat interface, edits made through the chat may not be reflected in the on-disk files that the test binary reads. If tests pass in chat but fail locally (or vice versa), verify the files on disk match what you expect before trusting the result. Always confirm with a local terminal run. +**Never add `--without-building` to a test run.** PlatformIO links every native test program to the single `$BUILD_DIR/$PROGNAME` path and attributes Unity output by text alone, so a run that only builds beforehand executes whichever suite was linked last under _every_ suite's name - all reporting PASSED. Build once with `--without-testing` to warm the shared src objects if you like; the run itself must still build. `bin/check-test-attribution.py` grades the JUnit reports for exactly this and is wired into both `bin/run-tests.sh` (RED) and CI. + **Raw `pio test` (no sanitizers, no verdict logic)** - use when you need to override the env or inspect verbose Unity output: ```bash diff --git a/test/TestUtil.cpp b/test/TestUtil.cpp index 58cd34c15..9f36ee17a 100644 --- a/test/TestUtil.cpp +++ b/test/TestUtil.cpp @@ -18,21 +18,149 @@ // The state checkpoint needs a POSIX directory walk, and only the host builds run these suites. // Note ARDUINO *is* defined on portduino, so it is not the right guard here. #if ARCH_PORTDUINO +#include "platform/portduino/PortduinoGlue.h" #include #include #include #include #include #include +#include #include #include +#include #endif +#if ARCH_PORTDUINO +// A test binary must not be reachable from the network. main.cpp's setup()/loop() are compiled out +// under PIO_UNIT_TESTING, so the phone API, MQTT and the web server are never started - but that is +// a property of today's guards, not something anything checks. A suite that pulled in a service +// which binds a port would otherwise open one on the developer's machine, silently, for the length +// of the run. Assert the absence instead of trusting it. +// +// Listening sockets only: an outbound connection is a different (and louder) problem, and gethostby* +// opens transient sockets that would make an any-socket check flap. +static void assertNoListeningSockets() +{ + // Socket fds appear as "socket:[inode]"; a listening TCP row in /proc/self/net carries st 0A. + std::set ours; + if (DIR *fds = opendir("/proc/self/fd")) { + while (struct dirent *e = readdir(fds)) { + char path[64], target[128]; + snprintf(path, sizeof(path), "/proc/self/fd/%s", e->d_name); + ssize_t n = readlink(path, target, sizeof(target) - 1); + if (n <= 0) + continue; + target[n] = '\0'; + unsigned long inode = 0; + if (sscanf(target, "socket:[%lu]", &inode) == 1) + ours.insert(std::to_string(inode)); + } + closedir(fds); + } + if (ours.empty()) + return; + + std::string offenders; + for (const char *table : {"/proc/self/net/tcp", "/proc/self/net/tcp6"}) { + FILE *f = fopen(table, "r"); + if (!f) + continue; + char line[512]; + bool header = true; + while (fgets(line, sizeof(line), f)) { + if (header) { + header = false; + continue; + } + // sl local_address rem_address st tx:rx tr:when retrnsmt uid timeout inode + char local[128] = {0}; + unsigned st = 0, uid = 0; + unsigned long inode = 0; + if (sscanf(line, "%*d: %127s %*s %x %*s %*s %*s %u %*d %lu", local, &st, &uid, &inode) != 4) + continue; + if (st != 0x0A) // TCP_LISTEN + continue; + if (ours.count(std::to_string(inode)) == 0) + continue; + offenders += " "; + offenders += local; + } + fclose(f); + } + if (offenders.empty()) + return; + + // Before UNITY_BEGIN(), so there is no Unity failure to record - and a test binary that has + // opened a port is not a result worth collecting. Fail the suite outright and say why. + fprintf(stderr, + "FATAL: test binary is listening on%s\n" + "A unit-test run must not be reachable. Something started a network service - check what\n" + "the suite constructs, and whether it belongs behind main.cpp's PIO_UNIT_TESTING guard.\n", + offenders.c_str()); + fflush(stderr); + exit(EXIT_FAILURE); +} +#endif + +#if ARCH_PORTDUINO +static bool environmentBaselined = false; + +// -s is how the harness keeps a test run off the host's radio: it makes portduinoSetup() skip the +// /etc/meshtasticd/config.yaml search and return before GPIO/SPI init. That job is done by the time +// any of this runs, and the flag's only remaining readers are behaviour we do want under test - +// wouldEncryptWithPKC() disables PKC while it is set. Clear it so suites exercise the production +// encode path; the radio choice is already made and is not revisited. +static void baselineEnvironment() +{ + portduino_config.force_simradio = false; + assertNoListeningSockets(); + environmentBaselined = true; +} +#endif + +void testAssertEnvironmentIntact(const char *testName) +{ +#if ARCH_PORTDUINO + // Not every suite calls initializeTestEnvironment() - test_atak does not - so the baseline + // cannot live only there, or those suites run with PKC off and skip the socket check. Establish + // it at the first RUN_TEST for whoever has not, and hold it from then on. + if (!environmentBaselined) { + baselineEnvironment(); + return; + } + + // Per test, not once per suite: a service that binds a port is opened by the code under test, + // not by the harness, so checking only at startup would miss every case that starts one. + assertNoListeningSockets(); + + if (!portduino_config.force_simradio) + return; + + // Hard exit rather than TEST_FAIL: this runs between tests, outside any Unity test frame, so + // there is no failure to longjmp into. Repairing the flag silently would be worse - it would + // leave the suite that broke it passing. + for (FILE *out : {stdout, stderr}) + fprintf(out, + "FATAL: force_simradio was set back on before %s\n" + "PKC is disabled while it is set, so the encode path under test falls back to channel\n" + "crypto and every later case asserts the wrong thing. A test that needs simradio must\n" + "restore the flag before it returns.\n", + testName ? testName : "(unknown test)"); + fflush(stderr); + exit(EXIT_FAILURE); +#else + (void)testName; +#endif +} + void initializeTestEnvironment() { concurrency::hasBeenSetup = true; consoleInit(); #if ARCH_PORTDUINO + baselineEnvironment(); + struct timeval tv; tv.tv_sec = time(NULL); tv.tv_usec = 0; diff --git a/test/TestUtil.h b/test/TestUtil.h index bb56d1096..d2a145e52 100644 --- a/test/TestUtil.h +++ b/test/TestUtil.h @@ -17,6 +17,14 @@ void testDelay(unsigned long ms); // place instead of being spread across 40-odd suites. void testStateCheckpoint(const char *testName, const char *sourceFile); +// Checked before every test, because the environment a suite starts in is not the one it keeps. +// initializeTestEnvironment() clears force_simradio once, and a test that sets it - directly, or by +// restoring a struct it saved before the clear - silently disables PKC for every test after it. +// wouldEncryptWithPKC() would then return false and the encode path would quietly fall back to +// channel crypto, which is a passing test asserting the wrong thing. Named per test so the culprit +// is the test that follows the one that broke it. +void testAssertEnvironmentIntact(const char *testName); + // Every RUN_TEST becomes a checkpoint. An unintended write has no matching assertion *by // definition* - nobody wrote a TEST_ASSERT for the nodes.proto write that broke test_admin_radio, // because nobody knew it happened - so attribution has to come from outside the test body. @@ -27,6 +35,7 @@ void testStateCheckpoint(const char *testName, const char *sourceFile); #undef RUN_TEST #define RUN_TEST(func, ...) \ do { \ + testAssertEnvironmentIntact(#func); \ UnityDefaultTestRun(func, #func, __LINE__); \ testStateCheckpoint(#func, __FILE__); \ } while (0) diff --git a/test/state-manifest.tsv b/test/state-manifest.tsv index 7420504e8..02870c573 100644 --- a/test/state-manifest.tsv +++ b/test/state-manifest.tsv @@ -39,21 +39,30 @@ # add, for a human to paste and justify. It never applies them itself, and CI never applies them at # all - an auto-accepted baseline is the same rot as an auto-updated snapshot. # +# errors= | .. | .. caps a suite's LOG_ERROR lines, default 100. A range, not a +# ceiling: for a fuzz suite the floor is the half that matters. test_fuzz_decode logging ~100k +# rejections is the suite working; the same suite logging none means it stopped feeding malformed +# input, and every case would still pass. Bounds are wide on purpose - they catch a path that has +# stopped running, not a drift of a few hundred lines. +# # suite flags reason -test_admin_radio writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat,Messages_default.msgs per-test NodeDB fixture, and the admin handlers under test persist config, channels and node metadata +test_admin_radio writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat,Messages_default.msgs errors=400 per-test NodeDB fixture, and the admin handlers under test persist config, channels and node metadata test_admin_session_repro writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto constructs a NodeDB, whose constructor persists a default set when the prefs directory is empty +test_event_channel_phone_api writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto constructs a NodeDB, whose constructor persists a default set when the prefs directory is empty +test_event_channel_router writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto errors=200 subclasses NodeDB for the event-channel fixtures; the base constructor persists a default set when the prefs directory is empty test_firmware_edition writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto persists an event firmware_edition in devicestate, then reboots a NodeDB to prove a vanilla build resets it -test_fuzz_packets writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat,Messages_default.msgs drives decode of fuzzed packets through the real NodeDB and message store +test_fuzz_decode errors=20000..250000 fuzzes protobuf decode; every rejection logs. A collapse to near zero means the corpus stopped reaching the decoder +test_fuzz_packets writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat,Messages_default.msgs errors=5000..60000 drives decode of fuzzed packets through the real NodeDB and message store test_hop_scaling writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto constructs a NodeDB to hold the hop-distance fixtures test_mesh_beacon writes=module.proto exercises the beacon's module-config save path test_mesh_module writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat module framework tests construct a NodeDB -test_mqtt writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto constructs a NodeDB for node lookups in the MQTT paths +test_mqtt writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto errors=1000..12000 constructs a NodeDB for node lookups in the MQTT paths test_nexthop_routing writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto next-hop selection reads and updates the node DB test_nodedb_blocked state=per-suite writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat saturates the DB with MAX_NUM_NODES-2 favourited nodes to test the protected cap; a later test's removeNodeByNum() persists that state, and the cap test depends on the fill from the test before it -test_packet_signing writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat needs a NodeDB holding both peers' keys for the PKI encode/decode paths +test_packet_signing writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat errors=300 needs a NodeDB holding both peers' keys for the PKI encode/decode paths test_pki_admin_fallback writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto needs a NodeDB holding admin keys for the fallback paths test_stream_api writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto drives real PhoneAPI handshakes, which read and persist config and the node DB test_traceroute_nexthop writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto traceroute route selection reads the node DB -test_traffic_management writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat constructs a NodeDB for the per-node rate-limit and dedup state +test_traffic_management writes=config.proto,module.proto,device.proto,channels.proto,nodes.proto,warm.dat errors=3000..12000 constructs a NodeDB for the per-node rate-limit and dedup state; test_tm_fuzz_nodenum_blitz feeds malformed payloads, and each rejection logs (measured 7985) test_transmit_history writes=transmit_history.dat persistence round-trip: what it asserts is that retransmission state survives a save/load test_warm_store writes=warm.dat persistence round-trip of the warm-tier snapshot, which is the tier's whole contract diff --git a/test/test_admin_session_repro/test_main.cpp b/test/test_admin_session_repro/test_main.cpp index c93c1fd94..36efc9a41 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/test/test_admin_session_repro/test_main.cpp @@ -21,10 +21,6 @@ #include "support/MockMeshService.h" #include -#ifdef ARCH_PORTDUINO -#include "platform/portduino/PortduinoGlue.h" -#endif - static constexpr NodeNum LOCAL_NODE = 0x0A0A0A0A; static constexpr NodeNum ADMIN_NODE = 0x0B0B0B0B; // authorized admin, sends remote admin to us static constexpr NodeNum QUERIED_NODE = 0x0C0C0C0C; // a remote we send admin requests to @@ -161,14 +157,6 @@ void setUp(void) nodeDB = mockNodeDB; myNodeInfo.my_node_num = LOCAL_NODE; -#ifdef ARCH_PORTDUINO - // The native test harness boots Portduino in simulated mode, and wouldEncryptWithPKC() - // hard-disables PKC whenever force_simradio is set. Left true, no outgoing admin request is - // ever key-pinned, so the pinning tests below cannot exercise what they are asserting. - // Model a real (non-sim) device instead. - portduino_config.force_simradio = false; -#endif - config = meshtastic_LocalConfig_init_zero; // A real device always holds a private key; without one perhapsEncode never picks PKC. config.security.private_key.size = 32; diff --git a/test/test_event_channel_router/test_main.cpp b/test/test_event_channel_router/test_main.cpp index c1f5e8bec..82700a795 100644 --- a/test/test_event_channel_router/test_main.cpp +++ b/test/test_event_channel_router/test_main.cpp @@ -10,9 +10,6 @@ #include "mesh/MeshService.h" #include "mesh/NodeDB.h" #include "mesh/Router.h" -#if ARCH_PORTDUINO -#include "platform/portduino/PortduinoGlue.h" -#endif #include #include #include @@ -121,9 +118,6 @@ struct SavedGlobals { MeshService *service; AirTime *airTime; concurrency::Lock *cryptLock; -#if ARCH_PORTDUINO - bool forceSimRadio; -#endif }; SavedGlobals saved; @@ -319,9 +313,6 @@ void setUp(void) saved.service = service; saved.airTime = airTime; saved.cryptLock = cryptLock; -#if ARCH_PORTDUINO - saved.forceSimRadio = portduino_config.force_simradio; -#endif testNodeDB = new TestNodeDB(); testNodeDB->clearTestNodes(); @@ -335,9 +326,6 @@ void setUp(void) memset(&myNodeInfo, 0, sizeof(myNodeInfo)); myNodeInfo.my_node_num = kLocalNode; service = nullptr; -#if ARCH_PORTDUINO - portduino_config.force_simradio = false; -#endif installChannels(); testAirTime = new AirTime(); @@ -379,9 +367,6 @@ void tearDown(void) router = saved.router; service = saved.service; airTime = saved.airTime; -#if ARCH_PORTDUINO - portduino_config.force_simradio = saved.forceSimRadio; -#endif } EVENT_ROUTER_TEST_ENTRY void setup() diff --git a/test/test_geocoord_distance/test_main.cpp b/test/test_geocoord_distance/test_main.cpp index de3430f1c..e54498a15 100644 --- a/test/test_geocoord_distance/test_main.cpp +++ b/test/test_geocoord_distance/test_main.cpp @@ -1,3 +1,8 @@ +// Deliberately does NOT include TestUtil.h. This suite is pure-function - no NodeDB, no router, no +// sockets, no PKC - so the harness-wide guards there (no listening sockets, force_simradio clear) +// would assert conditions it cannot reach, and initializeTestEnvironment()'s RTC and OSThread setup +// would add portduino globals it otherwise never touches. Suite-level state cleanliness is still +// checked from outside by bin/pio-test-isolate.sh, which wraps every suite regardless. #include "configuration.h" #include "gps/GeoCoord.h" #include diff --git a/test/test_meshpacket_serializer/test_serializer.cpp b/test/test_meshpacket_serializer/test_serializer.cpp index db863ca3c..0ddb4ca0b 100644 --- a/test/test_meshpacket_serializer/test_serializer.cpp +++ b/test/test_meshpacket_serializer/test_serializer.cpp @@ -1,3 +1,8 @@ +// Deliberately does NOT include TestUtil.h. This suite is pure-function - no NodeDB, no router, no +// sockets, no PKC - so the harness-wide guards there (no listening sockets, force_simradio clear) +// would assert conditions it cannot reach, and initializeTestEnvironment()'s RTC and OSThread setup +// would add portduino globals it otherwise never touches. Suite-level state cleanliness is still +// checked from outside by bin/pio-test-isolate.sh, which wraps every suite regardless. #include "test_helpers.h" #include #include diff --git a/test/test_pki_admin_fallback/test_main.cpp b/test/test_pki_admin_fallback/test_main.cpp index 5c3b408c9..127d4f999 100644 --- a/test/test_pki_admin_fallback/test_main.cpp +++ b/test/test_pki_admin_fallback/test_main.cpp @@ -9,6 +9,7 @@ // The whole feature is compiled out when PKI is excluded. #if !(MESHTASTIC_EXCLUDE_PKI) +#include "UptimeClock.h" #include "mesh/Channels.h" #include "mesh/CryptoEngine.h" #include "mesh/NodeDB.h" @@ -148,6 +149,11 @@ void setUp(void) void tearDown(void) { + // The rate-limit case drives a virtual timebase; leave the real clock for everyone else, and + // re-stamp the budget so the next case does not measure a virtual stamp against real millis. + Time::useRealClock(); + resetAdminKeyFallbackBudget(); + delete mockNodeDB; mockNodeDB = nullptr; nodeDB = nullptr; @@ -205,8 +211,10 @@ void test_wrong_admin_key_does_not_decode(void) // The fallback is budget-limited against flooding; see Router.cpp for why the budget is global. void test_admin_key_fallback_is_rate_limited(void) { - // Start from a full bucket regardless of what earlier tests consumed (8 tokens, one per 250ms). - delay(2500); + // Drive the virtual clock: on the wall clock the eight decodes below have to beat the 250ms + // refill, which is ~31ms each - CI misses that and the bucket refills mid-drain. + Time::setTestMillis(1000000); + resetAdminKeyFallbackBudget(); // re-stamp against the virtual clock we just switched to uint8_t otherPub[32], otherPriv[32]; crypto->generateKeyPair(otherPub, otherPriv); @@ -225,7 +233,7 @@ void test_admin_key_fallback_is_rate_limited(void) TEST_ASSERT_NOT_EQUAL_MESSAGE(DECODE_SUCCESS, perhapsDecode(&blocked), "fallback should be budget-limited"); // The budget refills, so the throttle is not a permanent lockout. - delay(600); + Time::advanceTestMillis(600); meshtastic_MeshPacket allowed = makePkiPacket(ADMIN_NODE, meshtastic_PortNum_PRIVATE_APP, 16, adminPriv); TEST_ASSERT_EQUAL_MESSAGE(DECODE_SUCCESS, perhapsDecode(&allowed), "budget should refill over time"); assertDecodedAndLearned(&allowed, adminPub); diff --git a/test/test_position_precision/test_main.cpp b/test/test_position_precision/test_main.cpp index fae50e87f..7497b42f6 100644 --- a/test/test_position_precision/test_main.cpp +++ b/test/test_position_precision/test_main.cpp @@ -8,10 +8,6 @@ #include #include #include -#if ARCH_PORTDUINO -#include "platform/portduino/PortduinoGlue.h" -#endif - static meshtastic_Position makePosition() { meshtastic_Position position = meshtastic_Position_init_default; @@ -332,9 +328,6 @@ static void test_eventCoordinatePolicy_coversPortsAndExcludesPki() waypoint.to = 0x12345678; config.security.private_key.size = 32; owner.is_licensed = false; -#if ARCH_PORTDUINO - portduino_config.force_simradio = false; -#endif TEST_ASSERT_TRUE(willUsePki(&waypoint)); TEST_ASSERT_FALSE(isBlockedEventCoordinatePacket(&waypoint)); #else diff --git a/test/test_traffic_management/test_main.cpp b/test/test_traffic_management/test_main.cpp index cfe06e7f5..d7d947d5a 100644 --- a/test/test_traffic_management/test_main.cpp +++ b/test/test_traffic_management/test_main.cpp @@ -211,6 +211,8 @@ class TrafficManagementModuleTestShim : public TrafficManagementModule MockNodeDB *mockNodeDB = nullptr; +static void installWellKnownPrimaryChannel(); // defined below, next to the other channel fixtures + static void resetTrafficConfig() { moduleConfig = meshtastic_LocalModuleConfig_init_zero; @@ -220,7 +222,9 @@ static void resetTrafficConfig() config = meshtastic_LocalConfig_init_zero; config.device.role = meshtastic_Config_DeviceConfig_Role_CLIENT; - channelFile = meshtastic_ChannelFile_init_zero; + // A real device always has a primary channel; leaving channels_count at 0 made every router + // lookup log "Invalid channel index", 12k lines of it, without testing anything. + installWellKnownPrimaryChannel(); owner.is_licensed = false; myNodeInfo.my_node_num = kLocalNode; diff --git a/test/test_utf8/test_main.cpp b/test/test_utf8/test_main.cpp index 7ce90f250..ebf47be9b 100644 --- a/test/test_utf8/test_main.cpp +++ b/test/test_utf8/test_main.cpp @@ -1,3 +1,8 @@ +// Deliberately does NOT include TestUtil.h. This suite is pure-function - no NodeDB, no router, no +// sockets, no PKC - so the harness-wide guards there (no listening sockets, force_simradio clear) +// would assert conditions it cannot reach, and initializeTestEnvironment()'s RTC and OSThread setup +// would add portduino globals it otherwise never touches. Suite-level state cleanliness is still +// checked from outside by bin/pio-test-isolate.sh, which wraps every suite regardless. #include "meshUtils.h" #include #include diff --git a/variants/native/portduino/platformio.ini b/variants/native/portduino/platformio.ini index 37d5bf2a0..81b96f3e8 100644 --- a/variants/native/portduino/platformio.ini +++ b/variants/native/portduino/platformio.ini @@ -32,9 +32,16 @@ build_flags = ${native_base.build_flags} ; assertions. Registered here rather than only in bin/run-tests.sh so a bare `pio test` and CI get ; the same boundary. See bin/pio-test-isolate.sh. ; https://docs.platformio.org/en/latest/projectconf/sections/env/options/test/test_testing_command.html +; -s matches [env:coverage]. The sandbox above only covers $HOME, but portduinoSetup() searches +; ./config.yaml and /etc/meshtasticd/config.yaml - absolute, so no $HOME sandbox can hide it. On a +; host running meshtasticd that config selects the real LoRa module and the run proceeds into GPIO +; and SPI setup, so a test run would drive the developer's own radio. -s short-circuits ahead of the +; config search and returns before hardware init. Suites asserting behaviour that simradio changes +; (PKC selection) clear the flag themselves in setUp, after the radio choice is already made. test_testing_command = ${platformio.src_dir}/../bin/pio-test-isolate.sh ${platformio.build_dir}/${this.__env__}/meshtasticd + -s [env:native-tft] extends = native_base