<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/3329/">https://reviewboard.asterisk.org/r/3329/</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;">One thing I notice about this test is that it is structured to call the first extension, then when that finishes, call the second, then when that finishes, call the third, etc.
Since no call depends on any of the previous call results, I think this test could be changed to originate all the calls at the same time and evaluate the UserEvents and Hangups as they arrive. This would have a few benefits:
1) The test will execute more quickly
2) You can get rid of the can_call member of your test class.
3) Your AMI hangup handler will be simplified greatly since you won't need to have a big if-else ladder to figure out what to do next. In fact, you may be able to just get rid of it altogether.
Doing it this way kind of screws up your detected member, though. As a simple workaround, in your new_exten_event_handler, you can determine whether to send the AMI hangup by checking event['AppData'] for the name of the AGI script. If event['AppData'] is 'waiting.agi' or 'executing.agi' then you issue the AMI hangup.</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/3329/diff/1/?file=55783#file55783line9" style="color: black; font-weight: bold; text-decoration: underline;">./asterisk/trunk/tests/agi/exit_status/configs/ast1/extensions.conf</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">9</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> same => n,ExecIf($[${AGISTATUS}=NOTFOUND]?UserEvent(TestResult,result:pass))</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 recommend sending a UserEvent if the AGISTATUS is not what is expected. Something like the following:
same => n,UserEvent(TestResult,result:${IF($[${AGISTATUS}=NOTFOUND]?pass:fail)})
Since your test script prints a message when the result is not "pass", making this alteration can help to more easily debug which test cases are misbehaving.</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/3329/diff/1/?file=55786#file55786line31" style="color: black; font-weight: bold; text-decoration: underline;">./asterisk/trunk/tests/agi/exit_status/run-test</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">31</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">rm -f $testdir/userA/cdr-csv/Master.csv ; touch $testdir/userA/cdr-csv/Master.csv</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">31</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> can_call will only be set to true if an AGI script ran</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;">This sentence is incomplete.</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/3329/diff/1/?file=55786#file55786line32" style="color: black; font-weight: bold; text-decoration: underline;">./asterisk/trunk/tests/agi/exit_status/run-test</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">32</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">32</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> detected is a temporary variable to deal with ASTERISK-23390</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;">Even though I'm advocating to just get rid of this member altogether, I think it's worth mentioning that the name could be improved. Since this is intended to indicated that the testsuite is supposed to interrupt the current AGI, something like "interrupt_agi" would probably be more clear.</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/3329/diff/1/?file=55786#file55786line88" style="color: black; font-weight: bold; text-decoration: underline;">./asterisk/trunk/tests/agi/exit_status/run-test</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">88</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span>fi</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">88</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> if event['context'] == 'waiting-extens':</pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">89</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">done</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">89</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> LOGGER.info("waiting.agi behaved as expected.")</pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">90</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">90</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> else:</pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">91</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "># Did we succeed?</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">91</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> LOGGER.info("executing.agi behaved as expected.")</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;">Change the else to explicitly check if event['context'] == 'executing-extens'
Add a final else clause that logs an error if an unexpected extension ends up calling the userevent from the h extension. Structure this so that success_count does not get incremented if this final else case is executed.</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/3329/diff/1/?file=55786#file55786line93" style="color: black; font-weight: bold; text-decoration: underline;">./asterisk/trunk/tests/agi/exit_status/run-test</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">93</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span>failure</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">93</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> LOGGER.info("Successful tests: %s" % self.success_count)</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;">Use %d since self.success_count is an integer.</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/3329/diff/1/?file=55786#file55786line96" style="color: black; font-weight: bold; text-decoration: underline;">./asterisk/trunk/tests/agi/exit_status/run-test</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">96</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span>exit 1</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">96</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> if self.success_count == 7:</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;">This use of 7 here is what is sometimes referred to as a "magic number" in programming, and it's not really a good thing. If this test were to be altered later to test some more situations or to remove some test cases, then whoever made the modifications would have to go through the source and change all of the 7's to 8's or 6's or whatever.
Instead, at the top of the file, declare a global variable, something like:
EXPECTED_SUCCESSES = 7
Then, everywhere where you are currently comparing to 7, just compare to EXPECTED_SUCCESSES instead.
In addition to the code maintainability I mentioned earlier, it also is more expressive; readers of the code will easily understand that you are comparing to the number of expected successes rather than a number they have to figure out the significance of.
In general, prefer symbolic names for things over bare numerals. Obviously, as with every rule, there are exceptions to this.</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/3329/diff/1/?file=55786#file55786line174" style="color: black; font-weight: bold; text-decoration: underline;">./asterisk/trunk/tests/agi/exit_status/run-test</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">174</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> LOGGER.info("The required amount of successful tests was not met.")</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;">Change this to an error instead of info, and print the value of test.success_count.</pre>
</div>
<br />
<p>- Mark Michelson</p>
<br />
<p>On March 11th, 2014, 7:54 p.m. UTC, Benjamin Keith Ford 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 Benjamin Keith Ford.</div>
<p style="color: grey;"><i>Updated March 11, 2014, 7:54 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-19167">ASTERISK-19167</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;">Notes:
- Is a sub-task of ASTERISK-19167
- Ignore userA directory; will be removed from repository
This test runs through a few different AGI scripts to verify that AGISTATUS returns the correct values:
1. Attempts to run an AGI script that does not exist. AGISTATUS returns NOTFOUND.
2. Attempts to run an AGI script that has an invalid path. AGISTATUS returns FAILURE.
3. Attempts to run an AGI script that has a non-executable interpreter. AGISTATUS returns FAILURE.
4. Attempts to run an AGI script that is non-executable. AGISTATUS returns FAILURE.
5. Runs an AGI script that will be hung up on while waiting for a command. AGISTATUS returns HANGUP.
6. Runs an AGI script that will be hung up on while executing a command. AGISTATUS returns HANGUP.
7. Runs an AGI script that exits normally. AGISTATUS returns SUCCESS.</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/agi/exit_status/waiting.agi <span style="color: grey">(PRE-CREATION)</span></li>
<li>./asterisk/trunk/tests/agi/exit_status/test-config.yaml <span style="color: grey">(4749)</span></li>
<li>./asterisk/trunk/tests/agi/exit_status/run-test <span style="color: grey">(4749)</span></li>
<li>./asterisk/trunk/tests/agi/exit_status/executing.agi <span style="color: grey">(PRE-CREATION)</span></li>
<li>./asterisk/trunk/tests/agi/exit_status/donothing.agi <span style="color: grey">(PRE-CREATION)</span></li>
<li>./asterisk/trunk/tests/agi/exit_status/configs/ast1/extensions.conf <span style="color: grey">(PRE-CREATION)</span></li>
<li>./asterisk/trunk/tests/agi/exit_status/badinterpreter3.agi <span style="color: grey">(PRE-CREATION)</span></li>
<li>./asterisk/trunk/tests/agi/exit_status/badinterpreter2.agi <span style="color: grey">(PRE-CREATION)</span></li>
<li>./asterisk/trunk/tests/agi/exit_status/badinterpreter.agi <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3329/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>