mirror of
https://github.com/meshtastic/firmware.git
synced 2026-09-12 22:29:00 -04:00
* test(native): add 14 suites for routing, persistence, parsing and identity gaps Coverage audit of the native test tree; adds the highest-value untested logic as 11 new suites and extends 3 existing ones (200 test functions). New: test_stream_framing, test_nodedb_boot_recovery, test_nodedb_legacy_migration, test_nodedb_v25_roundtrip, test_nodedb_identity_hygiene, test_channel_keys, test_reliable_ack_matrix, test_hop_start_policy, test_routing_response_hops, test_phone_api_config_dump, test_observer. Extended: test_rtc, test_mqtt, test_xmodem. Two source changes the audit produced: - StreamAPI::handleRecStream copied stream->read()'s `cInt < 0` EOF check into the buffer-fed path, where there is no EOF sentinel; with signed char any byte >= 0x80 (START1 is 0x94) aborted the parse. Read the byte as uint8_t directly. Latent on develop (no callers), pinned by test_stream_framing. - Extract the post-decode pre-hop predicate from Router::handleReceived into shouldSkipHandleForPostDecodeHop() (NodeDB.h) so test_hop_start_policy drives the exact expression the router calls. No behavior change. test/state-manifest.tsv declares the suites that construct a NodeDB. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): address review - harden observer dispatch, trim comments Review follow-ups on the coverage-audit suites: - Observable::notifyObservers() erased list nodes while holding an iterator into them, so an observer that unobserves itself from onNotify corrupted the dispatch. Today the only self-detacher (PhoneAPI::onNotify -> checkConnectionTimeout -> close -> unobserve) survives solely because it returns -1 and aborts the chain before the increment; that unwritten contract is now gone. Removal during a dispatch nulls the entry and the outermost notify sweeps afterwards, which keeps self-detach, next-detach and destruction-during-notify all safe without an allocation. Hoisting the next iterator instead would have inverted the hazard and broken the existing next-detach case. Two regression tests added. - Correct the documented caller of shouldSkipHandleForPostDecodeHop: the call is in Router::dispatchReceived, not handleReceived. - Cast hop fields to unsigned at the %u call site in test_hop_start_policy. - Trim the new suites' file headers to the one-or-two-line rule in AGENTS.md. - Rename eight test functions whose names were exactly `test_` + 35 chars: that is the shape of a Lob API key, so trufflehog flagged them as secrets and failed the Trunk CI check. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): revert the observer dispatch change, keep the contract test Backs out the notifyObservers() deferred-removal hardening from the previous commit. It was reviewer-driven scope creep: nothing in the coverage audit needed it, no test required it, and it changes dispatch semantics in a header with ~76 observe() call sites on native verification alone. The hazard it addressed is not reachable today. The only observer that unobserves itself from onNotify is PhoneAPI (onNotify -> checkConnectionTimeout -> close -> unobserve), and it returns -1, which aborts the chain before the iterator is advanced past the erased node. test_self_detach_with_abort_during_notify stays: it passes against the unmodified dispatch and pins that the -1 is load-bearing, so a later cleanup that "simplifies" it away goes red. The unsafe variant (self-detach returning 0) is documented in a comment rather than tested, since asserting it would be asserting UB. * fix(serial): recover the frame behind a stray framing marker A byte that failed the START2 check was discarded rather than re-tested as a possible START1, so 0x94 0x94 0xc3 ... lost the real frame: one corrupted byte on a noisy UART silently dropped the frame behind it. Re-test the byte in place instead. Applied to both copies of the receive state machine. readStream() is the one that matters in the field - it is the serial path every phone client uses - while handleRecStream() still has no callers on develop. Strictly widens what the parser accepts; no frame that parsed before parses differently. test_stream_framing covers it on both receive paths, plus a run of stray markers and a START1-then-unrelated-byte resync. This was originally documented as a known gap in the framing suite. Fixing it instead was NomDeTom's call on review: a passing test asserting the bad behavior is what makes it hard to change later, and it is the same defect shape as the signedness fix three functions away. Also: use Throttle::deadlinePassed() in test_reliable_ack_matrix rather than a bare millis() compare, matching the house deadline rule. * test(native): cover the stray-marker resync on the buffer path too The stray-marker fix went into both copies of the receive state machine, but only test_stray_start1_before_frame_still_delivers drove both. The repeated- marker and unrelated-byte cases drove readStream() alone, so a regression in handleRecStream() would have gone unnoticed by two of the three. Verified load-bearing: reverting only the handleRecStream() half of the fix turns test_repeated_stray_start1_before_frame_still_delivers red on the new assertion. test_start1_then_unrelated_byte_resyncs stays green under that mutation by design - its failing byte is 0x00, where both branches reset to 0 - and covers the other half of the ternary. Also drops the stale header on test_stray_start1_before_frame_still_delivers, which still described the gap as pinned-as-is after the fix landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(native): make the hop-start truth table assert the rows it prints test_truth_table_summary was six TEST_MESSAGE lines and no assertion, so it reported as a case that could not fail - the anti-pattern #11517 names in its unfinished assertion-presence lint, and the one exception to NomDeTom's "no RUN_TEST without an assertion" pass over this PR. The printed row and the checked expectation now come from one struct, so the summary cannot narrate a table the predicates no longer implement. It also covers the consequence columns the per-row tests do not assert together: classifyHopStart, shouldDropPacketForPreHop and shouldSkipHandleForPostDecodeHop for the same packet, with the expectations gated on MESHTASTIC_PREHOP_DROP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
274 lines
10 KiB
C++
274 lines
10 KiB
C++
// RoutingModule::getHopLimitForResponse - the hop budget stamped on every reply/ACK/NAK - and
|
|
// MeshModule::setReplyTo() applying it, driven through getHopsAway()'s sentinel rules.
|
|
|
|
#include "MeshTypes.h" // before TestUtil.h: provides NodeNum etc.
|
|
#include "TestUtil.h"
|
|
#include <cstdlib> // exit(), needed on both guard branches
|
|
#include <unity.h>
|
|
|
|
// Event mode compiles out the uncapped long-path branch and swaps the configured limit for the
|
|
// event hop limit; this suite pins the standard-mode branches only (the event cap is covered by
|
|
// test_default's event-mode group).
|
|
#if !USERPREFS_EVENT_MODE
|
|
|
|
#include "configuration.h"
|
|
#include "mesh/MeshModule.h"
|
|
#include "mesh/NodeDB.h"
|
|
#include "modules/RoutingModule.h"
|
|
#include <cstdio>
|
|
|
|
static constexpr NodeNum kRequester = 0x22222222;
|
|
|
|
static RoutingModule *testRoutingModule = nullptr;
|
|
|
|
// A received request packet whose hop fields we control. Decoded packets carry the bitfield flag
|
|
// that getHopsAway() uses to decide whether hop_start==0 is genuine or a legacy-firmware zero.
|
|
static meshtastic_MeshPacket makeRequest(uint8_t hopStart, uint8_t hopLimit, bool decoded = true, bool hasBitfield = true)
|
|
{
|
|
meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero;
|
|
p.from = kRequester;
|
|
p.to = 0x11111111;
|
|
p.id = 0xABCD1234;
|
|
p.hop_start = hopStart;
|
|
p.hop_limit = hopLimit;
|
|
if (decoded) {
|
|
p.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
|
|
p.decoded.has_bitfield = hasBitfield;
|
|
} else {
|
|
p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag;
|
|
p.encrypted.size = 8;
|
|
}
|
|
return p;
|
|
}
|
|
|
|
static meshtastic_MeshPacket makeReply()
|
|
{
|
|
meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero;
|
|
p.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
|
|
return p;
|
|
}
|
|
|
|
void setUp(void)
|
|
{
|
|
config.lora.hop_limit = 3;
|
|
}
|
|
|
|
void tearDown(void) {}
|
|
|
|
// ===========================================================================
|
|
// Group 1 - unknown hop distance: every unreliable-header shape must fall back
|
|
// to the configured limit, never to a value derived from the bogus fields.
|
|
// ===========================================================================
|
|
|
|
void test_encrypted_hop_start_zero_falls_back_to_configured_limit(void)
|
|
{
|
|
// Encrypted packet: the bitfield is unreadable, so hop_start==0 cannot be trusted.
|
|
auto request = makeRequest(0, 0, /*decoded=*/false);
|
|
TEST_ASSERT_EQUAL_UINT8(3, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_decoded_legacy_no_bitfield_falls_back_to_configured_limit(void)
|
|
{
|
|
// Pre-2.3.0 senders never populate hop_start and pre-2.5.0 senders never set the bitfield.
|
|
auto request = makeRequest(0, 0, /*decoded=*/true, /*hasBitfield=*/false);
|
|
TEST_ASSERT_EQUAL_UINT8(3, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_forged_hop_start_below_hop_limit_falls_back_to_configured_limit(void)
|
|
{
|
|
// hop_start < hop_limit is impossible for an honest sender; getHopsAway() rejects it.
|
|
auto request = makeRequest(2, 5);
|
|
TEST_ASSERT_EQUAL_UINT8(3, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_hostile_hop_start_wraps_negative_falls_back_to_configured_limit(void)
|
|
{
|
|
// hop_start is 3 bits on the wire but 8 bits via local injection: 255 - 0 narrows to
|
|
// int8_t -1 in getHopsAway(), which lands in the same "unknown" fallback (any
|
|
// hop_start - hop_limit >= 128 reads as negative).
|
|
auto request = makeRequest(255, 0);
|
|
TEST_ASSERT_EQUAL_UINT8(3, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
// ===========================================================================
|
|
// Group 2 - known hop distance: hopsUsed + 2 margin, its clamp boundary, and
|
|
// the intentionally uncapped long-path branch.
|
|
// ===========================================================================
|
|
|
|
void test_direct_neighbor_response_gets_two_hop_margin(void)
|
|
{
|
|
config.lora.hop_limit = 7;
|
|
auto request = makeRequest(3, 3); // 0 hops used
|
|
TEST_ASSERT_EQUAL_UINT8(2, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_two_hops_used_gets_margin_of_two(void)
|
|
{
|
|
config.lora.hop_limit = 7;
|
|
auto request = makeRequest(3, 1); // 2 hops used
|
|
TEST_ASSERT_EQUAL_UINT8(4, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_margin_just_below_boundary_still_applies(void)
|
|
{
|
|
config.lora.hop_limit = 7;
|
|
auto request = makeRequest(7, 3); // 4 hops used: 4 + 2 = 6 < 7
|
|
TEST_ASSERT_EQUAL_UINT8(6, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_margin_at_boundary_clamps_to_configured_limit(void)
|
|
{
|
|
config.lora.hop_limit = 7;
|
|
auto request = makeRequest(7, 2); // 5 hops used: 5 + 2 == 7, not < 7 -> clamp
|
|
TEST_ASSERT_EQUAL_UINT8(7, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_hops_equal_to_limit_returns_limit(void)
|
|
{
|
|
config.lora.hop_limit = 7;
|
|
auto request = makeRequest(7, 0); // 7 hops used == limit: not "more than", no margin room
|
|
TEST_ASSERT_EQUAL_UINT8(7, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_long_path_exceeds_configured_limit_uncapped(void)
|
|
{
|
|
// Intentional exceed: a request that took more hops than our configured limit gets a
|
|
// response with the same hop count, otherwise the reply dies short of the requester.
|
|
auto request = makeRequest(7, 0); // 7 hops used, configured limit 3
|
|
TEST_ASSERT_EQUAL_UINT8(7, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
// ===========================================================================
|
|
// Group 3 - zero-hop requester
|
|
// ===========================================================================
|
|
|
|
void test_zero_hop_requester_gets_zero_hop_response(void)
|
|
{
|
|
// hop_start==0 with the bitfield present is a genuine "0 hops requested": the sender is
|
|
// modern firmware that deliberately sent direct-only, so the response stays local too.
|
|
auto request = makeRequest(0, 0, /*decoded=*/true, /*hasBitfield=*/true);
|
|
TEST_ASSERT_EQUAL_UINT8(0, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
// ===========================================================================
|
|
// Group 4 - configured-limit edges through Default::getConfiguredOrDefaultHopLimit
|
|
// ===========================================================================
|
|
|
|
void test_config_above_hop_max_clamps_to_hop_max(void)
|
|
{
|
|
config.lora.hop_limit = 10; // out-of-range config (protobuf allows up to 255)
|
|
auto request = makeRequest(0, 0, /*decoded=*/false);
|
|
TEST_ASSERT_EQUAL_UINT8(HOP_MAX, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_config_zero_yields_zero_for_unknown_hops(void)
|
|
{
|
|
// Pins current behavior: getConfiguredOrDefaultHopLimit(0) passes the zero through (no
|
|
// default substitution), so an unknown-distance requester gets a 0-hop response.
|
|
config.lora.hop_limit = 0;
|
|
auto request = makeRequest(0, 0, /*decoded=*/false);
|
|
TEST_ASSERT_EQUAL_UINT8(0, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
void test_config_zero_known_hops_returns_hops_used(void)
|
|
{
|
|
// With a zero configured limit, any known hop count is "more than the limit" and is used
|
|
// as-is - a zero config does not strand replies to multi-hop requesters.
|
|
config.lora.hop_limit = 0;
|
|
auto request = makeRequest(3, 1); // 2 hops used
|
|
TEST_ASSERT_EQUAL_UINT8(2, testRoutingModule->getHopLimitForResponse(request));
|
|
}
|
|
|
|
// ===========================================================================
|
|
// Group 5 - setReplyTo() stamps the computed hop limit onto reply packets
|
|
// ===========================================================================
|
|
|
|
void test_setreplyto_stamps_computed_hop_limit_and_reply_fields(void)
|
|
{
|
|
config.lora.hop_limit = 7;
|
|
auto request = makeRequest(3, 1); // 2 hops used -> response hop limit 4
|
|
request.channel = 2;
|
|
request.want_ack = true;
|
|
|
|
auto reply = makeReply();
|
|
setReplyTo(&reply, request);
|
|
|
|
TEST_ASSERT_EQUAL_HEX32(kRequester, reply.to);
|
|
TEST_ASSERT_EQUAL_UINT8(2, reply.channel);
|
|
TEST_ASSERT_EQUAL_UINT8(4, reply.hop_limit);
|
|
TEST_ASSERT_TRUE(reply.want_ack);
|
|
TEST_ASSERT_EQUAL_HEX32(request.id, reply.decoded.request_id);
|
|
TEST_ASSERT_EQUAL(meshtastic_MeshPacket_Priority_RELIABLE, reply.priority);
|
|
}
|
|
|
|
void test_setreplyto_preserves_existing_priority(void)
|
|
{
|
|
auto request = makeRequest(0, 0, /*decoded=*/false); // unknown hops -> configured limit 3
|
|
request.want_ack = false;
|
|
|
|
auto reply = makeReply();
|
|
reply.priority = meshtastic_MeshPacket_Priority_ACK;
|
|
setReplyTo(&reply, request);
|
|
|
|
TEST_ASSERT_EQUAL_UINT8(3, reply.hop_limit);
|
|
TEST_ASSERT_FALSE(reply.want_ack);
|
|
TEST_ASSERT_EQUAL(meshtastic_MeshPacket_Priority_ACK, reply.priority);
|
|
}
|
|
|
|
// ===========================================================================
|
|
|
|
void setup()
|
|
{
|
|
initializeTestEnvironment();
|
|
UNITY_BEGIN();
|
|
|
|
testRoutingModule = new RoutingModule();
|
|
routingModule = testRoutingModule; // setReplyTo() reaches the module through the global
|
|
|
|
printf("\n=== unknown hop distance falls back to configured limit ===\n");
|
|
RUN_TEST(test_encrypted_hop_start_zero_falls_back_to_configured_limit);
|
|
RUN_TEST(test_decoded_legacy_no_bitfield_falls_back_to_configured_limit);
|
|
RUN_TEST(test_forged_hop_start_below_hop_limit_falls_back_to_configured_limit);
|
|
RUN_TEST(test_hostile_hop_start_wraps_negative_falls_back_to_configured_limit);
|
|
|
|
printf("\n=== known hop distance: margin, clamp, uncapped long path ===\n");
|
|
RUN_TEST(test_direct_neighbor_response_gets_two_hop_margin);
|
|
RUN_TEST(test_two_hops_used_gets_margin_of_two);
|
|
RUN_TEST(test_margin_just_below_boundary_still_applies);
|
|
RUN_TEST(test_margin_at_boundary_clamps_to_configured_limit);
|
|
RUN_TEST(test_hops_equal_to_limit_returns_limit);
|
|
RUN_TEST(test_long_path_exceeds_configured_limit_uncapped);
|
|
|
|
printf("\n=== zero-hop requester ===\n");
|
|
RUN_TEST(test_zero_hop_requester_gets_zero_hop_response);
|
|
|
|
printf("\n=== configured-limit edges ===\n");
|
|
RUN_TEST(test_config_above_hop_max_clamps_to_hop_max);
|
|
RUN_TEST(test_config_zero_yields_zero_for_unknown_hops);
|
|
RUN_TEST(test_config_zero_known_hops_returns_hops_used);
|
|
|
|
printf("\n=== setReplyTo integration ===\n");
|
|
RUN_TEST(test_setreplyto_stamps_computed_hop_limit_and_reply_fields);
|
|
RUN_TEST(test_setreplyto_preserves_existing_priority);
|
|
|
|
exit(UNITY_END());
|
|
}
|
|
|
|
void loop() {}
|
|
|
|
#else // USERPREFS_EVENT_MODE
|
|
|
|
void setUp(void) {}
|
|
void tearDown(void) {}
|
|
|
|
void setup()
|
|
{
|
|
initializeTestEnvironment();
|
|
UNITY_BEGIN();
|
|
exit(UNITY_END());
|
|
}
|
|
|
|
void loop() {}
|
|
|
|
#endif
|