Fix #734, continue cleanup of SB unit test #737
Conversation
Makes the SB unit test closer to recommended UT assert patterns Do not keep a separate "TestStat" state variable outside UT assert. Do not report separate status messages from the asserts. Use UT assert. Do not "reset" in the middle of a test routine, split into separate test routines where this is done. No need for "START" and "STARTBLOCK" or "ENDBLOCK". UT assert has messages for these test actions. Each block should be a separate test routine and then these are unnecessary.
jphickey
added
the
CCB:Ready
|
TestStat is/was useful for "falling through" if you have a failure, all other test calls would just be skipped. I am on the fence whether that's preferred or not--it makes it easier to find the root cause of the failure, rather than having to wade through all of the following errors, but there may be situations where we might want those calls to run even in the case of a failure (particularly teardown calls.) |
Yeah, but this was the only module that did that. If we want the concept we can add a feature to UT assert to exit a test function early and move on to the next test after the first failure. But in most other modules it just proceeds with the remainder of the test. Sure, you might get a lot of superfluous "FAIL" cases due to the first failure but that's not really a problem - the rule of thumb when debugging a UT issue should be to focus on the first reported failure. I definitely wouldn't say there was ever any confusion about where to focus attention within a swath of failure messges - it's the always the first reported problem. |
| @@ -3242,7 +3285,10 @@ void Test_CFE_SB_SetGetMsgId(void); | |||
| ** \sa #UT_GetActualPktLenField, #UT_Report | |||
| ** | |||
| ******************************************************************************/ | |||
| void Test_CFE_SB_SetGetUserDataLength(void); | |||
| void Test_CFE_SB_SetGetUserDataLength_Cmd(void); | |||
There was a problem hiding this comment.
dumb question - why are individual test fn's defined in the .h file? It's not like anything else is calling them...
|
CCB 2020-06-10: APPROVED, Addressing at 600K tests issue in #726 |
astrogeco
added
CCB-20200610
CCB:Approved
Describe the contribution
Makes the SB unit test closer to recommended UT assert patterns
TestStatstate variable outside UT assert.STARTandSTARTBLOCKorENDBLOCKmacros. UT assert has messages for these test actions. Each block should be a separate test routine and then these are unnecessary.Fixes #734
Testing performed
Run all unit tests and confirm passing. No FSW code changes here, only UT.
Expected behavior changes
SB Unit tests now use the UT assert framework more as intended.
System(s) tested on
Ubuntu 20.04
Additional context
This changes some items that were only reported on failure to a normal assert statement, which means its always reported. The side effect is that there are now about 600,000 "test cases" in the SB test. Most of that is from remaining validation of the length field, in particular the User Length test, which validates every possible uint16 value (65536), for each of the 4 header structures (x4), for both the return value and packet content (x2) totalling 524288 test cases for this one function. There are similar examples for cmd code but as this is only an 8-bit field it only results in a few thousand cases.
If it is not desirable to have so many test cases, recommendation is to change the unit test to not loop through every possible value. No other functions are tested this way i.e. we do not call other APIs accepting a uint16 with every representable value.
Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.