[asterisk-dev] [Code Review] 4413: Testsuite: Simulate phones and control from YAML.

jbigelow reviewboard at asterisk.org
Thu Feb 12 18:24:39 CST 2015 


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/phones.py, lines 86-88
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line86>
> >
> >     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

Yup, that's what I meant to do :)


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/phones.py, lines 115-117
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line115>
> >
> >     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?

I blame my python plugin for vim which just shifts twice. I installed the pep8 utility which pointed out a couple others which I have also fixed.


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/phones.py, line 156
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line156>
> >
> >     self.current_call doesn't seem to be used for anything.

Removed.


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/phones.py, lines 190-191
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line190>
> >
> >     Should this only be done if self.phone is None? Seems like this would be redundant after the initial setting.

That would make sense.


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/phones.py, line 192
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line192>
> >
> >     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?

As far as I know self.call would never be None when a pjsua callback is called such as on_state().


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/phones.py, lines 197-204
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line197>
> >
> >     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]

The first suggestion is actually what I initially was using. However I found that the call object wasn't always being removed from the list. Debugging showed the id() of self.call not matching any in the list which I believe is because self.call is set to weakref.proxy(call) in pjsua.CallCallback(). Comparing the SIP Call-ID of the call objects should be unique enough to find the one to remove from the list.


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/phones.py, line 246
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line246>
> >
> >     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()

Renamed.


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/phones.py, line 270
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line270>
> >
> >     Can you add a comment about why the lock is being used here?

Fixed this by removing the locking :)

Apparently a lock isn't needed when making call. Unless perhaps a callback isn't passed and then set later using call.set_callback() but that's not the case here.


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/phones.py, lines 272-275
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71348#file71348line272>
> >
> >     If this exception occurs, does lock get properly unlocked?

Fixed this by removing the locking :)

Apparently a lock isn't needed when making call. Unless perhaps a callback isn't passed and then set later using call.set_callback() but that's not the case here.


> On Feb. 12, 2015, 12:42 p.m., Mark Michelson wrote:
> > /asterisk/trunk/lib/python/asterisk/pjsua_mod.py, lines 258-259
> > <https://reviewboard.asterisk.org/r/4413/diff/1/?file=71349#file71349line258>
> >
> >     Is this change necessary since you ended up overriding reg_success() in your new pluggable object?

If that condition is removed then it will blow up if callback_module & callback_method are not defined in YAML and pjsua_mod.PJsua is being used as a test module. If it remains with the same scenario then things won't blow up and no user code is called into(maybe there's a need for that?). Suggestions?


- jbigelow


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


On Feb. 12, 2015, 10:25 a.m., jbigelow wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4413/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2015, 10:25 a.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/20150213/d233ee27/attachment-0001.html>

More information about the asterisk-dev mailing list