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

[Mark Michelson (Code Review)]

Mark Michelson has posted comments on this change.

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


Patch Set 2:

(6 comments)

Thanks for the review!

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 
Sounds good to me.


Line 86:         self.bridge1 = None
> Another fyi - The bridge1 and bridge2 could be made into a class and you co
I like it.


Line 121:                 self.call_carol()
> I think that you are calling this twice, once in the BobCallCallback.on_sta
Well spotted! But, this is actually done intentionally. The reason is that we cannot guarantee the order of events. Bob's state may change to CONFIRMED before there are two channels in the Asterisk bridge, or it may happen the other way around.

With this setup, they both attempt to call into the call_carol() function, and the call_carol() function will simply return early if the state is not such that calling Carol makes sense.


Line 132:                 self.transfer_call()
> Here, too, this seems to be called twice; once in the CarolCallCallback.on_
And here it's the same deal as with your previous observation.


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 
This is a safeguard to ensure that we don't attempt to call Carol in a later stage of the test, say, after we've already performed the transfer. Looking again, I bet I could remove the check of the state from this function; however, the state itself as a class member is still necessary.

The flow for a transfer goes as follows:

Alice calls Bob, and they enter bridge 1.
Alice calls Carol, and they enter bridge 2.
Alice performs the transfer. Alice leaves both bridge 1 and 2.
Now, the transfer code may move Bob out of bridge 1 and into bridge 2, or it may move Carol out of bridge 2 and into bridge 1. In either case, we detect the bridged state the same way as the original bridges with Alice: a BridgeEnter event with 2 channels in it. By maintaining the state of the test, we can determine whether a BridgeEnter with 2 channels means to continue on to the next state, or whether it means to hang up the calls because the test is complete. We also can detect if we get unexpected events and fail the test, as well.


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 
See my reply about the BOB_CALLED state.


-- 
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-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes