[asterisk-dev] [Code Review] sip_tls_call test added to external test suite

[jrose]

> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, line 6
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line6>
> >
> >     remove

Check.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, line 7
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line7>
> >
> >     same

Check.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, line 28
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line28>
> >
> >     You can use:
> >     
> >     self.self.create_fastagi_factory() for this.  It will then create a callback called fastagi_connect

Check.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, line 30
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line30>
> >
> >     Move to top of code block

Check.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, line 35
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line35>
> >
> >     again, not needed

Check.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, lines 40-44
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line40>
> >
> >     I was thinking of this at lunch, and trying to test.  I don't believe we will need to build the full path name to the files, the Asterisk process should be able to read them if the files exists in the same directory asterisk is executed from.
> >     
> >     I'm testing this now and will update.

As long as the path is a variable, it makes sense to dynamically generate this included information.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, lines 45-52
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line45>
> >
> >     This can be replaced with:
> >     self.create_asterisk(2)
> >     
> >

Check.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, line 66
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line66>
> >
> >     I'm not a fan of wait() or sleep.  Because we test across a multiple machines with different processors, this may cause the test to bounce.
> >     
> >     We could detect AMI events and respond as required.  This will help to keep the test more dynamic regardless of CPU speed.

Haven't addressed this yet, but will when I change it over to use AMI.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, lines 74-82
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line74>
> >
> >     I'm not a fan of this code, while checking the CDR is an option, I believe AMI would be better.

Same as above.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, line 89
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line89>
> >
> >     remove this.  Asterisk should be started prior to executing run(). See comment below.

Check.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, lines 93-98
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line93>
> >
> >     I'd rather see this in the sip.conf file.

Check.


> On 2011-06-20 15:33:00, Paul Belanger wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test, line 109
> > <https://reviewboard.asterisk.org/r/1276/diff/2/?file=17079#file17079line109>
> >
> >     above this line:
> >     test.start_asterisk()
> >     
> >     When we run() a test, asterisk should already be up and running.  Otherwise, the load time for Asterisk will be factored into the test.
> 
> jrose wrote:
>     Hmmm, most of this stuff was copied wholesale from the iax basic call test.  I'll work at making these changes, but it might be good to have a look at changing the iax basic call test as well since it's been suggested a couple times now as a good starting example.

And check to this as well.


- jrose


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


On 2011-06-20 14:25:48, jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1276/
> -----------------------------------------------------------
> 
> (Updated 2011-06-20 14:25:48)
> 
> 
> Review request for Asterisk Developers, Russell Bryant, David Vossel, and Paul Belanger.
> 
> 
> Summary
> -------
> 
> First, you can ignore the text files.  spacespacespace.txt was just used while I was working out shell command stuff and I accidentally added it to this diff and I've also removed test-output.txt.
> 
> I'm still not perfectly sure how this is going to work with the cert files.  I'd guess it'll be fine regardless of the machine, but I haven't tested this on another machine yet that didn't create these cert files.
> 
> This test uses the basic-call test in IAX2 as a base.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/ast1/manager.general.conf.inc PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/ast2/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/ast2/manager.general.conf.inc PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/ast2/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/helper1 PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/helper2 PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/ca.cfg PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/ca.crt PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/ca.key PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/serverA.crt PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/serverA.csr PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/serverA.key PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/serverA.pem PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/serverB.crt PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/serverB.csr PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/serverB.key PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/serverB.pem PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/configs/keys/tmp.cfg PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_call/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/tests.yaml 1633 
>   /asterisk/trunk/tests/tests.yaml 1633 
> 
> Diff: https://reviewboard.asterisk.org/r/1276/diff
> 
> 
> Testing
> -------
> 
> How did I test the test?  Mostly by checking the sip debug logs to make sure the call was going through as a SIP/TLS call.  The usual flow of SIP messages was there and it resembled my regular SIP/TLS calls.
> 
> 
> Thanks,
> 
> jrose
> 
>

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