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.

Sign up for GitHub

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

Jump to bottom

Fix #1626, rename/clean CFE coverage assert macros #1627

Merged
merged 1 commit into from
Jun 21, 2021
Merged

Fix #1626, rename/clean CFE coverage assert macros #1627

merged 1 commit into from
Jun 21, 2021

Conversation

jphickey
Copy link
Contributor

Describe the contribution
Rename CFE coverage test assert macros in ut_support.h to have consistent name prefix.

Also adds a "VOIDCALL" and "RESOURCEID_EQ" macro for logging void functions and ID checks, respectively.

Fixes #1626

Testing performed
Build and run all unit tests

Expected behavior changes
None - just renames macros for consistency, and simplifies the implementation of some (reusing existing macro where feasible).

System(s) tested on
Ubuntu

Additional context
Discussed initially in CCB on 2021-06-16, as part of discussion on #1624, then split off into a separate issue/PR.

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

@jphickey jphickey force-pushed the fix-1626-cfe-utassert-macro-names branch 3 times, most recently from f07f902 to 8c4cf23 Compare June 17, 2021 16:41
@jphickey
Copy link
Contributor Author

This should be good to go, the "Build cFE documentation" workflow issue is not related, and should be corrected by #1630

@skliper skliper self-requested a review June 17, 2021 19:04
@jphickey
Copy link
Contributor Author

After getting into some of the table services use cases, I'm finding it would be better/more useful if these macros had a pass/fail return value like the OSAL ones do. The problem, particularly with SETUP, is that there is no way to tell if your setup failed and if therefore e.g. a loop should stop.

@jphickey jphickey force-pushed the fix-1626-cfe-utassert-macro-names branch from 8c4cf23 to 0869452 Compare June 21, 2021 14:50
@jphickey
Copy link
Contributor Author

Re-requesting review, this version seems to work well and I confirmed this set of macros should be sufficient for all CFE core test cases.

Biggest difference between this and the previous set is that the "impl" functions (underneath the macro) return the pass/fail status of the test case as a bool. There are some CFE test cases, particularly in TBL, that need to break out of a loop if setup fails, and this pattern allows that to happen.

Note to reviewers, the actual changes are in ut_support.h and ut_support.c ... the changes to all other modules in this PR should be a straight search and replace just to use the new macro names, nothing else. Actual changes/improvement to test cases comes in the next PR.

@jphickey
Fix nasa#1626, rename/clean CFE coverage assert macros
9568c88 
Rename CFE coverage test assert macros in ut_support.h to have
consistent name prefix.

Adds implementation functions for completely generic signed/unsigned
comparison asserts, and wrapper macros to invoke those functions.
These functions return the pass/fail status of the assert as a bool
value, so the test case can act on the result.

Also adds a "VOIDCALL" and "RESOURCEID_EQ" macro for logging void
functions and ID checks, respectively.
@jphickey jphickey force-pushed the fix-1626-cfe-utassert-macro-names branch from 0869452 to 9568c88 Compare June 21, 2021 14:56
@jphickey
Copy link
Contributor Author

Pushed again to resolve formatting check issues ....

@astrogeco
Copy link
Contributor

astrogeco commented Jun 21, 2021 •
edited
Loading

DEVS:2021-06-21, We need a "gold standard" way of doing things.

  • the benefit of macros is that they prevents copy-paste errors
  • the "equal" macros might have confusing restrictions
  • Remove CFE_UtAssert_EQUAL, it is ambiguous
  • Open new issue for adding "true/false" and "impl" into the generic ut-assert
  • See https://github.com/nasa/cfe/issues/847 for ut-assert documentation

@astrogeco astrogeco changed the base branch from main to integration-candidate June 21, 2021 23:03
@astrogeco astrogeco merged commit 99f481d into nasa:integration-candidate Jun 21, 2021
@astrogeco astrogeco mentioned this pull request Jun 21, 2021
Merged
astrogeco added a commit to nasa/cFS that referenced this pull request Jun 21, 2021
@astrogeco
IC:2021-06-22, Add cfe assert rename PR
0271ccb 
nasa/cFE#1627, rename/clean CFE coverage assert macros
@astrogeco astrogeco mentioned this pull request Jun 21, 2021
Merged
@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Jun 23, 2021
@astrogeco
Copy link
Contributor

CCB:2021-06-23 APPROVED

astrogeco added a commit to nasa/cFS that referenced this pull request Jun 24, 2021
@astrogeco
IC:2021-06-22, cfe, osal, and coding standard
0d2cd48 
*Combines*

- nasa/elf2cfetbl#81
- nasa/tblCRCTool#52
- nasa/ci_lab#88
- nasa/sch_lab#83
- nasa/sample_app#150
- nasa/sample_lib#64
- nasa/to_lab#100

*Includes*

- nasa/cFE#1630, correct path to users guide warning log
- nasa/cFE#1621, add additional test cases for Child Tasks
- nasa/cFE#1608, Add cfe functional tests to CI
- nasa/cFE#1627, rename/clean CFE coverage assert macros
- nasa/cFE#1623, Added UT tests for cFE ES Api
- nasa/cFE#1634, Expand CDS Functional Tests.
- nasa/cFE#1633, add test log file
- nasa/cFE#1594, Event ID updates
- nasa/cFE#1624, scrub all UT_Report calls

- nasa/osal#1066, implement missing parameter/retcode test permutations

- nasa/cFS-GroundSystem#182, Add test start command script for cmdUtil
- nasa/tblCRCTool#51, add printf conversion casts

** Implement Coding Standard in CodeQL **

- nasa/cFS-GroundSystem#180
- nasa/elf2cfetbl#80
- nasa/tblCRCTool#49

- nasa/ci_lab#87
- nasa/sch_lab#79
- nasa/sample_app#149
- nasa/sample_lib#63
- nasa/to_lab#99
astrogeco added a commit to nasa/cFS that referenced this pull request Jun 24, 2021
@astrogeco @skliper
@jphickey @ArielSAdamsNASA @zanzaben @pepepr08
IC:2021-06-22, cfe, osal, and coding standard
1afe0e9 
*Combines*

- nasa/cFE#1632, v6.8.0-rc1+dev726
- nasa/osal#1079, v5.1.0-rc1+dev548

- nasa/ci_lab#88, v2.4.0-rc1+dev42
- nasa/sch_lab#83, v2.4.0-rc1+dev40
- nasa/sample_app#150, v1.2.0-rc1+dev66
- nasa/sample_lib#64, v1.2.0-rc1+dev38
- nasa/to_lab#100, v2.4.0-rc1+dev49

- nasa/elf2cfetbl#81, v3.2.0-rc1+dev30
- nasa/tblCRCTool#52, v1.2.0-rc1+dev33
- nasa/cFS-GroundSystem#183, v2.2.0-rc1+dev52

*Includes*

- nasa/cFE#1630, correct path to users guide warning log
- nasa/cFE#1621, add additional test cases for Child Tasks
- nasa/cFE#1608, Add cfe functional tests to CI
- nasa/cFE#1627, rename/clean CFE coverage assert macros
- nasa/cFE#1623, Added UT tests for cFE ES Api
- nasa/cFE#1634, Expand CDS Functional Tests.
- nasa/cFE#1633, add test log file
- nasa/cFE#1594, Event ID updates
- nasa/cFE#1624, scrub all UT_Report calls

- nasa/osal#1066, implement missing parameter/retcode test permutations

- nasa/cFS-GroundSystem#182, Add test start command script for cmdUtil
- nasa/tblCRCTool#51, add printf conversion casts

** Implement Coding Standard in CodeQL **

- nasa/cFS-GroundSystem#180
- nasa/elf2cfetbl#80
- nasa/tblCRCTool#49

- nasa/ci_lab#87
- nasa/sch_lab#79
- nasa/sample_app#149
- nasa/sample_lib#63
- nasa/to_lab#99

Co-authored-by: Jacob Hageman <skliper@users.noreply.github.com>
Co-authored-by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored-by: Ariel Adams <ArielSAdamsNASA@users.noreply.github.com>
Co-authored-by: Alex Campbell <zanzaben@users.noreply.github.com>
Co-authored-by: Jose F Martinez Pedraza <pepepr08@users.noreply.github.com>
@jphickey jphickey deleted the fix-1626-cfe-utassert-macro-names branch August 3, 2021 15:19
@skliper skliper added this to the 7.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepepr08 pepepr08 pepepr08 approved these changes

@skliper skliper skliper approved these changes

@zanzaben zanzaben Awaiting requested review from zanzaben

Assignees
No one assigned
Labels
CCB:Approved Indicates code review and approval by community CCB CCB:FastTrack
Projects
None yet
Milestone
Caelum
Development

Successfully merging this pull request may close these issues.

Consistency in CFE coverage test helper macros
4 participants
@jphickey @astrogeco @pepepr08 @skliper