[Asterisk-code-review] Add local attended transfer tests with direct media, hold, a... (testsuite[master])

Fri May 22 09:28:15 CDT 2015

John Bigelow has posted comments on this change.

Change subject: Add local attended transfer tests with direct media, hold, and REFER/Replaces.
......................................................................

Patch Set 1:

(24 comments)

https://gerrit.asterisk.org/#/c/483/1//COMMIT_MSG
Commit Message:

Line 28: 
> Missing tag: Reported By: Matt Jordan
Done

https://gerrit.asterisk.org/#/c/483/1/lib/python/asterisk/phones.py
File lib/python/asterisk/phones.py:

Line 204: if call in self.__op_calls[operation]:
        :             return True
        :         return False
> You could just return call in self.__op_calls[operation]
Done

Line 217: if call not in self.__op_calls[operation]:
        :             return
        :         self.__op_calls[operation] = \
        :                 [x for x in self.__op_calls[operation] if x != call]
> It is true that there is no need to do the list comprehension if the call i
Done

Line 368: if len(phone.calls) < 1:
        :         msg = "'%s' must have 1 active call to put on hold!" % phone.name
        :         test_object.stop_reactor()
        :         raise Exception(msg)
        :     if phone.calls[0].info().media_state == pj.MediaState.LOCAL_HOLD:
        :         LOGGER.debug("Call is already on local hold. Ignoring...")
        :         return
        :     if phone.is_call_op_tracked(phone.calls[0], operation='hold'):
        :         LOGGER.debug("Call hold operation is already in progress. Ignoring...")
        :         return
        :     if phone.calls[0].info().state != pj.CallState.CONFIRMED:
        :         LOGGER.debug("Call is not fully established. Retrying hold shortly.")
        :         reactor.callLater(.25, hold, test_object, triggered_by, ari, event,
        :                           args)
        :         return
        :     if phone.calls[0].info().media_state != pj.MediaState.ACTIVE:
        :         LOGGER.debug("Call media is not active. Ignoring...")
        :         return
> This could use some whitespace between conditionals.
Done

Line 413: if phone.calls:
        :         if phone.is_call_op_tracked(phone.calls[0], operation='transfer'):
        :             LOGGER.debug("Call transfer operation is already in progress.")
        :             return
        :     if transfer_type == "attended":
> I would add a blank line between the 2 conditionals.
Done

Line 421:                 phone.calls[1].info().state != pj.CallState.CONFIRMED):
> I would align phone.calls[1] under phone.calls[0].
Done

https://gerrit.asterisk.org/#/c/483/1/tests/channels/pjsip/transfers/attended_transfer/nominal/packet_sniffer.py
File tests/channels/pjsip/transfers/attended_transfer/nominal/packet_sniffer.py:

> There are four places in this file where iteritems() is used (line 156, lin
Done

Line 1: #/usr/bin/env python
> Should be #!/usr...
D'oh!

Line 55:         """Create listener, add callback, and add AMI observer."""
> You are missing your Keyword Arguments section of your docstring.
Done

Line 65:     def set_pcap_defaults(self, module_config):
> The docstring is missing Keyword Arguments section.
I've added keyword arg to docstring. I'll leave set_pcap_defaults as a method unless there's a good reason to make it a function.

Line 67: if not module_config.get('bpf-filter'):
       :             module_config['bpf-filter'] = 'udp port 5060'
       :         if not module_config.get('register-observer'):
       :             module_config['register-observer'] = True
       :         if not module_config.get('debug-packets'):
       :             module_config['debug-packets'] = True
       :         if not module_config.get('device'):
       :             module_config['device'] = 'lo'
       :         if not module_config.get('buffer-size'):
       :             module_config['buffer-size'] = 4194304
       :         if not module_config.get('snaplen'):
       :             module_config['snaplen'] = 2000
> You can use module_config[''] = module_config.get('value', default) instead
I copied it from another test but I've made it even better now.

Line 81:         """Callback when AMI connects. Sets AMI instance."""
> Even though ami_connect is part of a contract, I think it's worth including
Done

Line 93: '200 OK' in packet.request_line or
       :                 'NOTIFY' in packet.request_line
> I would align '200 OK' and 'NOTIFY' with 'INTIVE'
I blame my vim plugin. Done.

Line 95:             if not packet.body:
> This could be moved to an and clause in the previous conditional.
Done

Line 188:             gen = (i for (i, d) in enumerate(endpoint_calls) if callid in d)
> i and d are not very descriptive names.
Done

Line 190:             if call_index is not None:
> You don't really need the break here:
Done

Line 196:     def get_call_state(self, callid):
> is_call_known, find_call and get_call_state are eerily similar. 
I've removed is_call_known() and replaced the references with checking if self.find_call() is None. I've updated get_call_state() to call self.find_call() return the state if found.

Line 208:             if state is not None:
> You don't really need the break here:
Done

Line 271: if not self.is_hold(packet):
        :                 # It's not due to a hold.
        :                 if self.get_call_state(callid) == "CONNECTING":
        :                     # If the call is connecting then this 200 OK means it's
        :                     # being answered.
        :                     self.update_call_state(callid, "CONNECTED")
> Could be combined into 1 conditional.
Done

Line 298:                     reinvited_endpoints)
> The indentation is over one too many tabs on line 298.
Done

Line 298:                     reinvited_endpoints)
> Align this line with "Endpoints
Done

Line 395:     def send_user_event(self, user_event, event_data):
> I think the same is true for this function send_user_event and the one on l
Eh? send_user_event() does contain Keyword Arguments in the docstring.

Line 407: """Override of phones.hold()
        : 
        :     The phones.hold() method checks if a hold is already in progress or not.
        :     When a 491 occurs the hold will remain in progress as pjsua doesn't raise
        :     an exception or change any state. This allows bypassing the checks so a
        :     hold can be attempted again.
        :     """
> Doesn't list arguments or return value.
Done

Line 421:             reactor.callLater(.25, phone.hold_call)
> Could this also result in an invalid operation? If so, the outer exception 
It sure can! Good catch. Implemented a twisted deferred with an error handler.

-- 
To view, visit https://gerrit.asterisk.org/483
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd1f9a3ea8bf6b80fe95efe3fcd880a55d5a7cae
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: John Bigelow <jbigelow at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: John Bigelow <jbigelow at digium.com>
Gerrit-Reviewer: Samuel Galarneau <sgalarneau at digium.com>
Gerrit-HasComments: Yes