<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/2139/">https://reviewboard.asterisk.org/r/2139/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 1st, 2012, 3:38 p.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;">So, a problem I have with this is that as far as I can tell, the mechanics of this test are still exactly the same as they were previously. The previous test had a 60 second timeout built in that's not here, but the TestCase has its own timeout built in instead, so that's really not a change. My fear is that this version of the test will bounce just as the previous version did.
The logs from test failures are likely long gone and the 'skip' line in the test-config.yaml file gave no indication to you why the test was failing, so it's hard to guess what might have been the problem. However, there are a couple of things about this test that throw up red flags for me.
1) The blind transfer in the dialplan. This seems like something that could easily be "missed" by Asterisk since the # is sent immediately after answering the call. It may be that the test was failing due to the blind transfer never actually occurring. I would suggest waiting for a Bridge event from Asterisk and then issuing an AMI redirect at that point. That eliminates some of the potential randomness from the test and would simplify the dialplan.
2) The file size check. It seems like a very unsophisticated way to determine if the AUDIOHOOK_INHERIT worked as expected. We have tools like test_events now that can be inserted into the code to tell for sure whether mixmonitor has stopped or whether an audiohook has been moved as expected. We didn't have that back when this test was originally written. Something more explicit than a file size check would likely make this test more stable.
Finally, I disapprove of the removal of the debugging messages that previously existed in this test. Those sort of messages are very helpful when trying to find out where or potentially why a test run has failed.</pre>
</blockquote>
<p>On October 1st, 2012, 4:16 p.m., <b>jrose</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;">From my local runs of the test, it wasn't just bouncing, it was failing every time. At first changing certain paths seemed to put it back on the right route, but as I started clearing away problems the whole thing started stalling and I eventually just couldn't figure out why it was stalling, so I kinda assumed the reason the test was being skipped was because it was just not working rather than that it was bouncing. I've tested it a decent number of times since then and it seems to be working now. I can go ahead and add a bit of a wait before sending the pound though, that's no problem.
If we want to actually make more significant changes to the test along the lines of how we check whether the mixmonitor has broken down once we've reached the end of the call, that's certainly doable.
...
In retrospect though, this test has been useless for determining the behavior of audiohook_inherit from the get-go because it's using a blind transfer, so there isn't going to be a masquerade. There is never really any risk of the mixmonitor falling off the call without having the audiohook inherit set anyway. I'll go ahead and change this to use an attended transfer. Oi.</pre>
</blockquote>
<p>On October 1st, 2012, 5:04 p.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;">Ah, I had been under the impression that the skipped tests were being skipped because they were bouncing, not because they were always failing. If that's the case, then I'd be willing to put the version you've made in and then monitor it for bounces. If it starts bouncing, then the test can be modified with the points I made above kept in mind.
You're right that in this case, a blind transfer won't involve a masquerade. Just to be clear, a blind transfer can involve a masquerade, but it's only if the party being transferred is not running a PBX. This is typically the case if the called party is the one that gets transferred. In fact, the AMI redirect I suggested won't work in this case since that won't involve a masquerade. You'll either have to change the test to
1) Put the Mixmonitor on the called party and then blind transfer or AMI redirect the called party or
2) Put the Mixmonitor on the calling party and then attended transfer the calling party.</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 don't even know how to put MixMonitor on the called party without relying on CLI commands to do so (at least in 1.8), so the attended transfer route seems to be the way to go.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 1st, 2012, 3:38 p.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/2139/diff/1/?file=31588#file31588line104" style="color: black; font-weight: bold; text-decoration: underline;">/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/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="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">100</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> def run(self):</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">59</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> if size > self.expected_filesize:</pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">101</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> self.ami_login()</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">60</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> LOGGER.info("Size %d is above the acceptable minimum of %d. Test Passed." % (size, self.expected_filesize))</pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">102</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> self.launch_test1()</pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">61</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> self.passed = True</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">62</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> else:</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">63</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> LOGGER.error("Size %d is below the acceptable minimum of %d. Test Failed." % (size, self.expected_filesize))</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;">This is really pedantic, but the error message in the else case could potentially print "Size 120000 is below the acceptable minimum of 120000. Test Failed."
I suggest either changing the if to use >= or to change the debug message to say "at or below".</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;">and got it.</pre>
<br />
<p>- jrose</p>
<br />
<p>On October 1st, 2012, 1:31 p.m., jrose 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 and Matt Jordan.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated Oct. 1, 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 part of an effort to get tests working that are currently being skipped due to failures. I went ahead and changed this test to use the TestCase class since the old way of doing things was driving me nuts with various problems.</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;">To test the test I ran the test with normal conditions, with an improper filename specified for the mixmonitor command, and also with turning off audiohook_inherit. All deviations caused failure as expected.</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/apps/mixmonitor_audiohook_inherit/test-config.yaml <span style="color: grey">(3480)</span></li>
<li>/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/configs/ast1/extensions.conf <span style="color: grey">(3480)</span></li>
<li>/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/configs/ast1/manager.conf <span style="color: grey">(3480)</span></li>
<li>/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/run-test <span style="color: grey">(3480)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2139/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>