[asterisk-dev] [Code Review] Unit Test Framework

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/447/#review1302
-----------------------------------------------------------



/trunk/include/asterisk/test.h
<https://reviewboard.asterisk.org/r/447/#comment3000>

    Let's name this included argument "ast_test_error_str"



/trunk/include/asterisk/test.h
<https://reviewboard.asterisk.org/r/447/#comment3001>

    I have some comments on how this works with regards to API/ABI.
    
    Instead of registering all of these test details via AST_TEST_REGISTER(), what would you think about taking an approach similar to the ast_cli such that the function gets called to fill in its own meta data.  The benefits would be:
    
    1) The summary, description, name, etc., all live in the code within the body of the test function.  It keeps the information more in one place.
    
    2) It will give us the ability to potentially add additional meta data in the future without having to change the register API (or ABI).



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3002>

    To be a bit more explicit (and safe), you could write this as ...
    
    static const char *foo[] = {
       [ENUM1] = "FOO",
       [ENUM2] = "BAR",
       [ENUM3] = "ASLKDJASDAS:DA",
    };



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3003>

    unsigned



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3013>

    "catagory" should be "category" throughout the file



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3004>

    unsigned



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3005>

    This is automatically initialized to 0 for you.



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3006>

    Remove the "__" from internal functions



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3007>

    enum?



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3008>

    Here is another way you could write this that I thought of.  It's a bit easier to read for me, but it's up to you.
    
    
    switch (mode) {
    case 2:
       if (strcmp(test->name, name)) {
          break;
       }
    case 1:
       if (test_cat_cmp(...)) {
          break;
       }
    case 0:
       execute = 1;
    }



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3009>

    Why would you include a test in the results that didn't run?



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3011>

    enum ftw!



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3012>

    Add \internal to documentation of internal functions.



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3014>

    AST_LIST_REMOVE_CURRENT() will not set cur to NULL, so you can just return cur at the end of the function instead of needed the test var.



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3015>

    This will return true for cat2 "/channels/sip" and cat1 "/channels/sippppppppppppppppppppppp/OMGZOMBIES/pancake"
    
    Is this intentional?



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3016>

    I don't think it really matters, but I'd rather swap the args to be: ast_calloc(1, sizeof(*test))



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3017>

    I would say that the summary and description are always applicable.  I would even prefer that we generate an error if they aren't provided.



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3018>

    Instead of hard coded numbers, I'd rather see usage of e->args with offsets when needed



/trunk/main/test.c
<https://reviewboard.asterisk.org/r/447/#comment3019>

    The 2nd const here is actually redundant


- Russell


On 2009-12-15 18:15:17, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/447/
> -----------------------------------------------------------
> 
> (Updated 2009-12-15 18:15:17)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> Summary
> -------
> 
> The Unit Test Framework is a new API that manages registration and execution of unit tests in Asterisk with the purpose of verifying the operation of C functions.
> 
> The Framework consists of a single test manager accompanied by a list of registered test functions defined within the code.  A test is defined, registered, and unregistered from the framework using a set of macros which allow the test code to only be compiled within asterisk when the TEST_FRAMEWORK flag is enabled in menuselect.  This allows the test code to exist in the same file as the C functions it intends to verify.  Registered tests may be viewed and executed via a set of new CLI commands.  CLI commands are also present for generating and exporting test results into xml and txt formats.
> 
> For more information and use cases please refer to the documentation provided at the beginning of the test.h file.
> 
> 
> Diffs
> -----
> 
>   /trunk/build_tools/cflags-devmode.xml 235225 
>   /trunk/include/asterisk/test.h PRE-CREATION 
>   /trunk/main/asterisk.c 235225 
>   /trunk/main/test.c PRE-CREATION 
>   /trunk/tests/test_heap.c 235225 
> 
> Diff: https://reviewboard.asterisk.org/r/447/diff
> 
> 
> Testing
> -------
> 
> test_heap.c has been modified to take advantage of the Test Framework.  I have executed and generated reports for these tests. 
> 
> 
> Thanks,
> 
> David
> 
>