[asterisk-dev] [Code Review] 2714: res_parking: Unit Tests

[Mark Michelson]

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



/trunk/res/parking/parking_tests.c
<https://reviewboard.asterisk.org/r/2714/#comment18246>

    Instead of this macro, you could have
    
    static const struct ast_party_caller alice_callerid = {
        .id.name.str = "Alice",
        .id.name.valid = 1,
        .id.number.str = "100",
        .id.number.valid = 1,
    };
    
    Then you wouldn't have to create local ast_party_caller structs in the test where an Alice channel is created.



/trunk/res/parking/parking_tests.c
<https://reviewboard.asterisk.org/r/2714/#comment18247>

    I'm not so certain why either of these are macros instead of functions.
    
    CREATE_ALICE_CHANNEL could be a function that returns the created channel (or NULL if it fails). As it is currently written, it could crash if ast_channel_alloc() returns NULL.
    
    HANGUP_CHANNEL could be a function that returns NULL so you could do:
    
    chan = hangup_channel(chan, cause);



/trunk/res/parking/parking_tests.c
<https://reviewboard.asterisk.org/r/2714/#comment18239>

    Why are these nanosleep calls here? Did you run across issues that caused the nanosleep calls to be required?
    
    Adding calls like this is one of the most typical causes of bouncing tests in the testsuite. It will work most of the time (and probably 100% of the time on your local machine), but every now and then, the failure that you experienced that caused you to add the nanosleep calls will happen on the Bamboo build agents.
    
    If there was some sort of issue that caused you to add the nanosleep calls, I'd highly recommend finding an alternative method instead.



/trunk/res/parking/parking_tests.c
<https://reviewboard.asterisk.org/r/2714/#comment18241>

    It would be a good idea to perform the same test you did in the create_lot test and ensure that the lot you created was disposed of.
    
    It may be worthwhile to expand the dispose_test_lot() function to have a second parameter that states whether we expect the lot to be removed after the call to parking_lot_remove_if_unused(). This way, the dispose_test_lot() can report if our expectations were met.



/trunk/res/parking/parking_tests.c
<https://reviewboard.asterisk.org/r/2714/#comment18243>

    All of these log messages should be ast_test_status_update() instead of ast_debug()



/trunk/res/parking/parking_tests.c
<https://reviewboard.asterisk.org/r/2714/#comment18245>

    A couple of other ideas along the same lines:
    
    * Off by one errors are pretty common problems, so it may be a good idea to test that you cannot create a parking lot with range starting with 703 or a parking lot with range ending with 701. These could supplement or outright replace the selected segment.
    
    * Try creating a parking lot with parking spaces that include the parking extension of the base lot (e.g., 690-700)



/trunk/res/parking/parking_tests.c
<https://reviewboard.asterisk.org/r/2714/#comment18250>

    Add a comment so it's easier to see what this #endif matches.



/trunk/res/parking/res_parking.h
<https://reviewboard.asterisk.org/r/2714/#comment18248>

    Since this function is only used by the tests, it should only be declared (and defined) when TEST_FRAMEWORK is defined.



/trunk/res/parking/res_parking.h
<https://reviewboard.asterisk.org/r/2714/#comment18249>

    Since this is only used by tests, it should only be declared and defined when TEST_FRAMEWORK is defined.



/trunk/res/parking/res_parking.h
<https://reviewboard.asterisk.org/r/2714/#comment18238>

    s/respected/respect to/


- Mark Michelson


On July 30, 2013, 10:35 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2714/
> -----------------------------------------------------------
> 
> (Updated July 30, 2013, 10:35 p.m.)
> 
> 
> Review request for Asterisk Developers, kmoore, Matt Jordan, and rmudgett.
> 
> 
> Bugs: ASTERISK-22138
>     https://issues.asterisk.org/jira/browse/ASTERISK-22138
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds the following unit tests to res_parking
> 
> create_lot: Creates a lot, adds it to the parking lot list, makes sure it can find it in the parking lot list, and then removes it and makes sure removal was successful.
> park_call: Creates/adds a parking lot, then parks test channel Alice into the parking lot. After a short sleep, Alice is then forcibly removed from the bridge and hung up.
> retrieve_call: Creates/adds a parking lot and parks Alice. After a short sleep, Alice is retrieved from the parking lot with normal park call retrieval functions, then hung up.
> park_extensions: Creates a parking lot which will register extensions. The Park and Parkedcall extensions are checked for their presence and then the parking lot is removed and destroyed. At this point the extensions are checked again to make sure there are no leftovers.
> 
> These tests live within the res_parking module because they test basic functionality of res_parking internal functions which aren't available to external modules.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/parking/parking_bridge.c 395747 
>   /trunk/res/parking/parking_bridge_features.c 395747 
>   /trunk/res/parking/parking_tests.c PRE-CREATION 
>   /trunk/res/parking/res_parking.h 395747 
>   /trunk/res/res_parking.c 395747 
> 
> Diff: https://reviewboard.asterisk.org/r/2714/diff/
> 
> 
> Testing
> -------
> 
> Created the tests and ran them.
> Made sure res_parking compiles and functions normally both with and without TEST_FRAMEWORK enabled.
> Varied the expectations to deliberately differ from the real expected results to see how the tests would handle failure.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130731/79623782/attachment-0001.htm>