[asterisk-dev] [Code Review] 2985: Test for consistent from on registers with same call-id

Thu Nov 7 16:30:52 CST 2013

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

Most of my critiques for this test addition are high-level issues as opposed to problems with the individual lines of code.

1) Be sure when writing new tests in python to run something like pylint or flake8 (which has a vim plugin) to ensure that the code is PEP-8 friendly.

2) Don't leave commented out code in your review. Comments that explain what is going on are great, but commented out code does not belong.

3) I'm not a fan of the use of the pcap library for outbound_reregister_from. The more common Asterisk testsuite method of ensuring that requests from Asterisk are structured as expected is to use SIPp to receive the request and use regex checking in the SIPp scenario to ensure that what is received is what is expected. The advantages of doing things this way over the method you have used are:
    a) The outbound_reregister_from test has a reactor timeout of 5 minutes. If using a SIPp scenario as the basis of the test, you can end the test as soon as the first re-registration transaction completes.
    b) Since SIPp usage is more integrated into the test suite, for this sort of test you wouldn't actually have to write any python code at all. Your test-config.yaml file would just need to indicate the SIPp scenario(s) to run and the testsuite will take care of the rest.

I'm also a bit curious to know if the Bamboo agents that automatically run the testsuite have pcap support and run as root.

4) What is the purpose of the tests/channels/SIP/outbound_reregister_from/3 file?

5) I'm not a fan of tests that start out passing and that fail as a result of incorrect data. I have two main reasons for disliking this method:
    a) The expected behavior of the test is made vague. I'm actually kind of in the dark about what the outbound_register_from test is intended to do. Is it expected to retransmit the same REGISTER for the entire test? Or is it expected to retransmit a REGISTER for a while and then try sending (and retransmitting) a new REGISTER?
    b) The test can pass under circumstances where it should really fail. To give an example, if there is some bug in chan_sip.c that causes it to never send out any REGISTER requests, these tests will pass because the pcap callback would never be called.

/asterisk/trunk/tests/channels/SIP/outbound_register_from/run-test
<https://reviewboard.asterisk.org/r/2985/#comment19425>

    (This comment applies to the other test files as well)

    It's probably best not to pass in a dumpfile here. The default behavior will dump the pcap to a file within the specific test run's log directory. This way, individual test runs will all have their dumps saved as opposed to overwriting the previous dump with each successive test run.

/asterisk/trunk/tests/channels/SIP/outbound_register_from/run-test
<https://reviewboard.asterisk.org/r/2985/#comment19424>

    In outbound_reregister_from, you check that the request is a REGISTER, but you don't do that in this test. While I don't suspect that there will be any non-REGISTER traffic sent, I think adding the check would be a good idea.

/asterisk/trunk/tests/channels/SIP/outbound_register_from/run-test
<https://reviewboard.asterisk.org/r/2985/#comment19419>

    This will never be run.

/asterisk/trunk/tests/channels/SIP/outbound_reregister_from/3
<https://reviewboard.asterisk.org/r/2985/#comment19421>

    This will never be run.

/asterisk/trunk/tests/channels/SIP/outbound_reregister_from/configs/ast1/sip.conf
<https://reviewboard.asterisk.org/r/2985/#comment19415>

    This peer definition is not necessary.

/asterisk/trunk/tests/channels/SIP/outbound_reregister_from/run-test
<https://reviewboard.asterisk.org/r/2985/#comment19422>

    red blob

/asterisk/trunk/tests/channels/SIP/outbound_reregister_from/run-test
<https://reviewboard.asterisk.org/r/2985/#comment19423>

    This will never be run.

- Mark Michelson

On Nov. 1, 2013, 9:11 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2985/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 9:11 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-12117
>     https://issues.asterisk.org/jira/browse/ASTERISK-12117
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> This test has Asterisk sending repeated REGISTERs to loopback and monitors them with pcap.  The Call-ID and From headers are extracted from the SIP messages to look for a change in From on the same dialog.
> 
> This test borrows code from pcap_demo, and must be run as root.
>  
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/channels/SIP/tests.yaml 4310 
>   /asterisk/trunk/tests/channels/SIP/outbound_reregister_from/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/outbound_reregister_from/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/outbound_reregister_from/configs/ast2/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/outbound_reregister_from/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/outbound_reregister_from/3 PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/outbound_register_from/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/outbound_register_from/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/outbound_register_from/configs/ast1/sip.conf PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2985/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

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