search

nasa / cFE

The Core Flight System (cFS) Core Flight Executive (cFE)
Apache License 2.0
408 stars 200 forks source link

UT stubs for CFE_SB_TimeStampMsg and CFE_SB_SetMsgTime very inconsistent #1026

Open jphickey opened 3 years ago

jphickey commented 3 years ago

Describe the bug These two functions do almost the same thing in FSW but the UT stubs have entirely different side effects. CFE_SB_TimeStampMsg stores the Message pointer in a UT buffer, but the CFE_SB_SetMsgTime stores the given time in the UT metadata for the message.

Expected behavior These should be more consistent. The CFE_SB_TimeStampMsg should update the metadata like CFE_SB_SetMsgTime does because that's what FSW expects.

System observed on: Code Inspection (N/A)

Additional context Noticed this as part of #937 review/discussion.

Probably also impacted by #998 .... perhaps we can just focus on getting stubs for the CFE_MSG module replacements right.

We should get away from storing the message pointer in ANY of these stubs - because it references internal data objects and the life cycle of this object may not be persistent (i.e. it could be on the stack) so storing the pointer passed to any of the SB message functions is probably not a good idea. The newer method of creating a UT "metadata" object associated with the message pointer is better, because it has a lifespan of the unit test case - so guaranteed to be still valid when the function under test returns.

Reporter Info Joseph Hickey, Vantage Systems, Inc.

jphickey commented 3 years ago

Ping @asgibson

skliper commented 3 years ago

Can we transition away from dependency on the metadata? I'd much rather NOT assume a "setter" stub sets up the return data for a "getter" stub. Note CFE_SB_SetMsgTime goes away w/ #777. I'd prefer the pattern implemented by the MSG module stubs... get the value from the buffer.

skliper commented 3 years ago

The justifications for this suggestion is that the setter/getter relationship is implementation dependent for many APIs, so it's not easy to abstract the metadata sufficiently. For example getting time from a cmd packet should return an error in some cases (wrong message type), or a valid time in others (for implementations that have time in cmd). The coverage tests instead should be explicitly providing the return values for all getters they care about to exercise the desired paths.

EDIT - also #777 removes all dependencies on the metadata except for Get/SetUserDataLength, and I'd like for this dependency to be removed also. This approach (unit test sets up returns) worked well for all the services and lab apps...

asgibson commented 3 years ago

I agree that the stubs should be consistent. My view is that stubs should only:

  1. Record the arguments received.
  2. Return the forced return value for non-void functions.
  3. Setup the hooks, so that any further implementation can be accomplished when required

This is biased to the fact that I want the stubs to be the lowest level faking of functions, because I use them in the smallest possible unit tests for production code. I understand that ut-assert is used for higher level testing and others may need this kind of metadata access/recording. My true preference would be for both "basic stub" and commonly used "hook implementations" to be made available by ut-assert. Barring that happening, consistency is best -- it should be all one way or the other, instead of one way here and another there (this has frustrated me on several occasions).

Interesting to note that CFE_SB_SetMsgId is a third style, doing both only on success: https://github.com/nasa/cFE/blob/32f3deeebf136ac427a6b779cb9e4854e73826f1/fsw/cfe-core/ut-stubs/ut_sb_stubs.c#L543-L557

skliper commented 3 years ago

Caution that CFE_SB_SetMsgId is deprecated. Should shift to using the CFE_MSG_SetMsgId (and other MSG module APIs). The old CFE_SB_SetMsgId stubs will go away soon.

asgibson commented 3 years ago

Understood, it was really just an aside to the fact that there may be more than the 2 ways that stubs are implemented that were already mentioned.

skliper commented 3 years ago

Understood, it was really just an aside to the fact that there may be more than the 2 ways that stubs are implemented that were already mentioned.

Copy, and I agree the variety of implementations is a source of frustration. Seems like many of these crept in to minimize required test changes, whereas I would have preferred updating the tests. Common, simple stub behavior is definitely preferable. I have no objections to adding common hooks that could be accessible by apps (a hook library of some sort). ut_support.c in cFE does this and more internally for cFE testing but it's probably overkill and likely has too much custom behavior.