mirror of
https://github.com/meshtastic/firmware.git
synced 2026-09-13 06:41:45 -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>
379 lines
12 KiB
C++
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() {}
|