[asterisk-dev] [Code Review] SIP TLS Registration Test

mjordan reviewboard at asterisk.org
Mon Oct 31 13:06:19 CDT 2011 

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



/asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/ast1/sip.conf
<https://reviewboard.asterisk.org/r/1548/#comment8800>

    Does sipdebug have to be on?



/asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/ast1/sip.conf
<https://reviewboard.asterisk.org/r/1548/#comment8801>

    Remove the commented out block.  If a future test should be written for deliberately failing authentication, then it should be probably written as a separate test (and when that task is assigned, there's a good chance no one will see this anyway)



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8788>

    Two comments on this function:
    
    1. If this is a globally useful function, move it to utils in the library
    
    2. This function redirects the test's stdout to the file.  When the test is opened, stdout is also redirected to the runtests.py stdin.  Have you checked that when this function completes, stdout is redirected back to the runtests.py stdin?



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8804>

    This description is no longer accurate, as you only create a single instance of Asterisk and use SIPp to communicate with it, and use don't use DTMF tones to detect test status



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8789>

    Remove print, use logger statements



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8790>

    Use a more descriptive name then helper1.  Since you are copying it from a different location, you can use either sip_helper.inc or something similar to indicate that its a temporary pre-test version of sip_helper.inc.



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8791>

    Replace with logger statement



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8799>

    You don't need to specify 1 here



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8796>

    Why do you copy the keys to another directory structure here?  It appears as if this will create a lot of folders not in the /tmp/ directory but in the current location of the testsuite.



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8792>

    You shouldn't need the pass keyword



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8793>

    You shouldn't need the pass keyword



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8797>

    You can replace this with an instance of SIPpScenario.  This keeps this test more in line with those that use both SIPp and the TestCase base class.



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8794>

    You shouldn't need the pass keyword



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8798>

    You don't need to specify count=1 here



/asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test
<https://reviewboard.asterisk.org/r/1548/#comment8795>

    You shouldn't need the pass keyword



/asterisk/trunk/tests/channels/SIP/sip_tls_register/sipp/registerv4.xml
<https://reviewboard.asterisk.org/r/1548/#comment8802>

    Why registerv4.xml?  May want to consider renaming the file to what it actually is.



/asterisk/trunk/tests/channels/SIP/sip_tls_register/sipp/registerv4.xml
<https://reviewboard.asterisk.org/r/1548/#comment8803>

    Probably should change the scenario name as well, and remove the comment regarding the [call_id] keyword.


- mjordan


On Oct. 27, 2011, 12:55 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1548/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2011, 12:55 p.m.)
> 
> 
> Review request for Asterisk Developers, Paul Belanger, opticron, and mjordan.
> 
> 
> Summary
> -------
> 
> test: channels/SIP/sip_tls_register
> 
> Works in a similar way to sip_register, but uses TLS (which means it has to manage certificate files) and it also uses the TestCase class as well to simplify things.
> 
> I made a few changes the other day to get sip_register working normally under my dev machines, and that went fine, but I left the skip option on for it because it mentioned something about failing under FreeBSD, which would imply to me that there is some other reason they were explicitly failing under FreeBSD.  If that's the case, this test also will probably fail under FreeBSD since it makes use of basically all the same library functions as the regular sip_register test.  I'd like some advice on checking whether this failure is still a problem.
> 
> There is also some copying of files that goes on to put relevant keys into the working directory, which if ran from the test suite directory means these keys will be placed in the main testsuite folder.  I'm not sure if I should be doing this (I do clean the files up afterwards, but it's rather awkward) or changing my working directory instead and simply set aside a folder for these files to already be accessible from in the first place.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/helper1 PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/ca.cfg PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/ca.crt PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/ca.key PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/serverA.crt PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/serverA.csr PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/serverA.key PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/serverA.pem PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/serverB.crt PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/serverB.csr PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/serverB.key PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/serverB.pem PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/configs/keys/tmp.cfg PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/sipp/registerv4.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_tls_register/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/tests.yaml 2654 
> 
> Diff: https://reviewboard.asterisk.org/r/1548/diff
> 
> 
> Testing
> -------
> 
> I made sure the test passes and that the log messages show that the SIP dialog is happening as what would be expected.  In both cases, this test seems to do what it's supposed to do.
> 
> 
> Thanks,
> 
> jrose
> 
>

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

More information about the asterisk-dev mailing list