<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/2131/">https://reviewboard.asterisk.org/r/2131/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Only a couple of minor python nitpicks.
Thanks for cleaning up the test!</pre>
<br />
<div>
<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/2131/diff/3/?file=31441#file31441line28" style="color: black; font-weight: bold; text-decoration: underline;">/asterisk/trunk/tests/channels/SIP/sip_outbound_address/originator.py</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">def __init__(self, module_config, test_object):</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">28</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="bp">self</span><span class="o">.</span><span class="n">ami</span> <span class="o">=</span> <span class="bp">None</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't see anyone checking ami for None later on?</pre>
</div>
<br />
<div>
<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/2131/diff/3/?file=31441#file31441line33" style="color: black; font-weight: bold; text-decoration: underline;">/asterisk/trunk/tests/channels/SIP/sip_outbound_address/originator.py</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">def __init__(self, module_config, test_object):</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">33</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Don't return at the end of the method unless you have something to return.</pre>
</div>
<br />
<div>
<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/2131/diff/3/?file=31441#file31441line38" style="color: black; font-weight: bold; text-decoration: underline;">/asterisk/trunk/tests/channels/SIP/sip_outbound_address/originator.py</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">def __init__(self, module_config, test_object):</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">38</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">As above.</pre>
</div>
<br />
<div>
<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/2131/diff/3/?file=31441#file31441line53" style="color: black; font-weight: bold; text-decoration: underline;">/asterisk/trunk/tests/channels/SIP/sip_outbound_address/originator.py</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">def __init__(self, module_config, test_object):</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">53</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="bp">self</span><span class="o">.</span><span class="n">ami</span><span class="o">.</span><span class="n">originate</span><span class="p">(</span><span class="n">channel</span> <span class="o">=</span> <span class="s">'Local/</span><span class="si">%s</span><span class="s">@default'</span> <span class="o">%</span></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">54</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">DESTINATIONS</span><span class="p">[</span><span class="bp">self</span><span class="o">.</span><span class="n">current_destination</span><span class="p">],</span></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">55</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">application</span> <span class="o">=</span></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">56</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="s">'Echo'</span><span class="p">)</span><span class="o">.</span><span class="n">addCallback</span><span class="p">(</span><span class="bp">self</span><span class="o">.</span><span class="n">success</span><span class="p">)</span><span class="o">.</span><span class="n">addErrback</span><span class="p">(</span><span class="n">failure</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I prefer this; pep8 valid and still only 4 lines:
dest = DESTINATIONS[self.current_destination]
deferred = self.ami.originate(channel='Local/%s@default' % dest,
application='Echo')
deferred.addCallback(self.success).addErrback(failure)
(and moving the dest assignment up will clean up the LOGGER.info too)</pre>
</div>
<br />
<p>- wdoekes</p>
<br />
<p>On September 25th, 2012, 1:31 p.m., Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated Sept. 25, 2012, 1:31 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is a rewrite of the sip_outbound_address test. This test had a "skip" flag set on it in apparently because the test was bouncing. Unfortunately, not having this test enabled resulted in the issue ASTERISK-19677.
The test was likely bouncing due to some odd timing being used (combination of Wait() dialplan app plus some sleep() calls in the test code). I've rewritten the test to no longer use Lua and instead use Python. It uses the SIPpTestCase. This test becomes either the second or third to have an "originator" module used alongside the SIPpTestCase. Perhaps it's time to consider making this a module central to the test suite?
As far as the test details are concerned, the test mostly is the same. The same extensions.conf is used and the same destinations are dialed. Now, instead of having the SIPp scenario that answers send back DTMF, the SIPp scenario simply answers and then hangs up. The test can be determined to pass or fail based on whether the SIPp scenario succeeds or fails. It will fail if Asterisk were to route the request to the wrong destination. This has made the test much simpler than it previously was.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Test passes with current 1.8 version of Asterisk.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/configs/ast1/extensions.conf <span style="color: grey">(3480)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/configs/ast1/sip.conf <span style="color: grey">(3480)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/originator.py <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/run-test <span style="color: grey">(3480)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/sipp/dtmf_2833_1.pcap <span style="color: grey">(UNKNOWN)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/sipp/dtmf_2833_2.pcap <span style="color: grey">(UNKNOWN)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/sipp/dtmf_2833_pound.pcap <span style="color: grey">(UNKNOWN)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/sipp/uas.xml <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/sipp/uas1.xml <span style="color: grey">(3480)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/sipp/uas2.xml <span style="color: grey">(3480)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/test-config.yaml <span style="color: grey">(3480)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/sip_outbound_address/test.lua <span style="color: grey">(3480)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2131/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>