[asterisk-dev] [Code Review] SIP Blind transfer tests

Mark Michelson reviewboard at asterisk.org
Mon Jan 23 10:59:53 CST 2012 

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


I made several comments on the first test that apply to all tests.

I think the coverage of the test could be improved by essentially re-running each test with the following variables:

1. Type of bridge in use. This can be controlled by the canreinvite setting and by adding various parameters to the Dial application calls in the dialplan.
   Case A: Native remote RTP bridge. Set canreinvite=yes, make sure codecs are same on channels and no extra dial parameters are added.
   Case B: Native local RTP bridge. Set canreinvite=no, make sure codecs are the same on channels and no extra dial parameters are added.
   Case C: Asterisk generic bridge. Set canreinvite=no, and add parameters like 't' or 'T' to the dial application.
2. Which phone gets hung up.
   Case A: Hang up the transferee.
   Case B: Hang up the transfer target.
3. Whether Asterisk or PJSUA hangs up the call. This is like variable 2 except that instead of sending 'h' commands to PJSUA, you use a Manager hangup request on Asterisk.
   Case A: Request hangup on the transferee's channel.
   Case B: Request hangup on the transfer target's channel.


/asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/run-test
<https://reviewboard.asterisk.org/r/1686/#comment9810>

    Channels are going to be up. Specifically, the A and C channels are going to be up because they're still bridged.



/asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/run-test
<https://reviewboard.asterisk.org/r/1686/#comment9809>

    I'm not a big fan of this. Waiting a set amount of seconds is the type of thing that can cause a test to fail for seemingly no good reason.
    
    I like the idea of calling the various functions here based on events that occur rather than elapsed time.
    
    So for instance, self.a_call_b should be called once the PJSUA processes are up and running. I don't know if subprocess.Popen waits to find out if the subprocess actually starts successfully. There's a reactor type for spawning processes that may be useful for this sort of thing. See http://twistedmatrix.com/documents/current/api/twisted.internet.interfaces.IReactorProcess.html
    
    self.b_transfer_a_to_c should be called when we receive an AMI event stating that channels A and B are bridged. In 1.4, you can wait on the "Link" manager event, and in 1.6.2+ you can wait on the "Bridge" manager event with the BridgeState set to "Link".
    
    From here, you can wait for another AMI bridge command stating that A and C are bridged.
    
    Doing all this would get rid of the arbitrary wait times. You're still bound by the overall test timeout, but that's just something we have to live with.



/asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/run-test
<https://reviewboard.asterisk.org/r/1686/#comment9811>

    I feel like it would be better to separate this into two distinct methods. This way, you can hang up, wait on a manager hangup event, make sure that there are no bridges still up and no channels still up, and then kill the PJSUA processes.
    
    Also, sending an 'h' to both pja and pjc is weird. It'll possibly result in a BYE glare in Asterisk. That's a good thing to test but not when we're just focusing on making sure blind transfers are working. Just send the hangup command to one of the two and the call will be torn down properly.



/asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/test-config.yaml
<https://reviewboard.asterisk.org/r/1686/#comment9812>

    Both instances of "Phone B" in this sentence should be "Phone A"



/asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_with_reinvite/configs/ast1/sip.conf
<https://reviewboard.asterisk.org/r/1686/#comment9814>

    Why is "canreinvite=no" commented out on this test? It's not commented out on callee_refer_only.



/asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_with_reinvite/test-config.yaml
<https://reviewboard.asterisk.org/r/1686/#comment9813>

    "Phone B is hung up, while Phone A and C are bridged together."



/asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_with_reinvite/configs/ast1/sip.conf
<https://reviewboard.asterisk.org/r/1686/#comment9815>

    Another inconsistency in the canreinvite setting. This test has nothing specified, whereas caller_refer_only has canreinvite=no set.


- Mark


On Jan. 23, 2012, 8:19 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1686/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2012, 8:19 a.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> This patch adds four SIP blind transfer tests to the testsuite.  For Phone A, Phone B, and Phone C, where Phone A initially calls Phone B, they test:
> 1. Phone A initiating a blind transfer of Phone B to Phone C with no re-INVITE prior to the REFER message
> 2. Phone A initiating a blind transfer of Phone B to Phone C with a re-INVITE
> 3. Phone B initiating a blind transfer of Phone A to Phone C with no re-INVITE prior to the REFER message
> 4. Phone B initiating a blind transfer of Phone A to Phone C with a re-INVITE
> 
> Note that adding the 'h' extension currently reproduces the bug (ASTERISK-19173) fixed by Mark on patch https://reviewboard.asterisk.org/r/1685/, hence its inclusion in the test.
> 
> 
> This addresses bug ASTERISK-19173.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19173
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_with_reinvite/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_with_reinvite/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_with_reinvite/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_with_reinvite/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_refer_only/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_refer_only/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_refer_only/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_refer_only/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_with_reinvite/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_with_reinvite/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_with_reinvite/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_with_reinvite/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/tests.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/tests.yaml 3000 
> 
> Diff: https://reviewboard.asterisk.org/r/1686/diff
> 
> 
> Testing
> -------
> 
> Need to test with Mark's patch - without the patch, Scenarios 3 and 4 will fail, while Scenario 1 results in an orphaned bridge.  It is expected that the patch resolves all three of those issues.
> 
> 
> Thanks,
> 
> Matt
> 
>

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

More information about the asterisk-dev mailing list