[asterisk-dev] [Code Review] New dialplan application VMSayname with unit test

[Jeff Peeler]

> On 2010-02-24 17:40:19, Mark Michelson wrote:
> > /trunk/apps/app_voicemail.c, lines 11635-11636
> > <https://reviewboard.asterisk.org/r/530/diff/1/?file=8279#file8279line11635>
> >
> >     Argh! My eyes!
> >     
> >     static const char TEST_CONTEXT[] = ...
> >     static const char TEST_EXTENSION[] = ...

I was doing some string concatenation with the defines, but I guess only twice is not enough for an eye sore.


> On 2010-02-24 17:40:19, Mark Michelson wrote:
> > /trunk/apps/app_voicemail.c, lines 11658-11659
> > <https://reviewboard.asterisk.org/r/530/diff/1/?file=8279#file8279line11658>
> >
> >     Would a call to ast_dummy_channel_alloc work here?

No, the audio playback requires some timing operations that are not available with a dummy channel.


> On 2010-02-24 17:40:19, Mark Michelson wrote:
> > /trunk/apps/app_voicemail.c, lines 11677-11681
> > <https://reviewboard.asterisk.org/r/530/diff/1/?file=8279#file8279line11677>
> >
> >     I think the use of the RETRIEVE and DISPOSE macros are unnecessary here since you are not expecting to actually find the greeting for this extension and context in ODBC or IMAP storage.
> >     
> >     I suppose the dynamic of this test could change a bit though if the manipulation you do on the file system down below were instead done in such a way that required storing and retrieving the files from IMAP or ODBC. I think that should be a separate unit test though...

Good point, I was thinking later on that ODBC/IMAP tests could be later added, but it probably does indeed belong in a separate test. Removed!


> On 2010-02-24 17:40:19, Mark Michelson wrote:
> > /trunk/apps/app_voicemail.c, lines 11696-11700
> > <https://reviewboard.asterisk.org/r/530/diff/1/?file=8279#file8279line11696>
> >
> >     Have you tried using:
> >     
> >     snprintf(dir2, sizeof(dir2), "%s%s", VM_SPOOL_DIR, TEST_CONTEXT);
> >     remove(dir2);
> >     
> >     I'm not sure if remove(3), when used on a directory, will complain about the directory having contents within.

I have and from what I understand remove actually calls unlink and rmdir. I guess there isn't a better way.


- Jeff


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


On 2010-02-24 16:19:09, Jeff Peeler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/530/
> -----------------------------------------------------------
> 
> (Updated 2010-02-24 16:19:09)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch adds a new application, VMSayName, which plays the greeting for a given voicemail user specified by extension and optionally context. If the greeting does not exist, currently the extension is played back.
> 
> 
> This addresses bug 14973.
>     https://issues.asterisk.org/view.php?id=14973
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_voicemail.c 248659 
> 
> Diff: https://reviewboard.asterisk.org/r/530/diff
> 
> 
> Testing
> -------
> 
> A unit test has been created to test the code paths used for playback in the case when the greeting is present and when it is not. Because the application is really just a wrapper function, the unit test is what needs to be examined more so than anything else.
> 
> 
> Thanks,
> 
> Jeff
> 
>