Unit test stubs need to be kept in sync with their respective real implementations

[skliper]

The "unit-test" code implements stub functions for all function calls which are //not// under test. In addition to stubs for the 6 core CFE functions, there are two other stub files:

• ut_bsp_stubs.c
• ut_osapi_stubs.c

The difficulty here is that both the API/prototype of all the functions as well as the real implementation of those functions come from an external source, specifically the PSP library and the OSAL library. If either the OSAL or PSP changes, the corresponding change must be made to the CFE stub files to keep them in sync. Futhermore, if another project tries to use the updated stub files with an older version of OSAL, the build will now break due to the mismatched definitions.

It would be nice to simply make a rule that a defined API is never going to change, but that is not practical.

Solution 1

Utilize a preprocessor macro to "tune" the function definitions in the stub files to match what the OSAL/PSP should be for that particular version

Advantages: Can be implemented right away without any synchronized change to OSAL/PSP libs.

Disadvantages: Does not scale. Code can get pretty messy with lots of #ifdef's if it changes more than once. Still requires CFE to "know" the OSAL/PSP prototypes so breaks the independence of the two libraries.

Solution 2

Move the stub implementation to the same component that provides the real implementation. For this, the ut_bsp_stubs.c file would be relocated to the PSP library and the ut_osapi_stubs.c file would be relocated to the OSAL library.

The PSP and OSAL builds would provide a separate UT stub product (a library) that the CFE unit test could link with.

Advantages: Arguably a more logical place for the stub code. Reduces need for synchronized changes in the future, better forward/backward compatibility between versions. Also, other (non-CFE) users of OSAL library could also use stub functions.

Disadvantages: Requires a synchronized change to get started (solution 1 could help here). Also would only work using CMake makefiles, although the GNU makefiles could probably be updated accordingly as well.

[skliper]

Imported from trac issue 32. Created by jphickey on 2015-01-26T10:47:44, last modified: 2019-03-05T14:57:55

[skliper]

Trac comment by jphickey on 2015-01-26 16:00:02:

Branch "trac-32-update_osapi_stubs" pushed as interim fix -- commit [changeset:f1488abe]

This updates the prototypes using the OSAL_API_VERSION macro as described by "Solution 1" above.

This is done as an interim fix, otherwise the code would not build after merging the OSAL ticket #44. With this change the code will build as expected.

The preferred approach would be "Solution 2" above, as this will keep the code much cleaner.

[skliper]

Trac comment by glimes on 2015-02-10 12:41:59:

Solution 3 would be to have the test call the live OSAL or PSP code. Do this enough and you can discard the stubs and eliminate all that duplicate maintenance. This may not always be possible, but may be easy to do in a surprisingly large number of tests, and might actually improve them.

But that's probably something to back-pocket for down the road.

[skliper]

Trac comment by jphickey on 2015-02-10 12:54:05:

Yes I agree it is generally better to use the real thing, but I can see quite a few instances where this might not be feasible. The goal of unit tests is to exercise all code paths.

Some "real" PSP functions do not fail (they never return anything other than CFE_PSP_SUCCESS in the active target's implementation) however CFE would rightfully check for an error response anyway since it supports many different PSPs and they aren't ALL implemented this way. Even for functions that do return error codes it might not be possible to get the specific combination of failures required to exercise a specific path through a test target function.

So in order to get code coverage in the unit tests, I see the stub functions as a necessary evil. But I also think they can be done much cleaner and made more manageable.

Keep in mind that the purpose of the black box (Lua) test scripts is exactly this purpose, to exercise the code with the //live// PSP on a //live// target to see what happens in the real world, or at least closer to the real world than a unit test is.

[skliper]

Trac comment by jphickey on 2015-04-06 11:30:08:

This is ready for review/merge

There are now two fix branches pushed for this issue:

• [changeset:f1488ab] represents "Solution 1". It can be merged if deemed useful... does not hurt anything, and it preserves the old makefile build.
• [changeset:b8581e6] represents "Solution 2". This does some refactoring of the code in ut_osapi_stubs.c to allow for a stub implementation of the OSAPI calls in OSAL. The make script is changed so as to prefer the OSAL implementation if available, so this should work with or without the corresponding OSAL change.

[skliper]

Trac comment by sstrege on 2015-04-06 12:49:01:

How was the number "40192" chosen for the OSAL API version?

Wouldn't the OSAL API changes require changes to all instances where these functions are called in the cFE?

[skliper]

Trac comment by jphickey on 2015-04-06 13:24:01:

Regarding the OSAPI version number "40192" -- in short, it is arbitrary. It is actually dependent on the corresponding OSAL fix, and really a placeholder for the correct number which will be determined based on the OSAL merges.

Typically in many projects the "next" version that is not yet released will use a very high value for the "revision" field. In this case I am simply referring to the version of OSAL that makes those corresponding prototype changes as "4.1.92" (i.e. some precursor to 4.2.0).

Side note -- the ugliness of the version-check macros in [changeset:f1488ab] IMO is one of the main reasons this stub code needs to be moved into OSAL - it avoids this problem in a much cleaner way.

Also, this does //not// impact anywhere that these functions are called. The prototype change is benign for users and the difference is transparent. It is only places where the function is implemented that it becomes a problem. In this case it highlights the more general problem that cFE is implementing functions for which it does not define the API.

[skliper]

Trac comment by glimes on 2015-04-07 13:11:32:

Tested changesets [changeset:f1488abe] [changeset:b8581e69] as part of the ic-2015-03-10 merge.

[skliper]

Trac comment by glimes on 2015-04-07 14:21:49:

Will retain the [changeset:f1488abe] changeset to retain compatibility
with the Makefile style builds, and the the [changeset:b8581e6] changeset
to provide CMake support. [edit: fixed error in 2nd changeset number]

[skliper]

Trac comment by glimes on 2015-04-13 15:06:50:

Part of integration candidate 2015-03-10,
committed to cFS CFE Development branch on 2015-04-10
as part of merge [changeset:7d6f6d0].

[skliper]

Trac comment by glimes on 2016-02-16 13:16:45:

Susie confirmed these tickets have been approved for CFE 6.5

[skliper]

Trac comment by jhageman on 2019-03-05 14:57:55:

Milestone renamed