[asterisk-dev] [Code Review] 3733: Added a pluggable module for pluggable_modules.py that allows the user to test different attributes of a given sound file

[Matt Jordan]

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



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23198>

    You can rewrite this as:
    
    self.auto_stop = module_config.get('auto-stop', False)
    
    I'd make auto_stop a boolean value and not a string (since that is what it is)



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23199>

    This can be re-written as:
    
    asterisk_instance = self.module_config[self.index].get('id', 0)



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23202>

    This assignment isn't necessary.
    
    When this function actually calculates the size of the file, you won't need to have this be a member variable at all.



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23200>

    It's always good to print out what actually happened as opposed to just the error message. I'd print out:
    
    LOGGER.error("File '%s' failed size check: expected %d, actual %d (tolerance +/- %d)" %
                 (self.filepath, size, filesize, tolerance))



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23203>

    This should just be a boolean value:
    
    if self.auto_stop:



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23204>

    I'm not sure why you're popping 'type' off of the action here.



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23205>

    I would register for the event prior to originating the call. That avoids any potential race conditions where the call proceeds much faster than the test.



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23206>

    This should just be a boolean check:
    
    if self.auto_stop:



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23209>

    This is not always true, since you may bail out here if another UserEvent is fired. This should only be logged out once you know for sure this is the UserEvent you want.



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23210>

    Don't de-register your handler until you know this is the UserEvent you want. This should come after your checks that bail out of the function.



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23208>

    If userevent is not in the event, this will crash, as you'll be calling lower() on a None object.
    
    Instead, you should do the following:
    
    userevent = event.get("userevent")
    if not userevent:
        return
    
    if userevent.lower() != "soundcheck":
        return
    
    



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23207>

    This should just be a boolean check:
    
    if self.auto_stop:



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23212>

    Rather than constantly indexing into self.module_config, pull out the config you want to work with:
    
    config = self.module_config[self.index]
    
    And then use that.



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23211>

    This check is overly complicated, since we can provide a default value for 'id' if the user didn't provide one.
    
    I would write this instead as:
    
    id = self.module_config[self.index].get('id', 0)
    if (ami.id != id):
        return
    
    



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23201>

    This should not be done here. The actual check for the filesize should be done in size_check.
    
    That removes the need to have filesize be a member variable.



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23213>

    I wouldn't check for the file size here, and I would invert the logic:
    
    if not os.path.exists(self.filepath):
        LOGGER.error("File '%s' does not exist!" % self.filepath)
        self.test_object.set_passed(False)
        ...
        return
    
    self.actions = self....



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23214>

    current_trigger will already throw an exception if 'trigger' and 'match' are not provided. Most of these exceptions are not necessary; simply index into the trigger and get the values.
    
    



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23215>

    Why do you care if a channel is provided?
    
    The user should be able to trigger a sound file check regardless of having a channel in the event.



/asterisk/trunk/lib/python/asterisk/pluggable_modules.py
<https://reviewboard.asterisk.org/r/3733/#comment23216>

    This check should be done earlier:
    
    id = current_trigger.get('id', 0)
    if ami.id != id:
        return


- Matt Jordan


On July 22, 2014, 1:46 p.m., Christopher Wolfe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3733/
> -----------------------------------------------------------
> 
> (Updated July 22, 2014, 1:46 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24010
>     https://issues.asterisk.org/jira/browse/ASTERISK-24010
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> Allows the user to test a recorded sound file in many different ways:
> 1) This test ALWAYS gets done- Checks whether the given sound file exists using a predefined path that can be created by either explicitly defining a filepath by declaring the filepath type as defined, or using a default path that is relative to the current test's var/spool/asterisk folder. From there, the user can add extensions to the file name to tack on relative folders (monitor/testaudio.wav being an example).
> 2) Optional- The sound file is checked whether it fits within a certain size criteria (measured in bytes).  A basis size and degree of size tolerance are determined by the user.  For example, if the size was 500000 and the tolerance was set to 50000, then the sound file's size would need to be somewhere between 450000 and 550000 in order to pass that test.
> 3) Optional- The sound file's sound energy levels are checked.  This is done by creating a Local channel that should get sent to a dialplan extension that should contain a BackgroundDetect application that fits the user's specifications. The variable that will be used to pass the sound file must be called SOUNDFILE, and a UserEvent must give off the name soundcheck in order for the event to be picked up.  A sample extension:
> [soundtest]
> exten => audio,1,Answer()
> same => n,Set(TALK_DETECTED=0)
> same => n,BackgroundDetect(${SOUNDFILE},1,20,,20000)
> same => n,GoToIf($[${TALK_DETECTED}=0]?pass:fail)
> same => n(fail),UserEvent(soundcheck, status: pass)
> same => n,Hangup()
> 
> same => n(pass),UserEvent(soundcheck, status: fail)
> same => n,Hangup()
> 
> A sound-file test only gets called when a specified trigger has gone off.  So far, this pluggable module only supports events as triggers.  The list of triggers matches to each instance of a sound-file test on a one-to-one basis (the first trigger starts the first test, and so on).
> Only passes after all tests specified by the user have been passed and the correct triggers have been received.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/sample-yaml/sound-check-config.yaml.sample PRE-CREATION 
>   /asterisk/trunk/lib/python/asterisk/pluggable_modules.py 5249 
> 
> Diff: https://reviewboard.asterisk.org/r/3733/diff/
> 
> 
> Testing
> -------
> 
> - Tried using the pluggable module by putting in incorrect input and purposefully leaving out input.  Picks those errors up.
> - Not sure if I was supposed to allow the user to name their own dialplan variable and Userevent name, so I left it as was.
> - Tested the different scenarios of setting the filepath- relative and defined.
> - Made sure the various tests could fail if a certain sound file didn't meet the size criteria or silence threshold criteria.
> - Made sure that more than one test could be run and that things could be run sequentially.
> 
> 
> Thanks,
> 
> Christopher Wolfe
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140724/a6d202d6/attachment-0001.html>