[asterisk-dev] [Code Review]: Initial tests for app_voicemail

[mjordan]

> On Aug. 10, 2011, 1:35 p.m., Paul Belanger wrote:
> > /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast1/extensions.conf, lines 11-17
> > <https://reviewboard.asterisk.org/r/1354/diff/2/?file=17850#file17850line11>
> >
> >     minor, but indent these.  Easier to read.

Done


> On Aug. 10, 2011, 1:35 p.m., Paul Belanger wrote:
> > /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast1/manager.general.conf.inc, line 2
> > <https://reviewboard.asterisk.org/r/1354/diff/2/?file=17851#file17851line2>
> >
> >     This can be removed, but bind to 127.0.0.1 by default

Done


> On Aug. 10, 2011, 1:35 p.m., Paul Belanger wrote:
> > /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast1/sip.conf, line 8
> > <https://reviewboard.asterisk.org/r/1354/diff/2/?file=17852#file17852line8>
> >
> >     Are you host settings correct for your peers?  Now that you have 2 instances?

I believe so - one instance of manager should be spawned on 127.0.0.1, the second on 127.0.0.2.  This follows the convention of the other apps/directory_* tests, as well as the others I've found that use two instances of Asterisk.


> On Aug. 10, 2011, 1:35 p.m., Paul Belanger wrote:
> > /asterisk/trunk/tests/apps/voicemail/voicemail.py, lines 1-9
> > <https://reviewboard.asterisk.org/r/1354/diff/2/?file=17861#file17861line1>
> >
> >     Might as well move this into lib/python/asterisk since other tests could use it.

Will do


> On Aug. 10, 2011, 1:35 p.m., Paul Belanger wrote:
> > /asterisk/trunk/tests/apps/voicemail/voicemail.py, line 35
> > <https://reviewboard.asterisk.org/r/1354/diff/2/?file=17861#file17861line35>
> >
> >     We should avoid hard coding asterisk paths, with an Asterisk instance you can access ast1.directories['spooldir'] to read the asterisk.conf value.

Fixed


> On Aug. 10, 2011, 1:35 p.m., Paul Belanger wrote:
> > /asterisk/trunk/tests/apps/voicemail/voicemail.py, line 40
> > <https://reviewboard.asterisk.org/r/1354/diff/2/?file=17861#file17861line40>
> >
> >     If we can avoid hardcoding the path for the test, it would be nice.  Maintenance issue

This one I'm not sure I can do.  It is arbitrary, granted, but the best path we have now is to the running directory through os.getcwd().  Note that this is for the same reason that MixMonitor hard codes its path - the sound file.


> On Aug. 10, 2011, 1:35 p.m., Paul Belanger wrote:
> > /asterisk/trunk/tests/apps/voicemail/voicemail.py, line 264
> > <https://reviewboard.asterisk.org/r/1354/diff/2/?file=17861#file17861line264>
> >
> >     blob

Doh


> On Aug. 10, 2011, 1:35 p.m., Paul Belanger wrote:
> > /asterisk/trunk/tests/apps/voicemail/voicemail.py, line 272
> > <https://reviewboard.asterisk.org/r/1354/diff/2/?file=17861#file17861line272>
> >
> >     only function missing a comment

Missing a comment because its private.  The public functions are all commented; if you're calling this directly you're bypassing the stuff that checks that the folder exists, etc.


> On Aug. 10, 2011, 1:35 p.m., Paul Belanger wrote:
> > /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/run-test, line 33
> > <https://reviewboard.asterisk.org/r/1354/diff/2/?file=17857#file17857line33>
> >
> >     You should be able to access this information from the TestCase class.  If not, we can add it.

Its not in TestCase currently.  As it turns out, I ended up removing the need for it when I cleaned up the asterisk instances - I'll just remove it.


- mjordan


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


On Aug. 10, 2011, 11:08 a.m., mjordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1354/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2011, 11:08 a.m.)
> 
> 
> Review request for Asterisk Developers and Paul Belanger.
> 
> 
> Summary
> -------
> 
> Added initial test for app_voicemail.  This just tests the nominal path for dialplan function VoiceMail().
> 
> Additional tests will be added for the rest of the dialplan applications and their features.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/lib/python/asterisk/asterisk.py 1803 
>   /asterisk/trunk/tests/apps/tests.yaml 1803 
>   /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast1/manager.general.conf.inc PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast1/voicemail.conf PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast2/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast2/manager.general.conf.inc PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/configs/ast2/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/run-test PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/leave_voicemail_nominal/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/sounds/talking.ulaw PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/tests.yaml PRE-CREATION 
>   /asterisk/trunk/tests/apps/voicemail/voicemail.py PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1354/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> mjordan
> 
>

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