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

Mon Jan 23 14:07:44 CST 2012

> On Jan. 23, 2012, 10:59 a.m., Mark Michelson wrote:
> > 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.

I disagree.  Most of the scenarios you have listed above are bridge layer tests - not SIP blind transfer tests.  While incredibly useful (and needed), once two channels are in a bridge (of whatever type they happen to be in), this test should no longer care - it succeeded in moving the bridge from A/B to (A/C|B/C).  Granted, sometimes the choice of bridge layer may affect the success/failure of the blind transfer, but I'd prefer to see that tested either through unit tests of the bridge layers, or through dedicated tests that check the functionality you listed above.  That isn't to say that we couldn't merge some of the bridge layer tests with this test just to try and make sure we get more of the corner cases, but there are some issues there.

a) With respect to remote bridging, resolving some localhost loopback addresses in Asterisk (127.0.0.2+) results in an INET family that is unspecified.  Asterisk then defaults to a local bridge, as opposed to a remote bridge.  For pjsua/SIPp, this can cause problems, as you sometimes need to bind on various loopbacks.  There are some potential workarounds to this - for example, the pjsua tests here all use 127.0.0.1 only and different ports - but there are some subtle issues to watch out for.
b) We don't have the best mechanism in place to change out an Asterisk configuration file in the middle of a test.  In general, a test installs the necessary Asterisk configuration files, spawns the Asterisk instances, runs the test, then shuts them down.  Restarting Asterisk instances, installing new configs, etc. is possible but would probably require some additional thought in the libraries that the testsuite is using to manage the Asterisk work - and is beyond the scope for this review.
c) Having a single test actually test a large number of permutations can be a good thing, but its a trade-off between keeping the number of your tests down (and reducing duplicate code) and increasing complexity of maintaining that single test.  For the permutations you've listed above, there would end up being 48 sub-tests, most of which would only be hitting some very small corner cases (at least with respect to SIP blind transfers - some of that functionality is important to the bridge layers).

At this point, the four tests on this patch hit the major SIP blind transfer scenarios and reproduce the regression.  I'd be fine with filing an issue to write tests for the bridging layers, and for possibly expanding how we handle these types of minor variations within a single test - but I don't see hacking these apart to run that many iterations or writing another 44 tests adding a lot.

> On Jan. 23, 2012, 10:59 a.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/run-test, line 44
> > <https://reviewboard.asterisk.org/r/1686/diff/1/?file=23515#file23515line44>
> >
> >     Channels are going to be up. Specifically, the A and C channels are going to be up because they're still bridged.

That's okay - the bridged channels are what we're looking for here.  I'll remove the comment, as its misleading.

> On Jan. 23, 2012, 10:59 a.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/run-test, lines 58-61
> > <https://reviewboard.asterisk.org/r/1686/diff/1/?file=23515#file23515line58>
> >
> >     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.

Nor am I.  This was based on some legacy SIP transfer tests.

I'll redo this based on AMI events.

> On Jan. 23, 2012, 10:59 a.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/run-test, lines 116-123
> > <https://reviewboard.asterisk.org/r/1686/diff/1/?file=23515#file23515line116>
> >
> >     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.

This is mostly just unneeded.  Only one client needs to hangup.  It can certainly be in a different method, and we can listen for the hangup event before stopping the test.  As far as chan_sip attempting to hang-up two channels at the same time, that's unlikely to cause any issues due to chan_sip's single threaded nature (and the fact that its something that should work, anyhow).  But yes, as you said, it should just be a single client hanging up.

> On Jan. 23, 2012, 10:59 a.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/test-config.yaml, lines 8-9
> > <https://reviewboard.asterisk.org/r/1686/diff/1/?file=23516#file23516line8>
> >
> >     Both instances of "Phone B" in this sentence should be "Phone A"

Whoops

> On Jan. 23, 2012, 10:59 a.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_with_reinvite/configs/ast1/sip.conf, line 2
> > <https://reviewboard.asterisk.org/r/1686/diff/1/?file=23518#file23518line2>
> >
> >     Why is "canreinvite=no" commented out on this test? It's not commented out on callee_refer_only.

Legacy debugging.  It should just be deleted.  The tests shouldn't care about this setting, as they should work regardless of its presence.

> On Jan. 23, 2012, 10:59 a.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_with_reinvite/test-config.yaml, lines 7-8
> > <https://reviewboard.asterisk.org/r/1686/diff/1/?file=23520#file23520line7>
> >
> >     "Phone B is hung up, while Phone A and C are bridged together."

Fixed

> On Jan. 23, 2012, 10:59 a.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_blind_transfer/caller_with_reinvite/configs/ast1/sip.conf, lines 1-3
> > <https://reviewboard.asterisk.org/r/1686/diff/1/?file=23526#file23526line1>
> >
> >     Another inconsistency in the canreinvite setting. This test has nothing specified, whereas caller_refer_only has canreinvite=no set.

See previous.

- Matt

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

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