simplify unit tests with utassert.h macros

[CDKnightNASA]

Is your feature request related to a problem? Please describe.
Currently a lot of OSAL unit test code is like the following (basically, run code, check the result, "force" an assert that it worked or didn't work):

    /*-----------------------------------------------------*/
    testDesc = "#2 Name-too-long-arg";

    res = OS_xxx(aVeryLoooooongName);
    if (res == OS_ERR_NAME_TOO_LONG)
        UT_OS_TEST_RESULT( testDesc, UTASSERT_CASETYPE_PASS);
    else
        UT_OS_TEST_RESULT( testDesc, UTASSERT_CASETYPE_FAILURE);

Describe the solution you'd like
Instead, this can be simplified to:

  UtAssert_True(OS_xxx(aVeryLoooooongName) == OS_ERR_NAME_TOO_LONG, "#2 Name-too-long-arg");

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context about the feature request here.

Requester Info
[email protected]

[jphickey]

These tests were originally implemented with an entirely different test framework, and we've been gradually trying to get them into the same model, but so far nobody has had time budget to rewrite them, so its been more of baby steps.

[CDKnightNASA]

A first pass would be just to update the "template" contained in the current unit test code. I can certainly do that and I suggest it get merged quickly so that future developments against the master branch follow the new paradigm. I'll do that.

[skliper]

It would be a great help to update the "template", I've got some help coming soon that can address applying that template across the rest of the tests. Thanks @CDKnightNASA .

[CDKnightNASA]

confused why this was closed?

[skliper]

Whoops.

[CDKnightNASA]

proposed re-write (wanted to get folks eyeballs on before I copy/paste it to all of the .c files. Or should we replace duplicates with a reference and have the template be in one file or in a separate file?)

Note also I removed the test # from the text reported...Is the test # valuable? It becomes a maintenance issue if you're adding off-nominal tests or removing tests, as you have to re-number all of the assert texts below.

/* Test code template for testing a single OSAL API with multiple test cases */

#if 0
void UT_os_sample_test(void)
{
    /*-----------------------------------------------------*
     * For each test case,
     *   1. Setup the test environment, if necessary
     *   2. Run the test inside an assert statement
     *   3. Reset the test environment, if necessary
     *
     * NOTES: "Not implemented" is always checked first but not
     *        being included as a test case, if the API is not
     *        implemented, the test should be skipped.
     *
     *        "Nominal" test case is always the last test case.
     *-----------------------------------------------------*/

    /* TODO: Setup the test environment, if necessary */

    if (OS_xxx() == OS_ERR_NOT_IMPLEMENTED)
    {
        UT_OS_TEST_RESULT("API not implemented", UTASSERT_CASETYPE_NA);
        return;
    }

    /* TODO: Reset the test environment here, if necessary */

    /*-----------------------------------------------------*/

    /* TODO: Setup the test environment here, if necessary */

    UtAssert_True(OS_xxx(NULL,..) == OS_INVALID_POINTER, "Null-pointer-arg");

    /* TODO: Reset the test environment here, if necessary */

    /*-----------------------------------------------------*/

    /* TODO: Setup the test environment here, if necessary */

    UtAssert_True(OS_xxx(aVeryLooooooongName) == OS_ERR_NAME_TOO_LONG, "Name-too-long-arg");
    /* TODO: Reset the test environment here, if necessary */

    /*-----------------------------------------------------*/

    /* TODO: Setup the test environment here, if necessary */

    UtAssert_True(OS_xxx(...) == OS_SUCCESS, "Nominal");

    /* TODO: Reset the test environment here, if necessary */
}
#endif

[skliper]

Maybe just implement a gold standard vs template (that way it's always up to date)? Or just drop it in a README vs actually in the *.c files? I don't think duplication is necessary.

[jphickey]

Please please please also include the actual value tested in all UtAssert_True statements whenever possible. We need useful log messages in the event of a failure.

For instance, some of the other unit test code leverages a macro like this:

/*
 * Macro to call a function and check its int32 return code
 */
#define UT_TEST_FUNCTION_RC(func,exp)           \
{                                               \
    int32 rcexp = exp;                          \
    int32 rcact = func;                         \
    UtAssert_True(rcact == rcexp, "%s (%ld) == %s (%ld)",   \
        #func, (long)rcact, #exp, (long)rcexp);             \
}

This, importantly, includes the actual numeric values within in the test log. The problem with something like UtAssert_True(OS_xxx(...) == OS_SUCCESS, "Nominal"); is that when it fails, you only get a very vague failure message in the log and no idea why it failed.

[CDKnightNASA]

Thinking about:

#define UT_TEST_FUNC_RC(func,exp) UtAssert_True(func == exp, "%s != %s", #func, #exp)

Numeric return codes are not all that useful/readable. And this doesn't require local scope/vars.

[jphickey]

No, the number is absolutely critical info for the poor soul who is assigned the task of debugging the failure.

One can easily look up the error symbol based on the number, and then grep for code that returns that symbol. That at least gives you a starting point. Without this, you are left to reproduce the failure somehow in a debugger, which isn't always as trivial as it sounds. I've been there way too many times.

This isn't realtime code; the most important thing should be to produce good, useful log output from a test that a developer can actually act on (even a little).

[jphickey]

To use the ever-applicable car analogy, this is like a "general car fault" light on the dashboard. When you go to the mechanic for a diagnosis, wouldn't you rather be able to say "I got a CAR_ERR_TEMP_TOO_HIGH error" rather than "I didn't get CAR_SUCCESS the last time I drove" ?

Any failure of these tests is likely to come back to the CCB (us) for diagnosis, so we should include as much info to make our lives easier.

[CDKnightNASA]

Note the straw-man has the macros defined in osal/src/unit-tests/inc, it might make more sense to move these to osal/ut_assert/{inc|src}...Part of the discussion at the CCB.