<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1686/">https://reviewboard.asterisk.org/r/1686/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 23rd, 2012, 10:59 a.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 23rd, 2012, 10:59 a.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1686/diff/1/?file=23515#file23515line44" style="color: black; font-weight: bold; text-decoration: underline;">/asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/run-test</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">44</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> self.ast[0].cli_exec("core show channels")# if channels are still up for some reason, we want to know that as well</pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Channels are going to be up. Specifically, the A and C channels are going to be up because they're still bridged.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">That's okay - the bridged channels are what we're looking for here. I'll remove the comment, as its misleading.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 23rd, 2012, 10:59 a.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1686/diff/1/?file=23515#file23515line58" style="color: black; font-weight: bold; text-decoration: underline;">/asterisk/trunk/tests/channels/SIP/sip_blind_transfer/callee_refer_only/run-test</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">58</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> reactor.callLater(5, self.a_call_b)</pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">59</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> reactor.callLater(10, self.b_transfer_a_to_c)</pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">60</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> reactor.callLater(15, self.ami_check_bridge)</pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">61</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> reactor.callLater(20, self.read_result)</pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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".