[asterisk-dev] [Code Review] Generic AMI Module for Events

[Matt Jordan]

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



/asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/TestRunner.py
<https://reviewboard.asterisk.org/r/2013/#comment12603>

    We may not want to make this assumption.
    
    For each path we append to the Python module load path, that path must be searched when a module is imported.  Since there are at least a hundred paths in the Test Suite - which is a number that I expect to continue to go up as well - that could end up impacting performance needlessly.
    
    I don't think its burdensome for a test writer to inform the framework code that "hey, this module isn't in the standard Test Suite packages, load it from here instead".
    



/asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/ami-config.yaml.sample
<https://reviewboard.asterisk.org/r/2013/#comment12610>

    This is a very good idea.
    
    Why don't we make a sample config folder for sample YAML configurations for these types of modules, similar to Asterisk's sample configuration folder?  Since we're likely to have a number of these, it may be worthwhile sticking them in their own directory.



/asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2013/#comment12609>

    This could use a docstring comment explaining the purpose of the class



/asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2013/#comment12604>

    Put a pass here so that its obvious that the base class method does nothing



/asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2013/#comment12605>

    And put a pass here as well



/asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2013/#comment12607>

    This could use a docstring comment explaining the purpose of the class



/asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2013/#comment12608>

    This could use a docstring comment explaining the purpose of the class



/asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2013/#comment12606>

    In keeping with the nomenclature previously defined, this probably should be AMIEventModule


- Matt


On July 8, 2012, 9:45 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2013/
> -----------------------------------------------------------
> 
> (Updated July 8, 2012, 9:45 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Summary
> -------
> 
> This is a new pluggable module for the Asterisk test suite intended to be used to verify AMI events.
> 
> There are two ways that this may be used.
> 
> * "headermatch": This is the simpler of the two methods. When user-specified conditions are met, then headers in an AMI event are checked for expected values. If the check fails, then the test fails as well.
> * "callback": For cases where more complicated checks are required than just AMI headers, then a user-defined callback may be called instead. This allows for the user to maintain their own test state to determine if the test has passed or failed.
> 
> There are some changes made outside the AMI module as well.
> 
> * TestCase has been updated to have a set_passed() method. Instead of directly modifying the 'passed' field of a test object, this method should be called. This way, if one module fails, other modules cannot override the failed state of the test. The CDR and ForkCDR modules have been updated accordingly
> * TestRunner has been modified to get rid of the "load-from-test" option. Instead, when module configuration is loaded, it automatically loads the test path into its loader's supported paths. This was done to get around a situation where I wanted the test path added to the supported paths but did not want the TestRunner to actually load the module in that path.
> 
> I have also included a sample YAML file that details the options available for the AMI module.
> 
> 
> Diffs
> -----
> 
>   /asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/ami.py 3290 
>   /asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/TestRunner.py 3290 
>   /asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/ami-config.yaml.sample PRE-CREATION 
>   /asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/TestCase.py 3290 
>   /asterisk/team/mmichelson/bridge-tests/lib/python/asterisk/cdr.py 3290 
>   /asterisk/team/mmichelson/bridge-tests/tests/cdr/ForkCdrModule.py 3290 
> 
> Diff: https://reviewboard.asterisk.org/r/2013/diff
> 
> 
> Testing
> -------
> 
> I tested this by tweaking the cdr_userfield test with different values. The only option that is not thoroughly tested is the 'id' field since SimpleTestCase only uses a single Asterisk instance. The intention, once this gets committed, is to use this in a battery of Bridging tests that will be written in the near future.
> 
> 
> Thanks,
> 
> Mark
> 
>

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