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

Tue Jan 20 22:03:30 CST 2015

> On Jan. 20, 2015, 5:52 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test, lines 70-72
> > <https://reviewboard.asterisk.org/r/4352/diff/1/?file=70737#file70737line70>
> >
> >     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?

No real reason, other than it acts as a virtual method in case someone didn't implement it. Since both derived classes do implement it, it doesn't add much value.

Removed.

> On Jan. 20, 2015, 5:52 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test, lines 238-241
> > <https://reviewboard.asterisk.org/r/4352/diff/1/?file=70737#file70737line238>
> >
> >     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')

I like that much better, actually. This got rewritten several times as I tried to figure out a more concise way of writing it; I should have thought of using sum(). Nice!

(Also, this is why I like Python.)

> On Jan. 20, 2015, 5:52 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test, lines 266-267
> > <https://reviewboard.asterisk.org/r/4352/diff/1/?file=70737#file70737line266>
> >
> >     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].

I had a dictionary there at one point. I actually switched back and forth several times.

I can't recall why I ended up going back to a list of tuples, but after looking at the code again, I agree. Changed to a dictionary.

> On Jan. 20, 2015, 5:52 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test, line 147
> > <https://reviewboard.asterisk.org/r/4352/diff/1/?file=70737#file70737line147>
> >
> >     A bit pedantic, but shouldn't you use "==" instead of "in" ?

I actually couldn't remember why I used 'in' either. As it turns out, the 'args' parameter is a list, so 'in' is actually more effective.

> On Jan. 20, 2015, 5:52 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test, lines 33-39
> > <https://reviewboard.asterisk.org/r/4352/diff/1/?file=70737#file70737line33>
> >
> >     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.

I'll have the base class do it, and remove it from the derived classes.

> On Jan. 20, 2015, 5:52 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/rest_api/channels/redirect/off-nominal/redirect_off_nominal.py, lines 2-3
> > <https://reviewboard.asterisk.org/r/4352/diff/1/?file=70741#file70741line2>
> >
> >     It's 2015 and Josh didn't write this :)

Well, yes, but I did use the vast majority of one of his tests as a template.

I'll give him secondary credit :-)

> On Jan. 20, 2015, 5:52 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/rest_api/channels/redirect/nominal/run-test, lines 48-50
> > <https://reviewboard.asterisk.org/r/4352/diff/1/?file=70737#file70737line48>
> >
> >     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.

My philosophy on exceptions is that throwing and handling exceptions should happen in exceptional cases. In this case, there's nothing exceptional about not having the method: there's lot of Stasis messages this doesn't handle. Hence, it defaults to None if we don't have a method for that message type, and passes over them accordingly.

- Matt

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

On Jan. 18, 2015, 9:31 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4352/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2015, 9:31 p.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/20150121/6eb8a82c/attachment-0001.html>