----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
