Update UT stubs/support such that user registered hook/return value override/etc skips the rest of the default stub logic #832

skliper · 2021-02-25T16:50:29Z

Is your feature request related to a problem? Please describe.
Some logic implemented in stubs after the hook call is dependent on a status of OS_SUCCESS or positive, which is overly restrictive for a stub user that wants full control stub behavior (ability to return a positive and explicitly control implementation)

Describe the solution you'd like
Provide a mechanism to skip stub logic if user has registered an "override" action.

Describe alternatives you've considered
None

Additional context
User requested

Requester Info
Jacob Hageman - NASA/GSFC

Ping @asgibson @jphickey

Added a new hook set method for override Added a getter method for stubs to check for override Added a global value for tracking current override setting Set global value to false at start and during reset call Added implementation of setting hook with stub override Added getter implementation

This reverts commit 2d6801f. Should not have committed to my main branch. Made a different branch, moved changes there. Will merge to my main later.

asgibson · 2021-03-01T15:34:41Z

My apologies for the two misfires above. The first, I did not want to use my own main branch, whereas the second I started from the wrong main commit from osal. I have fixed both issues and will post my concept when I finalize the test outputs today.

asgibson · 2021-03-05T21:26:08Z

Took me a little longer, but the result is better. My quick fix was flawed because it did not use the UT Entry, but it does that now. I also was going to add the Va version, but as I could not find a suitable test subject (if someone has one let me know and I will add it back). I ran a test suite against the changes. First with tests that show the control, then with the update that shows the only time stub behavior changes is when the override is requested. There will be more details in the pull request.

Removed UT_DoSetOverride. Due to the Va function not being adopted, there is only 1 use of this code and it does not require encapsulation. Removed in defference to not adding more functions.

Added function to check for override Added set hook function to provide override Created new entry type for stub override Implemented standard override function A Va override version appears unlikely to be used Implemented public access to override check function.

jphickey · 2021-04-12T20:18:49Z

Looking at this one more closely now... The issue I have is that part of the "rest of the default stub logic" isn't always so easily skippable. Some of that logic is intended to handle the return code translation. It is easy to skip all remaining logic when the return type is int32 because its just a pass through. However for non-int32 return values we still must return something, and unfortunately the current hook prototype is assuming int32 so it needs some sort of translation for a non-integer return value, I don't see a way around that.

So in short, while I think this is a good improvement to allow this (and probably something we should do!) ... it will likely require an update to every single stub routine. Luckily I think it can be backward compatible so it doesn't have to break existing tests, and it will be confined to coverage test domain (no FSW change). But it still is going to be a large change.

jphickey · 2021-04-12T20:29:59Z

My recommendation/proposal here is to convert all the existing stubs to separate the "Default" logic into a separate function, that itself can be a similar to a hook function, but modified (perhaps) to better handle the different return types rather than assuming int32 like the current hook prototype does.

This way, registering a replacement will do just that - replace the stub impl function in its entirety - whereas not registering anything will use the default stub impl like it does today.

This will require (basically) new stubs for every function to support this. But it should be backward compatible and transparent to test cases, which is good.

asgibson · 2021-04-14T15:09:57Z

It was explained that what was needed is a "do no harm" approach so that existing code is unaffected and that there was no desire to overhaul all the stubs as you are suggesting. These assumptions went into the approach used in the solution presented.

The "rest of the default stub logic" is always skippable because that is the contract one gets when using the override. Iff one decides that the override is required, then it becomes user responsibility to worry about what happens after UT_DEFAULT_IMPL(<function_name>) because the effect is that none of that code will run. Whatever is necessary here can be user defined (or not!) in the hook.

The override solution will require that any remaining logic in stubs is able to be bypassed, but this does not necessitate creating a separated function nor does it need the stub impl function replaced entirely. The stubs are fine how they are for use in unit tests, only the logic after the impl is the problem. That remaining logic in the stub is not controllable from a unit test. Sometimes unit tests want the remaining logic, sometimes they need more control. If unit tests cannot control everything outside of the code under test then they become burdensome instead of helpful (fairly certain, @jphickey, you have experienced this issue as the 4 to 5 times it takes to create coverage tests over and above time developing production code).

The only stub overhaul I am advocating for is to reduce them to require full setup from tests, which means reducing them to registering their arguments and returning "forced" values. However, that is the approach that is most harmful and under the above assumptions, not acceptable.

jphickey · 2021-04-14T15:45:20Z

The "rest of the default stub logic" is always skippable

No, it's not. At least not for a function with a return value. Part of the post-hook logic is to propagate and/or fabricate a return code, for stubs that return a value (i.e. most of them, anything non-void).

What's more, is that because the post-hook logic is currently "fixed" in the stub code, in order to implement what's being requested (skipping the fixed logic) would necessitate changing every stub, either way, to make it not fixed anymore.

I have reviewed PR #839 and I honestly don't see how that solves the issue at all, it would still require every single stub implementation to check UT_StubIsOverridden() and skip the rest.

In what I'm suggesting, the stub updates can be scripted/generated code. So while it is a substantial change, it can be (partly) automated at least, so its not an enormous undertaking. Furthermore, it can be done in such a way that existing test cases and stubs won't break, meeting the "do no harm" criteria, because the existing stubs can be reused as a default hook.

It will still require the stub to be updated in order for an override to work as expected though, I can't see a way around that no matter how this is done.

asgibson · 2021-04-14T16:11:43Z

No, it's not. At least not for a function with a return value. Part of the post-hook logic is to propagate and/or fabricate a return code, for stubs that return a value (i.e. most of them, anything non-void).

All post logic now becomes the responsibility of the hook, period. This is the override contract. What prompted this was an attempt to ignore a return value set!

What's more, is that because the post-hook logic is currently "fixed" in the stub code, in order to implement what's being requested (skipping the fixed logic) would necessitate changing every stub, either way, to make it not fixed anymore.

It was stated as much in the pull request ("Stubs will need updated to make the check for the override, but that is all that will need to change in them."), except that stubs that do not have follow on logic will not need to be updated because there is nothing that needs overridden.

In what I'm suggesting, the stub updates can be scripted/generated code. So while it is a substantial change, it can be (partly) automated at least, so its not an enormous undertaking. Furthermore, it can be done in such a way that existing test cases and stubs won't break, meeting the "do no harm" criteria, because the existing stubs can be reused as a default hook.

It would be good to see a concrete representation of what you are describing. It appears that the solutions are similar although it also seems that the revamping of the stubs is more work than would be necessary. The revamping solution is attempting to fix other issues? The override solution is ready to go, except for stubs that have follow on logic allowing the override (which can be scripted/generated) and directly solves the stated issue, "Update UT stubs/support such that user registered hook/return value override/etc skips the rest of the default stub logic."

jphickey · 2021-04-14T18:21:25Z

Stubs will need updated to make the check for the override

This is very much one of those "easier said than done" things. It requires manual inspection of hundreds of stubs to figure out the right spot to put the check. It is tedious and time consuming, and prone to human error, not nearly as simple as it sounds, especially for items that do not return int32 type.

What I'm suggesting is that, instead of manually "fixing" every stub in its current form, is convert the existing stubs to hooks, and use a script to (re-)generate the actual stubs to fit this model better.

It will still require some manual touch-up to change the existing stubs into hooks (this is in fact what I'm doing right now), but this can be partly automated, thus easier and less error prone. So while this is also somewhat tedious, its not as bad as trying to shoehorn something into every existing stub.

jphickey · 2021-04-14T18:35:34Z

Just to clarify this one point:

All post logic now becomes the responsibility of the hook, period. This is the override contract.

The problem is, this contract is inadequate for any non-void function (i.e. anything with a return value of any kind). This is the point I was trying to make earlier.

Case in point, many functions look something like this (taking "CFE_ES_GetAppInfo" as a simple example of something that looks like it doesn't have any post logic, but actually does):

    status = UT_DEFAULT_IMPL(CFE_ES_GetAppInfo);

    return status;

The simple act of propagating the return status from the hook (UT_DEFAULT_IMPL) up the stack to the caller is something that cannot become the responsibility of the hook right now, using the current hook data model. This is part of what I consider the "post-hook" logic. While this logic is certainly trivial here (just propagate the value) it is not zero, and cannot be "skipped".

In this case what we need to do is to extend the hook data model a bit to handle return values more appropriately, so it can be set directly by a hook, and not rely on the stub itself to propagate or translate it. Then the statement "All post logic now becomes the responsibility of the hook, period" is more feasible.

asgibson · 2021-04-14T19:00:22Z

If a true fix is given, then how it is accomplished matters little to me.

A solution was provided because it appeared unimportant to anyone except myself, but that was only because I am the only person currently running into a problem with it. While it has gone unsolved, a workaround has been required for any problematic test, which all need to be removed when a fix is in place (truthfully these particular workarounds could be left in there, but tests should not have zombie code and removal is easy).

I look forward to seeing the final solution.

asgibson · 2021-04-14T19:19:06Z

To clarify: I do not consider

    status = UT_DEFAULT_IMPL(CFE_ES_GetAppInfo);

    return status;

to be part of that "follow on logic." Why? Because this is under the control of the unit test already, by defining what the return will be. The function has to return a value! I'm talking about any of the stuff between those statements. I have no need, desire or will to change how that is working because it doesn't get in the way like this does:

    status = UT_DEFAULT_IMPL(CFE_MSG_ValidateChecksum);
    if (status >= 0)
    {
        UTASSERT_GETSTUB(UT_Stub_CopyToLocal(UT_KEY(CFE_MSG_ValidateChecksum), (uint8 *)IsValid, sizeof(*IsValid)) ==
                         sizeof(*IsValid));
    }

    return status;

This is the whole issue and has nothing to do with the return value per se. It has to do with assumptions about the logic used for the implementation of a function. Stubs should never make assumptions about how what they are stubbing is implemented, they should be unintelligent. This is the purveyance of hooks and why I have repeatedly said that the stubs shouldn't even have this follow on logic, but getting the forced return value and returning it? that is not in question!

asgibson · 2021-04-14T20:00:41Z

/*
 * -----------------------------------------------------------
 * Stub implementation of CFE_MSG_GetMsgId
 * -----------------------------------------------------------
 */
int32 CFE_MSG_GetMsgId(const CFE_MSG_Message_t *MsgPtr, CFE_SB_MsgId_t *MsgId)
{
    UT_Stub_RegisterContextGenericArg(UT_KEY(CFE_MSG_GetMsgId), MsgPtr);
    UT_Stub_RegisterContextGenericArg(UT_KEY(CFE_MSG_GetMsgId), MsgId);

    int32 status;

    status = UT_DEFAULT_IMPL(CFE_MSG_GetMsgId);

    if (!UT_StubIsOverridden(UT_KEY(CFE_MSG_GetMsgId)))
    {
    
        if (status >= 0)
        {
            UTASSERT_GETSTUB(UT_Stub_CopyToLocal(UT_KEY(CFE_MSG_GetMsgId), (uint8 *)MsgId, sizeof(*MsgId)) ==
                            sizeof(*MsgId));
        }

    }

    return status;
}

A simple script using regex could find anywhere there exists code, in a stub, like that above from the status = yadda yadda to the return status and easily offset it into an if statement with the correct UT_KEY.

Gets the job done, no impact to whomever is not using it, is easily removable/upgradable later.

asgibson · 2021-04-14T20:05:51Z

Also, easy to test. Run the unit tests before the change, then run them after. If no tests are affected, there was no behavior changes; exactly what is required. Only tests that use the override will even know the change is in there.

jphickey · 2021-04-15T13:26:58Z

The functions that actually return int32 are not good examples here. Non-int32 returns are the problem. Granted these are the minority (maybe 10% of the functions?) but they exist, and UT stubs need to support them. Consider something like the stub for CFE_TIME_SysTime.

CFE_TIME_SysTime_t CFE_TIME_GetTAI(void)
{
    int32              status;
    CFE_TIME_SysTime_t Result = {0};

    status = UT_DEFAULT_IMPL(CFE_TIME_GetTAI);
    if (status == 0)
    {
        UT_Stub_CopyToLocal(UT_KEY(CFE_TIME_GetTAI), &Result, sizeof(Result));
    }

    return Result;
}

I repeat, there is no way for to set the actual return value of this function from a hook, because it isn't an int32. This is a design flaw/weakness of the original stub API, in that it assumes everything can be expressed in an int32 return value, but reality is that it cannot be.

If we are going to invest the time to add a new registration API and update every stub to check for it, why would we want to bake in the same API weakness/issue in the new stuff too? This is my sticking point - I'm not about to update every single stub and shoehorn something in that will only work for 90% of functions, leaving the others in a broken/incomplete state. If I'm going to update every stub, I want the end result to work for 100% of functions.

I am not sure why I'm sensing pushback here, what I'm suggesting is going to work for 100% of functions, any return type, it'll be backward compatible, and cleaner/simpler to understand and implement (IMO). I just have some remaining cleanup items to do but should have a PR finished this week for review.

asgibson · 2021-04-15T15:34:34Z

I am not pushing back, just advocating for a solution that is already available. A solution I took the time and effort to do and just didn't want to go to waste; which it really doesn't even if a another solution is selected because I suppose it did get something accomplished.

If a true fix is given, then how it is accomplished matters little to me.

I had stated as much above and will very much appreciate a more comprehensive fix.

What I find worrisome is putting in work, it dragging out while I do not have a fix (having to use the workaround then remove it when a fix is in place) and then basically being told that the solution offered doesn't work, won't work, can't work and would be difficult to implement because my investigations discovered otherwise. I disagree with all these statements. If that is what you consider push back, that is for you to decide. It took weeks just to get a look and as it turns out all I really had to do in the first place was nothing. The lesson learned from that is: don't bother.

I have been told that submissions are desired, but it does feel your responses were somewhat dismissive of any of the work or investigation that was performed. You make it sound as if the offered solution is so difficult and could not possibly be scripted to get the updates done, but that the solution you have decided upon will have little issue being scripted. I can even work around the non int32 returns with scripting, this is not a difficult hurdle to overcome even if a little more work. @jphickey you stated:

In what I'm suggesting, the stub updates can be scripted/generated code. So while it is a substantial change, it can be (partly) automated at least, so its not an enormous undertaking.

Saying that one solution can be implemented by scripting changes and another not as one of the criteria for not using the offered solution is highly suspect and dismissive, in my opinion. I was waiting to see if the solution was accepted before writing something up to do the automated updating, which appears to have been the correct choice.

I do understand that these ut-assert updates are considered low priority, but for me, since it is what I use on a daily basis, it is not. Therefore, what you may also be sensing as push back is frustration with the amount of time having passed without a solution in place, then being told it'll be longer.

Adds the concept of a "handler" function to UT assert. A handler is basically the custom logic that exists between the hook function and the return to the stub caller. In current UT stubs, this is hard coded, and it generally comprises setting output parameters and translating return values as needed. This concept adds the basic plumbing to allow the handler to be configured just like a hook function already does. The difference is that the handler is directly responsible for setting all outputs. This also includes a script to auto-generate stub functions that match this pattern. Given an API header file, the script extracts the declarations, and generates a source file with stub definitions that rely on a separate handler to deal with the needed outputs. Note this initial commit only adds the basic framework. It does not change any existing stubs or tests, and is fully backward compatible, as it is a new feature and it is a no-op unless actually configured/used by the stub or test case. Follow on commits will update the stubs to use this pattern.

To imporove scriptability of generating the shared layer stubs, makes two minor adjustments: - adds a typedef for the callback in OS_ObjectIdIteratorProcessEntry. This avoids the complex syntax of function pointer argument, which is hard to read but also harder to parse in a script, too. - Splits the "os-shared-printf.h" header into two parts, adding a new header file "os-shared-console.h". This is because OS_ConsoleOutput_Impl is implemented in a separate unit in the source, and this allows a better relationship between stub files and header files.

Adds the concept of a "handler" function to UT assert. A handler is basically the custom logic that exists between the hook function and the return to the stub caller. In current UT stubs, this is hard coded, and it generally comprises setting output parameters and translating return values as needed. This concept adds the basic plumbing to allow the handler to be configured just like a hook function already does. The difference is that the handler is directly responsible for setting all outputs. This also includes a script to auto-generate stub functions that match this pattern. Given an API header file, the script extracts the declarations, and generates a source file with stub definitions that rely on a separate handler to deal with the needed outputs. Note this initial commit only adds the basic framework. It does not change any existing stubs or tests, and is fully backward compatible, as it is a new feature and it is a no-op unless actually configured/used by the stub or test case. Follow on commits will update the stubs to use this pattern.

skliper · 2021-04-16T14:29:15Z

@asgibson - The issues you discovered and documented with the original stub implementation as well as your contributions and participation in addressing them have all helped immensely with the continuous improvement of the product. I see the extension/enhancement to address the final 10% of the stubs w/ non-int32 returns as building on the hard work you have put in up to this point. None of this would have happened without your contributions, which we do value!

To imporove scriptability of generating the shared layer stubs, makes two minor adjustments: - adds a typedef for the callback in OS_ObjectIdIteratorProcessEntry. This avoids the complex syntax of function pointer argument, which is hard to read but also harder to parse in a script, too. - Splits the "os-shared-printf.h" header into two parts, adding a new header file "os-shared-console.h". This is because OS_ConsoleOutput_Impl is implemented in a separate unit in the source, and this allows a better relationship between stub files and header files.

The "built-in" logic from existing stubs is converted to a handler function and moved to a different source file. The generated stub references this function if it exists and uses it as a default handler, so the default behavior is not changed. In this pattern the stubs in this directory are strictly limited to the same public APIs that are defined in header files under src/os/inc. This has the side effect of removing some internal stubs required for coverage test. Those stubs will be reinstated in the coverage specific stubs where other internal functions are.

Update the coverage-specific (shared layer internal) stubs to use generated stubs.

jphickey · 2021-04-16T15:00:23Z

@asgibson Apologies if my comments came across as not considering your original change set. I certainly did use the concept suggested in the final PR (currently in progress). If you look at the first commit in the PR (d563717 right now, assuming I don't need to amend/rebase it) the UT_SetHandlerFunction() function proposed is basically the same idea as the UT_SetHookOverrideStubFunction() in PR #839. I only tweaked the name and the function pointer argument type to address the pre-existing shortcomings in the Ut_HookFunc_t prototype. I also added two of them - a normal and a variadic one - to mirror the current hook APIs.

But to be clear - I like this feature a lot - and its going to make a substantial improvement in the overall usability and functionality of ut assert. Because it is such a major improvement I don't want to relegate it to be an "Add-on" or obscure extension that only works with a subset of stub types. It needs to be a first-class feature, fully integrated, and fully supported.

While I understand and appreciate the original intent of changing as little as possible and re-using what was there, the inability to effectively handle non-int32 return types prevents it from becoming the first-class feature that I'd like it to be. Again, this is not a shortcoming of your initial PR, its a shortcoming of the original hook API design. But since we are adding a new API as well as updating all stubs either way, it presents the perfect opportunity to also address that shortcoming, not just re-use the original (limited) hook API.

The "built-in" logic from existing stubs is converted to a handler function and moved to a different source file. The generated stub references this function if it exists and uses it as a default handler, so the default behavior is not changed. In this pattern the stubs in this directory are strictly limited to the same public APIs that are defined in header files under src/os/inc. This has the side effect of removing some internal stubs required for coverage test. Those stubs will be reinstated in the coverage specific stubs where other internal functions are.

Update the coverage-specific (shared layer internal) stubs to use generated stubs.

asgibson · 2021-04-16T15:24:57Z

I do appreciate and accept the apology, but truly unnecessary. This is business, I understand. I just wanted to be sure that my frustration was understood for what it was and not taken as push back on the fix. This is one of my favorite lines I have seen about software, ever:

It needs to be a first-class feature, fully integrated, and fully supported.

My apologies for underestimating your commitment to the end goal. I do look forward to seeing and using the final solution.