mirror of
https://github.com/meshtastic/firmware.git
synced 2026-09-20 05:18:54 -04:00
create-pull-request/update-protobufs-develop-from-develop
12
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
bca7c0b480 |
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 |
||
|
|
af56a11f00 |
Replace native-suite-count file with dynamic test discovery (#11413)
* Derive the native suite count on the fly instead of registering it in a file test/native-suite-count was a manually-maintained register of the test_* directory count, reconciled against the actual directories by bin/run-tests.sh (as an AMBER verdict) and by a dedicated suite-count-check CI job. The reconciliation only ever guarded the file itself: the check that matters - suites that actually ran vs. the test_* directories on disk - already derives its expected count from a directory walk, so the file added a bookkeeping step to every suite addition/removal without adding signal. Remove the file and everything that existed to keep it honest: - bin/run-tests.sh: drop the canonical-count file read, the count-mismatch AMBER verdict, and the [canonical: x/y] suffix; the verdict lines already carry ran/expected from the directory walk. The shuffle seed suffix stays. - test_native.yml: delete the suite-count-check job and its needs: edges. - Docs (copilot-instructions.md, AGENTS.md, test/README.md) and the test-script comments now describe the count as derived from test/test_* at run time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APCEfNjd1X7ErDHEzT6Dqd * Add suite-shrinkage-check: fail a PR that silently loses a test_* suite With test/native-suite-count gone, nothing in CI noticed the suite set shrinking: platformio test discovers and runs whatever test_* directories exist, and bin/run-tests.sh derives its expected count from the same walk, so a suite directory lost in a bad rebase or an overzealous cleanup just means fewer suites run - every remaining check stays green. Restore that tripwire git-aware instead of file-based: on pull_request runs, compare the test_* directory list at the PR's merge base against the PR result. A vanished suite fails the job unless its name appears in the PR title, PR body, or a commit message in the PR's range - a deliberate removal satisfies that by stating what it removes; an accidental loss cannot. Other events skip: they have no natural base, and PRs are where accidents arrive. No job depends on this one (a skipped job would skip its dependents). Incidentally: test/ currently holds 47 test_* directories while the deleted count file said 46 - the manual register had already drifted, which is exactly the bookkeeping failure mode this replaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APCEfNjd1X7ErDHEzT6Dqd * Re-pad the verdict table after shortening the AMBER row Shrinking the AMBER cell left the table's column padding inconsistent, which trunk (prettier + markdownlint MD060) rejects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APCEfNjd1X7ErDHEzT6Dqd --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
de6b23190a |
Test suite rebuild (#11322)
* docs(nodedb): make the native node cap unambiguous The native node cap was stated in four places that disagreed, and the disagreement already caused a wrong diagnosis: a saturated 200-node database looked arithmetically impossible because the cap had been read as 248, computed from a header that does not apply on this platform. The real value is 198. On portduino MAX_NUM_NODES is not a compile-time constant at all - the variant defines it as `portduino_config.MaxNodes`, resolved at runtime, default 200 and settable per host with `General: MaxNodes`. variant.h is reached before mesh-pb-constants.h, so that header's ARCH_PORTDUINO branch never fires and its plausible-looking 250 is dead code. - #error-guard the dead branch rather than leave a wrong number where people grep. The guard found a real defect: seven translation units reach mesh-pb-constants.h without configuration.h (SerialConsole.cpp, StreamAPI.cpp, PacketAPI.cpp, ServerAPI.cpp, PiWebServer.cpp, ServiceEnvelope.cpp, MeshtasticOTA.cpp, and test/TestUtil.cpp), so each was compiling with a different MAX_NUM_NODES - and therefore a different PACKETHISTORY_MAX - than the rest of the build. Each now includes configuration.h first. It cannot be included from mesh-pb-constants.h itself: that reaches SerialConsole.h through DebugConfiguration.h and closes a cycle. - Name the bare 250 in getMaxNodesAllocatedSize() NODEDB_MIGRATION_LOAD_CEILING. It is a decode allowance for files written by larger-cap firmware, not a cap, and it read like one. - Fix docs/node_info_stores.md, which named the wrong source and a "10-250" range that is wrong for native, and the copilot-instructions tunables line that said "portduino 250". * test(harness): give each suite its own scratch HOME and report leftovers Native suites shared one directory. Every suite that constructs a NodeDB loads and saves ~/.portduino/default/prefs/ - nodes.proto, config.proto, channels.proto, module.proto, device.proto, warm.dat, transmit_history.dat - and nothing cleared it, so state leaked suite -> suite within a run and run -> every run after it. A test run could also rewrite a real meshtasticd node database on the same machine. Per-run isolation does not fix this: the leak is generated inside a single run, so the boundary has to be per suite. bin/pio-test-isolate.sh runs each suite in its own scratch $HOME, registered as test_testing_command for env:native and env:coverage so a bare `pio test` and CI get the same boundary, not just bin/run-tests.sh. It runs the binary unchanged and exits with its exit code, so PlatformIO's pass/fail is untouched. Overriding HOME here rather than around `pio` also sidesteps the blocker that a bare HOME= breaks pio's own ~/.platformio/penv/bin/pio lookup. Leftovers are reported as a second axis, PASS/FAIL x CLEAN/DIRTY, because an unintended write has no matching assertion by definition - nobody writes TEST_ASSERT for a save they do not know is happening. The harness asserts it from outside, so it applies to every suite without the author opting in. - Only the *set of changed paths* is asserted, never contents. Hashes answer the boolean "did this change?" and nothing more; content baselines over protobuf bytes would churn on every NodeInfoLite field added, which is how snapshot suites become noise. - Deliberate writes are declared in test/state-manifest.tsv - one central file, suite / flags / mandatory reason. run-tests.sh prints the opt-out count on every run. - Granularity follows the state flag, so the two ship together: per-test by default (TestUtil redefines RUN_TEST to checkpoint after each test, naming the exact test that dirtied things), suite boundary for state=per-suite, where carrying state across test cases is the declared behaviour. - A declared write that does NOT happen is reported as MISSING, not folded into DIRTY. It catches silently broken persistence; a warning for now, since some are conditional. - Graded AMBER, not RED. With isolation in place DIRTY means "undeclared", not "dangerous", and a check that lands red on day one gets switched off. Guard the guard, both halves: state_assert_empty() refuses to run a suite against a sandbox that is not empty (otherwise the after-diff measures against the wrong baseline and reports CLEAN while meaning nothing), and bin/test-state-check.sh drives the real wrapper with fixtures asserting CLEAN / CLEAN / DIRTY / MISSING plus both directions of the empty assertion. A checker that silently matches everything would otherwise pass forever. --write-manifest proposes entries for a human to paste and justify; it never applies them, and neither does CI. * test(harness): stop reporting Unity's exit code as a signal A native suite ends in exit(UNITY_END()), and UNITY_END() returns the failure count. PlatformIO's native runner reads that non-zero exit code as a POSIX signal number, so four failures print "Program received signal SIGILL", five print "SIGTRAP", and the suite is classified [ERRORED] rather than [FAILED]. There is no crash. The signal name tracks the failure count and nothing else - it moved SIGILL -> SIGTRAP when a diagnostic probe added a fifth failure - and it cost hours of hunting a memory bug that did not exist, on an env (native) that carries no sanitizer at all. It also explains the phantom extra test case in the totals: the runner adds a synthetic entry for the signal it thinks it saw. run-tests.sh now says so inline whenever a signal line appears, and the three agent-facing docs say it too. * test(admin): isolate NodeDB and globals per test setUp() did `if (!nodeDB) nodeDB = new NodeDB();` and never deleted it, so 83 of the 85 tests shared one never-reset database and never restored config, owner, devicestate or channelFile. The fixture that does restore them was opt-in and armed by exactly two tests. The setUp comment claiming the rest "set their own config/region state and are unaffected" was not true - the admin handlers under test write all four globals. Route every test through the fixture instead: setUp saves the globals and installs a fresh NodeDB, tearDown restores and deletes it. The two tests that armed it themselves no longer need to. All 85 pass, so nothing was silently relying on the shared state. It costs about 7% of the suite's runtime (a NodeDB construction is a loadFromDisk plus, with a region set, key generation) - worth paying to write the phase 3 tests against a clean fixture rather than 83 tests' residue. Also cap the per-test attribution in the run summary at five entries; the full list stays in the suite's sandbox. * test(fs): cover the bounded file-manifest walk getFiles() runs on every phone sync via STATE_SEND_FILEMANIFEST, and nothing asserted any of its bounding behaviour. It does execute unasserted from test_stream_api's handshakes, but the cap, the depth limit, the wasLimited paths, overlong-path rejection and capacity release were all unguarded. Eight tests, all describing what the code does today: today's code is already correct here, since #10778 landed the by-reference collectFiles(), the 64-entry cap, the strlcpy bounds and the swap-idiom release. They pass on arrival, which is the point - this is the baseline a later change has to leave alone. Two things they do not cover, and cannot: - Moving reserve() outside the __cpp_exceptions guard. Exceptions are on natively, so the #else branch is not compiled. The suite's job there is to prove that change alters nothing observable. - The file.name() null guard. No in-tree backend returns null; the guard is defensive. The manifest-release test pins the swap idiom rather than calling PhoneAPI's releaseFilesManifest(), which is file-local. It asserts capacity() == 0, not just size() == 0 - a size-only check passes on clear(), which is the bug #7924 shipped. Suite count 43 -> 44, recounted against the directories rather than copied. * test(admin): assert node-DB metadata saves skip the radio reload set_favorite_node, set_ignored_node and toggle_muted_node each persist a NodeInfoLite bit and nothing else. MeshService::reloadConfig() gates its region re-derivation and configChanged notification on saveWhat & (SEGMENT_CONFIG | SEGMENT_CHANNELS), so a SEGMENT_NODEDATABASE-only save already skips the live radio reconfigure. Pure characterization - all three pass on develop. Worth pinning because that reconfigure is the path implicated in the WisMesh Tag favourite-node crash, and develop asserts nothing about it: widening the saveWhat mask or reordering the check would currently go unnoticed. Ported from the config-save series along with ConfigChangedCounter (an Observer<void *> counting configChanged notifications, the only externally visible signal that the reload branch was taken) and TEST_NODE_NUM. They join the existing suite, so no suite-count change. * refactor(menu): extract the mute toggle into a named function The node menu's mute action was inline in a banner-callback lambda, and that lambda only ever runs via screen->showOverlayBanner() - which is why nothing in MenuHandler.cpp was reachable from a test. Lift the `selected == Mute` branch into menuHandler::toggleNodeMuted(uint32_t) and call it from the lambda. Behaviour-neutral by construction: same statements, same order, same bare saveToDisk(). The null check moves into the function, so the call site no longer needs its own lookup. Verified by the native build and suite; the byte-identical-image check on a headroom-constrained nRF52 board was not run locally - CI's firmware-size comment covers it. Three tests come with it, all describing today's behaviour: - the bit flips both ways and no configChanged fires (develop never calls reloadConfig on this path); - an unknown node is a no-op rather than a write; - and the segment mask. Flipping one NodeInfoLite bit currently rewrites all five segments via bare saveToDisk(). That is asserted deliberately, with the comment naming it as characterization of a known defect: a pending fix narrows it to SEGMENT_NODEDATABASE, and when it lands this assertion is expected to change, which makes the improvement visible in the diff instead of silent. saveToDisk() is not virtual, so the mask is observed through its effect - remove the five prefs files, toggle, and see which reappear. * docs(test): make every suite count a pointer to the canonical one test/native-suite-count is the registered total and is machine-checked against test/test_* on every full run and by the suite-count-check CI job. Every other statement of the count is a copy that drifts: copilot-instructions said 12, AGENTS.md said 19, and the real number is 44. Replace both literals with a pointer to the file, say explicitly that no document should state the count as a literal, and reframe the two suite listings as descriptions rather than inventories - they carry per-suite information the count does not, so they stay, but nothing should infer completeness from their length. Register the new FS suite in both. * test(harness): randomise suite order, reproducibly Landed last, deliberately. Randomising an order-dependent suite set does not find bugs so much as convert a silent pass into intermittent red, and the first instinct is to revert the randomisation rather than fix the coupling. Phases 1-2 removed the coupling; this keeps it removed. Both runners previously hid order dependence behind a fixed order that happened to differ between them, and neither order was chosen: CI's area rules put admin first, PlatformIO's local discovery is reverse alphabetical and put it last. CI was green by accident. - bin/run-tests.sh --shuffle / --seed <n>. The seed defaults to HEAD's short SHA: one order per commit, so a red is replayable and attributable to the diff instead of flaky, while the project keeps exploring orders. Printed at the start and carried into the RESULT line, so a verdict is replayable from that line alone; the full order is printed on failure, because for an order-dependent failure the order is the diagnostic. - The shuffle is a 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. - Shuffling needs one `pio test -f <suite>` invocation per suite - PlatformIO orders by its own os.walk() over test/ and filters only select - which measures at about 4.7s per suite of extra startup. - CI shuffles its area order, seeded from GITHUB_SHA and printed with the command to replay it locally. Intra-area order stays PlatformIO's; controlling it there would mean per-suite invocations, which is a cost worth deciding separately. Also records the 16 measured entries in test/state-manifest.tsv, each with its reason, taken from a full run's --write-manifest output rather than guessed. * test(default): cover the region-throttle interval overload getConfiguredOrDefaultMsScaled(configured, default, nodes, TrafficType) is the overload every telemetry and position module actually calls, and nothing referenced TrafficType anywhere under test/. All four of its behaviours were unguarded: the no-region guard, the throttle <= 1 short-circuit, the multiply, and the 64-bit overflow clamp. The throttles are real, not hypothetical - EU_866 carries PROFILE_LITE, which sets both positionThrottle and telemetryThrottle to 10, so a change here moves broadcast spacing in that region by an order of magnitude. Each test pins numOnlineNodes at the congestion threshold and uses ROUTER, which never congestion-scales, so the coefficient is 1 and the throttle is the only variable. The overflow case needs a base above INT32_MAX/10, hence three days rather than one. * ci(test): keep pull-request suite order fixed, seed the rest Shuffling the area order on every run - including pull_request - would turn a contributor's PR red for an ordering they did not choose, which is how a randomisation gets reverted instead of the coupling being fixed. That is the exact dynamic the ordering work was sequenced last to avoid, and the previous commit walked straight into it. - pull_request keeps the fixed declared area order. - push and schedule shuffle, seeded from the commit SHA: deterministic per commit, printed, attributable, and never blocking someone else's PR. - A suite_order_seed input on workflow_call and workflow_dispatch overrides both, so a specific failing order can be replayed anywhere, including on a PR. The run log prints which mode it took, the resulting order, and the local command to replay it. * ci(test): satisfy CKV_GHA_7 and yamllint on the seed input The seed is reachable through workflow_call, which callers can pass programmatically. The workflow_dispatch copy tripped checkov's "workflow_dispatch inputs MUST be empty" rule, and suppressing it was not worth it: replaying a specific order is a local operation, and the run log already prints the exact bin/run-tests.sh command to do it. * style(menu): apply the node-ID format convention RadioInterface.cpp documents the rule: 0x%08x in logs, !%08x in user-facing display. MenuHandler held every remaining exception - seven logs printing bare %08X, and two display labels doing the same. Repo-wide there are now no bare %08X node IDs left in log calls. * ci(test): pass workflow inputs through env, not shell interpolation suite_order_seed and github.event_name were spliced into the run: script as ${{ }} text, so a value carrying shell metacharacters would execute as code on the runner rather than being read as data. semgrep (run-shell-injection) and zizmor (template-injection) both flag it. Both now arrive as environment variables and are read as "$VAR". * refactor(test): share the seeded shuffle between the harness and CI bin/run-tests.sh and test_native.yml each carried a byte-identical copy of the MINSTD Fisher-Yates awk. The workflow prints "replay locally: ./bin/run-tests.sh --shuffle --seed $seed" after a shuffled CI run, and that instruction is only true while the two agree - drift would be announced by a replay quietly reproducing a different order than the one that failed. Extract shuffle_suites() to bin/lib/shuffle.sh and source it from both. Permutations verified identical across seeds before and after the move. * fix(test): correct the shared-state MISSING check and summary join Three defects in the new harness: state_classify() matched declarations two different ways - state_path_declared() for "undeclared", a hand-rolled regex for "missing". Interpolating an entry into an ERE also let a metacharacter in a manifest name match a file that is not the declared one. Both directions now go through the one helper. `paste -sd'; '` does not join with "; ": with -s, paste cycles through a multi-character delimiter one character per join, so paths rendered as "a;b c;d e". Replaced with an awk join. test-state-check.sh ran on after a failed cd instead of stopping (SC2164). ./bin/test-state-check.sh: 6/6 fixtures pass, MISSING included. * fix(portduino): bound General.MaxNodes MaxNodes was validated only for <= 0. Any positive value, including a typo'd or pasted-in one, propagates to MAX_NUM_NODES and scales both the node DB and the nodes.proto decode ceiling - failing at boot with no obvious cause. The ceiling is a sanity bound, not a capability limit; raise it if a host genuinely needs more. * docs(nodedb): reconcile the capacity tables The property matrix omitted the ESP32-S3 100-node flash tier that the platform table above it lists, and neither mentioned that the WASM build overrides MaxNodes to 80 in wasm_config_apply(). * fix(nodedb): make mesh-pb-constants.h self-sufficient on portduino The ARCH_PORTDUINO #error assumed it was unreachable in a normal build. It is not: the vendored device-ui sources include this header without configuration.h, which broke both native-tft docker builds. Include configuration.h here instead, ahead of every compile-time default - variant.h overrides MAX_RX_TOPHONE as well as MAX_NUM_NODES, so placing it lower in the file just moves the divergence to a redefinition. The #error stays as a backstop for the case where that include genuinely stops providing the cap. Verified with the native env's own flags: a TU including only this header now compiles, normal-order use of both macros compiles, and NodeDB.cpp compiles. * fix(portduino): raise the MaxNodes ceiling to 16000 Marked artificial: nothing in the node DB fails at 16001. 16000 sits just under the 16384 (128 x 128) population where HopScalingModule saturates its sampling denominator and starts dropping nodes, so a host inside the bound still gets meaningful hop recommendations. * lint(trunk): advise on node IDs logged as bare %08x RadioInterface.cpp documents the convention - 0x%08x in logs, !%08x in display - but nothing enforced it, which is how the MenuHandler cluster drifted. 22 call sites in PacketHistory, NodeInfoModule and PositionModule are still off it. A trunk linter rather than a CI grep job, because trunk checks changed files: new violations get flagged without a 22-site cleanup landing in an unrelated PR. Modelled on the existing too-many-defined definition. Scoped to values it can tell are IDs - an ID-shaped argument (->num, .from, getNodeNum) or message text naming one. A 32-bit hex that is not an ID is out of scope, so the CRC32 logs in ethOTA.cpp are correctly ignored. Emits "note", trunk's only non-blocking level: "warning" and "info" both exit non-zero and would gate CI, which is not what a log-format nit deserves. The pre-existing sites are line-scoped in the allowlist, so a new bad call in those same files is still caught. * lint(trunk): stop exempting the known node-id-format sites The seeded allowlist made the rule green by declaring the backlog acceptable. Empty it instead, so the 22 pre-existing sites are reported and get cleaned up by whoever next edits those files. Costs nothing to do: the rule emits "note", so these are non-blocking either way. The allowlist stays for its real purpose - a value the linter misreads as an ID. * style: log node and packet IDs as 0x%08x Clears the 22 sites the node-id-format linter reports, so the rule starts from zero rather than from a backlog nobody can see - trunk suppresses pre-existing findings by default, so left alone these would not have surfaced on edit the way an empty allowlist implies. Format strings only; no argument or control flow changes. The !%08x user-facing display forms are deliberately untouched - that is the other half of the same convention. * test(harness): build once up front, so suite timings mean something run-tests.sh fused build and run in a single pio invocation, so whichever suite PlatformIO's directory walk reached first absorbed the entire src compile and reported it as its own duration. On a real run that made a 0.03s suite report 13m21s, and hid the build cost from every other number in the summary. Do what .github/workflows/test_native.yml already does: one --without-testing build pass, then run with --without-building. Measured on a full 44-suite run - the build is now a single reported figure and 968 test cases execute in 1.9s, with no suite above 0.084s. Build output goes to its own log rather than $LOG: the outcome regexes match "error:" and "[ERRORED]", so a compiler diagnostic sharing that file would read as a test failure. Both red paths now keep the log they quote from. $LOG and the build log are mktemps the EXIT trap removes, so the three grepped lines were previously all anyone ever saw - and the cause is usually further up than the first [FAILED]. * test(harness): keep the run log on every red path bin/pio-test-isolate.sh already keeps a failing or DIRTY suite's sandbox and log under .pio/test-state/<suite>/. What was missing is the cross-suite view: $LOG is a mktemp the EXIT trap deletes, so run-tests.sh quoted three grepped lines from a file that no longer existed by the time anyone looked. Preserve it as .pio/build/<env>/test-failure.log from both red paths - including "no success summary found", which said "see log" while preserving nothing, and which is exactly the case where the build died before any suite ran and so left no per-suite sandbox either. Cleared at the start of every run, so a green run cannot leave a red one's log lying around looking current. * fix(test): report the real failure count on a shuffled red A shuffled run is one `pio test` invocation per suite, all appending to the same log, so the log carries one PlatformIO "N test cases:" summary per suite. verdict_red() took `tail -1`, which reports whatever the LAST suite did: a failure in suite 3 printed a "0 failed" summary from suite 44 directly under "RED - failures detected:". Sum the summaries instead. A single summary line - every unshuffled run - is passed through verbatim, so the familiar output is byte-identical. The patterns are passed to the awk helper as strings rather than /regex/ literals: awk evaluates a regex literal in argument position as `$0 ~ /re/`, so the callee would receive 0 or 1 and silently sum garbage. * fix(test): do not emit an empty suite name for an empty shuffle `printf '%s\n' "$@"` with no arguments still writes one empty line, and both callers read shuffle_suites through mapfile, so an empty suite list arrived as a single suite named "". Return before the printf when there is nothing to shuffle. * test(harness): state and enforce the Linux host requirement The native harness is a Linux tool: bash 4+ (mapfile), GNU coreutils and GNU find (-printf, md5sum, -executable). Most of that predates this branch - mapfile and both find predicates are already on develop - but none of it was written down, so the requirement was there to be discovered rather than read. Refuse to start on a non-Linux uname instead of degrading. On a BSD userland this would not fail cleanly: it would mis-hash the sandbox and mis-read the suite list, and still print a verdict. A state check that silently measures the wrong thing is worse than one that declines to run. Carrying a per-host fallback was the alternative, and it buys a second code path that nothing in CI exercises. bin/test-native-docker.sh already exists for macOS and non-Linux hosts, and the native-macos PlatformIO env is a build target for meshtasticd, not a test host - the isolation wrapper is registered for env:native and env:coverage only. Documented in the script header, test/README.md, and both agent docs. * fix(test): terminate every suite with exit(UNITY_END()) Two sites across two suites ended on a bare UNITY_END(). That ends the reporting, not the suite: setup() returns, the runtime goes on calling loop(), and the process runs forever. PlatformIO does not notice - it reports a suite from its Unity output, not from process exit - so the suite passes, the run goes green, and the binary stays resident. Thirteen of them had accumulated on one dev box, the oldest 19 hours old. The costs are quiet by construction: - the per-suite sandbox is deleted underneath a live process, so its CLEAN/DIRTY verdict describes what the suite had written when the harness stopped looking, not what it left behind; - .gcda coverage and LeakSanitizer's report both flush from atexit handlers, so a suite that never exits contributes no coverage and gets no leak check; - each survivor pins its own deleted 94 MB binary, which du cannot see. One of the two is the #else of an architecture guard, which is the easiest one to get wrong - it looks like there is nothing to clean up. test_mqtt has a correct exit(UNITY_END()) in its live branch, so a "does this file call exit() anywhere" check passes the file whole. test_serial had two more. develop's serial-config validation rework restructured that suite - the architecture guard is gone and both remaining branches now exit correctly - so this commit no longer has anything to change there; bin/lint-unity-exit.sh, added later on this branch, is what keeps it that way. test/README.md gets a section on it, since the skeleton showing the right shape had not stopped this happening. * test(harness): detect and reap suites that outlive their run A suite that never exits was invisible: PlatformIO reports a suite from its Unity output, so the run stayed green while the binary kept running. Two checks, because they fail differently. Runtime, in bin/pio-test-isolate.sh: the sandbox $HOME is mktemp-unique per suite, so any process still holding it is a survivor of that suite. Matching on the environment rather than a remembered PID identifies one whatever its parentage - a fork, a grandchild, a process already reparented to init - none of which a $! comparison catches. Reaped before the after-fingerprint is taken, so that fingerprint measures a tree nobody is still writing to, and so a run cannot leave processes accumulating on the host. Recorded as a sixth summary column and graded AMBER: the tests did pass, but the CLEAN verdict and the coverage were measured under a false assumption. Author-time, as bin/lint-unity-exit.sh, wired into trunk at "note" like node-id-format: every UNITY_END() must be wrapped in exit(). The rule is per occurrence, and that is the point - a file-level "calls exit() somewhere" check passes test_serial and test_mqtt, which have a correct one in their live branch and a bare one in the #else. Running it over the tree turned up test_mqtt, which the file-level pass had missed. It allows `int rc = UNITY_END(); ...; exit(rc)`, used by test_packet_signing to restore globals between the summary and the exit. That is where the rule gives ground: capturing and never exiting would leak and is not flagged. Flagging a correct idiom would push someone to "fix" working code. bin/test-state-check.sh gains a survivor fixture, asserting the wrapper both reports and reaps - a detector that only reports leaves the host accumulating processes, which is half the harm. 8/8. * fix(lint): make the unity-exit scanner statement-aware The rule judged one physical line at a time, which reports two kinds of correct code as bare: /* a comment that happens to mention UNITY_END() */ <- interior lines were never stripped exit( UNITY_END()); <- exit( and the macro never met On a probe of both, two of three findings were wrong. This is a note-level rule whose whole job is advice, and bin/lint-node-id-format.sh already says why that matters: a false positive costs more than a miss. One that cries wolf gets ignored, and the real finding goes with it. Carry /* ... */ state across lines and accumulate logical statements before testing, with a 12-line cap so one unclosed call cannot swallow the rest of the file - the same structure lint-node-id-format.sh uses, so the two custom linters in bin/ work alike rather than each having its own idea. Verified both directions: the develop-era sources still produce the same four findings, the fixed tree produces none, and a probe covering block-comment interiors, wrapped exit(), line comments, return UNITY_END() and capture-then- exit reports only the genuinely bare calls - including a complete block comment followed by real bare code on the same line, which the state machine has to keep live. Reported by CodeRabbit on #11322. * fix(lint): tokenise instead of pattern-matching, and self-test it Second round of review findings on the same scanner, all confirmed by direct test before changing anything. Six defects, one root cause: layered regexes cannot tokenise C++. False positives (correct code reported): - UNITY_END() inside a string literal read as code False negatives (real leaks missed): - a string containing "/*" opened comment state and swallowed later lines - greedy .* removed everything between two block comments on one line, taking a bare call with it - myexit(UNITY_END()) matched the exit() exemption as a substring - x == UNITY_END() and total += UNITY_END() matched the assignment exemption Replaced with a character-level scan carrying comment state, and token-bounded exemptions: exit must be a whole identifier, and the capture form must be a plain `=`. Raw string literals are still not modelled - there are none under test/, and delimiter tracking for a case that does not occur would be untested code guarding untested code, so it is documented rather than guessed at. Also drops the `return UNITY_END()` exemption. It only terminates from main(), there is no main() under test/, and from a helper it just returns a count. bin/test-lint-unity-exit.sh pins all fifteen cases, every false positive and false negative found in review among them. The rule has been wrong twice in a way that looked fine by inspection; it needed a self-test more than it needed another careful reading. Two further findings in the same review: - bin/run-tests.sh dropped PASSTHRU in shuffled mode, so `--shuffle -vvv` built verbosely and then ran quietly. The shuffled loop now forwards EXTRA_ARGS, which is PASSTHRU minus the -f pair it supplies per suite. - bin/run-tests.sh did not guard `cd "$ROOT_DIR"`. And one that did not reproduce: the survivor fixture's glob does find the pid file (verified with the lookup instrumented - the earlier failure was an artifact of running the script from /tmp, where SCRIPT_DIR cannot resolve). The assertion was still weak, because an empty pid took the "not running" branch and passed vacuously. It now fails if the pid was never recorded, and finds the file by search rather than assuming a directory depth. Reported by CodeRabbit on #11322. * fix(lint): report each UNITY_END occurrence at its own location The self-test only asked "did the linter say anything", so it could not have caught a wrong line, a wrong column, or a missing second finding. Fixtures now assert the exact diagnostics as line:col, and the first run of that assertion found two real problems. The caret pointed at the wrong occurrence. For `exit(UNITY_END()); UNITY_END();` the verdict was right but the column was 17 - the wrapped call - because the scanner stripped terminating forms out of the whole statement and then reported the first occurrence it had seen. Two bare calls on one line reported once. Judged per occurrence now, by looking back through whitespace at what wraps it, so both the count and the caret are right. That also needed a position map from strip_noncode(): removing a comment or collapsing a literal shifts every later column, and counting occurrences in the raw line does not recover it either - TEST_MESSAGE("... UNITY_END() ..."); UNITY_END(); has two occurrences in the raw text and one in the code. Four of the expected columns I wrote by hand were also wrong, off by one. The linter was right in every case; the assertions were not. They are computed from the fixture text now rather than pasted from output, because a baseline accepted from the tool it is testing asserts nothing. 17 fixtures, including the two-on-one-line case from review and its mirror. Reported by CodeRabbit on #11322. |
||
|
|
21e3a583bd |
Yaml check for Meshtasticd (#11224)
* feat(portduino): add `meshtasticd --check` config validator Users hand-writing files in /etc/meshtasticd/config.d/ get no feedback when a key is misplaced, misspelled or duplicated: meshtasticd silently ignores what it does not read, so a broken config looks identical to a working one. Add a --check mode that loads the configuration exactly as startup does, then reports what it found and exits: - Duplicate keys, via the yaml-cpp Parser/EventHandler stream. The Node API cannot see them because the map is already collapsed by the time it exists, and yaml-cpp keeps the FIRST occurrence, so a later override is discarded. - Unknown or misnested keys, against a schema mirroring what loadConfig() reads, with a hint naming the section a stray key actually belongs to. - rfswitch_table validation: unrecognised pins, mode rows whose length does not match the pin list, values that are not HIGH/LOW, and unknown modes. - Cross-file overlap: every .yaml in the config directory merges into one portduino_config, so the file loaded LAST wins, the opposite of the within-file rule. Those files are read in filesystem order, not alphabetical. - A warning when more than one file defines a Lora section: spidev, spiSpeed, gpiochip, DIO2_AS_RF_SWITCH, DIO3_TCXO_VOLTAGE and USB_PID/VID/Serialnum are assigned unconditionally with a default every time one is seen, so any of them not repeated in the last file loaded is silently reset. - The resolved gpiochip/line for each pin, since a line that exists on the wrong chip is claimed successfully and then silently does nothing. Exits non-zero when errors were found so it can also gate CI over bin/config.d/**, keeping one implementation rather than a second schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(portduino): flag pins that resolve to -1 in --check A pin key whose value will not convert to a number falls back to RADIOLIB_NC (-1) while still being marked enabled, and initGPIOPin() then trips an assertion inside LinuxGPIOPin rather than failing cleanly. YAML indentation makes this easy to hit by accident: a stray line under "CS: 8" folds into the value as a multi-line scalar, so the file parses, the daemon crashes with a stack trace from a library file, and --check reported "Configuration looks good" while printing "pin -1" two lines above. Report it as an error naming the likely cause instead. Also correct a comment claiming unparseable config.d files are skipped silently. They are not: loadConfig() prints "*** Exception ..." with the line and column. It is the discarded return value, not the diagnostic, that makes the file's absence from the merged config easy to miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(portduino): cover `meshtasticd --check` with fixtures and a fuzz suite Adds the tests the config validator was missing, and the checks and fixes that writing them turned up. The theme throughout is configuration that the YAML parser accepts but that does not mean what it looks like it means. Tests ----- bin/test-config-check.sh - 57 assertions driving a built meshtasticd against test/fixtures/portduino-config (50 fixtures plus two config.d trees). A shell test rather than a Unity suite because both behaviours under test are properties of the process: --check is judged by its exit status and printed report, and the "a normal run rejects a bad config" path ends in exit() inside portduinoSetup(), neither of which is reachable from a suite that links one translation unit. Every fixture carries a comment header naming its planted fault and the expected finding, so it can be read on its own. Coverage: * a clean config for each of the ten radio module families (RF95, sx1262, sx1268, LLCC68, sx1280, lr1110, lr1120, lr1121, sim, auto), asserted both findings-free and resolving to that module, so a silent fallback to sim cannot pass * LR11xx rfswitch tables: unrecognised pins, rows longer and shorter than the pin count, levels that are not exactly HIGH, a missing pins list, more than five pins, a scalar table, unknown MODE_ keys, a MODE_ row stranded one level out, and a legal partial table * the PA gain table in both accepted shapes, entries outside the uint16 range it is stored in, and more than the 22 points that are kept * values of the wrong type, split by consequence: the two settings read with no fallback stop meshtasticd starting, everything else is silently replaced by its default * out-of-range and unit mistakes: TCXO voltage written in millivolts, ports outside their usable range, an over-long StatusMessage * MAC sources: both keys set at once, a malformed address, an interface that does not exist * structural faults: duplicate keys, non-mapping and unknown sections, a key left at the top level, a sequence at the document root, an empty file, unreadable pins, unparseable YAML * cross-file behaviour over a config.d directory, including the switch tables that do not override each other * five configs run WITHOUT --check, each of which must still be refused, so check mode cannot quietly make the normal path permissive test/test_fuzz_config - adversarial fuzzing of the checker itself, the "the tool meant to diagnose your config crashes on it" failure mode. Scope is deliberately narrow: yaml-cpp does the parsing and is fuzzed upstream, so what is exercised here is our code above the parse, above all the duplicate-key detector, which is the one hand-rolled piece and walks the raw parser event stream with its own stack. Groups: the checked-in fixtures as a seed corpus, 3000 byte mutations of them (flips, truncation, insertion, splicing, deletion), and structural torture (nesting to 4096 in flow and block style, duplicate keys at depth, anchors, aliases and merge keys, 64KB keys, 256KB scalars, multi-document files). A fourth group of random bytes is present but disabled behind FUZZ_CONFIG_RANDOM_BYTES: it was half the runtime for the least return, since uniform noise is rejected on the first token. The contract is crash-freedom and termination under AddressSanitizer, not any particular finding. CI runs the shell test in the existing native simulator job; the fuzz suite is picked up by the existing ^test_fuzz_ area rule. native-suite-count 40 -> 41. The fixtures are exempt from trunk in .trunk/trunk.yaml, since prettier rejects the duplicate keys and bad indentation that are the point of them. Checker fixes found while writing the tests ------------------------------------------- --check reported a clean exit 0 on configs meshtasticd then refuses to boot, the worst failure a diagnostic tool can have. Four hard exits inside loadConfig() killed the report before it printed: an unparseable file, an unknown Lora.Module, MACAddress and MACAddressSource both set, and HUB75 on a build without it. All are now reported as findings, and all are still refused on a normal run. New validation: Lora.Module against the accepted spellings, which are matched exactly and inconsistently cased, with a suggestion when only case differs; a per-key value type table covering ~85 keys, tested by asking yaml-cpp to perform the same conversion loadConfig() will so it cannot drift; the PA gain table; DIO3_TCXO_VOLTAGE, which is in volts and multiplied by 1000, so the millivolt value everything else uses silently asks for 1800V; APIPort and Webserver.Port ranges; MaxNodes; StatusMessage truncation; MAC address and source; and an unreadable ConfigDirectory. Also fixes a crash: a ConfigDirectory that cannot be read threw an uncaught filesystem_error from directory_iterator and aborted meshtasticd with SIGABRT, taking --check down with it. It now fails cleanly. Two smaller ones: cppcheck's uselessCallsSubstr on the ancestor walk, which was failing every check job; and the duplicate-key detector's stack pop, which was unguarded and relied on yaml-cpp emitting balanced events. Switch tables are the one place "the file loaded last wins" is false. The loader only ever writes HIGH and never writes LOW back, so a HIGH from an earlier file survives a later file that clears it and the radio drives the OR of every table loaded. Confirmed with --output-yaml. Reported as an error for now; the loader itself is left alone, as that changes RF behaviour. * fix(portduino): report CH341 pins as adapter indexes, not gpiochip lines --check printed "Resolved GPIO lines (what meshtasticd will try to claim)" for every config, listing a gpiochip and line for each Lora pin and advising they be confirmed against gpiodetect and gpioinfo. For spidev: ch341 every part of that is false. portduinoSetup() skips initGPIOPin() for every Lora pin when spidev is ch341 and hands the raw numbers to Ch341Hal, so nothing is claimed from a gpiochip -- and on Windows and macOS, where a USB adapter is the only way to attach a radio, there is no gpiochip, gpiodetect or gpioinfo to check against in the first place. The checker had no ch341 coverage at all: not one fixture used it, so the whole USB-SPI path went unexercised. The summary now splits on the transport. A ch341 device gets its pins listed as adapter indexes with the gpiod advice dropped, and a gpiochip or line mapping written alongside it is reported: those are read, stored, and never used. Also: "RF switch table: not set" read as a gap on an SX126x, where there is nothing to set. setRfSwitchTable() is only ever called for an LR11xx, so absence is now "not needed for this module" everywhere else, and "not resolved yet" for auto, which has no module to judge against. Fixtures: usb-ch341.yaml (clean, the meshstick shape) and ch341-gpiochip.yaml. CI fix ------ test-native was RED on "config.d overrides are reported", which wanted 2 warnings and got 1. The fixture's two config.d files name different modules, so which one wins -- and whether the LR11xx-without-a-switch-table warning fires -- depends on the order the filesystem returns them in. That is the very thing the fixture exists to demonstrate, so the count is no longer asserted; the report's own order caveat is asserted instead. Review fixes ------------ The unreadable-ConfigDirectory diagnostic was the one new print in PortduinoGlue.cpp not gated behind !configCheck, so it landed ahead of the report header and broke the clean output the rest of the change is careful to keep. Docs: rfswitch-valid.yaml carries seven modes, not eight, and empty-file.yaml is comments-only rather than zero bytes. * style(portduino): trim --check comment blocks and reconcile suite count Condense the multi-paragraph comment blocks in the --check validator to the one-to-two-line convention, and bump test/native-suite-count to 42 for the test_fuzz_config suite added here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
8e104a909b |
fix(admin): persist TAK module config (team color / member role) (#11216)
Setting TAK team/role ACKed and rebooted but stored nothing: handleSetModuleConfig had no tak case, saveToDisk never set has_tak, and handleGetModuleConfig had no TAK_CONFIG case. Add all three, plus a native suite sweeping every ModuleConfig submessage through set -> save -> load -> get and a TAK value-fidelity suite. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
3becaf2d95 | emdashes begone (#10847) | ||
|
|
90f01c2b95 | docs: update test instructions to prefer bin/run-tests.sh; fix suite count to 19; add Copilot interface caveat | ||
|
|
ab882c5619 |
EU regions merge (#10675)
* stronger together * validate 2.4ghz regions * less noise * you're right, and that shapens the analysis significantly * sassy rejoinder |
||
|
|
de345939af |
Automatic variable hop limits based on mesh activity and size estimation (#10176)
* asdf * Implement SphereOfInfluenceModule for traffic management and eviction tracking * Implement Sphere of Influence module for dynamic hop limit adjustment and role-based floor * Update SAMPLING_DENOMINATOR to improve mesh size estimation accuracy * Add debug logging for scale factor estimation and per-hop node counts in SphereOfInfluenceModule * Enable variable hop limits and role-based hop floors in Sphere of Influence module * Respond to copilot review * Disable variable hop limits and role-based hop floors in Sphere of Influence module * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Implement adaptive sampling for unique node ID tracking in Sphere of Influence module * Add state persistence for Sphere of Influence module * Enhance Sphere of Influence module with state management and adaptive sampling adjustments * Refactor hop scaling functionality: remove SphereOfInfluenceModule and introduce HopScalingModule - Deleted SphereOfInfluenceModule.h, consolidating its responsibilities into the new HopScalingModule. - Added HopScalingModule.h and HopScalingModule.cpp to manage hop scaling logic, including eviction tracking and sampling-based mesh size estimation. - Implemented methods for recording evictions and packet senders, estimating scale factors, and computing required hops based on node activity. - Introduced state persistence for hop scaling parameters to maintain continuity across reboots. - Enhanced thread safety and modularity by utilizing concurrency features. * Guard out STM32. Sowwy. * Refactor HopScalingModule: enhance sampling logic and improve state management * Add unit tests for HopScalingModule: implement mock database and various test scenarios * Refactor test output in HopScalingModule tests: replace printf with TEST_MESSAGE for better integration with Unity * Refactor HopScalingModule logging: replace lastStatusMode with descriptive mode names for improved readability * Refactor test_main.cpp: change NodeNum variable to static and improve comments for clarity * Remove unnecessary delay in setup function for improved test performance * Add missing include for MeshTypes in test_main.cpp * Refactor HopScalingModule tests: enhance mesh topology scenarios and improve test clarity * Update HopScalingModule tests: flesh out node scenarios and improve clarity for dense and sparse mesh cases * Fix politeness factor calculations in HopScalingModule and update related test scenarios for clarity. Remove outdated design doc. * Enhance HopScalingModule: add sampled estimate for scaling decisions and refactor initial run state management * Add sample traffic injection for HopScaling tests to enhance sampledEst visibility * Enhance HopScalingModule: adjust windowFraction calculation for early triggers and improve test output formatting * Enhance HopScalingModule: add jitter functionality to sampling denominator and update tests for consistent behavior * Enhance HopScalingModule: implement adaptive sampling denominator adjustment and add reset functionality for tests * Enhance HopScalingTestShim: add test-only clock and window helpers, update injectSampleTraffic for adaptive sampling, and improve scenario summary output * Enhance HopScalingModule: add detailed documentation for functions, improve clarity of jitter and sampling logic, and reset functionality in tests * Enhance HopScalingModule: add evictionEstimate parameter to estimateScaleFactor and update related logging for improved mesh size estimation * Enhance HopScalingModule: adjust effective rolls calculation for improved accuracy, add eviction estimate logic, responding to all copilot review points * Implement CompactHistogram for parallel hop scaling sampling - Added CompactHistogram class to track node hop distances with bitwise sampling. - Integrated CompactHistogram into HopScalingModule for independent packet sampling. - Updated NodeDB to feed both the hop scaling module and the new histogram sampler. - Enhanced HopScalingModule with methods to sample packets for the histogram and retrieve hop distribution statistics. - Implemented tests for CompactHistogram functionality, including sampling, window rolling, and adaptive denominator scaling. - Updated existing tests to validate the integration of the new histogram sampling mechanism. * Enhance CompactHistogram and HopScalingModule: add per-hop distribution functionality, improve time handling for unit tests, and refine test setup for deterministic behavior * CompactHistogram: add mesh size estimation, improve entry replacement logic, and update logging for per-hop distribution * Refactor HopScalingModule and CompactHistogram integration - Removed the suggestedHopFromCompactHistogram function to streamline hop suggestion logic. - Updated HopScalingModule to directly utilize CompactHistogram's internal methods for hop suggestions and sampling. - Enhanced logging in HopScalingModule to provide detailed histogram statistics. - Modified test cases to ensure comprehensive coverage of new histogram behaviors and sampling logic. - Improved node ID distribution in tests to better exercise sampling mechanisms. - Ensured that filtering denominators are held for 12 hours before dropping, enhancing stability in sampling. * Refactor CompactHistogram to support 13-hour activity tracking and introduce politeness regimes - Updated the bitfield structure to accommodate 13-hour seen tracking. - Changed the logic in rollHour() to analyze activity over the last 0-2 hours vs. 1-3 hours for politeness factor calculation. - Introduced three politeness levels: GENEROUS, DEFAULT, and STRICT based on recent activity ratios. - Adjusted filtering and sampling logic to reflect the new 13-hour tracking period. - Updated unit tests to validate new behavior and ensure proper functionality of politeness regimes. * Enhance CompactHistogram and HopScalingModule for improved sampling and decision-making - Introduced a session-specific hash seed in CompactHistogram to reduce bias in node ID sampling. - Updated sampling logic to use hashed node IDs instead of raw IDs for filtering and entry management. - Added histogram rollover tracking in HopScalingModule to ensure proper decision-making after initial data collection. - Adjusted logging to reflect the active state of the histogram and its comparison with NodeDB advisory hops. - Enhanced unit tests to validate new sampling logic and memory layout changes. * Expose hashNodeId for testing in CompactHistogram * Add mesh trend statistics to CompactHistogram for enhanced activity tracking * Implement histogram state persistence in CompactHistogram with save and load functions * Refactor CompactHistogram to improve entry management and enhance rollHour logging * feat: add HopScalingModule for adaptive hop limit recommendations Introduces HopScalingModule, a sampled hop-distance histogram that recommends the minimum hop limit needed to reach ~40 nodes, and automatically reducing the hops as the mesh grows. Key design: - 512-byte packed histogram (128 × 4-byte Record entries) embedded in a new HopScalingModule. - Each Record: 16-bit node hash, 3-bit hop distance, 13-bit seen bitmap - Sampling filter: only nodes where (hash & (denom-1)) == 0 are kept; denominator doubles on overflow and halves when utilisation is low - Hourly rollHour(): tallies per-hop counts, walks scaled buckets to find the minimum hop satisfying TARGET_AFFECTED_NODES (40), applies a politeness extension based on recent/older activity ratio, shifts all seen bitmaps, and persists state to /prefs/hopScalingState.bin - Hop recommendation gated by bootstrap (requires >=1 rollHour before overriding HOP_MAX) - NodeDB calls samplePacketForHistogram() on every non-MQTT rx packet - Module also estimates total mesh size and logs useful information about mesh characteristics. Changes: - src/modules/HopScalingModule.h/.cpp: new module - src/mesh/NodeDB.cpp: wire up samplePacketForHistogram - src/mesh/Router.cpp: consume getLastRequiredHop() - test/test_hop_scaling/: 12-test suite covering all mesh topologies and anticipated operational requirements * test: increase run iterations in sparse to dense transition test * feat: refactor HopScalingModule to use RUNS_PER_HOUR constant and improve logging * feat: enhance HopScalingModule with filtering denominator management and add tests for state transitions * refactor: remove CompactHistogram module and related files * address copilot review comments * Tweak: packet sampling only lora * ove role-based hop floor logic and related definitions into the module - keep it in one place. * Refactor MockNodeDB to use nodeInfoLiteSetBit for MQTT flag setting * Refactor hop scaling parameters and logic to integer maths and put default values in defaults.h. Small flash size reduction, no functional impact. * Update unit test preprocessor directives to PIO_UNIT_TESTING for consistency * refactor: improve test output organization and clarity in hop scaling tests --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> |
||
|
|
2d6f2ba1a4 | docs: enhance README with verbose test output instructions and usage examples | ||
|
|
5d55353939 | Some fixes and tidies for testing both online and in unit_tests | ||
|
|
f396200d38 |
Add authoring guide for native unit tests in README.md (#10201)
* Add authoring guide for native unit tests in README.md * Enhance documentation for agent tooling and native unit tests in README and related files --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |