[asterisk-dev] [Code Review] 3420: Testsuite: Call Files' Max Retries

[Kevin Harwell]

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



./asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3420/#comment21332>

    As this class has specific configuration data it uses it'd be good to have that information in the class comment.



./asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3420/#comment21335>

    Since you mentioned this earlier I'll go ahead and mention this as a note so it can be tracked as well.  For certain tests, archive and don't delete, the call files remain on the system once the tests completes.  Once the test has passed/failed, but before stopping the test these files should be removed.



./asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3420/#comment21330>

    Suggestion: It looks like self.channels just keeps track of event results.  You could probably just use a boolean variable to track this,  instead of a list, since if one fails the test is considered failed.  Should be able to just "and" the results together.  Or better yet can you can just fail and stop the test on the first failure even (unless there is a reason not to).



./asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3420/#comment21331>

    Looks like "max_retries" is a required value, so it either needs a default or an error message stating it wasn't given.  If not required for all tests might be best to break that test functionality out into a subclass.



./asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3420/#comment21333>

    I don't understand what the test-result option is used for?  Are there some cases where a failure is considered passing?



./asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3420/#comment21329>

    Swap these checks as there is no reason to do the "all" check if there are no channels.  Actually, might want to check that the "all" function doesn't check for the empty list itself.



./asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3420/#comment21328>

    Someone else might be able to comment on the precedence for this, but you could have one user event with different result types and then switch off that.  For instance, the result of alwaysdelete could be 'deleted' instead of 'pass'.
    
    If that's not a great option I'd at least recommend changing the event names of "CallFileResult' and 'TestResult' to something more descriptive.



./asterisk/trunk/tests/pbx/call_file_retries_alwaysdelete/test-config.yaml
<https://reviewboard.asterisk.org/r/3420/#comment21334>

    Since you are checking the pass/fail in the code I don't think you need to have this check here in the yaml as well.
    
    Note - This applies to the other tests as well.


- Kevin Harwell


On April 4, 2014, 6:15 p.m., Scott Emidy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3420/
> -----------------------------------------------------------
> 
> (Updated April 4, 2014, 6:15 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23218
>     https://issues.asterisk.org/jira/browse/ASTERISK-23218
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> These tests involved checking that call files max retries are functioning as planned through four tests:
> 
> 1) The first test (call_file_retries_fail) required that the call file originates a local channel to a dialplan extension that will always fail, and checks to make sure that it ran through all of its max retries.
> 
> 2) The second test (call_file_retries_success) involves a call file that originates a local channel that will fail once, but then is answered before it hits its max retries.
> 
> 3) The third test (call_file_retries_alwaysdelete) consists of checking whether or not the call file was deleted from the [astspooldir]'s outgoing folder when the alwaysdelete option is set to 'no'.
> 
> 4) The fourth and final test (call_file_retries_archive) consists of checking whether or not the call file was placed in [astspooldir]'s outgoing_done folder when archive is set to 'yes'.
> 
> 
> Diffs
> -----
> 
>   ./asterisk/trunk/tests/pbx/tests.yaml 4935 
>   ./asterisk/trunk/tests/pbx/call_file_retries_success/test-config.yaml PRE-CREATION 
>   ./asterisk/trunk/tests/pbx/call_file_retries_success/configs/ast1/extensions.conf PRE-CREATION 
>   ./asterisk/trunk/tests/pbx/call_file_retries_fail/test-config.yaml PRE-CREATION 
>   ./asterisk/trunk/tests/pbx/call_file_retries_fail/configs/ast1/extensions.conf PRE-CREATION 
>   ./asterisk/trunk/tests/pbx/call_file_retries_archive/test-config.yaml PRE-CREATION 
>   ./asterisk/trunk/tests/pbx/call_file_retries_archive/configs/ast1/extensions.conf PRE-CREATION 
>   ./asterisk/trunk/tests/pbx/call_file_retries_alwaysdelete/test-config.yaml PRE-CREATION 
>   ./asterisk/trunk/tests/pbx/call_file_retries_alwaysdelete/configs/ast1/extensions.conf PRE-CREATION 
>   ./asterisk/trunk/lib/python/asterisk/pluggable_modules.py 4935 
> 
> Diff: https://reviewboard.asterisk.org/r/3420/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Scott Emidy
> 
>

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