Add common infrastructure for Rule-Based Testing#4977
Add common infrastructure for Rule-Based Testing#4977thomas-bc wants to merge 11 commits intonasa:develfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds shared infrastructure to simplify Rule-Based Testing (RBT) rules in F Prime unit tests, then refactors the Svc::Ccsds::ApidManager unit tests to use the new approach (shadow state + grouped rules implemented as tester methods).
Changes:
- Introduces
FW_RBT_DEFINE_RULEinTestUtilsto generate rule structs that delegate to tester member functions (enabling existingASSERT_*macros inside rule bodies). - Refactors
ApidManagerUTs into aTestStateshadow-model and per-group rule implementation.cppfiles. - Updates the ApidManager UT CMake registration to compile the new rule/state sources.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TestUtils/RuleBasedTesting.hpp | Adds a macro-based pattern for defining STest::Rule types that delegate to tester methods. |
| Svc/Ccsds/ApidManager/test/ut/TestState/TestState.hpp | Introduces an ApidManager shadow-state model API for rule preconditions/actions. |
| Svc/Ccsds/ApidManager/test/ut/TestState/TestState.cpp | Implements shadow-state helpers (sequence count tracking + random APID selection). |
| Svc/Ccsds/ApidManager/test/ut/Rules/GetSeqCount.cpp | Adds rule implementations for exercising getApidSeqCountIn. |
| Svc/Ccsds/ApidManager/test/ut/Rules/ValidateSeqCount.cpp | Adds rule implementations for exercising validateApidSeqCountIn. |
| Svc/Ccsds/ApidManager/test/ut/ApidManagerTestMain.cpp | Updates tests to instantiate/apply the new nested rule types and run randomized scenarios. |
| Svc/Ccsds/ApidManager/test/ut/ApidManagerTester.hpp | Refactors the tester to use FW_RBT_DEFINE_RULE and the new ApidManagerTestState. |
| Svc/Ccsds/ApidManager/test/ut/ApidManagerTester.cpp | Removes legacy embedded rule/helper implementations from the tester .cpp. |
| Svc/Ccsds/ApidManager/CMakeLists.txt | Adds new UT source files (shadow state + rule groups) to the unit test target. |
| this->clearHistory(); | ||
|
|
||
| // Use constexpr local to avoid ODR-use of the static constexpr member | ||
| constexpr U8 maxTrackedApids = ApidManager::MAX_TRACKED_APIDS; |
There was a problem hiding this comment.
In GetSeqCount__NewOk__action, ApidManager::MAX_TRACKED_APIDS is a U16, but it’s being stored into a constexpr U8. This narrows the value and can silently truncate if the configured APID enum grows beyond 255 entries, causing the table-full logic to misbehave. Use auto, U16, or size_t for maxTrackedApids to match the constant’s type and the size() comparison.
| constexpr U8 maxTrackedApids = ApidManager::MAX_TRACKED_APIDS; | |
| constexpr U16 maxTrackedApids = ApidManager::MAX_TRACKED_APIDS; |
| // ====================================================================== | ||
|
|
||
| #include "Svc/Ccsds/ApidManager/test/ut/TestState/TestState.hpp" | ||
|
|
There was a problem hiding this comment.
This file uses std::next, but does not include <iterator>. Relying on indirect includes from other headers is non-portable and can break builds on stricter standard library implementations. Add #include <iterator> in this translation unit (or in the header where std::next is used).
| #include <iterator> |
| @@ -0,0 +1,68 @@ | |||
| // ====================================================================== | |||
| // \title ApidManagerTestState.hpp | |||
There was a problem hiding this comment.
The file header’s \title says ApidManagerTestState.hpp, but the actual filename is TestState.hpp. This makes it harder to search/trace headers and can confuse generated docs. Please update the title to match the real path/filename (or rename the file if the title is intended).
| // \title ApidManagerTestState.hpp | |
| // \title TestState.hpp |
bocchino
left a comment
There was a problem hiding this comment.
Looks great! I like the simplification into one macro. I just had one suggested revision to the naming in the macro.
| void GROUP_NAME##__##RULE_NAME##__action(); \ | ||
| struct GROUP_NAME##__##RULE_NAME : public STest::Rule<TESTER_TYPE> { \ | ||
| GROUP_NAME##__##RULE_NAME() : STest::Rule<TESTER_TYPE>(#GROUP_NAME "." #RULE_NAME) {} \ | ||
| bool precondition(const TESTER_TYPE& tester) override { \ |
There was a problem hiding this comment.
I find it confusing to call the type TESTER_TYPE and the parameter tester here. According to STest conventions, the names should be something like TestState and testState. Also, if we generalize the naming, then this macro can be generally useful for testing with rules, not necessarily in the context of an F Prime Tester class.
There was a problem hiding this comment.
The general usage requirement for this macro, as I understand it, is that the rule preconditions and actions are implemented in the same class as the STest test state. This is a more general requirement than saying "the STest test state must be an F Prime tester class." The F Prime tester class meets this requirement.
Change Description
Preface to #3134
The goal here is to provide a “standard" pattern for doing Rule-Based Testing. This does not mean restricting to only that pattern, but I will next document how to work with this specific pattern, and provide tooling to work with it.
ASSERT_EVENTS_*etc. macros to be usable within the rule body by... simply making the body of the rule delegate to a member function of the tester. this is all declared / handled by the macro, and users need simply to implement each body.Future Work
fprime-util new --rule-based-testing--> bootstraps the RBT structure (TestState folder and class definition, Rules/*.cpp file)fprime-util new --rulefor each rule, as we need to insert the MACROs inside the ComponentTester.hpp which would be very brittle and not much added value. Instruct users to copy-paste is fine imo.fprime-util new --rule-groupthat creates theRuleGroup.cppfile and a TODO rule ?AI Usage (see policy)
Help think through the architecture and refactor the ApidManager test code to the new architecture