[asterisk-dev] [Code Review] 4352: testsuite: Add tests for ARI redirect; PJSIP Transfer

Tue Jan 20 17:52:38 CST 2015

-----------------------------------------------------------
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
> 
>

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