[asterisk-dev] Change in testsuite[master]: Rewrite sip_attended_transfer test to stop failing.

[Ashley Sanders (Code Review)]

Ashley Sanders has posted comments on this change.

Change subject: Rewrite sip_attended_transfer test to stop failing.
......................................................................


Patch Set 2: Code-Review-1

(6 comments)

Overall, this is pretty good. The flow was mostly easy to follow. I only have a few findings with respect to some opportunities to use abstractions (that could help reduce the complexity of working around the caveats of the pjsua api).

https://gerrit.asterisk.org/#/c/19/2/tests/channels/SIP/sip_attended_transfer/attended_transfer.py
File tests/channels/SIP/sip_attended_transfer/attended_transfer.py:

Line 36: class BobCallCallback(pj.CallCallback):
Just as something to mention, BobCallCallback and CarolCallCallback can be reduced into one class, with the function to callback as a ctor parameter. The transfer object itself is not needed.


Line 86:         self.bridge1 = None
Another fyi - The bridge1 and bridge2 could be made into a class and you could have a attributes on the class, bridge_id and 'active' or 'is_bridged' to hold the state of the object, rather than having 2 variables for each bridge. I can see this pattern as being a little bit cleaner and easier to read if anyone ever has to come back and revisit this in the future.


Line 121:                 self.call_carol()
I think that you are calling this twice, once in the BobCallCallback.on_state handler (ln 48) callback and here again, on the ami.bridge_enter handler.


Line 132:                 self.transfer_call()
Here, too, this seems to be called twice; once in the CarolCallCallback.on_state handler (ln 69) and again here, in the ami.bridge_enter handler.


Line 154:         if (self.state == BOB_CALLED and self.bridge1_bridged and
I don't think you need the BOB_CALLED state; if bob's call is up, then you know that Bob has made his call.


Line 162:         if (self.state == CAROL_CALLED and self.bridge2_bridged and
I don't think you need the CAROL_CALLED state; if carol's call is up, then you know that Carol has made her call.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1676801d90bcafc28ba25e8b6889f40ab08cc90e
Gerrit-PatchSet: 2
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-HasComments: Yes