<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/2985/">https://reviewboard.asterisk.org/r/2985/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 7th, 2013, 4:30 p.m. CST, <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;">Most of my critiques for this test addition are high-level issues as opposed to problems with the individual lines of code.
1) Be sure when writing new tests in python to run something like pylint or flake8 (which has a vim plugin) to ensure that the code is PEP-8 friendly.
2) Don't leave commented out code in your review. Comments that explain what is going on are great, but commented out code does not belong.
3) I'm not a fan of the use of the pcap library for outbound_reregister_from. The more common Asterisk testsuite method of ensuring that requests from Asterisk are structured as expected is to use SIPp to receive the request and use regex checking in the SIPp scenario to ensure that what is received is what is expected. The advantages of doing things this way over the method you have used are:
a) The outbound_reregister_from test has a reactor timeout of 5 minutes. If using a SIPp scenario as the basis of the test, you can end the test as soon as the first re-registration transaction completes.
b) Since SIPp usage is more integrated into the test suite, for this sort of test you wouldn't actually have to write any python code at all. Your test-config.yaml file would just need to indicate the SIPp scenario(s) to run and the testsuite will take care of the rest.
I'm also a bit curious to know if the Bamboo agents that automatically run the testsuite have pcap support and run as root.
4) What is the purpose of the tests/channels/SIP/outbound_reregister_from/3 file?
5) I'm not a fan of tests that start out passing and that fail as a result of incorrect data. I have two main reasons for disliking this method:
a) The expected behavior of the test is made vague. I'm actually kind of in the dark about what the outbound_register_from test is intended to do. Is it expected to retransmit the same REGISTER for the entire test? Or is it expected to retransmit a REGISTER for a while and then try sending (and retransmitting) a new REGISTER?
b) The test can pass under circumstances where it should really fail. To give an example, if there is some bug in chan_sip.c that causes it to never send out any REGISTER requests, these tests will pass because the pcap callback would never be called.
</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 understand your hesitance regarding pcap, I chose it to get to a functioning test as quickly as possible rather than larger design considerations, and for that it served it's purpose in aiding me to explore the original issue, create the patch, and confirm it was correct.
The purpose of the 5 minute timeout is to allow a sequence of a successful registration followed by a number of re-registrations, and in each case insure that for each Call-ID there is no more than one unique From tag. Since for this test, Call-ID doesn't change (only one registration), that could be reduced to insuring a consistent From tag. The chan_sip settings adjusting expiry cause Asterisk to re-register several times during the 5 minute timeout, with the test failing if a change in From-tag is noticed. I am agreed that it would be better to test for the success case rather than the failure case - even if the code resulted in slightly more convoluted logic, it would be easier to understand. Also, I should have documented what the goal and function of the test was better. In retrospect, changing the test to set success and bail out early after x number of re-registrations would have been a much better approach.
The 3 file was an error and should not have been included.
I do not know if the Bamboo agents will be able to run the pcap tests, but will endeavor to find a way to use SIPp or other non-root method to perform this test, and then submit it in place of this review request, having checked the code first.</pre>
<br />
<p>- Scott</p>
<br />
<p>On November 1st, 2013, 4:11 p.m. CDT, Scott Griepentrog wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Scott Griepentrog.</div>
<p style="color: grey;"><i>Updated Nov. 1, 2013, 4:11 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-12117">ASTERISK-12117</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
testsuite
</div>
<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 test has Asterisk sending repeated REGISTERs to loopback and monitors them with pcap. The Call-ID and From headers are extracted from the SIP messages to look for a change in From on the same dialog.
This test borrows code from pcap_demo, and must be run as root.
</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/tests.yaml <span style="color: grey">(4310)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/outbound_reregister_from/test-config.yaml <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/outbound_reregister_from/run-test <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/outbound_reregister_from/configs/ast2/sip.conf <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/outbound_reregister_from/configs/ast1/sip.conf <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/outbound_reregister_from/3 <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/outbound_register_from/test-config.yaml <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/outbound_register_from/run-test <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/channels/SIP/outbound_register_from/configs/ast1/sip.conf <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2985/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>