Skip to content

Commit fcb5b1f

Browse files
committed
test: make TMM fixture cleanup abort-safe in tearDown()
Several tests reset per-test global state (trafficManagementModule pointing at a stack `module`, owner.public_key, the RTC fake clock) only after their assertions. A TEST_ASSERT_* failure longjmps out and skips that trailing cleanup, leaving the state to dangle into later cases - concretely, the next setUp()'s resetNodes() would call trafficManagementModule->purgeAll() on a destroyed object. Move the resets into tearDown(), which runs unconditionally between tests, and drop the now-redundant per-test cleanups. Addresses a CodeRabbit review comment on PR meshtastic#11050. clod helped too
1 parent 84ed705 commit fcb5b1f

1 file changed

Lines changed: 18 additions & 11 deletions

File tree

test/test_traffic_management/test_main.cpp

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1243,9 +1243,7 @@ static void test_tm_nodeinfo_updateUserHook_writesThrough(void)
12431243
request.hop_limit = 3;
12441244
TEST_ASSERT_EQUAL_INT(static_cast<int>(ProcessMessage::CONTINUE), static_cast<int>(module.handleReceived(request)));
12451245
TEST_ASSERT_EQUAL_UINT32(0, static_cast<uint32_t>(mockRouter.sentPackets.size()));
1246-
1247-
trafficManagementModule = nullptr;
1248-
mockNodeDB->rollHotStore(); // updateUser admitted the node to the hot store
1246+
// trafficManagementModule and the hot store are reset in tearDown()/setUp().
12491247
}
12501248

12511249
/**
@@ -1284,8 +1282,7 @@ static void test_tm_nodeinfo_removeNode_purgesCaches(void)
12841282
TEST_ASSERT_FALSE(module.copyUser(kTargetNode, out, nullptr));
12851283
TEST_ASSERT_EQUAL_UINT8(0, module.getNextHopHint(kTargetNode));
12861284
TEST_ASSERT_EQUAL_INT(-1, module.peekNodeInfoFlagsForTest(kTargetNode));
1287-
1288-
trafficManagementModule = nullptr;
1285+
// trafficManagementModule is reset in tearDown().
12891286
}
12901287

12911288
/**
@@ -1980,8 +1977,7 @@ static void test_tm_nodeinfo_resetNodes_purgesAllCaches(void)
19801977
TEST_ASSERT_FALSE(module.copyUser(kTargetNode, out, nullptr));
19811978
TEST_ASSERT_EQUAL_INT(-1, module.peekNodeInfoFlagsForTest(kTargetNode));
19821979
TEST_ASSERT_EQUAL_UINT8(0, module.getNextHopHint(kTargetNode));
1983-
1984-
trafficManagementModule = nullptr;
1980+
// trafficManagementModule is reset in tearDown().
19851981
}
19861982

19871983
/**
@@ -2001,9 +1997,7 @@ static void test_tm_nodeinfo_cache_dropsFrameCarryingOwnerKey(void)
20011997
TEST_ASSERT_EQUAL_INT(-1, module.peekNodeInfoFlagsForTest(kTargetNode));
20021998
uint8_t key[32] = {0};
20031999
TEST_ASSERT_FALSE(module.copyPublicKey(kTargetNode, key, nullptr));
2004-
2005-
owner.public_key.size = 0; // restore for later tests
2006-
memset(owner.public_key.bytes, 0, sizeof(owner.public_key.bytes));
2000+
// owner.public_key is reset in tearDown() (guaranteed even if an assertion above aborts).
20072001
}
20082002
#endif // !MESHTASTIC_EXCLUDE_PKI
20092003
#endif
@@ -3139,7 +3133,20 @@ void setUp(void)
31393133
{
31403134
resetTrafficConfig();
31413135
}
3142-
void tearDown(void) {}
3136+
void tearDown(void)
3137+
{
3138+
// Runs even when a TEST_ASSERT_* aborts a test mid-way (Unity longjmps out, skipping any
3139+
// cleanup the test itself does after the assertion), so per-test global state can never
3140+
// dangle into later cases. The dangling-pointer path is concrete: a test points the global
3141+
// at its stack `module`, and the next setUp()'s resetNodes() would then call
3142+
// trafficManagementModule->purgeAll() on a destroyed object.
3143+
trafficManagementModule = nullptr;
3144+
owner.public_key.size = 0;
3145+
memset(owner.public_key.bytes, 0, sizeof(owner.public_key.bytes));
3146+
// Neutralize the RTC fake clock a fallback test may have set (the virtual s_testNowMs is
3147+
// already reset by setUp via resetTrafficConfig).
3148+
setBootRelativeTimeForUnitTest(0);
3149+
}
31433150

31443151
TM_TEST_ENTRY void setup()
31453152
{

0 commit comments

Comments
 (0)