<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 29th, 2012, 10:39 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 think you can get rid of the file size check from this test entirely. In my view, the goal of this test is to ensure that the MixMonitor continues recording after the transfer occurs. Your test event coupled with your user event ensure that is the case. This test shouldn't care about the actual recording since there is already a MixMonitor test that ensures that MixMonitor records audio to a file.</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;">See, this reminds me of something I was just looking at last week. Confbridge uses mixmonitor to record itself, and Terry's big confbridge state patch introduced an issue where the mixmonitor thread wouldn't be initiated when using the CLI and AMI commands to start recording a conference. However, another function would be executed which would send events indicating that the recording had started. No tests that we have currently written picked up on this error, but if one used the record feature and used a file check like this instead of or in addition to observing the events, catching this error would have been trivial.
In short, what I'm getting at is that it's perfectly feasible for our test events to fail us. Having a check for the file gives us something a little more concrete. For that reason, I don't really want to get rid of the filesize check all together. I wouldn't be averse to simplifying it to not care so much about specific file sizes (anything greater than 2000 would probably be fine), but I worry about ditching that aspect of the test all together.</pre>
<br />
<p>- jrose</p>
<br />
<p>On October 23rd, 2012, 2:54 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. 23, 2012, 2:54 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/configs/ast1/extensions.conf <span style="color: grey">(3496)</span></li>
<li>/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/configs/ast1/features.conf <span style="color: grey">(PRE-CREATION)</span></li>
<li>/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/configs/ast1/manager.conf <span style="color: grey">(3496)</span></li>
<li>/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/run-test <span style="color: grey">(3496)</span></li>
<li>/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/test-config.yaml <span style="color: grey">(3496)</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>