<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2790/">https://reviewboard.asterisk.org/r/2790/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 22nd, 2013, 7:41 p.m. UTC, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2790/diff/1/?file=44401#file44401line1155" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/bridge_channel.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int bridge_channel_write_dtmf_stream(struct ast_bridge_channel *bridge_channel, const char *dtmf)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static void testsuite_notify_feature_success(struct ast_channel *chan, const char *dtmf)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1154</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">ast_test_suite_event_notify</span><span class="p">(</span><span class="s">"FEATURE_DETECTION"</span><span class="p">,</span> <span class="s">"Result: fail"</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On August 23rd, 2013, 1:07 p.m. UTC, <b>opticron</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On August 23rd, 2013, 2:24 p.m. UTC, <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
<br />
<p>- Mark</p>
<br />
<p>On August 22nd, 2013, 4:38 p.m. UTC, Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated Aug. 22, 2013, 4:38 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/AST-1200">AST-1200</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/main/bridge_channel.c <span style="color: grey">(397345)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2790/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>