Files
Ben MeadorsandClaude Opus 5 83fd62b756 test(native): add 14 suites for routing, persistence, parsing and identity gaps (#11515)
* 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>
2026-08-18 12:41:08 +00:00

379 lines
12 KiB
C++

// Unit tests for src/Observer.h: notification order, the nonzero-return abort chain,
// CallbackObserver dispatch, ~Observer auto-detach, and list mutation from inside onNotify.
#include "Arduino.h"
#include "Observer.h"
#include "TestUtil.h"
#include <cstdio>
#include <string>
#include <unity.h>
// Tags of observers in the order their onNotify ran, e.g. "ABC". Cleared in setUp.
static std::string callOrder;
// An observer that records its calls and can optionally mutate observer lists from inside
// onNotify - the mid-notify hazard the detach/attach-during-notify tests drive.
class RecordingObserver : public Observer<int>
{
public:
explicit RecordingObserver(char _tag) : tag(_tag) {}
char tag;
int returnCode = 0;
int calls = 0;
int lastArg = 0;
// When set, onNotify detaches detachWho from detachFrom before returning.
Observer<int> *detachWho = nullptr;
Observable<int> *detachFrom = nullptr;
// When set, onNotify attaches attachWho to attachTo before returning.
Observer<int> *attachWho = nullptr;
Observable<int> *attachTo = nullptr;
protected:
int onNotify(int arg) override
{
callOrder += tag;
calls++;
lastArg = arg;
if (detachWho && detachFrom)
detachWho->unobserve(detachFrom);
if (attachWho && attachTo)
attachWho->observe(attachTo);
return returnCode;
}
};
// Target class for the CallbackObserver member-pointer dispatch tests.
class CallbackTarget
{
public:
int calls = 0;
int lastArg = 0;
int handle(int arg)
{
calls++;
lastArg = arg;
return 0;
}
int handleAbort(int arg)
{
calls++;
lastArg = arg;
return 42;
}
};
// --- basic delivery ---
void test_notify_with_no_observers_returns_zero()
{
Observable<int> subject;
TEST_ASSERT_EQUAL(0, subject.notifyObservers(99));
}
void test_notify_order_and_arg()
{
Observable<int> subject;
RecordingObserver a('A'), b('B'), c('C');
a.observe(&subject);
b.observe(&subject);
c.observe(&subject);
TEST_ASSERT_EQUAL(0, subject.notifyObservers(42));
TEST_ASSERT_EQUAL_STRING("ABC", callOrder.c_str()); // insertion order
TEST_ASSERT_EQUAL(42, a.lastArg);
TEST_ASSERT_EQUAL(42, b.lastArg);
TEST_ASSERT_EQUAL(42, c.lastArg);
// Delivery is not one-shot: a second notify reaches everyone again.
TEST_ASSERT_EQUAL(0, subject.notifyObservers(43));
TEST_ASSERT_EQUAL_STRING("ABCABC", callOrder.c_str());
TEST_ASSERT_EQUAL(2, b.calls);
TEST_ASSERT_EQUAL(43, b.lastArg);
}
// --- abort contract ---
void test_nonzero_return_aborts_chain_and_propagates()
{
Observable<int> subject;
RecordingObserver a('A'), b('B'), c('C');
a.observe(&subject);
b.observe(&subject);
c.observe(&subject);
b.returnCode = 7;
TEST_ASSERT_EQUAL(7, subject.notifyObservers(1));
TEST_ASSERT_EQUAL_STRING("AB", callOrder.c_str());
TEST_ASSERT_EQUAL(0, c.calls); // chain stopped before C
// Clearing the abort restores full delivery.
b.returnCode = 0;
callOrder.clear();
TEST_ASSERT_EQUAL(0, subject.notifyObservers(2));
TEST_ASSERT_EQUAL_STRING("ABC", callOrder.c_str());
}
// --- CallbackObserver ---
void test_callback_observer_dispatches_member_function()
{
Observable<int> subject;
CallbackTarget target;
CallbackObserver<CallbackTarget, int> cb(&target, &CallbackTarget::handle);
cb.observe(&subject);
TEST_ASSERT_EQUAL(0, subject.notifyObservers(1234));
TEST_ASSERT_EQUAL(1, target.calls);
TEST_ASSERT_EQUAL(1234, target.lastArg);
}
void test_callback_observer_return_code_aborts_chain()
{
Observable<int> subject;
CallbackTarget target;
CallbackObserver<CallbackTarget, int> cb(&target, &CallbackTarget::handleAbort);
RecordingObserver after('X');
cb.observe(&subject);
after.observe(&subject);
TEST_ASSERT_EQUAL(42, subject.notifyObservers(5));
TEST_ASSERT_EQUAL(1, target.calls);
TEST_ASSERT_EQUAL(0, after.calls); // callback's abort code stopped the chain
}
// --- lifecycle: destructor auto-detach ---
void test_destroyed_observer_is_not_notified()
{
Observable<int> subject;
RecordingObserver a('A'), c('C');
a.observe(&subject);
RecordingObserver *b = new RecordingObserver('B');
b->observe(&subject);
c.observe(&subject);
TEST_ASSERT_EQUAL(0, subject.notifyObservers(1));
TEST_ASSERT_EQUAL_STRING("ABC", callOrder.c_str());
delete b; // ~Observer must remove it from the observable's list
callOrder.clear();
TEST_ASSERT_EQUAL(0, subject.notifyObservers(2)); // ASan-clean: no dangling pointer left behind
TEST_ASSERT_EQUAL_STRING("AC", callOrder.c_str());
}
void test_observer_watching_two_observables_detaches_from_both()
{
Observable<int> subject1;
Observable<int> subject2;
{
RecordingObserver x('X');
x.observe(&subject1);
x.observe(&subject2); // re-target onto a second observable: both now deliver
subject1.notifyObservers(1);
subject2.notifyObservers(2);
TEST_ASSERT_EQUAL(2, x.calls);
TEST_ASSERT_EQUAL(2, x.lastArg);
} // x destroyed here - must have detached from both observables
callOrder.clear();
TEST_ASSERT_EQUAL(0, subject1.notifyObservers(3));
TEST_ASSERT_EQUAL(0, subject2.notifyObservers(4));
TEST_ASSERT_EQUAL_STRING("", callOrder.c_str());
}
// --- duplicate observe / unobserve semantics ---
void test_duplicate_observe_delivers_twice_and_unobserve_removes_all()
{
Observable<int> subject;
RecordingObserver a('A');
a.observe(&subject);
a.observe(&subject); // current semantics: second observe means double delivery
TEST_ASSERT_EQUAL(0, subject.notifyObservers(9));
TEST_ASSERT_EQUAL_STRING("AA", callOrder.c_str());
TEST_ASSERT_EQUAL(2, a.calls);
// One unobserve removes every entry (std::list::remove semantics), not just one.
a.unobserve(&subject);
callOrder.clear();
TEST_ASSERT_EQUAL(0, subject.notifyObservers(10));
TEST_ASSERT_EQUAL_STRING("", callOrder.c_str());
TEST_ASSERT_EQUAL(2, a.calls);
}
void test_unobserve_of_never_observed_observable_is_noop()
{
Observable<int> subject;
RecordingObserver a('A'), stranger('S');
a.observe(&subject);
stranger.unobserve(&subject); // never attached: must be a safe no-op
TEST_ASSERT_EQUAL(0, subject.notifyObservers(1));
TEST_ASSERT_EQUAL_STRING("A", callOrder.c_str());
TEST_ASSERT_EQUAL(0, stranger.calls);
}
// --- list mutation from inside onNotify (the safe cases) ---
void test_detach_of_earlier_observer_during_notify()
{
Observable<int> subject;
RecordingObserver a('A'), b('B'), c('C');
a.observe(&subject);
b.observe(&subject);
c.observe(&subject);
b.detachWho = &a; // B removes already-visited A mid-notify
b.detachFrom = &subject;
TEST_ASSERT_EQUAL(0, subject.notifyObservers(1));
TEST_ASSERT_EQUAL_STRING("ABC", callOrder.c_str()); // A was visited before removal; C unaffected
b.detachWho = nullptr;
callOrder.clear();
TEST_ASSERT_EQUAL(0, subject.notifyObservers(2));
TEST_ASSERT_EQUAL_STRING("BC", callOrder.c_str()); // A stays detached
}
void test_detach_of_later_observer_during_notify()
{
Observable<int> subject;
RecordingObserver a('A'), b('B'), c('C');
a.observe(&subject);
b.observe(&subject);
c.observe(&subject);
a.detachWho = &c; // A removes not-yet-visited C mid-notify
a.detachFrom = &subject;
TEST_ASSERT_EQUAL(0, subject.notifyObservers(1));
TEST_ASSERT_EQUAL_STRING("AB", callOrder.c_str()); // iteration stays valid, C never called
TEST_ASSERT_EQUAL(0, c.calls);
a.detachWho = nullptr;
callOrder.clear();
TEST_ASSERT_EQUAL(0, subject.notifyObservers(2));
TEST_ASSERT_EQUAL_STRING("AB", callOrder.c_str());
}
// Tightest safe case: removing the node the iterator will step to next. std::list relinks A's
// next pointer when B's node is erased, so ++iterator lands on C.
void test_detach_of_immediately_next_observer_during_notify()
{
Observable<int> subject;
RecordingObserver a('A'), b('B'), c('C');
a.observe(&subject);
b.observe(&subject);
c.observe(&subject);
a.detachWho = &b;
a.detachFrom = &subject;
TEST_ASSERT_EQUAL(0, subject.notifyObservers(1));
TEST_ASSERT_EQUAL_STRING("AC", callOrder.c_str());
TEST_ASSERT_EQUAL(0, b.calls);
a.detachWho = nullptr;
callOrder.clear();
TEST_ASSERT_EQUAL(0, subject.notifyObservers(2));
TEST_ASSERT_EQUAL_STRING("AC", callOrder.c_str());
}
// Self-detach is only safe when the observer also aborts the chain: returning nonzero exits
// before the iterator is advanced past the node unobserve() just erased. PhoneAPI is the one
// observer in the tree that does this (onNotify -> checkConnectionTimeout -> close() ->
// unobserve, returning -1), and its -1 is load-bearing, not incidental. A self-detaching
// observer that returned 0 would walk a freed node - not covered here, because asserting that
// would be asserting UB; notifyObservers() has to be hardened before it can be tested.
void test_self_detach_with_abort_during_notify()
{
Observable<int> subject;
RecordingObserver a('A'), b('B'), c('C');
a.observe(&subject);
b.observe(&subject);
c.observe(&subject);
b.detachWho = &b;
b.detachFrom = &subject;
b.returnCode = -1;
TEST_ASSERT_EQUAL(-1, subject.notifyObservers(1));
TEST_ASSERT_EQUAL_STRING("AB", callOrder.c_str()); // C never runs: the chain aborted
TEST_ASSERT_EQUAL(0, c.calls);
b.detachWho = nullptr;
b.returnCode = 0;
callOrder.clear();
TEST_ASSERT_EQUAL(0, subject.notifyObservers(2));
TEST_ASSERT_EQUAL_STRING("AC", callOrder.c_str());
}
void test_attach_during_notify_is_safe_and_delivers_next_time()
{
Observable<int> subject;
RecordingObserver a('A'), b('B'), c('C'), d('D');
a.observe(&subject);
b.observe(&subject);
c.observe(&subject);
a.attachWho = &d; // A appends D mid-notify (push_back never invalidates list iterators)
a.attachTo = &subject;
TEST_ASSERT_EQUAL(0, subject.notifyObservers(1));
// The pre-existing observers all ran, in order. Whether the same pass also reaches the
// freshly appended D is deliberately not asserted - a hardened notifyObservers that
// snapshots the list would legitimately change that, and it should not go red for it.
TEST_ASSERT_EQUAL_STRING("ABC", callOrder.substr(0, 3).c_str());
a.attachWho = nullptr;
callOrder.clear();
TEST_ASSERT_EQUAL(0, subject.notifyObservers(2));
TEST_ASSERT_EQUAL_STRING("ABCD", callOrder.c_str()); // D is a full participant from now on
}
// --- Unity lifecycle ---
void setUp(void)
{
callOrder.clear();
}
void tearDown(void) {}
void setup()
{
initializeTestEnvironment();
UNITY_BEGIN();
printf("\n=== Basic delivery ===\n");
RUN_TEST(test_notify_with_no_observers_returns_zero);
RUN_TEST(test_notify_order_and_arg);
printf("\n=== Abort contract ===\n");
RUN_TEST(test_nonzero_return_aborts_chain_and_propagates);
printf("\n=== CallbackObserver ===\n");
RUN_TEST(test_callback_observer_dispatches_member_function);
RUN_TEST(test_callback_observer_return_code_aborts_chain);
printf("\n=== Lifecycle ===\n");
RUN_TEST(test_destroyed_observer_is_not_notified);
RUN_TEST(test_observer_watching_two_observables_detaches_from_both);
printf("\n=== Duplicate observe / unobserve ===\n");
RUN_TEST(test_duplicate_observe_delivers_twice_and_unobserve_removes_all);
RUN_TEST(test_unobserve_of_never_observed_observable_is_noop);
printf("\n=== Mutation during notify (safe cases) ===\n");
RUN_TEST(test_detach_of_earlier_observer_during_notify);
RUN_TEST(test_detach_of_later_observer_during_notify);
RUN_TEST(test_detach_of_immediately_next_observer_during_notify);
RUN_TEST(test_self_detach_with_abort_during_notify);
RUN_TEST(test_attach_during_notify_is_safe_and_delivers_next_time);
exit(UNITY_END());
}
void loop() {}