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

[Mark Michelson]

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


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.


/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/run-test
<https://reviewboard.asterisk.org/r/2139/#comment13836>

    If you're going to update the copyright, change it to be
    
    2010-2012



/asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/run-test
<https://reviewboard.asterisk.org/r/2139/#comment13843>

    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".


- Mark


On Oct. 1, 2012, 1:31 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2139/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2012, 1:31 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/test-config.yaml 3480 
>   /asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/configs/ast1/extensions.conf 3480 
>   /asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/configs/ast1/manager.conf 3480 
>   /asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/run-test 3480 
> 
> 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/20121001/f502a708/attachment.htm>