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

[jrose]

> On Oct. 1, 2012, 3:38 p.m., Mark Michelson wrote:
> > 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.
> 
> jrose wrote:
>     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.
> 
> Mark Michelson wrote:
>     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.

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.


> On Oct. 1, 2012, 3:38 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/apps/mixmonitor_audiohook_inherit/run-test, lines 104-108
> > <https://reviewboard.asterisk.org/r/2139/diff/1/?file=31588#file31588line104>
> >
> >     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".

and got it.


- jrose


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


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/57a2ea2a/attachment-0001.htm>