[asterisk-dev] [Code Review] 2498: Test Suite: Pluggable CEL verification module

Matt Jordan reviewboard at asterisk.org
Wed May 8 11:05:15 CDT 2013 

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



/asterisk/trunk/configs/cel.conf
<https://reviewboard.asterisk.org/r/2498/#comment16398>

    We probably need to also modify the default manager.conf to add the CEL class authorization to the permissions for the [user] account



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16401>

    Although I know we have a few 'initializing foo' debug messages lying around, we can probably do without them. The module load runner should (hopefully) let us know when we are creating a particular module, so this ends up being redundant.
    
    (If it doesn't, then a debug statement does belong there)



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16403>

    You only really need to use 'get' if you expect that the attribute might not be in the dictionary - in which case, 'get' returns None.
    
    Here, you're chaining calls to get together, which means if 'cel' isn't in the instance dictionary, you're going to throw an exception when you call 'get' on an object that is None.
    
    This will end up being a bit more compact if you just write:
    
    origpartorder = instance['cel']['partialorder']



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16410>

    In the interest of maintainability, I'd convert this into a private function, something like:
    
    self.__verify_match_requirements(self.match_requirements)
    



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16404>

    Spaces between items please:
    
    for item in range(0, len(self.match_requirements)):
    
    
    In a later finding, I'm going to note that expected event ordering shouldn't require the index of the CEL event, so match_requirements should probably turn into a dictionary of match requirements, where the key is the ID of the event as provided by the YAML



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16406>

    Holy long debug statement. I'd shorten that up to something like:
    
    LOGGER.debug('No expected header for %s: using ^$')
    



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16412>

    Why not convert the names to lowercase during your initialization and verification? That way you don't have to do it each time here, and you "know" that the items should match up



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16420>

    
    Two things:
    
    1) When a test fails, usually all you see are the warning/error messages. Referencing that this is failing due to a CEL check would be good.
    
    2) Running slightly contrary to that statement, try to condense down the error messages slightly. This could be rephrased as:
    
    Required CEL key '%s' not found in received event
    



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16413>

    Since we're failing the test, this should at least be a warning



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16414>

    Failing a test => warning or higher



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16415>

    This feels like a debug statement



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16416>

    warning



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16417>

    And debug



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16418>

    Warning



/asterisk/trunk/lib/python/asterisk/ami.py
<https://reviewboard.asterisk.org/r/2498/#comment16421>

    This can do two things that will simplify the later calls that call it:
    
    1) Fail the test on failure
    2) Log the pass/fail success directly here



/asterisk/trunk/sample-yaml/ami-config.yaml.sample
<https://reviewboard.asterisk.org/r/2498/#comment16402>

    I think having the identifiers be implicit and numeric will make this difficult, particularly when we start to get into having CEL checks with a few dozen requirements/matches.
    
    I'd have each requirement have an ID that can be a string. That way, you can have things like:
    
     after: 'sip_foo_created'
     before: 'sip_foo_hungup' 


- Matt Jordan


On May 7, 2013, 6:29 p.m., jbigelow wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2498/
> -----------------------------------------------------------
> 
> (Updated May 7, 2013, 6:29 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: ASTERISK-21422
>     https://issues.asterisk.org/jira/browse/ASTERISK-21422
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> This uses AMI to allow verification of CEL messages. It is based on the AMIOrderedHEaderMatchInstance but differs in that it's specifically for checking CEL messages and that a partial order may be specified to allow messages to be out of order.
> 
> A condition match item of "Event: CEL" is always used and can not be overridden however additional condition match items may be specified. The list of requirements are checked in the order listed unless the optional 'partialorder' is specified.
> 
> Best explained by the description in the sample yaml file:
> 
> * If the event does successfully meet the 'match' critera of the
>   requirement expected and...
>   * a 'partialorder' is not specified, then this requirement
>     passes. Therefore the second requirement in the
>     requirements list is then expected.
>   * a 'partialorder' is specified and...
>      * the criteria is also met, then the requirement
>        passes. Therefore the second requirement in the
>         requirements list is then expected.
>      * the criteria is not met, the test is failed.
> * If it does not meet the 'match' criteria of the requirement
>    expected and...
>    * a partial order is not specified, the test is failed.
>    * a partial order is specified, it will remember the
>      requirement it is on and check if the 'match' criteria of
>      the next requirement in the list is met. It will continue
>      with each requirement in the order listed (where all the
>      criteria hasn't previously been met) until the 'match'
>      criteria is met or the end of the requirements list has
>      been reached at which point the test is failed.
>       * If the 'match' critera is successfully met and a...
>          * partial order is not specified, the test is
>            failed as the event arrived to early.
>          * partial order is specified and it's criteria
>            is not met, the test is failed.
>          * partial order is specified and it's criteria
>            is met, it is considered a successful match. It will
>            then move on and expect the next requirement in the
>            list from the point of the remembered requirement.
> 
> Note: If the 'match' criteria is met but not the 'partialorder' criteria of a requirement, the test is failed instead of checking other listed requirements. I did not run into a scenario where this prevented a test from properly failing or passing during my testing. It appears it would be very uncommon to run into this. 
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/lib/python/asterisk/ami.py 3764 
>   /asterisk/trunk/configs/cel.conf 3764 
>   /asterisk/trunk/sample-yaml/ami-config.yaml.sample 3764 
> 
> Diff: https://reviewboard.asterisk.org/r/2498/diff/
> 
> 
> Testing
> -------
> 
> * Tested by creating a simple test that used the SimpleTestCase test object to create a local channel and execute some dialplan. This created an environment where the CEL events would not always be in the same order upon each execution. Specifying a partial order on some of the requirements allowed to successfully work around the CEL events not being in the same order thus allowing the test to pass. Used it to also ensure expected failures did in fact cause the test to fail.
> * Tested by modifying the bridge/bindxfer_nominal/ test (which actually performs a blind transfer) to use this and again was able to use the partialorder on some of the requirements to work around the CEL events not always being in the same order.
> * Also tested doing the same with the bridge/blindxfer_setup/ test
> * Tested to ensure that only those EventName's listed in the requirements are checked and those not are ignored.
> 
> 
> Thanks,
> 
> jbigelow
> 
>

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

More information about the asterisk-dev mailing list