[asterisk-dev] [Code Review]: Get mixmonitor_audiohook_inherit test working again

[Mark Michelson]

> On Oct. 29, 2012, 10:39 a.m., Mark Michelson wrote:
> > 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.
> 
> jrose wrote:
>     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.

Okay, having a safety net in place to determine if your events are inaccurate vs. whether the feature is actually working makes sense. My main concern was with giving the test a way to potentially (falsely) fail. My suggestion regarding file size checking is to be sure to give plenty of tolerance.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2139/#review7330
-----------------------------------------------------------


On Oct. 29, 2012, 12:06 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2139/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2012, 12:06 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/configs/ast1/extensions.conf 3496 
>   /asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/configs/ast1/features.conf PRE-CREATION 
>   /asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/configs/ast1/manager.conf 3496 
>   /asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/run-test 3496 
>   /asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/test-config.yaml 3496 
> 
> Diff: https://reviewboard.asterisk.org/r/2139/diff
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20121029/f4d3449e/attachment-0001.htm>