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

[Ashley Sanders]

Ashley Sanders 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

(11 comments)

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

Line 28: 
Missing tag: Reported By: Matt Jordan


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, line 187, line 205, line 292), so I will just make one comment here, instead of littering the file with lots of redundant comments.

iteritems() has been replaced with items() in python 3
dict.iterkeys(), dict.iteritems() and dict.itervalues() methods are no longer supported in python 3.

I think with this knowledge, it would be ideal to convert to using dict.items() to iterate over the elements of the dictionary - such that this code is future-proof, well, now-proof :p

-- http://stackoverflow.com/a/10458567
-- https://docs.python.org/3.4/whatsnew/3.0.html?highlight=iteritems


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


Line 65:     def set_pcap_defaults(self, module_config):
The docstring is missing Keyword Arguments section.

Also, set_pcap_defaults could be a function since it is not using the self parameter.


Line 81:         """Callback when AMI connects. Sets AMI instance."""
Even though ami_connect is part of a contract, I think it's worth including a Keyword Arguments section in the docstring -- for consistency
-- to honor the pythonic guidelines
-- to help any newcomers who may not realize that this is part of a contract


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

for endpoint, endpoint_calls in self.calls.iteritems():
            gen = (i for (i, d) in enumerate(endpoint_calls) if callid in d)
            call_index = next(gen, None)
            if call_index is not None:
                return (endpoint, call_index)
        return None


Line 196:     def get_call_state(self, callid):
is_call_known, find_call and get_call_state are eerily similar. 

is_call_known does pretty much a condensed version of what find_call and get_call_state are doing. If a call with the given caller id exists in the collection of endpoint_calls, it returns True, which is what your break statement is doing - the thing exists, so stop searching.

I think that you could make get_call_state a composite of returning the second element of the tuple returned from find_call.

find_call could be modified slightly to accept as a method parameter some value that would tell it to search either the entire enumerated endpoint_calls collection or a particular element of the collection.

is_call_known could be replaced with find_call:
e.g.
   call = self.find_call(some_caller_id_value)
   if call is not None:
       # do what you would have done before when
       # is_call_known returned True
   else:
       # do what you would have done before when
       # is_call_known returned False

This would save you from having to define practically the same logic in two different methods and could spare you from needing the third method all together.


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

for endpoint, endpoint_calls in self.calls.iteritems():
            gen = (call[callid] for call in endpoint_calls if callid in call)
            state = next(gen, None)
            if state is not None:
                return state
        return None


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


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 line 406, hold, that I said for ami_connect. Even though they are part of a contract, I think it's worth including a Keyword Arguments section in the docstrings:
-- for consistency 
-- to honor the pythonic guidelines 
-- to help any newcomers who may not realize that this is part of a contract


Line 421:             reactor.callLater(.25, phone.hold_call)
Could this also result in an invalid operation? If so, the outer exception is not going to catch it and the world as we know it will implode.


-- 
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