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

CFE_EVS_SendEvent stub should provide full context #603

Closed
gilliganeric opened this issue Apr 10, 2020 · 11 comments · Fixed by #713 or #729
Closed

CFE_EVS_SendEvent stub should provide full context #603

gilliganeric opened this issue Apr 10, 2020 · 11 comments · Fixed by #713 or #729
Assignees
Labels
enhancement Priority: Mission Feature or bug related to stakeholder needs unit-test
Milestone

Comments

@gilliganeric
Copy link

gilliganeric commented Apr 10, 2020

Describe the bug
The CFE_EVS_SendEvent stub for ut-assert does not provide its full context. It only provides the first argument, EventId, providing no way to for unit tests to verify the correct EventType or event message text was sent.

To Reproduce
Steps to reproduce the behavior:

  1. See the CFE_EVS_SendEvent stub. Only EventID is registered with the context and copied.

Expected behavior
The CFE_EVS_SendEvent stub should provide its full context for unit testing purposes, including the EventID, EventType, and the event message string. My preference would be that the event message text provided would not include the format specifiers and instead be the resulting string with the format specifiers replaced with the appropriate text. I've been convinced and also talked myself out of that preference.

Code snips

int32 CFE_EVS_SendEvent(uint16 EventID,
                        uint16 EventType,
                        const char *Spec,
                        ...)
{
    int32 status;

    UT_Stub_RegisterContext(UT_KEY(CFE_EVS_SendEvent), &EventID);
    status = UT_DEFAULT_IMPL(CFE_EVS_SendEvent);

    if (status >= 0)
    {
        UT_Stub_CopyFromLocal(UT_KEY(CFE_EVS_SendEvent), (uint8*)&EventID, sizeof(EventID));
    }

    return status;
}

System observed on:

  • cFE 6.7.11

Additional context
n/a

Reporter Info
Eric Gilligan NASA/GSFC-5820

@astrogeco astrogeco added the Priority: Mission Feature or bug related to stakeholder needs label Apr 10, 2020
@gilliganeric
Copy link
Author

Thinking about this more, I'd also be fine with the Context getting the string as passed into CFE_EVS_SendEvent with the format specifiers still in it, as long as the extra arguments are put into the Context as well.

@skliper
Copy link
Contributor

skliper commented Apr 10, 2020

I think @jphickey has examples for getting format and arguments.

@jphickey
Copy link
Contributor

Yes, this stub will need an update to pass through more context if UT test cases need this.

The framework has the facility to do this, via the UT_DefaultStubImplWithArgs routine, which can pass through the full context in a va_list to a hook implementation. Then in the hook implementation one can do whatever is needed.

I'd still recommend against passing it to [v]snprintf, as that is now crossing the bridge of no longer validating the direct outputs of the unit, but rather mimicking/duplicating something that the FSW might do and validating that. Very different thing. (stubs shouldn't duplicate FSW logic).

@gilliganeric
Copy link
Author

I agree. Disregard that preference in the original description.

Would this look like ArgPtr's last element in the hook function's UT_StubContext_t being a va_list containing the variable number of arguments?

@asgibson
Copy link
Contributor

I completely agree with not passing it to [v]snprintf, but I would like to see the default implementation of the CFE_EVS_SendEvent stub make the arguments available for checking without having to use a hook.

@jphickey
Copy link
Contributor

The "Fixed" arguments, up to and including the spec string, can be registered using calls to UT_Stub_RegisterContext().

The use va_start to capture all the remaining variable arguments, and invoke UT_DefaultStubImplWithArgs with va list as the final argument, then do va_end after it returns.

For an example of a stub that uses this mechanism, see OS_printf in: https://github.com/nasa/osal/blob/master/src/ut-stubs/osapi-utstub-printf.c

Usage with hook function example is in sample_lib: https://github.com/nasa/sample_lib/blob/master/unit-test/coveragetest/coveragetest_sample_lib.c

@astrogeco astrogeco added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 14, 2020
@skliper
Copy link
Contributor

skliper commented Apr 15, 2020

CCB 20200415 - Ok to add and pass it through

@skliper skliper added enhancement and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 15, 2020
@astrogeco astrogeco added CCB:Ready Ready for discussion at the Configuration Control Board (CCB) CCB - 20200415 Priority: Mission Feature or bug related to stakeholder needs and removed Priority: Mission Feature or bug related to stakeholder needs CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 20, 2020
@skliper
Copy link
Contributor

skliper commented May 12, 2020

@gilliganeric - can you confirm #673 does what you need, or clarify what would still be necessary?

@skliper skliper added this to the 6.8.0 milestone May 12, 2020
@ericgilligan-nasa
Copy link

It looks like #673 provides the Event Type and Spec, but not the variable arguments as a va_list as described by @jphickey earlier. Is that correct?

@skliper
Copy link
Contributor

skliper commented May 14, 2020

Correct, we can leave this open until that's implemented:

The use va_start to capture all the remaining variable arguments, and invoke UT_DefaultStubImplWithArgs with va list as the final argument, then do va_end after it returns.

For an example of a stub that uses this mechanism, see OS_printf in: https://github.com/nasa/osal/blob/master/src/ut-stubs/osapi-utstub-printf.c

Usage with hook function example is in sample_lib: https://github.com/nasa/sample_lib/blob/master/unit-test/coveragetest/coveragetest_sample_lib.c

@jphickey
Copy link
Contributor

Ahh yes, sorry I overlooked that during my review of the change. If you want to have the actual data available in your hook this needs to use the UT_DefaultStubImplWithArgs() function to pass them all through.

jphickey added a commit to jphickey/cFE that referenced this issue May 21, 2020
Add a va_list to capture the full set of variable arguments
on all CFE_EVS event calls.
jphickey added a commit to jphickey/cFE that referenced this issue May 21, 2020
jphickey added a commit to jphickey/cFE that referenced this issue Jun 1, 2020
Add a va_list to capture the full set of variable arguments
on all CFE_EVS event calls.

Also add va_list for CFE_ES_WriteToSyslog
astrogeco added a commit that referenced this issue Jun 2, 2020
Fix #603, Full context info for EVS event stubs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Priority: Mission Feature or bug related to stakeholder needs unit-test
Projects
None yet
6 participants