[asterisk-dev] [Code Review] 4413: Testsuite: Simulate phones and control from YAML.
Mark Michelson
reviewboard at asterisk.org
Thu Feb 12 12:42:22 CST 2015
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4413/#review14450
-----------------------------------------------------------
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24971>
I think this is a good case for using "is" instead of equality. I imagine that callers of this function are going to be passing a reference to the same account object that is stored on the phone_obj rather than a separate object with equivalent properties.
Also, to avoid the continue statement, you can just use a positive comparison instead of a negative one:
if account is phone_obj.account:
return phone_obj
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24970>
A couple of PEP 8 points:
1) When catching exceptions, use the syntax "except ErrorType as ErrorInstance" instead of "except ErrorType, ErrorInstance". So here, it would be "except pj.Error as err"
2) A PEP 8 checker will complain about the indentation of str(err)) on the final line I've highlighted. It will claim it should be lined up like this:
raise Exception("Exception occurred while making call: '%s'" %
str(err))
See how "str" is lined up to show how it belongs to the parentheses?
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24972>
I suggest breaking this into two separate functions: blind_transfer and attended_transfer.
1) The nomenclature for the transfer_type has to be looked up in the source in order to know what values are accepted.
2) The transfer_uri is only useful for blind transfers. By having two separate functions, you can more clearly group which parameters are relevant and which are irrelevant for a specific type of transfer. So the attended transfer, as currently defined, would take no parameters and a blind transfer would take a transfer URI. If it became useful later to optionally specify that an attended transfer should go the other direction or should affect different calls, that could be added solely to the attended transfer function.
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24973>
self.current_call doesn't seem to be used for anything.
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24974>
Should this only be done if self.phone is None? Seems like this would be redundant after the initial setting.
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24975>
I suspect it's not an issue, but is there ever a chance that self.call could be None when the on_state() callback is called?
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24976>
This seems overly complicated for what you are trying to do here. Why not just
try:
self.phone.calls.remove(self.call)
except ValueError:
# Log an error or something
If that doesn't work because equality of calls doesn't evaluate as you'd expect, then this is a situation where I expect that a single object reference is being passed around everywhere rather than separate call objects. You could take the approach of reconstructing the self.phone.calls list like this:
self.phone.calls = [call for call in self.phone.calls if call is not self.call]
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24977>
I think this needs to be altered to always call self.call.hangup() if final is 1. If a transfer attempt fails, the phone is still responsible for hanging up the call in that situation. So if you perform a blind transfer to a non-existent extension in the dialplan, or you perform a blind transfer to an extension that exists but hangs up with Congestion() or Busy() for some reason, the phone still needs to hang up the call when receiving the final transfer response.
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24978>
This function's name is misleading because it does not actually log anything. It should either
1) Be renamed to something indicating that it is just gathering call information into a string
or
2) Be altered to actually log the call info using LOGGER.debug()
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24979>
Can you add a comment about why the lock is being used here?
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24980>
If this exception occurs, does lock get properly unlocked?
/asterisk/trunk/lib/python/asterisk/phones.py
<https://reviewboard.asterisk.org/r/4413/#comment24981>
Remove this since the reactor will be stopped later when checking the truth of res.
/asterisk/trunk/lib/python/asterisk/pjsua_mod.py
<https://reviewboard.asterisk.org/r/4413/#comment24982>
Is this change necessary since you ended up overriding reg_success() in your new pluggable object?
- Mark Michelson
On Feb. 12, 2015, 4:25 p.m., jbigelow wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4413/
> -----------------------------------------------------------
>
> (Updated Feb. 12, 2015, 4:25 p.m.)
>
>
> Review request for Asterisk Developers and Mark Michelson.
>
>
> Bugs: ASTERISK-24578
> https://issues.asterisk.org/jira/browse/ASTERISK-24578
>
>
> Repository: testsuite
>
>
> Description
> -------
>
> Pluggable modules to place, receive, and transfer (blind/attended) calls to simulate phones using PJSUA and YAML configuration. Calls are placed and/or transferred using the new pluggable action module. This should allow many currrent and future tests to easily send/receive calls to/from Asterisk along with transferring calls within YAML configuration.
>
> The pluggable test module (phones.PjsuaPhoneController) initializes the PJSUA accounts and once all have registered, the account callbacks are setup and are ready to receive calls. The pluggable action module (pluggable_modules.PjsuaPhoneActionModule) provides the ability to place calls and transfer calls using the accounts from YAML and the action is referenced with 'pjsua_phone'. The only time a call is hung up by this is when a transfer is performed and a 200 OK sipfrag NOTIFY is received. None of the modules set a pass/fail result and are only for driving and manipulating calls.
>
> See attached file for YAML demo.
>
>
> Diffs
> -----
>
> /asterisk/trunk/lib/python/asterisk/pluggable_modules.py 6379
> /asterisk/trunk/lib/python/asterisk/pjsua_mod.py 6379
> /asterisk/trunk/lib/python/asterisk/phones.py PRE-CREATION
>
> Diff: https://reviewboard.asterisk.org/r/4413/diff/
>
>
> Testing
> -------
>
> * Tested placing calls, receiving calls, transfering via blind & attended.
> * Pylint score of 9.40/10 for phones.py
> * See attached test-config.yaml for a demonstration.
>
>
> File Attachments
> ----------------
>
> Demonstration
> https://reviewboard.asterisk.org/media/uploaded/files/2015/02/11/659ab31f-8401-4f24-be5e-da1db0be3156__test-config.yaml
>
>
> Thanks,
>
> jbigelow
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150212/ac0788ad/attachment-0001.html>
More information about the asterisk-dev
mailing list