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

[jrose]

> On July 31, 2013, 7:47 p.m., Mark Michelson wrote:
> > /trunk/res/parking/parking_tests.c, lines 46-47
> > <https://reviewboard.asterisk.org/r/2714/diff/3/?file=43256#file43256line46>
> >
> >     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.

This was a style borrowed from the CDR tests which Matt suggested I use as a guide. I'm fine with changing it though.


> On July 31, 2013, 7:47 p.m., Mark Michelson wrote:
> > /trunk/res/parking/parking_tests.c, lines 50-61
> > <https://reviewboard.asterisk.org/r/2714/diff/3/?file=43256#file43256line50>
> >
> >     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);

Likewise with the CDR test example.


> On July 31, 2013, 7:47 p.m., Mark Michelson wrote:
> > /trunk/res/parking/parking_tests.c, lines 238-242
> > <https://reviewboard.asterisk.org/r/2714/diff/3/?file=43256#file43256line238>
> >
> >     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.

This was also used in the CDR tests when doing short duration bridges like this. I'm not certain whether it's necessary at the moment.


> On July 31, 2013, 7:47 p.m., Mark Michelson wrote:
> > /trunk/res/parking/parking_tests.c, lines 254-326
> > <https://reviewboard.asterisk.org/r/2714/diff/3/?file=43256#file43256line254>
> >
> >     All of these log messages should be ast_test_status_update() instead of ast_debug()

Ah, OK. I was curious about that since they are reporting on details that I wouldn't necessarily expect to be understandable/interprettable to non-developers.


> On July 31, 2013, 7:47 p.m., Mark Michelson wrote:
> > /trunk/res/parking/parking_tests.c, lines 563-570
> > <https://reviewboard.asterisk.org/r/2714/diff/3/?file=43256#file43256line563>
> >
> >     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)

Alrighty, I'll add these in.


- jrose


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


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/af4eb300/attachment-0001.htm>