ut_assert header files not included in 'make osalguide'

[asgibson]

Describe the bug When building the osal guide with 'make osalguide' the ut_assert header files are not included

To Reproduce Steps to reproduce the behavior:

1. make osalguide
2. browse guide, no ut_assert files present

Expected behavior ut_assert files included in osal guide

Code snips https://github.com/nasa/cFE/blob/06c08268f2d7a7fe72aceec8c33201ce6ccba2f4/cmake/mission_build.cmake#L204-L208

System observed on: RHEL 7.6

Additional context Adding: "${osal_MISSION_DIR}/ut_assert/inc/*.h" after osconfig-example in the above code includes ut_assert headers in the guide.

Reporter Info Alan Gibson NASA GSFC/587

[skliper]

I'd actually like to make the ut_assert guide as stand alone.

[skliper]

Note there's also additional macros/support in the cfe/fsw/cfe-core/unit-test directory ut_support.c and ut_support.h files.

[asgibson]

I'd actually like to make the ut_assert guide as stand alone.

I would be good with that as a solution.

Note there's also additional macros/support in the cfe/fsw/cfe-core/unit-test directory ut_support.c and ut_support.h files.

Thank you for that. Are these meant for use outside of the cfe specific unit tests? Should app unit tests have access to these files?

[skliper]

Are these meant for use outside...

Great question. Doesn't look like we included them for the cfe test apps, yet it's where a few potentially useful macros and APIs are (although some look cfe specific that you'd probably want to avoid). @jphickey @CDKnightNASA - what was the thinking on app use of ut_support? Limit app unit tests to just ut_assert?

[asgibson]

If there are significant helper functions within these files that are generic and would assist unit testers by reducing time and effort, I would be for allowing those to be used outside of the cfe unit tests. These types of functions should be moved to ut_assert for wider use. If though as you say,

some look cfe specific

those should not be exposed outside of cfe unit tests.

I have not had the time to look over what items would be candidates for this action.

[jphickey]

My opinion is to not expose ut_support to anything else outside CFE UT. It is really just some glue logic to make old (i.e. CFE 6.5.0 and before) test cases work with a common UT assert and not rewrite them. Some elements are quite hacky to make stuff work - they aren't particularly good examples of the right way to do things anyway. Ideally I'd like to see these files shrink in the future so that not even CFE uses them and they can be removed.

[skliper]

Where I'm confused is that's where the ASSERT macros were added by @CDKnightNASA, which I thought were meant for general use. Should they be moved to osal/ut_assert?

[jphickey]

I thought those were specific to the CFE (internal) unit test paradigms - for the purpose of removing redundant patterns within the CFE unit test case implementation(s).

I'm fine with these files staying around for implementation of common patterns within the CFE core UT - I retract my previous comment about complete removal... they do certainly provide a useful spot for common code within CFE core tests. But they could use some cleanup (I have a future PR coming soon to help on that front, too).

But I'm not convinced they are generic enough, nor the patterns are complex or commonly needed enough to justify the cost of another "public" (i.e. cross-module, backward compatible/version stabilized) API. I'm unsure that that the developer time saved by direct reuse of these (small/trivial) functions meets the cost/benefit of developer time spent maintaining the API, doing documentation and deprecation/backward compatibility, etc that would be required if they were publicly exposed for other modules to use.

I see them more as a template/example that the other unit tests could clone and own as local functions. Again, that's because, for the most part, there is no real logic or algorithms in them - just a collection of commonly-used patterns or "convenience routines" which consolidate a bunch of code that would otherwise be cut and pasted into every test case.

But at the end of the day, it's just a judgement call if there is enough "meat" within this library to justify the cost of documenting and stabilizing its API to make it public. If consensus is that it is justified, it can certainly be done, but my opinion is that it (still) isn't quite there. (but of course that's just my opinion)

[CDKnightNASA]

Where I'm confused is that's where the ASSERT macros were added by @CDKnightNASA, which I thought were meant for general use. Should they be moved to osal/ut_assert?

I see these utility macros as being limited to cfe unit test code. OSAL/PSP/apps might want a different paradigm (they could certainly replicate the macros if they so choose.)

[CDKnightNASA]

Where I'm confused is that's where the ASSERT macros were added by @CDKnightNASA, which I thought were meant for general use. Should they be moved to osal/ut_assert?

I see these utility macros as being limited to cfe unit test code. OSAL/PSP/apps might want a different paradigm (they could certainly replicate the macros if they so choose.)

Also one other drawback to making these public is we'd want to add a namespace prefix like CFE_UT_blahblah that makes code less compact and readable. In my coding practice I like to use "private" macros and functions (static fn's) whenever possible without namespace prefixes.

[asgibson]

I see these utility macros as being limited to cfe unit test code. OSAL/PSP/apps might want a different paradigm (they could certainly replicate the macros if they so choose.)

If there is a generic concept that anyone could use I would be in favor of elevating that for use elsewhere, but cfe specific functionality should stay put. I cannot form a good opinion on any of the cfe's ut_support functionality being generic because I have not seen what is available; I have had no reason, until now, to examine the cfe specific unit tests in depth.

This brings to mind a question: Why is ut_assert not a separate project?

[jphickey]

Why is ut_assert not a separate project?

In the past it was more tightly coupled to the OSAL BSP. In theory it is now modular enough that it could be forked off from OSAL, but then it would break the concept of standalone/baseline OSAL that could be downloaded/cloned and built on its own without any other dependencies. As OSAL needs UT assert for its own tests, and CFE/PSP all need OSAL, including this code within OSAL means its guaranteed to always be there. So even though it isn't really part of OSAL proper, having it reside in that repo is still convenient and simplifies the dependencies.

[skliper]

Note ut_assert documentation is now included in the detaildesign (make doc): https://github.com/nasa/osal/blob/fafb045e7a5311e19affe0ddab724f1604e739f4/docs/osal-detaildesign.doxyfile.in#L16

Could still add stand alone, so leaving this open if someone wants to add it.