[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/#review12670
-----------------------------------------------------------



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

    Remove this stray line



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

    Two technicalities with pydoc comments:
    
    1. You should not have a space between the start of a pydoc comment and the summary:
    """This class allows the user ...
    
    2. The first line should be a summary, and should typically be one line only. A fuller description can be on subsequent lines.
    
    See http://legacy.python.org/dev/peps/pep-0008/#documentation-strings for more information.



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

    Rename sound_file_instances to "instance_config":
    
    self.instance_config = instance_config
    
    Better: "module_config":
    
    self.module_config = module_config
    
    You'll have more in there then just sound files to verify.



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

    I don't think you need this check. If someone didn't provide a sound_file_instances object in their configuration, they are way off base and you should let an exception fire later.



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

    These functions aren't necessary, as you shouldn't be stopping the test on a single pass or failure condition. You should be stopping the test:
    (a) Only if configured to do so
    (b) Only when all checks have completed
    
    Instead of having these methods, just call set_passed on the test_object appropriately where these are currently called.



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

    pydoc: Put the summary on the same line as """



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

    This doesn't need to specify a member variable. Instead it should just calculate and return the name of the file to go find.
    
    Defer setting a member variable to the function that calls it (if it is still needed)



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

    Don't break up indexing a variable onto multiple lines. On first inspection, this looks like you are missing commas.
    
    If you broke this up to avoid a PEP8 line length warning, perform the look up outside of the formatting:
    
    ast_instance = self.test_object.ast[asterisk_instance]
    base_path = ast_instance.base
    spool_dir = ast_instance.directories["astspooldir"]
    
    self.filepath = ...
    



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

    This assignment isn't needed; just use self.filesize



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

    This should be a WARNING or ERROR



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

    Don't call sound_check_actions here. This should simply perform the size_check.



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

    So you can actually be tricky here and do something a bit niftier:
    
    action = self.actions[self.action_index]
    action['variable']={'SOUNDFILE': energyfile}
    dfr = ami.originate(**action)
    
    The ** will apply the dictionary as keyword arguments to the originate function. So long as the contents of that dictionary map to the keyword arguments of the function, it will work just fine.



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

    I'd structure this differently:
    
    (1) First, try to get rid of your indexes. Generally, you should be popping the current thing to go 'execute' off the list of sound files/actions, and passing that item to the functions that do the work.
    
    (2) Next, don't do two things in a function. This is iterating both through the sound files to check, as well as the actions. This should just be handling executing the next action to run - leave whether or not all sound files have been executed to whatever calls this. (As an aside, this function should take in two parameters - the actions to execute, as well as the ami parameter)
    
    (3) In this function, you can do something like the following to execute the next action:
    
    action = actions.pop()
    
    if action['type'] == 'size_check':
        self.size_check(action, ami)
    elif action['type'] == 'energy_check':
        self.energy_check(action, ami)
    else:
        LOGGER.ERROR('Unknown action type: %s' % action['type'])
        raise ValueError()
    
    if (len(self.actions) == 0):
        # All done!
        return False
    else:
        return True
    
    The caller of this should continue to call it until False is returned.
    
    (4) Update your size_check and energy_check functions to take in the action and AMI object. That way, they don't have to do all the indexing as well.



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

    I wouldn't call this recursively here. Simply return when this check is finished.



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

    Two problems with this approach:
    
    (1) If you're constantly casting this to an int, then just make the config provide an int.
    
    (2) Casting None to an int (which is what get can return) will throw an Exception. If not specifying the id is not allowed, then don't use get, just index directly.
    
    That being said, you should allow the user to not have to specify the AMI ID:
    
    if ami.id != self.sound_file_instances.get('id', 0):
        return
    



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

    Don't break if statements up and put the next line with equivalent indentation:
    
    if (not self.sound_file.get('file_name')
        or not self.sound_file.get('file_path_type')):
    
    



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

    (1) Rename set_sound_file to get_sound_file_location or build_sound_file_location (something like that)
    
    (2) Change usage to:
        sound_file = self.build_sound_file_location(self.sound_file['file_name'],
                                                    self.sound_file['file_path_type'],
                                                    self.sound_file['defined_path'])
    



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

    There's a lot of member attribute usage where variables should not need to be a member attribute. A variable should only be a class member when the class has a relationship to that attribute; here, this class (being the pluggable module) is keeping track of a lot of state that it shouldn't be the owner of.
    
    I would pull the action out of the actions to execute, and pass that off to a function to go execute them:
    
    actions = self.sound_file.get('actions')
    self.execute_actions(actions, ami)


- Matt Jordan


On July 9, 2014, 1:19 p.m., Christopher Wolfe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3733/
> -----------------------------------------------------------
> 
> (Updated July 9, 2014, 1:19 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/tests/apps/mixmonitor/testaudio2.raw UNKNOWN 
>   /asterisk/trunk/tests/apps/mixmonitor/testaudio1.raw UNKNOWN 
>   /asterisk/trunk/sample-yaml/sound-check-config.yaml.sample PRE-CREATION 
>   /asterisk/trunk/lib/python/asterisk/pluggable_modules.py 5168 
> 
> 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/20140717/1a71709d/attachment-0001.html>