Sample tests should not use CFE_SB_InitMsg stub to initialize a message

[asgibson]

Describe the bug
CFE_SB_InitMsg is used in coveragetest_sample_app.c to initialize a message so that the CFE_SB_SendMsg stub has a valid message to use. This is not proper form. A unit test should not be calling a stub to provide this setup. Stubs are to be used by production code in place of the actual calls in order for a unit test to isolate the code under test, they should not be used as utility functions for the unit tests.

To Reproduce
See Code snips.

Expected behavior
Provide a ut_assert utility that has the functionality to create a cFS message for use by a unit test.

Code snips

sample_app/unit-test/coveragetest/coveragetest_sample_app.c

Lines 414 to 417 in c1348d6

	CFE_SB_InitMsg(&SAMPLE_AppData.HkBuf.MsgHdr,
	SAMPLE_APP_HK_TLM_MID,
	sizeof(SAMPLE_AppData.HkBuf),
	true);

System observed on:
RHEL 7.6

Additional context
This is related to a cFE issue. If the CFE_SB_SendMsg stub worked as described in there, most situations where this is used would not even be required (just verifying the message pointer instead of the actual message itself).
nasa/cFE#818

Reporter Info
Alan Gibson NASA GSFC/587

[jphickey]

I have to disagree here -- not clear what makes this "not proper form" ....

The test routine is invoking the stub routine directly because the function under test relies on this having been called during init, so the test case must replicate that state. I don't see how providing an alternate API to set up the message state is better than simply calling the CFE_SB_InitMsg stub directly. It would do the same thing, wouldn't it? Why make a second function to do the same thing?

[jphickey]

FWIW I took a closer look and I'm not sure if its even necessary anymore with the current version of the SB stubs. It might have been rendered moot by recent changes to SB. Did you try to run the test without this call?

[asgibson]

No, but I had not thought of doing that. To which change are you referring?

[asgibson]

Also, does that mean changes to ut_assert are not being reflected in the sample_app tests?

[asgibson]

I have to disagree here -- not clear what makes this "not proper form"

Yeah, "not proper form" is ambiguous, I will elaborate below.

The test routine is invoking the stub routine directly because the function under test relies on this having been called during init, so the test case must replicate that state. I don't see how providing an alternate API to set up the message state is better than simply calling the CFE_SB_InitMsg stub directly. It would do the same thing, wouldn't it? Why make a second function to do the same thing?

I would say this is pedantic, but a "stub" of a production code call is written for use by the code under test during testing, not for the unit test to call it directly. The way I was taught to do unit testing was to isolate the code under test. The use of the stub implementation to provide a service to the unit test appears to be only done out of convenience than an actual design strategy.

What happens when the stub changes and what the test needs in a situation like this diverge? At the very least have a test utility call that uses CFE_SB_InitMsg and then that variability is built in. However, if this has changed as you said in your FWIW then that is helpful. Which change specifically addresses this? Also, where is the best location to find ut_assert documentation?

[asgibson]

Also, for me, unit tests provide documentation about the production code and I try to get them to read like a story. I realize that it is not the same for many people. When I'm looking at a unit test and I see a call that looks to be directly into the production API, but is not the code under test I begin to question what is being done and why.

[jphickey]

Stubs are always providing a "Service" to the test routine. The two modules (test executive + stub routines) work in tandem and the test executive will always depend on the stubs having some particular implementation (precondition/postcondition) - even if its just that it returns the requested/configured return code, its still a contract. I just don't see how invoking a stub directly to trigger the desired effect this is any different than configuring it via some other API.

The "stub updates" I was referring to earlier were part of nasa/cFE#618, basically this decoupled the SB getter/setter routines from the message buffer itself - making them independent. (i.e. so a "getter" can output any desired value without actually having to set that value inside a message buffer somehow). The problem at the time was that the stubs effectively re-implemented a CCSDS (v1) header accessor, which broke when FSW was configured for extended headers. But it may have also rendered this initialization moot too.

[jphickey]

Also worth noting is that the current stub library implementations intentionally try not to have a separate config/control interface. They share the public API from FSW, without extending it. (other than the state kept in the low level UtAssert stub API, of course). It was done this way to avoid having to maintain yet another public interface for stubs beyond the FSW interface.

[asgibson]

I think of the contract as more "when you are called from the code under test do this thing for me."

To be honest though, the problem is not in this particular usage. It really stems from this: nasa/cFE#818 (comment). The call to CFE_SB_InitMsg is an artifact of this problem.

The Test_SAMPLE_ReportHousekeeping test as written does not test that CFE_SB_SendMsg was called at all. It only tests that the memory in the setup location is equal to what is expected; again, this is pedantic, but how do we know that the call to CFE_SB_SendMsg is what set those memory locations? The test does not verify this as written, the only thing this test verifies is that the CFE_SB_SendMsg stub is working as expected (which in a round about way verifies the call, but unit tests should be direct not "round about"). I could change the stub to always save the same memory pattern making sure that CmdCounter and ErrCounter end up as 22 and 11. Of course, that will not happen (who would do such a thing?), but it underlines why the test is invalid.

This issue is akin to passing in variable arguments with a string, then testing that the string was built as expected by the stub which is not the code under test. However, if we test that the expected message address was received at CFE_SB_SendMsg we are testing that the CUT sent that value and I don't even need there to be an actual message to test that, just the pointer, thus removing the need to CFE_SB_InitMsg. This is the behavior of the code under test (what we really want to test), it has no knowledge of what is at the address, it just sends it. What CFE_SB_SendMsg does with that value is up to its unit tests to verify.

[skliper]

Note you can check how many times a stub was called in your test. See UT_GetStubCount(). Doesn't solve the address problem, but you can tell how many times a unit under test calls a stub. I'll check to see how many things in the framework break if we update SendMsg to return the address.