[asterisk-dev] [Code Review] 4256: testsuite: check for channel leak on failed blonde transfer

[Matt Jordan]

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



/asterisk/trunk/lib/python/asterisk/bridge_test_case.py
<https://reviewboard.asterisk.org/r/4256/#comment24553>

    I don't think this change was necessary for this test. See comments on the YAML later.



/asterisk/trunk/lib/python/asterisk/channel_test_condition.py
<https://reviewboard.asterisk.org/r/4256/#comment24549>

    (1) I'd remove the "POSSIBLE" - if a test specifies how many allowed channels it expects at the end of a test and there are channels remaining, we are (a) going to fail the test, and (b) something is wrong. Let's just call it a channel leak.
    
    (2) Since this is going to be going out as an error message already, I wouldn't bother shouting :-)
    
    (Either that, or we should SHOUT OUT ALL THE ERROR MESSAGES!!!!) 



/asterisk/trunk/lib/python/asterisk/channel_test_condition.py
<https://reviewboard.asterisk.org/r/4256/#comment24550>

    Pedantic:
    
    Pydoc strings don't have a space between the begin of documentation (""") and the text:
    
    """Mock CLI output base class"""



/asterisk/trunk/tests/bridge/atxfer_fail_blonde/configs/ast1/extensions.conf
<https://reviewboard.asterisk.org/r/4256/#comment24554>

    I'm assuming the Wait(5) here is to allow the transferer time to hang up.
    
    If that's the case, then this is technically a bit racey - yes, some build agents will still mess this up.
    
    I'd use a UserEvent here instead, then drop the channel into Echo. Write a pluggable module that issues a hangup AMI action to the transferer when that occurs. Once you detect the hangup of the transferer, hangup the channel in the transfer target.



/asterisk/trunk/tests/bridge/atxfer_fail_blonde/test-config.yaml
<https://reviewboard.asterisk.org/r/4256/#comment24552>

    I'd remove this.
    
    (1) A 5 minute time out is not a good idea to specify in generic test runs. This will typically just hang a build agent if something does go wrong.
    
    (2) It's not necessary for this test. Hanging for 5 minutes should never occur in a simple blonde transfer.



/asterisk/trunk/tests/bridge/atxfer_fail_blonde/test-config.yaml
<https://reviewboard.asterisk.org/r/4256/#comment24551>

    Minimum version should be the minimum expected Asterisk version that contains the fix.
    
    (1) Are we sure this should run against 11?
    (2) What minimum version of 13 should this include?


- Matt Jordan


On Dec. 12, 2014, 4:10 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4256/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2014, 4:10 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24513
>     https://issues.asterisk.org/jira/browse/ASTERISK-24513
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> This test starts an attended transfer, converts to blonde mode by hanging up the transferer, and then fails the transfer by hanging up the transferee.  Then after allowing the recall attempts to expire, checks to insure that there are not leaked channels.
> 
> Improvements to bridge_test_case: added support for setting reactor timeout
> 
> Improvements to channel_test_condition: count the actual channels listed in "core show channels" output to check for leaks.  Also added unittest.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/bridge/tests.yaml 6082 
>   /asterisk/trunk/tests/bridge/atxfer_fail_blonde/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/bridge/atxfer_fail_blonde/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/lib/python/asterisk/channel_test_condition.py 6082 
>   /asterisk/trunk/lib/python/asterisk/bridge_test_case.py 6082 
> 
> Diff: https://reviewboard.asterisk.org/r/4256/diff/
> 
> 
> Testing
> -------
> 
> Currently fails while ASTERISK-24513 is not yet patched.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

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