----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4352/#review14248 -----------------------------------------------------------
/asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/refer/sipp/alice.xml <https://reviewboard.asterisk.org/r/4352/#comment24715> I don't know what this is causing in Asterisk, if anything, but the to-tag that you are using here is incorrect. You should be able to just use [last_To:] here since the REFER that the SIPp scenario receives should have a to-tag already. /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/refer/sipp/alice.xml <https://reviewboard.asterisk.org/r/4352/#comment24716> Same finding here: remove the tag. /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/refer/sipp/alice.xml <https://reviewboard.asterisk.org/r/4352/#comment24717> Remove the tag. /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/refer/sipp/alice.xml <https://reviewboard.asterisk.org/r/4352/#comment24718> Remove the tag. /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test <https://reviewboard.asterisk.org/r/4352/#comment24721> Like my criticism of hangup_channels() below, I'm not sure why this exists. This sets self.test_object to the incoming test_object, but both subclasses do this as well, so there's no real point to this. Either this can be removed, or the subclasses can just not set self.test_object = test_object on themselves. /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test <https://reviewboard.asterisk.org/r/4352/#comment24720> python is cool. Any particular reason you've gone with the None default here rather than something like: try: callback = getattr(self, 'handle_{0}'.format(msg_type.lower())) except AttributeError: pass else: callback(message) ? Just curious since there's technically nothing wrong with this approach. /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test <https://reviewboard.asterisk.org/r/4352/#comment24719> Maybe this is just a philosophical thing, but why have this method? The only reason I could think of is if you had a subclass of BaseReceiver that did not define hangup_channels and you did not want to have an AttributeError thrown when subclass_instance.hangup_channels() was called. But in this case, the two subclasses have hangup_channels() defined on them. So why have a method on the base class that doesn't do anything? /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test <https://reviewboard.asterisk.org/r/4352/#comment24722> Comment says "ast2 or ast3" but the code checks for ast3 and ast4. /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test <https://reviewboard.asterisk.org/r/4352/#comment24724> s/erceived/received/ /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test <https://reviewboard.asterisk.org/r/4352/#comment24723> A bit pedantic, but shouldn't you use "==" instead of "in" ? /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test <https://reviewboard.asterisk.org/r/4352/#comment24725> Same finding here, use "==" instead of "in" /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test <https://reviewboard.asterisk.org/r/4352/#comment24726> This is probably fine, but it took me a sec to figure out exactly what was going on. What about: if sum(len(chan_list) for _, chan_list in self.channels) == 0: LOGGER.info('All channels hung up') It's a matter of whether you favor a generator expression over a list comprehension, I guess. Even if you don't want to switch to using sum(), I suggest changing to "_" instead of "server" since server is not actually being used in the comprehension and "_" is a commonplace indicator of an unused variable. Edit: After suggesting down below that possibly self.channels would be better served as a dictionary, my suggested change here now becomes: if sum(len(chan_list) for chan_list in self.channels.itervalues()) == 0: LOGGER.info('All channels hung up') /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test <https://reviewboard.asterisk.org/r/4352/#comment24727> This list comprehension is really strange. To me, this just screams that self.channels is the wrong type of data structure and that a dictionary would fit better. If self.channels were a dictionary, this just becomes: channels = self.channels[instance]. /asterisk/trunk/tests/rest_api/channels/redirect/off-nominal/redirect_off_nominal.py <https://reviewboard.asterisk.org/r/4352/#comment24728> It's 2015 and Josh didn't write this :) - Mark Michelson On Jan. 19, 2015, 3:31 a.m., Matt Jordan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4352/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2015, 3:31 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24015 and ASTERISK-24703 > https://issues.asterisk.org/jira/browse/ASTERISK-24015 > https://issues.asterisk.org/jira/browse/ASTERISK-24703 > > > Repository: testsuite > > > Description > ------- > > This patch adds tests for https://reviewboard.asterisk.org/r/4316/, which > includes both tests for PJSIP's .transfer channel callback and the ARI > /channels/[id]/redirect operation. > > *PJSIP Tests* > - Test transferring an unanswered channel to a PJSIP endpoint, which > responds to the initial INVITE request with a 302 > - Test transferring an answered channel to a PJSIP endpoint, which sends a > REFER request to the target > - Test transferring an unanswered channel to a SIP URI via PJSIP, which > responds to the initial INVITE request with a 302 > - Test transferring an answered channel to a SIP URI via PJSIP, which sends > a REFER request to the target > > *ARI Tests* > - Off-nominal testing of the new operation, verifying that the various off > nominal error response codes are returned as expected > - Nominal testing of the operation. For fun, this spawns four Asterisk > instances (one call generator, one load balancer, and two destinations) - and > proceeds to load balance 'calls' between the two destination instances. > > As a pre-emptive note: > (1) The off-nominal test makes use of the ARI event matcher, as it requires a > PJSIP channel and tests off nominal error response codes. Along with needing > to originate a second channel, the Python callback for this is relatively > self contained and limited, both of which remove most of the benefit of > driving the whole thing in YAML. > (2) The nominal test is written in "old style" - a single run-test. I can't > really see how anyone would re-use portions of it, but it was a fun test to > write to show the power of the new operation - plus it does exercise the > operation quite a lot. > > > Diffs > ----- > > /asterisk/trunk/tests/rest_api/channels/tests.yaml 6302 > /asterisk/trunk/tests/rest_api/channels/redirect/tests.yaml PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/off-nominal/test-config.yaml > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/off-nominal/redirect_off_nominal.py > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/off-nominal/configs/ast1/pjsip.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/off-nominal/configs/ast1/extensions.conf > PRE-CREATION > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/test-config.yaml > PRE-CREATION > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast4/pjsip.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast4/http.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast4/extensions.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast3/pjsip.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast3/http.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast3/extensions.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast2/pjsip.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast2/http.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast2/extensions.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast1/pjsip.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast1/http.conf > PRE-CREATION > > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/configs/ast1/extensions.conf > PRE-CREATION > /asterisk/trunk/tests/channels/pjsip/transfers/tests.yaml 6302 > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/tests.yaml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/refer/test-config.yaml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/refer/sipp/alice.xml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/refer/configs/ast1/pjsip.conf > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/refer/configs/ast1/extensions.conf > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/redirect/test-config.yaml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/redirect/sipp/alice.xml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/redirect/configs/ast1/pjsip.conf > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/uri/redirect/configs/ast1/extensions.conf > PRE-CREATION > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/tests.yaml > PRE-CREATION > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/tests.yaml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/refer/test-config.yaml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/refer/sipp/alice.xml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/refer/configs/ast1/pjsip.conf > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/refer/configs/ast1/extensions.conf > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/redirect/test-config.yaml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/redirect/sipp/alice.xml > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/redirect/configs/ast1/pjsip.conf > PRE-CREATION > > /asterisk/trunk/tests/channels/pjsip/transfers/asterisk/endpoint/redirect/configs/ast1/extensions.conf > PRE-CREATION > > Diff: https://reviewboard.asterisk.org/r/4352/diff/ > > > Testing > ------- > > > Thanks, > > Matt Jordan > >
-- _____________________________________________________________________ -- 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
