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 #596, scrub all UT_Report calls #1624

Merged
merged 10 commits into from
Jun 23, 2021

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Jun 16, 2021

Describe the contribution
Scrub through all UT reporting calls and replace with preferred macro where possible.

  • All calls to check status/return code are replaced with macro that logs all arguments and return value
  • All calls that involved multiple conditions AND'ed together are replaced with individual asserts on each condition.

Fixes #596
Fixes #470

Testing performed
Build and run unit tests, confirm logging is as intended, and coverage level is not reduced

Expected behavior changes
Better, more complete context information is logged into the file
All individual assert conditions are logged separately (no more checking of multiple AND'ed values/conditions in a single report)

System(s) tested on
Ubuntu

Additional context
This also removes all the "free-form" info that was in the logs, since it wasn't really providing value (a developer will go to the source file/line number and see full context, nor was it complete or consistent enough to provide any value for cross referencing or documentation).

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

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jun 16, 2021
@jphickey
Copy link
Contributor Author

Pushing as draft for initial review/discussion, only "ES" module is done so far.

@jphickey jphickey force-pushed the fix-596-ut-report-macros branch 2 times, most recently from 204d2a9 to c55dba2 Compare June 16, 2021 15:24
@astrogeco
Copy link
Contributor

CCB:2021-06-16 APPROVED

  • There's a Ressource ID equal macro in cfe_test.h
  • Should we take out the notice of how the test failed?
  • What to do with the macros' prefixes? Keep them, having the prefix indicates that it is not part of the generic UTAssert set

@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Jun 16, 2021
@jphickey jphickey force-pushed the fix-596-ut-report-macros branch from c55dba2 to 90b9c39 Compare June 22, 2021 13:43
jphickey added 8 commits June 22, 2021 09:46
Update ES coverage test to use preferred macros.

Adds dedicated assert macros for checking fixed-length
string buffers, and for checking memory offsets.

Also adds an improved implemention of the syslog/printf check
which filters out newlines (keeps log more parseable).
Update EVS coverage test to use preferred macros
Update SB coverage test to use preferred macros.

Adds a dedicated assert macro for checking SB MsgId values.
Update MSG coverage test to use preferred macros
Update SBR coverage test to use preferred macros
Update TBL coverage test to use preferred macros
Update TIME coverage test to use preferred macros
Update FS coverage test to use preferred macros.
@jphickey jphickey force-pushed the fix-596-ut-report-macros branch from 90b9c39 to e313573 Compare June 22, 2021 13:46
@jphickey
Copy link
Contributor Author

Updated for review. This is based on the current "integration-candidate" (pending final baseline) but otherwise is ready to go.

@jphickey jphickey requested review from skliper and pepepr08 June 22, 2021 13:49
Clean up the assert functions and macros which are no longer
used after updating all coverage tests to use preferred macros.
@jphickey jphickey force-pushed the fix-596-ut-report-macros branch from e313573 to 890cc87 Compare June 22, 2021 15:10
Add required coverage test cases to achieve 100% line coverage in FS
@jphickey jphickey force-pushed the fix-596-ut-report-macros branch from dd849d8 to 442634d Compare June 22, 2021 20:38
@jphickey
Copy link
Contributor Author

Added commit to fix #470 onto this PR to avoid conflict/baseline issue, as the FS test changes would not merge cleanly with the other scrub.

This gets 100% line and 100% function coverage, and about 96% branch coverage. There are 5 untested branches total, 4 of which are "Endian Check" therefore fixed in hardware (not possible to change except by running on other arch). The last flagged item was the while loop with two OR'ed conditions, they typically both become false at the same time, there is never a way in the current code that only the second condition will be false, but I prefer to keep both stop conditions just in case.

@skliper
Copy link
Contributor

skliper commented Jun 23, 2021

@astrogeco - AFAIK this should either be merged with IC or a discussion topic at the CCB?

@astrogeco
Copy link
Contributor

Put it on the agenda, didn't merge because of draft status

@astrogeco
Copy link
Contributor

CCB:2021-06-23

  • LESSON LEARNED: do not use a single assert for multiple things. This obfuscates things if there's actual problems. Use one condition per assert!

@jphickey jphickey marked this pull request as ready for review June 23, 2021 16:30
@astrogeco astrogeco changed the base branch from main to integration-candidate June 23, 2021 16:31
@jphickey jphickey merged commit 0701923 into nasa:integration-candidate Jun 23, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Jun 23, 2021
nasa/cFE#1624, scrub all UT_Report calls
astrogeco added a commit to nasa/cFS that referenced this pull request Jun 24, 2021
*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
*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 <[email protected]>
Co-authored-by: Joseph Hickey <[email protected]>
Co-authored-by: Ariel Adams <[email protected]>
Co-authored-by: Alex Campbell <[email protected]>
Co-authored-by: Jose F Martinez Pedraza <[email protected]>
@jphickey jphickey deleted the fix-596-ut-report-macros 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
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit test - split "AND"-ed conditionals into separate asserts Incomplete coverage test for src/fs
4 participants