[asterisk-dev] [Code Review] 3600: testsuite: SIPNotify + PJSIPNotify behavioral tests

[Matt Jordan]

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



/asterisk/trunk/lib/python/asterisk/sipp.py
<https://reviewboard.asterisk.org/r/3600/#comment22148>

    The "W" in there does not read well at all. We're also using the acronym "SIP" - in all caps - while CamelCasing the acronym "Ami".
    
    How about one of:
    
    SIPpAMITestCase
    SIPpActionTestCase
    SIPpAMIActionTestCase
    



/asterisk/trunk/lib/python/asterisk/sipp.py
<https://reviewboard.asterisk.org/r/3600/#comment22149>

    Your class also needs a pydoc comment



/asterisk/trunk/lib/python/asterisk/sipp.py
<https://reviewboard.asterisk.org/r/3600/#comment22151>

    You don't actually need ami_command. What you need are:
    
    ami_args - to execute the action
    
    Optionally, ami_delay
    
    You need to decide if an AMI action is required. You have a mix here: the constructor allows it to be optional, but the execution on AMI connection expects self.ami_args and self.ami_delay to both exist



/asterisk/trunk/lib/python/asterisk/sipp.py
<https://reviewboard.asterisk.org/r/3600/#comment22150>

    Why are you referencing the private self._test_config when you were just passed it as a parameter to this function?



/asterisk/trunk/lib/python/asterisk/sipp.py
<https://reviewboard.asterisk.org/r/3600/#comment22154>

    pydoc comment



/asterisk/trunk/lib/python/asterisk/sipp.py
<https://reviewboard.asterisk.org/r/3600/#comment22155>

    pydoc comment



/asterisk/trunk/lib/python/asterisk/sipp.py
<https://reviewboard.asterisk.org/r/3600/#comment22152>

    Right now, in your constructor, self.ami_command is optional, and the ami_delay/ami_args attributes are only extracted if it exists. The parameters ami_delay and ami_args are thus not always set, which means this can easily throw an exception if an action isn't specified.
    
    If this requires these attributes, extract them in the constructor and throw an exception there if they aren't present.



/asterisk/trunk/lib/python/asterisk/sipp.py
<https://reviewboard.asterisk.org/r/3600/#comment22156>

    No space between """ and the start of the summary comment


- Matt Jordan


On June 11, 2014, 6:04 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3600/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 6:04 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> Tests for the following behavior in PJSIPNotify action:
> *PJSIPNotify sends notify messages with specified custom headers
> *PJSIPNotify does not override reserved headers
> *PJSIPNotify sends notify message content in the order that it was specified in the manager action
> 
> Tests the following behavior in SIPNotify action:
> *SIPNotify sends notify messages with specified custom headers
> *SIPNotify sends notify message content in the order that it was specified in the manager action
> 
> SIPNotify does not prevent modification of reserved headers, so that is not tested for SIPNotify. Whether or not it should is another question.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/channels/pjsip/ami/tests.yaml 5094 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/tests.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/reserved_headers/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/reserved_headers/sipp/options.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/reserved_headers/configs/ast1/pjsip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/custom_headers/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/custom_headers/sipp/options.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/custom_headers/configs/ast1/pjsip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/content/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/content/sipp/options.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/ami/pjsip_notify/content/configs/ast1/pjsip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/tests.yaml 5094 
>   /asterisk/trunk/tests/channels/SIP/ami/tests.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/ami/sip_notify/tests.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/ami/sip_notify/custom_headers/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/ami/sip_notify/custom_headers/sipp/options.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/ami/sip_notify/custom_headers/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/ami/sip_notify/content/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/ami/sip_notify/content/sipp/options.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/ami/sip_notify/content/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/lib/python/asterisk/sipp.py 5094 
> 
> Diff: https://reviewboard.asterisk.org/r/3600/diff/
> 
> 
> Testing
> -------
> 
> Ran tests, mangled the expectations to create failures for each test.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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