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

[Samuel Galarneau]

Samuel Galarneau has posted comments on this change.

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


Patch Set 1: Code-Review-1

(13 comments)

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]


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 isn't in the list in the first place but it's not so expensive that you couldn't just do the list comprehension and not bother with checking to see if the call is in the list.


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.


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.


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


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:

Line 1: #/usr/bin/env python
Should be #!/usr...


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 of using conditionals for this.


Line 93: '200 OK' in packet.request_line or
       :                 'NOTIFY' in packet.request_line
I would align '200 OK' and 'NOTIFY' with 'INTIVE'


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


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


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.


Line 298:                     reinvited_endpoints)
Align this line with "Endpoints


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.


-- 
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: John Bigelow <jbigelow at digium.com>
Gerrit-Reviewer: Samuel Galarneau <sgalarneau at digium.com>
Gerrit-HasComments: Yes