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

Mark Michelson mmichelson at digium.com
Fri Jul 23 09:03:31 CDT 2010 


> On 2010-07-23 04:55:47, Russell Bryant wrote:
> > /asterisk/trunk/runtests.py, line 92
> > <https://reviewboard.asterisk.org/r/796/diff/4/?file=11830#file11830line92>
> >
> >     I have been keeping a list of custom dependencies in the README.txt file.  Add this one there.

Will do.


> On 2010-07-23 04:55:47, Russell Bryant wrote:
> > /asterisk/trunk/tests/sip_attended_transfer_v6/run-test, lines 75-79
> > <https://reviewboard.asterisk.org/r/796/diff/4/?file=11834#file11834line75>
> >
> >     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"

I'll change it here and in all the other transfer tests that use this.


> On 2010-07-23 04:55:47, Russell Bryant wrote:
> > /asterisk/trunk/tests/sip_register/run-test, lines 67-72
> > <https://reviewboard.asterisk.org/r/796/diff/4/?file=11843#file11843line67>
> >
> >     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.

Sounds like a plan.


> On 2010-07-23 04:55:47, Russell Bryant wrote:
> > /asterisk/trunk/tests/sip_register/run-test, line 69
> > <https://reviewboard.asterisk.org/r/796/diff/4/?file=11843#file11843line69>
> >
> >     unnecessary semicolon

Old C habits die hard :(


> On 2010-07-23 04:55:47, Russell Bryant wrote:
> > /asterisk/trunk/tests/udptl/run-test, line 63
> > <https://reviewboard.asterisk.org/r/796/diff/4/?file=11856#file11856line63>
> >
> >     left over note to self?

Yep, this was from before I was sure exactly what FAXOPT(status) would say on a successful FAX.


> On 2010-07-23 04:55:47, Russell Bryant wrote:
> > /asterisk/trunk/tests/udptl/run-test, line 56
> > <https://reviewboard.asterisk.org/r/796/diff/4/?file=11856#file11856line56>
> >
> >     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.

Cool, I definitely like the named parameters approach.


> On 2010-07-23 04:55:47, Russell Bryant wrote:
> > /asterisk/trunk/tests/sip_attended_transfer_v6/run-test, line 45
> > <https://reviewboard.asterisk.org/r/796/diff/4/?file=11834#file11834line45>
> >
> >     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().

Sure thing.


> On 2010-07-23 04:55:47, Russell Bryant wrote:
> > /asterisk/trunk/tests/sip_attended_transfer_v6/run-test, lines 119-121
> > <https://reviewboard.asterisk.org/r/796/diff/4/?file=11834#file11834line119>
> >
> >     PEP8 specifies the maximum line length as 79 characters.  You can set python specific vimrc settings to help enforce this.

*sigh* fine.


> On 2010-07-23 04:55:47, Russell Bryant wrote:
> > /asterisk/trunk/tests/sip_register/run-test, lines 130-131
> > <https://reviewboard.asterisk.org/r/796/diff/4/?file=11843#file11843line130>
> >
> >     This is fine, but I suppose a more pythonic way to say it is:
> >     
> >     if not register_test.passed:

Sure thing


- Mark


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


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