[asterisk-dev] [Code Review] 3329: AGI Exit Status Test

[Mark Michelson]

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


One thing I notice about this test is that it is structured to call the first extension, then when that finishes, call the second, then when that finishes, call the third, etc.

Since no call depends on any of the previous call results, I think this test could be changed to originate all the calls at the same time and evaluate the UserEvents and Hangups as they arrive. This would have a few benefits:

1) The test will execute more quickly
2) You can get rid of the can_call member of your test class.
3) Your AMI hangup handler will be simplified greatly since you won't need to have a big if-else ladder to figure out what to do next. In fact, you may be able to just get rid of it altogether.

Doing it this way kind of screws up your detected member, though. As a simple workaround, in your new_exten_event_handler, you can determine whether to send the AMI hangup by checking event['AppData'] for the name of the AGI script. If event['AppData'] is 'waiting.agi' or 'executing.agi' then you issue the AMI hangup.


./asterisk/trunk/tests/agi/exit_status/configs/ast1/extensions.conf
<https://reviewboard.asterisk.org/r/3329/#comment20835>

    I recommend sending a UserEvent if the AGISTATUS is not what is expected. Something like the following:
    
    same => n,UserEvent(TestResult,result:${IF($[${AGISTATUS}=NOTFOUND]?pass:fail)})
    
    Since your test script prints a message when the result is not "pass", making this alteration can help to more easily debug which test cases are misbehaving.



./asterisk/trunk/tests/agi/exit_status/run-test
<https://reviewboard.asterisk.org/r/3329/#comment20833>

    This sentence is incomplete.



./asterisk/trunk/tests/agi/exit_status/run-test
<https://reviewboard.asterisk.org/r/3329/#comment20839>

    Even though I'm advocating to just get rid of this member altogether, I think it's worth mentioning that the name could be improved. Since this is intended to indicated that the testsuite is supposed to interrupt the current AGI, something like "interrupt_agi" would probably be more clear.



./asterisk/trunk/tests/agi/exit_status/run-test
<https://reviewboard.asterisk.org/r/3329/#comment20837>

    Change the else to explicitly check if event['context'] == 'executing-extens'
    
    Add a final else clause that logs an error if an unexpected extension ends up calling the userevent from the h extension. Structure this so that success_count does not get incremented if this final else case is executed.



./asterisk/trunk/tests/agi/exit_status/run-test
<https://reviewboard.asterisk.org/r/3329/#comment20836>

    Use %d since self.success_count is an integer.



./asterisk/trunk/tests/agi/exit_status/run-test
<https://reviewboard.asterisk.org/r/3329/#comment20838>

    This use of 7 here is what is sometimes referred to as a "magic number" in programming, and it's not really a good thing. If this test were to be altered later to test some more situations or to remove some test cases, then whoever made the modifications would have to go through the source and change all of the 7's to 8's or 6's or whatever.
    
    Instead, at the top of the file, declare a global variable, something like:
    
    EXPECTED_SUCCESSES = 7
    
    Then, everywhere where you are currently comparing to 7, just compare to EXPECTED_SUCCESSES instead.
    
    In addition to the code maintainability I mentioned earlier, it also is more expressive; readers of the code will easily understand that you are comparing to the number of expected successes rather than a number they have to figure out the significance of.
    
    In general, prefer symbolic names for things over bare numerals. Obviously, as with every rule, there are exceptions to this.



./asterisk/trunk/tests/agi/exit_status/run-test
<https://reviewboard.asterisk.org/r/3329/#comment20834>

    Change this to an error instead of info, and print the value of test.success_count.


- Mark Michelson


On March 11, 2014, 7:54 p.m., Benjamin Keith Ford wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3329/
> -----------------------------------------------------------
> 
> (Updated March 11, 2014, 7:54 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-19167
>     https://issues.asterisk.org/jira/browse/ASTERISK-19167
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> Notes:
> - Is a sub-task of ASTERISK-19167
> - Ignore userA directory; will be removed from repository
> 
> This test runs through a few different AGI scripts to verify that AGISTATUS returns the correct values:
> 1. Attempts to run an AGI script that does not exist. AGISTATUS returns NOTFOUND.
> 2. Attempts to run an AGI script that has an invalid path. AGISTATUS returns FAILURE.
> 3. Attempts to run an AGI script that has a non-executable interpreter. AGISTATUS returns FAILURE.
> 4. Attempts to run an AGI script that is non-executable. AGISTATUS returns FAILURE.
> 5. Runs an AGI script that will be hung up on while waiting for a command. AGISTATUS returns HANGUP.
> 6. Runs an AGI script that will be hung up on while executing a command. AGISTATUS returns HANGUP.
> 7. Runs an AGI script that exits normally. AGISTATUS returns SUCCESS.
> 
> 
> Diffs
> -----
> 
>   ./asterisk/trunk/tests/agi/exit_status/waiting.agi PRE-CREATION 
>   ./asterisk/trunk/tests/agi/exit_status/test-config.yaml 4749 
>   ./asterisk/trunk/tests/agi/exit_status/run-test 4749 
>   ./asterisk/trunk/tests/agi/exit_status/executing.agi PRE-CREATION 
>   ./asterisk/trunk/tests/agi/exit_status/donothing.agi PRE-CREATION 
>   ./asterisk/trunk/tests/agi/exit_status/configs/ast1/extensions.conf PRE-CREATION 
>   ./asterisk/trunk/tests/agi/exit_status/badinterpreter3.agi PRE-CREATION 
>   ./asterisk/trunk/tests/agi/exit_status/badinterpreter2.agi PRE-CREATION 
>   ./asterisk/trunk/tests/agi/exit_status/badinterpreter.agi PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3329/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Keith Ford
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140313/2a17905a/attachment-0001.html>