[asterisk-dev] [Code Review] 2790: Create test events necessary for bridging tests to pass

[Mark Michelson]

> On Aug. 22, 2013, 7:41 p.m., Matt Jordan wrote:
> > /trunk/main/bridge_channel.c, line 1155
> > <https://reviewboard.asterisk.org/r/2790/diff/1/?file=44401#file44401line1155>
> >
> >     Why do we still need this event?
> >     
> >     If we have a 'feature detection' event, then can't the tests just look for that event and fail if they don't get it?
> 
> opticron wrote:
>     It is possible to wait for the absence of an event, but the failure indication significantly reduces test duration by allowing the test to avoid relying on a timeout. Looking through the testsuite, about 40% of the feature checks specify that the feature attempt should fail.
> 
> Matt Jordan wrote:
>     Except that those will get fired *a lot*. There are lots of other tests that pass DTMF through channels while they are in a bridge - having more events passing through them increases the burden on those tests.

There are two reasons for this event:

1) It explicitly creates an event that says "I did not match the DTMF to a feature"
2) It alerts the bridge test object that the bridge is in a state where a new DTMF feature may be attempted.

As Kinsey stated, instead of an explicit "no feature detected" event, you could instead use the reactor's timer to implicitly determine that no feature was detected. I don't like this because

1) It slows down the tests (which are already quite slow)
2) I generally avoid using time-based determinators in the test suite because they lead to intermittent failures. A two-second delay on my test box may be enough to guarantee a feature was not detected, but in a more loaded and slower build agent, that may sometimes be too small a time to guarantee that we have not yet received the successful feature detection event.
3) It further complicates the bridge test object.

I'm not as sure about the claim about "lots" of other tests that pass DTMF through bridged channels. Grepping for "dtmf" and "DTMF" in the testsuite shows me that the only tests that fit the criteria are the bridge tests (that want this event) and the confbridge tests. Other tests for DTMF (such as SIP DTMF detection and voicemail) do not involve bridges. IMO, adding an extra event like this is no worse than setting a channel variable, since that also results in more new events for the test.

I have a potential compromise on this one. You'll notice that there are two places in this diff where the FEATURE_DETECTION failure event fires:

1) A lone DTMF digit is received that is not a partial match for any DTMF hooks on the bridge channel
2) DTMF is received that partially matches a DTMF hook on a bridge channel but then ends up not fully matching anything either after further digits are entered or the featuredigittimeout fires.

Currently, the bridge test object configures the DTMF features to use distinct single-digit codes. Therefore event 1) is the one that actually fires when no feature is detected. This is also the event that is likely to be spammy for other tests. Instead, I can change the bridge tests to use multi-digit feature codes that all begin with the same digit sequence. This way, I could remove event 1) and just leave event 2). This way, event 2) will fire for the bridge tests, and it will be less likely to cause issue for other tests unless it just so happens that the DTMF being issued during those other tests happens to initially partially match a DTMF hook on the bridge channel. What do you think of that?


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2790/#review9491
-----------------------------------------------------------


On Aug. 22, 2013, 4:38 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2790/
> -----------------------------------------------------------
> 
> (Updated Aug. 22, 2013, 4:38 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: AST-1200
>     https://issues.asterisk.org/jira/browse/AST-1200
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Kinsey already modified the bridge test object in the test suite to be able to handle the new bridging events that trunk has.
> 
> This set of changes adds the FEATURE_DETECTION events into the bridging framework so that bridge tests can operate properly.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/bridge_channel.c 397345 
> 
> Diff: https://reviewboard.asterisk.org/r/2790/diff/
> 
> 
> Testing
> -------
> 
> I used the disconnect test as the basis to see if bridge test object can function properly. The disconnect test passes the actual bridging portion. The test fails, however, when it attempts to see if the CEL records match the expected output.
> 
> There are, however, individual tasks currently assigned to people to get the individual tests running properly, so items such as CEL mismatches will be handled in those tasks.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130823/8fb0652f/attachment.htm>