[asterisk-dev] [Code Review] 3105: Testsuite: Test PJSIP hold and unhold for various conditions for INVITE SDPs

Matt Jordan reviewboard at asterisk.org
Thu Jan 9 21:32:41 CST 2014 

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


While I know this was copied from the test I wrote for chan_sip, you should go ahead and clean up the implementation for PEP 8 compliance. Some of the ones here are issues I know it will complain about; you may want to run pylint on the code to check for others.


/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20016>

    I'd go with """ for pydoc string comments. That seems more in-line with how most libraries and projects use said strings for documentation.



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20017>

    LOGGER, not logger



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20018>

    Document the class.



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20019>

    Document the constructor.



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20020>

    Use super(SIPHold, self).__init__() instead of explicitly calling the constructor in TestCase.



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20021>

    As it turns out, it's typically better to use the single _ to indicate a private data member, as opposed to __.
    
    Using __ will cause name mangling of the attribute. It generally is only necessary to prevent subclasses from overriding the member accidentally; as such, it's probably not necessary here.



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20022>

    Documentation; use super



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20023>

    Document all of these functions



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20024>

    Declaring member variables out of a constructor will trigger PEP 8. You should declare them in the constructor.



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20025>

    Documentation, super where appropriate.



/asterisk/trunk/tests/channels/pjsip/hold/run-test
<https://reviewboard.asterisk.org/r/3105/#comment20026>

    Remove the start_asterisk/stop_asterisk calls. They are no longer necessary, as they are virtual functions that are automatically called when Asterisk is started/stopped.


- Matt Jordan


On Jan. 6, 2014, 3:59 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3105/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2014, 3:59 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp, Matt Jordan, and Mark Michelson.
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> Creates tests similar to the channels/SIP/sip_hold tests. Using the PJSIP channel driver, the following scenarios are tested:
> 
> 1. Put a call on hold by setting media attributes to sendonly. Unhold by setting media back to sendrecv
> 2. (new) as above, but unhold by simply not including an SDP (some devices are known to do this apparently and a patch is on reviewboard to handle that scenario in PJSIP).
> 3. Set on hold by setting the contact IP to 0.0.0.0, return to normal by providing normal SDP
> 4. Combine both the IP hold and media restriction hold methods.
> 
> There are a few noteworthy differences here to the original tests in SIP hold that might require some additional attention.
> 
> First, simply running the SIPP scenarios as is with PJSIP yielded an inability to match the new INVITES to the existing PJSIP dialogs. In addition to one invite from each scenario appearing to point to the wrong peer in the invite used to resume/unhold the call, the appropriate To tag was left out of the reinvites for both holding and unholding. I'm not sure how this worked in the first place.
> 
> Second, I noticed that verifying for hold/unhold behavior in the test script is performed by checking for Music On Hold start and stop events.  This mostly works for hold, but for unhold in particular it's unreliable since music on hold stop events will also be issued if the call simply ends. Because of that, I went ahead and added listeners for Hold and Unhold events which are required in a similar fashion to the existing MOH start and stop events.
> 
> I'm willing to apply these changes to the SIP hold test, but this does appear to be a noteworthy variance in behavior which needs to be taken note of... particularly the differences in the SIPP scenarios that needed to be made.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/channels/pjsip/tests.yaml 4539 
>   /asterisk/trunk/tests/channels/pjsip/hold/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/hold/sipp/phone_B_unhold_sans_sdp.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/hold/sipp/phone_B_media_restrict.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/hold/sipp/phone_B_IP_restrict.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/hold/sipp/phone_B_IP_media_restrict.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/hold/sipp/phone_A.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/hold/sipp/inject.csv PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/hold/run-test PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/hold/configs/ast1/pjsip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/hold/configs/ast1/extensions.conf PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3105/diff/
> 
> 
> Testing
> -------
> 
> Ran the tests. Checked output of user events in Asterisk. Confirmed that without the patch in 3106 that this test fails due to not receiving an Unhold event for the second scenario described above.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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

More information about the asterisk-dev mailing list