Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1124, add generic asserts from CFE coverage testing #1125

Merged

Conversation

jphickey
Copy link
Contributor

Describe the contribution
Migrates some of the assert extensions that were added for CFE coverage testing to the UtAssert level, so they can be used
in a wider scope of tests, not just CFE coverage.

The existing macros (e.g. UtAssert_INT32_EQ) are updated to use the same facility.

All macros are now also a single line and return the boolean pass/fail status, which was a useful feature of the CFE macros.

Fixes #1124

Testing performed
Build and run all tests (functional + coverage) and confirm results are normal.

Expected behavior changes
No operational change to existing asserts, but formatting of some logs may change a little bit, so it may affect some scripts that are looking for very specific message (although it did not affect any of my log parsing scripts).

System(s) tested on
Ubuntu

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey marked this pull request as draft July 30, 2021 17:23
@jphickey
Copy link
Contributor Author

I'm marking as "draft" for now - there is just one minor issue, and I'd like to solicit opinions/ideas from the dev team.

When migrating the macros from CFE and making generic,, I dropped the "CFE" prefix, so CFE_UtAssert_TRUE and CFE_UtAssert_FALSE become just UtAssert_TRUE and UtAssert_FALSE, respectively.

It all works, but, there is an existing UtAssert macros called UtAssert_True (for arbitrary expressions w/full user-defined message). So what we are left with are two macros that differ only in capitalization, which is not ideal.

If anyone has opinions on how we should resolve this while not breaking too much,, please share!

Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, I'd be nice to see all the different ways subtests are added convert to using this common method! No rush, but great to see a common utility. I don't have any good ideas for the UtAssert_True challange... want to avoid breaking things so UtAssert_True is preferably untouchable right now (although that's the one that would probably make the most sense to change).

@zanzaben
Copy link
Contributor

I don't love atmost and atleast, can they just be GreaterThanEqual and LessThanEqual instead. More blunt about what they are that way.

As for TRUE, The best thing would be to change the old True but in the short term I would say to change the name of the new TRUE to some other synonym like TRUTHFUL or NORMAL just to prevent confusion and then eventually update them both some time in the future.

@jphickey
Copy link
Contributor Author

Thanks for the input, I kind of came to the same conclusion that these two things are basically fixed:

  • Cannot change the name of the existing UtAssert_True
  • Cannot leave two macros that differ only in case (that's just fundamentally mean, and probably against some coding standards)

So this leaves the only option of coming up with an alternate name for the UtAssert_TRUE macro.

I'm currently leaning toward UtAssert_BOOL_TRUE and UtAssert_BOOL_FALSE (as this follows the same pattern as UtAssert_INT32_EQ et al).

@jphickey jphickey force-pushed the fix-1124-more-generic-asserts branch from e4e9dbc to 5b26ad4 Compare July 30, 2021 22:01
@jphickey jphickey marked this pull request as ready for review July 30, 2021 22:01
@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Jul 30, 2021
@jphickey
Copy link
Contributor Author

Last push reflects the BOOL_TRUE/BOOL_FALSE idea, so should be generally OK now.

I'm open to renaming ATLEAST/ATMOST too, my only objection is that the macro name can get pretty long. I'm hesitant to make something as long as UtAssert_INT32_GREATERTHANOREQUAL, but I suppose we could. Do you think something like UtAssert_INT32_MINIMUM would be clearer (its shorter at least)?

@skliper
Copy link
Contributor

skliper commented Aug 2, 2021

My preference would be one of the "common" abbreviations like GT/GE/LT/LE or GTE/LTE. We use EQ so just extending that pattern.

@jphickey jphickey force-pushed the fix-1124-more-generic-asserts branch 3 times, most recently from 8f2fe0b to 32d7e16 Compare August 2, 2021 22:27
Migrates some of the assert extensions that were added for CFE
coverage testing to the UtAssert level, so they can be used
in a wider scope of tests, not just CFE coverage.

The existing macros (e.g. UtAssert_INT32_EQ) are updated to
use the same facility.

All macros are now also a single line and return the boolean
pass/fail status, which was a useful feature of the CFE macros.
@jphickey jphickey force-pushed the fix-1124-more-generic-asserts branch from 32d7e16 to ecd276b Compare August 2, 2021 22:28
@jphickey
Copy link
Contributor Author

jphickey commented Aug 2, 2021

OK - last push should address the naming conventions, and does a little more cleanup (group like-type macros together, etc). Had to revise a couple times due to typical issues (clang-format, fat finger, etc), but hopefully good now.

@skliper, @zanzaben please re-review

@jphickey jphickey requested a review from skliper August 2, 2021 22:30
@jphickey
Copy link
Contributor Author

jphickey commented Aug 2, 2021

Note - in an attempt at being proactive I also added an "NEQ" (not equal) variation of the macro in this commit, although nothing uses it yet. Since the comparison type exists and the code is implemented, I figured it's worth making a macro for as well, just in case someone wants it in the future.

@astrogeco astrogeco changed the base branch from main to integration-candidate August 3, 2021 19:06
@astrogeco astrogeco merged commit a9bb454 into nasa:integration-candidate Aug 3, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Aug 4, 2021
Includes

nasa/cFE#1752, Add null pointer check to table GetAddresses and ReleaseAddresses

nasa/osal#1122, Add UtAssert_MIR macro

nasa/osal#1125, add generic asserts from CFE coverage testing

nasa/osal#1121, add osapi-shell-stubs.c to OSAL stub library
@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Aug 4, 2021
@astrogeco
Copy link
Contributor

astrogeco commented Aug 4, 2021

CCB:2021-08-04 APPROVED

  • Should we output both decimal and HEX?
    • Decimal makes it easy to read but it's easier to convert when needed with an external script

astrogeco added a commit to nasa/cFS that referenced this pull request Aug 6, 2021
**Combines**

nasa/cFE#1759, v6.8.0-rc1+dev810
nasa/osal#1126, v5.1.0-rc1+dev586

**Includes**

*cFE*

- nasa/cFE#1752, Add null pointer check to table GetAddresses and ReleaseAddresses
- nasa/cFE#1742, Remove SB get last message sender info requirement
- nasa/cFE#1732, Fix #1725 Update UTs to use UtAssert_MIR
- nasa/cFE#1736, Add Functional Tests cFE Message ID
- nasa/cFE#1707, Add Time Conversion Functional Test
- nasa/cFE#1739, Add cast to MIR prints

*osal*

- nasa/osal#1122, Add UtAssert_MIR macro
- nasa/osal#1125, add generic asserts from CFE coverage testing
- nasa/osal#1121, add osapi-shell-stubs.c to OSAL stub library

Co-authored-by: Jacob Hageman <[email protected]>
Co-authored-by: Joseph Hickey <[email protected]>
Co-authored-by: Alex Campbell <[email protected]>
Co-authored-by: Jose F Martinez Pedraza <[email protected]>
Co-authored-by: Niall Mullane <[email protected]>
Co-authored-by: Ariel Adams <[email protected]>
Co-authored-by: Paul <[email protected]>
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
@jphickey jphickey deleted the fix-1124-more-generic-asserts branch February 24, 2022 13:47
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
Fix nasa#1123, Convert global CFE_SB_Default_Qos to macro
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate CFE generic integer assert tests to common UT Assert
4 participants