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 #1784, add CFE assert macros to functional test #1790

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Aug 10, 2021

Describe the contribution
Adds the following macros to CFE assert library in cfe_assert.h:

  • CFE_UtAssert_STATUS_OK
  • CFE_UtAssert_STATUS_ERROR
  • CFE_UtAssert_RESOURCEID_EQ
  • CFE_UtAssert_RESOURCEID_UNDEFINED
  • CFE_UtAssert_MEMOFFSET_EQ
  • CFE_UtAssert_MSGID_EQ

Fixes #1784

Testing performed
Build and run all tests (including with some new test cases that use the new macros) and confirm all is working as expected.

Expected behavior changes
None right now, these are new macros that test cases are not using yet.

System(s) tested on
Ubuntu

Additional context
Provides improved feature parity with coverage test environment, gives a common macro to use for common tests/asserts, and more consistent naming convention.

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 Aug 10, 2021
@jphickey jphickey force-pushed the fix-1784-cfeassert-macros branch from 4daa2f0 to 82d4843 Compare August 10, 2021 18:13
@jphickey jphickey force-pushed the fix-1784-cfeassert-macros branch from 82d4843 to 97790b2 Compare August 10, 2021 18:19
@astrogeco astrogeco requested review from skliper and zanzaben August 10, 2021 18:34
@jphickey jphickey force-pushed the fix-1784-cfeassert-macros branch from d44b91c to 59eed74 Compare August 11, 2021 01:00
@jphickey
Copy link
Contributor Author

Updated per discussion in today's tag-up to remove SETUP and TEARDOWN macros.

Side note / food for thought -- my preference has always been to err toward including too much information in test logs, as you can always ignore the extra detail if it isn't relevant to your goals. However, you can't go the other way, and use information you don't have in the log.

Therefore, I still would prefer to keep SETUP/TEARDOWN macros, as it only affects the details in the log file - which can be ignored/discarded when the goal is to show system-level functionality. But I still do think there are cases where this detail can be useful in the log file, and you can't use it if its not there.

@jphickey
Copy link
Contributor Author

Probably also worth noting that the OSAL functional tests also have UT_SETUP and UT_TEARDOWN macros for this same purpose, and this is the same type/level of functional test. So I still think it should be there for consistency with all of our other tests.

modules/cfe_assert/inc/cfe_assert.h Outdated Show resolved Hide resolved
@astrogeco astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 11, 2021
@astrogeco
Copy link
Contributor

CCB:2021-08-11

  • Debate naming "UT_SUCCESS" macro

@jphickey jphickey force-pushed the fix-1784-cfeassert-macros branch from 59eed74 to 5b02dc9 Compare August 11, 2021 18:37
@jphickey jphickey dismissed zanzaben’s stale review August 11, 2021 18:40

Fixed in latest update

@jphickey jphickey requested review from skliper and zanzaben August 11, 2021 18:40
@jphickey
Copy link
Contributor Author

The latest update changes the name of the SUCCESS/NOT_SUCCESS asserts to CFE_UtAssert_STATUS_OK and CFE_UtAssert_STATUS_ERROR respectively.

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.

Log message update requested

modules/cfe_assert/src/cfe_assert_runner.c Outdated Show resolved Hide resolved
@jphickey jphickey force-pushed the fix-1784-cfeassert-macros branch from 5b02dc9 to 4f9d3df Compare August 11, 2021 20:03
@jphickey jphickey requested a review from skliper August 11, 2021 20:05
Adds the following macros to CFE assert library in cfe_assert.h:

- CFE_UtAssert_STATUS_OK
- CFE_UtAssert_STATUS_ERROR
- CFE_UtAssert_RESOURCEID_EQ
- CFE_UtAssert_RESOURCEID_UNDEFINED
- CFE_UtAssert_MEMOFFSET_EQ
- CFE_UtAssert_MSGID_EQ
Change cFE_FTAssert macros defined in cfe_testcase to use the
macros now provided in cfe_assert.h instead.
@jphickey jphickey force-pushed the fix-1784-cfeassert-macros branch from 4f9d3df to c2d0ee7 Compare August 11, 2021 20:07
@jphickey
Copy link
Contributor Author

Last push was for whitespace/clang-format fixup ... 7th time is the charm on this hopefully

@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Aug 11, 2021
@astrogeco astrogeco changed the base branch from main to integration-candidate August 12, 2021 13:14
@astrogeco astrogeco merged commit 490c9b7 into nasa:integration-candidate Aug 12, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Aug 13, 2021
*Combines*

nasa/cFE#1808

*Includes*
nasa/cFE#1790, Port "CFE_UtAssert_SuccessCheck" and related macros from coverage test to functional test #1784, add CFE assert macros to functional test

nasa/cFE#1779, Adds invalid id syslog to for CFE_ES_DeleteApp and CFE_ES_ReloadApp and verifies required reporting

Co-authored-by: Jacob Hageman <[email protected]>
Co-authored-by: Joseph Hickey <[email protected]>
astrogeco added a commit to nasa/cFS that referenced this pull request Aug 17, 2021
**Combines**

nasa/cFE#1808, v6.8.0-rc1+dev873

**Includes**

- nasa/cFE#1790, Port "CFE_UtAssert_SuccessCheck" and related macros from coverage test to functional test #1784, add CFE assert macros to functional test
- nasa/cFE#1779, Adds invalid id syslog to for CFE_ES_DeleteApp and CFE_ES_ReloadApp and verifies required reporting
- nasa/cFE#1785, Stop memory leak & add cds size test.
- nasa/cFE#1765, Mark read only inputs as const
- nasa/cFE#1804, Check resource ID idx is less than max
- nasa/cFE#1778, Add ES Application Behavior Functional Tests
- nasa/cFE#1801, Update CFE_ES_RunLoop documentation
- nasa/cFE#1745, Add Message Api Functional Test
- nasa/cFE#1798, update in/out status and nonnull/nonzero tags
- nasa/cFE#1783, Add External Time Source Functional Tests
- nasa/cFE#1800, Add Perf API functional tests

Co-authored-by: Jacob Hageman <[email protected]>
Co-authored-by: Joseph Hickey <[email protected]>
Co-authored-by: Alex Campbell <[email protected]>
Co-authored-by: Niall Mullane <[email protected]>
Co-authored-by: Paul <[email protected]>
astrogeco added a commit to nasa/cFS that referenced this pull request Aug 17, 2021
**Combines**

nasa/cFE#1808, v6.8.0-rc1+dev873

**Includes**

- nasa/cFE#1790, Port "CFE_UtAssert_SuccessCheck" and related macros from coverage test to functional test #1784, add CFE assert macros to functional test
- nasa/cFE#1779, Adds invalid id syslog to for CFE_ES_DeleteApp and CFE_ES_ReloadApp and verifies required reporting
- nasa/cFE#1785, Stop memory leak & add cds size test.
- nasa/cFE#1765, Mark read only inputs as const
- nasa/cFE#1804, Check resource ID idx is less than max
- nasa/cFE#1778, Add ES Application Behavior Functional Tests
- nasa/cFE#1801, Update CFE_ES_RunLoop documentation
- nasa/cFE#1745, Add Message Api Functional Test
- nasa/cFE#1798, update in/out status and nonnull/nonzero tags
- nasa/cFE#1783, Add External Time Source Functional Tests
- nasa/cFE#1800, Add Perf API functional tests

Co-authored-by: Jacob Hageman <[email protected]>
Co-authored-by: Joseph Hickey <[email protected]>
Co-authored-by: Alex Campbell <[email protected]>
Co-authored-by: Niall Mullane <[email protected]>
Co-authored-by: Paul <[email protected]>
@jphickey jphickey deleted the fix-1784-cfeassert-macros branch August 17, 2021 20:26
@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.

Port "CFE_UtAssert_SuccessCheck" and related macros from coverage test to functional test
4 participants