[asterisk-dev] [Code Review] New SIP tests to exercise IPv6 support

Russell Bryant russell at digium.com
Fri Jul 23 04:55:47 CDT 2010 

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

Ship it!


All of my comments are just formatting or minor suggestions for improvements.  Feel free to merge this when you're ready.


/asterisk/trunk/runtests.py
<https://reviewboard.asterisk.org/r/796/#comment5389>

    I have been keeping a list of custom dependencies in the README.txt file.  Add this one there.



/asterisk/trunk/tests/sip_attended_transfer_v6/run-test
<https://reviewboard.asterisk.org/r/796/#comment5390>

    I know that you started by copying another test, but I wanted to point it out for future code, if nothing else.
    
    As stated in README.txt, I would like to keep our python code formatted according to PEP8: http://www.python.org/dev/peps/pep-0008/.
    
    The function name here should be read_result() as opposed to readResult().



/asterisk/trunk/tests/sip_attended_transfer_v6/run-test
<https://reviewboard.asterisk.org/r/796/#comment5391>

    I would rather see an explicit check for the key being valid instead of exception handling.
    
    if "bridgedchannel" not in result[0]:
        print "bridgedchannel not a valid key"



/asterisk/trunk/tests/sip_attended_transfer_v6/run-test
<https://reviewboard.asterisk.org/r/796/#comment5392>

    PEP8 specifies the maximum line length as 79 characters.  You can set python specific vimrc settings to help enforce this.



/asterisk/trunk/tests/sip_register/run-test
<https://reviewboard.asterisk.org/r/796/#comment5393>

    spaces around =



/asterisk/trunk/tests/sip_register/run-test
<https://reviewboard.asterisk.org/r/796/#comment5395>

    change function names to be PEP8 compliant



/asterisk/trunk/tests/sip_register/run-test
<https://reviewboard.asterisk.org/r/796/#comment5394>

    trailing whitespace



/asterisk/trunk/tests/sip_register/run-test
<https://reviewboard.asterisk.org/r/796/#comment5397>

    I think you can simplify this a bit.  Instead of an array of dictionaries, make self.sipps a dictionary, using the peer name as the key.



/asterisk/trunk/tests/sip_register/run-test
<https://reviewboard.asterisk.org/r/796/#comment5396>

    unnecessary semicolon



/asterisk/trunk/tests/sip_register/run-test
<https://reviewboard.asterisk.org/r/796/#comment5398>

    If you change self.sipps to a dictionary keyed on peer, then you would iterate it by doing:
    
    for peer, i in self.sipps:
       pass



/asterisk/trunk/tests/sip_register/run-test
<https://reviewboard.asterisk.org/r/796/#comment5399>

    This is fine, but I suppose a more pythonic way to say it is:
    
    if not register_test.passed:



/asterisk/trunk/tests/udptl/run-test
<https://reviewboard.asterisk.org/r/796/#comment5400>

    I like the usage of AMI originate here.  We should probably start encouraging the usage of this instead of self.asterisk.cli_exec("channel originate ..."), which is the trend I had started.
    
    Also, as an improvement to this, python supports keyword arguments.  Hopefully starpy defines default values for all of those parameters you are specifying as None.  Assuming that is the case, you can specify your 3 arguments by name instead.



/asterisk/trunk/tests/udptl/run-test
<https://reviewboard.asterisk.org/r/796/#comment5401>

    left over note to self?


- Russell


On 2010-07-22 10:15:42, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/796/
> -----------------------------------------------------------
> 
> (Updated 2010-07-22 10:15:42)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Five new tests:
> 
> 1. sip_attended_transfer_v6. Identical to sip_attended_transfer except it uses IPv6 addresses.
> 2. sip_one_legged_transfer_v6. Identical to sip_one_leggeed_transfer except it uses IPv6 addresses.
> 3. sip_register. An IPv4 and IPv6 SIPp client register with Asterisk.
> 4. udptl. One instance of Asterisk sends a T.38 FAX to another instance of Asterisk.
> 5. udptl_v6. Same as udptl except IPv6 addresses are used.
> 
> In addition, the README file has been updated with instructions on how to enabled IPv6 support in pjsua. Also, a custom dependency for IPv6-capable pjsua has been added to runtests.py.
> 
> This is my first time writing tests in python, so let me know if I'm doing anything the hard way and I'll be an eager learner.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/README.txt 545 
>   /asterisk/trunk/runtests.py 545 
>   /asterisk/trunk/tests/sip_attended_transfer_v6/configs/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/sip_attended_transfer_v6/configs/manager.conf PRE-CREATION 
>   /asterisk/trunk/tests/sip_attended_transfer_v6/configs/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/sip_attended_transfer_v6/run-test PRE-CREATION 
>   /asterisk/trunk/tests/sip_attended_transfer_v6/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/sip_one_legged_transfer_v6/configs/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/sip_one_legged_transfer_v6/configs/manager.conf PRE-CREATION 
>   /asterisk/trunk/tests/sip_one_legged_transfer_v6/configs/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/sip_one_legged_transfer_v6/run-test PRE-CREATION 
>   /asterisk/trunk/tests/sip_one_legged_transfer_v6/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/sip_register/configs/manager.conf PRE-CREATION 
>   /asterisk/trunk/tests/sip_register/configs/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/sip_register/run-test PRE-CREATION 
>   /asterisk/trunk/tests/sip_register/sipp/registerv4.xml PRE-CREATION 
>   /asterisk/trunk/tests/sip_register/sipp/registerv6.xml PRE-CREATION 
>   /asterisk/trunk/tests/sip_register/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/tests.yaml 545 
>   /asterisk/trunk/tests/udptl/1page.tif UNKNOWN 
>   /asterisk/trunk/tests/udptl/configs1/logger.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl/configs1/manager.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl/configs1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl/configs2/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl/configs2/logger.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl/configs2/manager.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl/configs2/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl/run-test PRE-CREATION 
>   /asterisk/trunk/tests/udptl/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/udptl_v6/1page.tif UNKNOWN 
>   /asterisk/trunk/tests/udptl_v6/configs1/logger.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl_v6/configs1/manager.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl_v6/configs1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl_v6/configs2/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl_v6/configs2/logger.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl_v6/configs2/manager.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl_v6/configs2/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/udptl_v6/run-test PRE-CREATION 
>   /asterisk/trunk/tests/udptl_v6/test-config.yaml PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/796/diff
> 
> 
> Testing
> -------
> 
> All tests pass against trunk, except for udptl_v6, since trunk does not currently support IPv6 addresses with UDPTL streams. That test does pass when paired with the diff in review 795, though.
> 
> 
> Thanks,
> 
> Mark
> 
>

More information about the asterisk-dev mailing list